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: Stabilize ConformanceKindEnvoyDaemonSet #26260

Merged

Conversation

mhofstetter
Copy link
Member

@mhofstetter mhofstetter commented Jun 15, 2023

This PR tries to stabilize and de-flake the check ConformanceKKindEnvoyDaemonSet - mainly by removing the explicit connectivity-check flags external-* and using the default ones.

Currently it's failing on main and branches with failures when calling the external services: e.g. https://github.com/cilium/cilium/actions/runs/5278470155/jobs/9547854043

πŸ“‹ Test Report
❌ 6/45 tests failed (10/292 actions), 10 tests skipped, 0 scenarios skipped:
Test [no-policies]:
  ❌ no-policies/pod-to-cidr/external-8888-1: cilium-test/client-6965d549d5-7gpbl (10.244.1.96) -> external-8888 (8.8.8.8:443)
connectivity test failed: 6 tests failed
Test [all-ingress-deny]:
  ❌ all-ingress-deny/pod-to-cidr/external-8888-0: cilium-test/client2-76f4d7c5bc-s5245 (10.244.1.212) -> external-8888 (8.8.8.8:443)
  ❌ all-ingress-deny/pod-to-cidr/external-8888-1: cilium-test/client-6965d549d5-7gpbl (10.244.1.96) -> external-8888 (8.8.8.8:443)
Test [all-ingress-deny-knp]:
  ❌ all-ingress-deny-knp/pod-to-cidr/external-8888-0: cilium-test/client-6965d549d5-7gpbl (10.244.1.96) -> external-8888 (8.8.8.8:443)
  ❌ all-ingress-deny-knp/pod-to-cidr/external-8888-1: cilium-test/client2-76f4d7c5bc-s5245 (10.244.1.212) -> external-8888 (8.8.8.8:443)
Test [to-cidr-external]:
  ❌ to-cidr-external/pod-to-cidr/external-8888-1: cilium-test/client2-76f4d7c5bc-s5245 (10.244.1.212) -> external-8888 (8.8.8.8:443)
Test [to-cidr-external-knp]:
  ❌ to-cidr-external-knp/pod-to-cidr/external-8888-0: cilium-test/client-6965d549d5-7gpbl (10.244.1.96) -> external-8888 (8.8.8.8:443)
  ❌ to-cidr-external-knp/pod-to-cidr/external-8888-1: cilium-test/client2-76f4d7c5bc-s5245 (10.244.1.212) -> external-8888 (8.8.8.8:443)
Test [client-egress-to-cidr-deny]:
  ❌ client-egress-to-cidr-deny/pod-to-cidr/external-8888-0: cilium-test/client-6965d549d5-7gpbl (10.244.1.96) -> external-8888 (8.8.8.8:443)
  ❌ client-egress-to-cidr-deny/pod-to-cidr/external-8888-1: cilium-test/client2-76f4d7c5bc-s5245 (10.244.1.212) -> external-8888 (8.8.8.8:443)

Additional changes in the check:

  • kind version update
  • KPR=strict

@mhofstetter mhofstetter added the release-note/misc This PR makes changes that have no direct user impact. label Jun 15, 2023
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/conformance-kind-envoy-ds branch 2 times, most recently from 53f9d4f to df430e2 Compare June 15, 2023 12:22
This commit updates the kind version to v0.19.0.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit steamlines the usage of `helm-set` by using a `=` to set the
value.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit changes KPR to strict in check
ConformanceKindEnvoyDaemonSet.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit removes the flags `external-*` from the connectivity tests
in ConformanceKindEnvoyDaemonSet. Therefore the defaults are used.

This should stabilize the execution of the check on GHA.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter changed the title test CI: Stabilize ConformanceKindEnvoyDaemonSet Jun 15, 2023
@mhofstetter mhofstetter added area/CI Continuous Integration testing issue or flake release-note/ci This PR makes changes to the CI. and removed release-note/misc This PR makes changes that have no direct user impact. labels Jun 15, 2023
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/conformance-kind-envoy-ds branch from d88e03f to 9c3c523 Compare June 15, 2023 13:24
@mhofstetter mhofstetter marked this pull request as ready for review June 15, 2023 13:37
@mhofstetter mhofstetter requested review from a team as code owners June 15, 2023 13:37
@mhofstetter mhofstetter requested a review from brlbil June 15, 2023 13:37
@mhofstetter
Copy link
Member Author

/test

@mhofstetter
Copy link
Member Author

counting four successful runs in a row 🟒 🟒 🟒 🟒 πŸ™

@mhofstetter mhofstetter added the kind/enhancement This would improve or streamline existing functionality. label Jun 15, 2023
@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 Jun 15, 2023
@joestringer
Copy link
Member

@mhofstetter Can you describe a bit around the background to changing KPR? Does that mean we're losing coverage for the non-KPR approach? Was this discussed already with other folks in @cilium/sig-datapath ?

The rest of the changes LGTM.

@mhofstetter
Copy link
Member Author

mhofstetter commented Jun 15, 2023

Can you describe a bit around the background to changing KPR? Does that mean we're losing coverage for the non-KPR approach? Was this discussed already with other folks in @cilium/sig-datapath ?

The rest of the changes LGTM.

@joestringer Thanks for reaching out. As its name indicate, the sole purpose of this workflow is to have at least some connectivity test coverage on the new feature where Envoy is deployed as separate DaemonSet.

Most other aspects are covered within Conformance E2E, where the different features (including KPR) are covered in the matrix properties of the jobs. (this has been introduced after i made a copy of an existing workflow to create this workflow)

At some point in the future we have to decide whether we're going to include the Envoy DaemonSet feature in the mentioned matrix too or whether it will become the new default. I had a short discussion about this with @brb while he was moving most workflows into the E2E workflow.

Therefore, the switch of KPR shouldn't impact anybody else - and more importantly: we shouldn't lose any actual coverage.

@joestringer
Copy link
Member

OK thanks. Part of where I'm coming from here is that if we need to set KPR for this CI workflow to be stable, that could be hiding a real issue underneath. We can make this workflow more stable by switching that, but I'm also aware that our users might not have the option to just enable KPR. It's very easy to toggle a few settings in different CI workflows and then just lose coverage of certain combinations, then we'll only find out a few months later once our users try to deploy it. If we think that there's a risk of this, I'd request that we file an issue to track any missing combinations or to investigate why this workflow needs KPR vs. not. In an ideal world, kube-proxy vs. KPR would be functionally equivalent, just KPR is better (performance) due to the different design. In practice though kube-proxy vs. KPR often does have real functional differences due to either bugs in the implementation or integration issues.

That said I recognize now that the ConformanceKindEnvoyDaemonSet test is specifically for this new deployment model with the separate Envoy, so the purpose of this test is not to validate kube-proxy vs. KPR, so I'm fine with this πŸ‘

@joestringer joestringer merged commit 8f360b4 into cilium:main Jun 15, 2023
62 checks passed
@mhofstetter mhofstetter deleted the pr/mhofstetter/conformance-kind-envoy-ds branch June 15, 2023 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake kind/enhancement This would improve or streamline existing functionality. 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