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

test: Use new test-verifier image in K8sVerifier #16231

Merged
merged 1 commit into from Jun 2, 2021

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented May 19, 2021

Until now, K8sVerifier was using the cilium-builder image to build the datapath and run verifier-test.sh. Having a K8sVerifier-specific image also allows us to include a patch for the tc binary, to increase the maximum size of the verifier log buffer. In the K8sVerifier test, we load all BPF programs in verbose mode, so the log buffer is always needed (vs. only in case of retry following a load error usually). A small log buffer can lead to a load failure that is actually a false positive (it's just the log buffer being too small and not an actual issue with the BPF program).

@pchaigno pchaigno added kind/bug/CI This is a bug in the testing code. area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI. needs-backport/1.9 labels May 19, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.8 May 19, 2021
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

You could also remove test/k8sT/manifests/test-verifier.yaml from the image update scripts here:

used_by=($(git grep -l CILIUM_BUILDER_IMAGE= images/*/Dockerfile) "test/k8sT/manifests/test-verifier.yaml" "test/k8sT/manifests/demo-customcalls.yaml")

Until now, K8sVerifier was using the cilium-builder image to build the datapath and run verifier-test.sh. However, with the new image builds, K8sVerifier is now the only consumer of cilium-builder in the test VMs. If we replace cilium-builder with a K8sVerifier-specific image, we can stop pre-pulling cilium-builder in the VM image.

It looks like the cilium-builder image is still used by another test manifest:

image: quay.io/cilium/cilium-builder:330ca61256fedd7b8cbfc34b31316c97d1880876@sha256:e535425dd77cf29f289fa402d8811491617e2b8fe741db4a36ca6acc590006c1

So I think we cannot stop pre-pulling it in the VM image yet after this PR. Or are we not running that test using the test VMs?

@aanm aanm added this to Needs backport from master in 1.9.9 May 27, 2021
@aanm aanm removed this from Needs backport from master in 1.9.8 May 27, 2021
Until now, K8sVerifier was using the cilium-builder image to build the
datapath and run verifier-test.sh. Having a K8sVerifier-specific image
also allows us to include a patch for the tc binary, to increase the
maximum size of the verifier log buffer. In the K8sVerifier test, we
load all BPF programs in verbose mode, so the log buffer is always
needed (vs. only in case of retry following a load error usually). A
small log buffer can lead to a load failure that is actually a false
positive (it's just the log buffer being too small and not an actual
issue with the BPF program).

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno changed the title test: Define a new image to use in K8sVerifier test: Use new test-verifier image in K8sVerifier May 28, 2021
@pchaigno pchaigno marked this pull request as ready for review May 28, 2021 12:46
@pchaigno pchaigno requested review from a team as code owners May 28, 2021 12:46
@pchaigno
Copy link
Member Author

test-1.20-4.19

@pchaigno
Copy link
Member Author

test-1.21-4.9

@pchaigno
Copy link
Member Author

test-1.16-netnext

@pchaigno
Copy link
Member Author

test-1.19-5.4

@pchaigno
Copy link
Member Author

k8s-1.19-kernel-5.4 hit known flake #15575. Other CI jobs are passing. Reviews are in. Marking as ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 31, 2021
@nathanjsweet nathanjsweet merged commit 4b3ec57 into cilium:master Jun 2, 2021
@pchaigno pchaigno deleted the image-for-test-verifier branch June 2, 2021 19:18
@joestringer joestringer added this to Backport pending to v1.9 in 1.9.10 Jul 19, 2021
@joestringer joestringer removed this from Needs backport from master in 1.9.9 Jul 19, 2021
@joestringer joestringer removed this from Backport pending to v1.9 in 1.9.10 Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow kind/bug/CI This is a bug in the testing code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants