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

gateway-api: Fix flaky conformance tests #24317

Merged
merged 16 commits into from Mar 17, 2023

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Mar 12, 2023

Description

The goal is to have unit tests for Conformance test cases for faster feedback and stability. As part of this PR, two flaky tests are fixed 🤞.

Testing

Testing was done in https://github.com/cilium/cilium/actions/runs/4402111837, which are having 20+ consecutive successful runs.

gateway-api: Fix flaky conformance tests

This is to update test fixture for HTTRRoute header matching from the
upstream. The main point is to split header matching condition to diff
sets (i.e. OR condition).

Signed-off-by: Tam Mach <tam.mach@cilium.io>
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 12, 2023
@sayboras sayboras changed the title Tam/gw flaky test gateway-api: Fix flaky conformance tests Mar 12, 2023
@sayboras sayboras added the release-note/ci This PR makes changes to the CI. label Mar 12, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 12, 2023
@sayboras sayboras added the area/servicemesh GH issues or PRs regarding servicemesh label Mar 12, 2023
@sayboras sayboras force-pushed the tam/gw-flaky-test branch 3 times, most recently from cd7923a to a497fc1 Compare March 12, 2023 15:35
This commit is to make sure that header matching rules are sorted in a
deterministic way, so that the behavior is predictable in envoy. The
main changes are as per below:

- Process http routes based on order from spec instead of random order
  from map.
- Use sort.Stable to reverse the original order of equal elements.
- Make sure that less(i, j) and less(j, i) return false if i-th and j-th
  elements are equal.

Kindly note that envoy will just iteratively check rule one by one, if a
match is found, subsequent rule will not be considered.

Fixes: cilium#23999
Signed-off-by: Tam Mach <tam.mach@cilium.io>
This commit is to make sure that the virtual hosts are created in the
same order of appearance of HTTP Route spec. The changes are to split
nested map into two separate maps, and handle uniqueness.

Fixes: cilium#24217
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
…n test

Signed-off-by: Tam Mach <tam.mach@cilium.io>
…n test

Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
…test

Signed-off-by: Tam Mach <tam.mach@cilium.io>
Also, taking this chance to replace depecrated Append attribute with
AppendAction.

Signed-off-by: Tam Mach <tam.mach@cilium.io>
This is to match with names used in upstream Conformance. The benefit is
to have faster lookup:

- If a particular test is failed
- If a particular test is missing

Signed-off-by: Tam Mach <tam.mach@cilium.io>
This was overlooked in previous commits.

Signed-off-by: Tam Mach <tam.mach@cilium.io>
This is a new tests introduced as part of v0.6.0.

Signed-off-by: Tam Mach <tam.mach@cilium.io>
This is a new tests introduced as part of v0.6.0.

Signed-off-by: Tam Mach <tam.mach@cilium.io>
This is a new tests introduced as part of v0.6.0. GHA action setting for
this test is enabled.

Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras marked this pull request as ready for review March 13, 2023 09:14
@sayboras sayboras requested review from a team as code owners March 13, 2023 09:14
@sayboras sayboras requested a review from a team as a code owner March 13, 2023 09:14
@sayboras
Copy link
Member Author

Full CI is not required, as the code changes are related to Gateway API/Ingress/Unit tests, which are verified as part of GHA.

Copy link
Contributor

@brlbil brlbil left a comment

Choose a reason for hiding this comment

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

👍 for workflow changes

Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

LGTM

@sayboras
Copy link
Member Author

sayboras commented Mar 14, 2023

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: migrate-svc restart count values do not match

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

@sayboras
Copy link
Member Author

sayboras commented Mar 15, 2023

/test-1.16-4.19

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: migrate-svc restart count values do not match

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

@sayboras
Copy link
Member Author

sayboras commented Mar 15, 2023

/mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19

👍 created #24379

@sayboras
Copy link
Member Author

Above failure is fixed in #24336, which is just merged. Re-run is not required.

Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @sayboras

@sayboras sayboras added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 17, 2023
@borkmann borkmann merged commit 2d6423e into cilium:master Mar 17, 2023
@sayboras sayboras added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label Mar 18, 2023
@sayboras sayboras deleted the tam/gw-flaky-test branch March 18, 2023 01:27
@sayboras sayboras mentioned this pull request Mar 18, 2023
2 tasks
@sayboras sayboras added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/servicemesh GH issues or PRs regarding servicemesh 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