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

ci: Pin k3d to use rancher/k3s:v1.21.7-k3s1 #2207

Merged
merged 5 commits into from
Oct 1, 2022

Conversation

dfarr
Copy link
Member

@dfarr dfarr commented Sep 24, 2022

Pinning the image to older version appears to stabilize the e2e tests

Makefile Outdated Show resolved Hide resolved
@dfarr
Copy link
Member Author

dfarr commented Sep 24, 2022

I think running the tests in parallel is causing a race condition where the last process to write the kube context to the ~/.kube/config file "wins". This is causing one of the e2e tests to succeed, and the other to fail. I will figure out a way to resolve this race condition and I suspect we should be good to go after that.

@whynowy
Copy link
Member

whynowy commented Sep 24, 2022

@dfarr - thanks for submitting the PR and appreciate your help! k3d/k3s is the the only tool we use across argo projects for local testing and e2e tests, could you help fix the issue still based on it?

@dfarr
Copy link
Member Author

dfarr commented Sep 26, 2022

@dfarr - thanks for submitting the PR and appreciate your help! k3d/k3s is the the only tool we use across argo projects for local testing and e2e tests, could you help fix the issue still based on it?

Sure! I took a look at k3s (not sure the relationship with k3d, but it appears as if they are sister projects) and I noticed that argo workflows is using it in their ci, I also noticed that there is a github action for it so I will give that a try.

Signed-off-by: David Farr <david_farr@intuit.com>
@dfarr dfarr changed the title Switch to kind for e2e tests Switch to ks3 for e2e tests Sep 28, 2022
@dfarr dfarr changed the title Switch to ks3 for e2e tests Switch to k3s for e2e tests Sep 28, 2022
run: |
echo /home/runner/go/bin >> $GITHUB_PATH
echo /usr/local/bin >> $GITHUB_PATH
- name: Restore go build cache
Copy link
Member Author

Choose a reason for hiding this comment

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

From [github action/cache docs](https://github.com/actions/cache/blob/main/examples.md#go---modules. I also noticed that argo workflows is using the cache action in this manner.

key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
restore-keys: |
${{ runner.os }}-go-
- name: Install and start K3S
Copy link
Member Author

Choose a reason for hiding this comment

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

I stole this verbatim from argo workflows.

Copy link
Member

Choose a reason for hiding this comment

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

Would prefer to use k3d instead of k3s directly, as we leverage its image registry feature when doing local development.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can leave k3d in the makefile, k3s is what argo events uses and it seems to be more consistent as the last 12 ci runs on our fork have all succeeded (first try).

- run: make lint
- run: git diff --exit-code

build-image:
Copy link
Member Author

Choose a reason for hiding this comment

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

I pulled this out into an independent step so it can be executed once rather than for each item in the e2e-tests matrix.

Copy link
Member

Choose a reason for hiding this comment

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

The idea is we also want to make it one step for local e2e testing, so I'm okay with each matrix testing to build the image for testing, even though it adds several minutes.

Makefile Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
- run: make lint
- run: git diff --exit-code

build-image:
Copy link
Member

Choose a reason for hiding this comment

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

The idea is we also want to make it one step for local e2e testing, so I'm okay with each matrix testing to build the image for testing, even though it adds several minutes.

@whynowy
Copy link
Member

whynowy commented Sep 28, 2022

If I'm not wrong, the reason it becomes stable is just because of using a low version k3s image. If that's the case, I would prefer to just add -i rancher/k3s:v1.21.7-k3s1 to k3d create cli.

The reason why port-forward and log stream become unstable with k3s higher versions remains unknown - it could be caused by some change on the way that k8s handling the requests, or some bugs in our test code, or could be related to the switch of docker to containerd.

Signed-off-by: David Farr <david_farr@intuit.com>
.github/workflows/ci.yaml Outdated Show resolved Hide resolved

run: |
make start
EventBusDriver=${{ matrix.driver }} make test-functional
Copy link
Member

Choose a reason for hiding this comment

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

This is not right - it's testing against the latest tag, which is the latest image from master branch. You could simply revert this part of changes, still use k3d and use the approach like https://github.com/argoproj/argo-events/pull/2218/files#diff-944291df2c9c06359d37cc8833d182d705c9e8c3108e7cfe132d61a06e9133ddR135.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it matter if we tag the image as latest though? The ci pipeline isn't used for pushing images to quay.

Copy link
Member

Choose a reason for hiding this comment

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

latest image is pushed to quay.io on master branch commit.

Makefile Outdated Show resolved Hide resolved
Signed-off-by: David Farr <david_farr@intuit.com>
@dfarr dfarr changed the title Switch to k3s for e2e tests Pin k3d to use rancher/k3s:v1.21.7-k3s1 Sep 29, 2022
Signed-off-by: David Farr <david_farr@intuit.com>
@whynowy
Copy link
Member

whynowy commented Sep 30, 2022

@dfarr - pin the k3s version.

Signed-off-by: David Farr <david_farr@intuit.com>
@dfarr
Copy link
Member Author

dfarr commented Oct 1, 2022

@dfarr - pin the k3s version.

Whoops, to much trial and error I must have reverted that too by mistake

Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@whynowy whynowy changed the title Pin k3d to use rancher/k3s:v1.21.7-k3s1 ci: Pin k3d to use rancher/k3s:v1.21.7-k3s1 Oct 1, 2022
@whynowy whynowy merged commit e99ab51 into argoproj:master Oct 1, 2022
whynowy pushed a commit that referenced this pull request Dec 12, 2022
Signed-off-by: David Farr <david_farr@intuit.com>
@dfarr dfarr deleted the update-ci branch January 18, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants