Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clone GitHub repos to the local Gitea during install #212

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

kispaljr
Copy link
Contributor

@kispaljr kispaljr commented Nov 3, 2023

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

It clones GitHub repositories that are registered in porch to the local Gitea during install.
This is needed to make the sandbox more self-contained and it is also a first step toward making it work behind a corporate http proxy (because it avoids Internet traffic originating in the clusters).

Which issue(s) this PR fixes:

It is a step toward fixing nephio-project/nephio#409 , but it will not completely fix the issue.

** Notes to reviewer:
Git cloning is deliberately done directly on the host machine instead of a pod running in a management cluster. This way we only need to assume that the localhost can reach the Internet (i.e. http proxy vars are setup correctly), but we don't have to assume that workloads running in the clusters can also reach the internet (that would require the errorprone setting of the no_proxy variable, that is almost impossible to do correctly.)

Does this PR introduce a user-facing change?:

no.

…ernet traffic originating in the clusters later.
Copy link
Contributor

nephio-prow bot commented Nov 3, 2023

Hi @kispaljr. Thanks for your PR.

I'm waiting for a nephio-project member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kispaljr kispaljr marked this pull request as ready for review November 3, 2023 16:20
@kispaljr
Copy link
Contributor Author

kispaljr commented Nov 3, 2023

/assign @electrocucaracha

@electrocucaracha, I see that you also made changes on how we handle stock git repos. Could you have a look if this was in line with your thinking?

Copy link
Member

@electrocucaracha electrocucaracha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to test it, but I made few suggestions on it. Don't forget to include the license headers on the files

e2e/provision/playbooks/mirror_git_repo_to_gitea.yaml Outdated Show resolved Hide resolved
e2e/provision/playbooks/mirror_git_repo_to_gitea.yaml Outdated Show resolved Hide resolved
e2e/provision/playbooks/mirror_git_repo_to_gitea.yaml Outdated Show resolved Hide resolved
e2e/provision/playbooks/mirror_git_repo_to_gitea.yaml Outdated Show resolved Hide resolved
e2e/provision/playbooks/mirror_git_repo_to_gitea.yaml Outdated Show resolved Hide resolved
e2e/provision/playbooks/cluster.yml Outdated Show resolved Hide resolved
@electrocucaracha
Copy link
Member

/assign @electrocucaracha

@electrocucaracha, I see that you also made changes on how we handle stock git repos. Could you have a look if this was in line with your thinking?

I put some comments on the draft. I think that makes sense, my only concern now it that prow hasn't triggered the CI yet.

@kispaljr
Copy link
Contributor Author

kispaljr commented Nov 3, 2023

/assign @electrocucaracha
@electrocucaracha, I see that you also made changes on how we handle stock git repos. Could you have a look if this was in line with your thinking?

I put some comments on the draft. I think that makes sense, my only concern now it that prow hasn't triggered the CI yet.

I think somebody (maybe @radoslawc ?) has to add an ok-to-test label to enable the CI tests

@electrocucaracha
Copy link
Member

/ok-to-test

Copy link
Contributor

nephio-prow bot commented Nov 3, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from electrocucaracha by writing /assign @electrocucaracha in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kispaljr
Copy link
Contributor Author

kispaljr commented Nov 3, 2023

/retest

@kispaljr
Copy link
Contributor Author

kispaljr commented Nov 3, 2023

Can someone help me with the install-integration and provision-linter tests? Is it an error in my code or in the tests?

@radoslawc
Copy link
Collaborator

Can someone help me with the install-integration and provision-linter tests? Is it an error in my code or in the tests?

@kispaljr that's a problem with test images, I've pushed the fix here #213

@kispaljr
Copy link
Contributor Author

kispaljr commented Nov 3, 2023

/kind cleanup

Comment on lines 11 to 33
- name: "Fetch kpt package: {{ item }}"
ansible.builtin.command:
cmd: "kpt pkg get --for-deployment --context {{ k8s.context }} '{{ nephio_example_repo_uri }}/{{ item }}@{{ nephio_pkg_version }}'"
chdir: "/tmp"
changed_when: false

- name: "Render kpt package: {{ item }}"
ansible.builtin.command:
cmd: "kpt fn render --context {{ k8s.context }}"
chdir: "/tmp/{{ item }}"
changed_when: false

- name: "Init kpt package: {{ item }}"
ansible.builtin.command:
cmd: "kpt live init --namespace default --context {{ k8s.context }}"
chdir: "/tmp/{{ item }}"
changed_when: false

- name: "Apply kpt package: {{ item }}"
ansible.builtin.command:
cmd: "kpt live apply --namespace default --reconcile-timeout 3600 --context {{ k8s.context }}"
chdir: "/tmp/{{ item }}"
changed_when: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a kpt Ansible module that you can use if you need and also a kpt Ansible role as well.

@kispaljr
Copy link
Contributor Author

/retest

@kispaljr
Copy link
Contributor Author

/retest

@kispaljr
Copy link
Contributor Author

kispaljr commented Nov 10, 2023

@electrocucaracha could you take a look?

  • the install role now works as follows:
    • expects a local_git input parameter with the fetch and push URIs of the Nephio management cluster-local git repo (it is Gitea in our case)
    • it is theoretically Gitea-independent, but right now our Repository resource only supports creating a new repo in Gitea
    • it uses only std git operations to copy external (Github) stock repos to the local git server
    • the molecule test of the role is now deploys gitea and metallb before the actual test
  • kpt CLI calls were replaced with the kpt Ansible module, as you suggested
  • all linter findings (including the ones not in my code) were fixed, or was marked with a noqa tag, as you sugested

@kispaljr
Copy link
Contributor Author

/retest

@kispaljr
Copy link
Contributor Author

/retest

@kispaljr
Copy link
Contributor Author

/retest

@electrocucaracha
Copy link
Member

@kispaljr I started testing this PR and putting some changes on my fork, I'll do a PR on your branch once it's ready

@kispaljr
Copy link
Contributor Author

@kispaljr I started testing this PR and putting some changes on my fork, I'll do a PR on your branch once it's ready

Thank you. I am struggling to figure out why the webui ReplicaSet times out consistently in the install-integration test.

@electrocucaracha
Copy link
Member

@kispaljr I started testing this PR and putting some changes on my fork, I'll do a PR on your branch once it's ready

Thank you. I am struggling to figure out why the webui ReplicaSet times out consistently in the install-integration test.

I've submitted a PR in your fork to apply most of the suggestions. It's hard to review given that your fork requires rebase some commits, but if you want to take a look these are the changes

@kispaljr
Copy link
Contributor Author

kispaljr commented Nov 20, 2023

@kispaljr I started testing this PR and putting some changes on my fork, I'll do a PR on your branch once it's ready

Thank you. I am struggling to figure out why the webui ReplicaSet times out consistently in the install-integration test.

I've submitted a PR in your fork to apply most of the suggestions. It's hard to review given that your fork requires rebase some commits, but if you want to take a look these are the changes

@electrocucaracha
I rebased your changes and pushed to the PR as-is. I have two comments:

  • unfortunately it didn't solve my problem with the install-integration test, it is still failing consistently with a webui deployment timeout. do you have an idea how to solve that?
  • removing all gitea specific input parameters from the install role and replacing it with generic "local git server" parameters was intentional from my part. I'd like to see the install role to be as independent from the actual kind of the git server that we deploy into the management cluster. I see that you reverted those changes. What is the reason behind this?

@electrocucaracha
Copy link
Member

@kispaljr I'm still seeing some differences between these changes and this other

@kispaljr
Copy link
Contributor Author

kispaljr commented Dec 1, 2023

/test install-integration

@kispaljr
Copy link
Contributor Author

kispaljr commented Dec 3, 2023

``> @kispaljr I'm still seeing some differences between these changes and this other

@electrocucaracha , you are right, they were different. So, I made the tip of my PR (0123635) identical to the tip of your PR (5a5c342).
Unfortunately the install-integration test consistently fails the exact same way as before. Do you have any idea why? Should I just adjust a timeout somewhere in the webui deployment process?

Here is the status of the nephio-webui ReplicaSet in the install-integration test logs for reference:

"status": {
                "conditions": [
                    {
                        "lastTransitionTime": "2023-12-01T17:14:00Z",
                        "lastUpdateTime": "2023-12-01T17:14:00Z",
                        "message": "Deployment does not have minimum availability.",
                        "reason": "MinimumReplicasUnavailable",
                        "status": "False",
                        "type": "Available"
                    },
                    {
                        "lastTransitionTime": "2023-12-01T17:24:01Z",
                        "lastUpdateTime": "2023-12-01T17:24:01Z",
                        "message": "ReplicaSet \"nephio-webui-5464c8dc64\" has timed out progressing.",
                        "reason": "ProgressDeadlineExceeded",
                        "status": "False",
                        "type": "Progressing"
                    }
                ],
                "observedGeneration": 1,
                "replicas": 1,
                "unavailableReplicas": 1,
                "updatedReplicas": 1

@electrocucaracha
Copy link
Member

I tried to replicate it locally, but it worked. I'll give a second try to see if something changes.

Regarding the e2e, it seems like it had issues to bring the Tunnel bridge up

/retest

Copy link
Contributor

nephio-prow bot commented Dec 4, 2023

@kispaljr: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
install-integration 0123635 link true /test install-integration
e2e-ubuntu-focal 0123635 link true /test e2e-ubuntu-focal

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@electrocucaracha
Copy link
Member

``> @kispaljr I'm still seeing some differences between these changes and this other

@electrocucaracha , you are right, they were different. So, I made the tip of my PR (0123635) identical to the tip of your PR (5a5c342). Unfortunately the install-integration test consistently fails the exact same way as before. Do you have any idea why? Should I just adjust a timeout somewhere in the webui deployment process?

Here is the status of the nephio-webui ReplicaSet in the install-integration test logs for reference:

"status": {
                "conditions": [
                    {
                        "lastTransitionTime": "2023-12-01T17:14:00Z",
                        "lastUpdateTime": "2023-12-01T17:14:00Z",
                        "message": "Deployment does not have minimum availability.",
                        "reason": "MinimumReplicasUnavailable",
                        "status": "False",
                        "type": "Available"
                    },
                    {
                        "lastTransitionTime": "2023-12-01T17:24:01Z",
                        "lastUpdateTime": "2023-12-01T17:24:01Z",
                        "message": "ReplicaSet \"nephio-webui-5464c8dc64\" has timed out progressing.",
                        "reason": "ProgressDeadlineExceeded",
                        "status": "False",
                        "type": "Progressing"
                    }
                ],
                "observedGeneration": 1,
                "replicas": 1,
                "unavailableReplicas": 1,
                "updatedReplicas": 1

There is a way to run integration tests locally and review the state of the VM, you can use the following instruction tox -e install -- --destroy never on the e2e/provision

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants