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: Fail ginkgo tests on warnings #29624

Merged
merged 5 commits into from
Dec 10, 2023

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Dec 5, 2023

This pull request makes the ginkgo tests fail on any level=warning agent or operator log. See commits for details.

I had to allowlist a bunch of warnings so we can at least stop the bleeding before we fix them. I ran the full ginkgo suite 12 times without any new warnings popping up so we should be good to go.

@pchaigno pchaigno added area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI. labels Dec 5, 2023
@pchaigno pchaigno changed the title Ginkgo fail on warnings test: Fail ginkgo tests on warnings Dec 5, 2023
@pchaigno pchaigno force-pushed the ginkgo-fail-on-warnings branch 21 times, most recently from fd34ccd to a4c5b7d Compare December 9, 2023 10:57
Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
We need to allowlist the following warning because it's expected in our
Kind-based CI:

     level=warning msg="Unable to ensure that BPF JIT compilation is
       enabled. This can be ignored when Cilium is running inside
       non-host network namespace (e.g. with kind or minikube)"
       error="could not open the sysctl file
       /host/proc/sys/net/core/bpf_jit_enable: open
       /host/proc/sys/net/core/bpf_jit_enable: no such file or
       directory" subsys=sysctl sysParamName=net.core.bpf_jit_enable
       sysParamValue=1

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
We currently have a lot of warnings in the ginkgo tests because we're
using the old, deprecated values for the kubeProxyReplacement flag. This
commit fixes it.

Fixes: 996331d ("Revert "Revert "agent: Add --kube-proxy-replacement=true|false""")
Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
If we don't do that, we'll end up with:

    level=warning msg="Falling back to iptables-based masquerading."
      error="BPF masquerade requires NodePort (--enable-node-port=\"true\")"
      subsys=daemon

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
We have many different warnings happening in CI. Let's allowlist them
for now, so we can at least enforce the new check and stop the bleeding.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno pchaigno marked this pull request as ready for review December 10, 2023 10:11
@pchaigno pchaigno requested review from a team as code owners December 10, 2023 10:11
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

The changes look good to me, however, I don't have all the context to review all warning messages (except for the one related to envoy/gateway api and ingress).

test/helpers/cons.go Show resolved Hide resolved
@pchaigno
Copy link
Member Author

I don't have all the context to review all warning messages (except for the one related to envoy/gateway api and ingress).

Me neither (except the ones related to datapath 😅). I opened issues for all of them if they required further investigation. My goal for now is really just to stop the bleeding, not to make a judgement call on whether any of these are legitimate or require fixes. That will take more time.

@pchaigno pchaigno added this pull request to the merge queue Dec 10, 2023
Merged via the queue into cilium:main with commit d745b28 Dec 10, 2023
58 checks passed
@pchaigno pchaigno deleted the ginkgo-fail-on-warnings branch December 10, 2023 16:45
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 11, 2023
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 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

3 participants