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

Avoid rate limit issue on kind test bed #6676

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KMAnju-2021
Copy link
Contributor

No description provided.

@KMAnju-2021 KMAnju-2021 marked this pull request as draft September 17, 2024 05:54
@KMAnju-2021 KMAnju-2021 changed the title skip to remove all the kind testbed images to avoid rate limit issue Skip to remove all the kind testbed images to avoid rate limit issue Sep 17, 2024
@KMAnju-2021 KMAnju-2021 changed the title Skip to remove all the kind testbed images to avoid rate limit issue Skip to remove all the images from kind testbed to avoid rate limit issue Sep 17, 2024
@rajnkamr rajnkamr changed the title Skip to remove all the images from kind testbed to avoid rate limit issue Avoid rate limit issue on kind test bed Sep 17, 2024
@rajnkamr rajnkamr added the area/test/infra Issues or PRs related to test infrastructure (Jenkins configuration, Ansible playbook, Kind wrappers label Sep 17, 2024
@KMAnju-2021 KMAnju-2021 force-pushed the kind-docker branch 6 times, most recently from c819cb8 to 7fe3f36 Compare September 17, 2024 10:16
@rajnkamr
Copy link
Contributor

For e2e tests on kind there are also common set of images which are pulled every-time like registry.k8s.io/e2e-test-images/agnhost:2.40"
antrea/nginx:1.21.6-alpine
antrea/toolbox:1.3-0 , if it is pulled already and available , kind testbed must retain these images

@KMAnju-2021 KMAnju-2021 force-pushed the kind-docker branch 10 times, most recently from 476df1c to 71166a5 Compare September 19, 2024 04:55
@KMAnju-2021 KMAnju-2021 marked this pull request as ready for review September 19, 2024 06:11
ci/jenkins/test.sh Outdated Show resolved Hide resolved
@KMAnju-2021
Copy link
Contributor Author

/test-kind-all

@KMAnju-2021
Copy link
Contributor Author

/test-kind-ipv6-e2e

@@ -201,8 +201,13 @@ function clean_antrea {
for antrea_yml in ${WORKDIR}/*.yml; do
kubectl delete -f $antrea_yml --ignore-not-found=true || true
done
docker images --format "{{.Repository}}:{{.Tag}}" | grep 'antrea'| xargs -r docker rmi -f || true
docker images | grep '<none>' | awk '{print $3}' | xargs -r docker rmi || true
if [[ $TESTBED_TYPE == "kind-flexible-ipam" || $TESTBED_TYPE == "kind" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the cleanup logic different based on the testbed type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same question, is it related to the rate limit issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@antoninbas @XinShuYang yes, no need to delete base images from the kind testbed. during run it will use images available on kind testbed no need to pull it again from docker..

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like such a change does not need to be specific to Kind tests?
However, one thing to keep in mind is that we always try to build the Antrea image with the latest available version of the base images (be it ubuntu or golang) in CI. To achieve that, a docker pull is essentially required and it may be subject to rate limiting, regardless of whether the correct version is already present or not - but maybe we get penalized less if the image is already present (fewer requests to the docker API?).

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have specified the version of the test images, this means they are unlikely to update like the latest version image. So I don't think it's necessary to remove them before each test run. Can we remove this change? @KMAnju-2021

Copy link
Contributor

Choose a reason for hiding this comment

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

@XinShuYang , We could consider tagging the test images with latest before pushing them to the Antrea repository ?

The versions of test images in conformance/networkpolicy may be updated as Kubernetes upgrades, so specifying the image versions helps us manage them more effectively. This is very important considering that we need to ensure compatibility when running CI jobs on different versions of Kubernetes clusters.

Copy link
Contributor

Choose a reason for hiding this comment

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

@XinShuYang is right. Tagging everything with latest is not a good approach here. It makes it hard to track which version is actually used and obfuscates things too much. A possible approach when the same tag shows up in too many places and we want to use the same version everywhere, is to write the current version to a file. This way we only ever have to update the version / tag in one place, instead of multiple scripts / test files.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, centralising the version in a single file would be preferable for test image version visibility, especially since we use the same test images across both regular and kind test beds. By centralising the version, we can ensure consistency across all scripts and test files, making version updates more straightforward and manageable. Better to move ahead with centralising the versions in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, centralising the version in a single file would be preferable for test image version visibility, especially since we use the same test images across both regular and kind test beds. By centralising the version, we can ensure consistency across all scripts and test files, making version updates more straightforward and manageable. Better to move ahead with centralising the versions in a separate PR.

I think it makes sense considering the single source of truth principle, but I’m okay with doing the refactor in a separate PR because it might involve refactoring both Linux and Windows jobs.

Copy link
Contributor

Choose a reason for hiding this comment

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

tracked #6697

ci/jenkins/test.sh Outdated Show resolved Hide resolved
ci/jenkins/test.sh Outdated Show resolved Hide resolved
ci/jenkins/test.sh Outdated Show resolved Hide resolved
hack/build-antrea-linux-all.sh Outdated Show resolved Hide resolved
hack/build-antrea-linux-all.sh Outdated Show resolved Hide resolved
ci/jenkins/test.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@jainpulkit22 jainpulkit22 left a comment

Choose a reason for hiding this comment

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

few suggestions

ci/jenkins/test.sh Show resolved Hide resolved
ci/jenkins/test.sh Show resolved Hide resolved
ci/jenkins/test.sh Outdated Show resolved Hide resolved
ci/jenkins/test.sh Outdated Show resolved Hide resolved
@KMAnju-2021 KMAnju-2021 force-pushed the kind-docker branch 2 times, most recently from 137aeaf to 9e3f55c Compare September 26, 2024 07:32
ci/jenkins/test.sh Show resolved Hide resolved
ci/jenkins/test.sh Outdated Show resolved Hide resolved
ci/jenkins/test.sh Outdated Show resolved Hide resolved
@KMAnju-2021 KMAnju-2021 force-pushed the kind-docker branch 4 times, most recently from 97cd52a to cb0d4cf Compare September 27, 2024 06:09
@KMAnju-2021
Copy link
Contributor Author

/test-kind-all

@antrea-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@KMAnju-2021
Copy link
Contributor Author

/test-kind-all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test/infra Issues or PRs related to test infrastructure (Jenkins configuration, Ansible playbook, Kind wrappers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants