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

k8sT/Egress: fixes #17581

Merged
merged 6 commits into from Oct 18, 2021
Merged

k8sT/Egress: fixes #17581

merged 6 commits into from Oct 18, 2021

Conversation

kkourt
Copy link
Contributor

@kkourt kkourt commented Oct 12, 2021

This PR includes fixes for the k8sT/egress test. It is a followup PR for #17517 (review).

Updates:

  1. @pchaigno noticed that the context helper was wrong, which lead to some kubectl commands being called with an empty namespace. In addition to that, I've added another commit to print the egress and nat tables in case of a failure. This is meant to help identify potential issues as reported in CI: v1.10: K8sEgressGatewayTest tunnel disabled with endpoint routes enabled Checks egress policy and basic connectivity both work: curl failed to connect with exit code 28 #17604.

  2. turns out that starting cilium at the begging was actually needed. fix the patch to just add a comment.

  3. removed = from tests names, to avoid:

    The Namespace "202110141537k8segressgatewaytesttunnel=disabledwithendpointrout" is invalid: metadata.name: Invalid value: "202110141537k8segressgatewaytesttunnel=disabledwithendpointrout": a DNS-1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is 'a-z0-9?')

  4. agent bailed out with:

    2021-10-14T20:49:33.226452400Z level=fatal msg="auto-direct-node-routes cannot be used with tunneling. Packets must be routed through the tunnel device." subsys=daemon

    adjusted configuration accordingly

@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 Oct 12, 2021
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Changes from original PR look good to me. The error you had previously looked unrelated to that particular test so probably an issue in your local setup. 🤞 it'll pass on the first try after the rebase.

@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 Oct 12, 2021
@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 Oct 12, 2021
@kkourt
Copy link
Contributor Author

kkourt commented Oct 12, 2021

/test

@kkourt kkourt force-pushed the pr/kkourt/egress-gw-test-fixes branch from 4756405 to 6101c8a Compare October 12, 2021 15:44
@kkourt
Copy link
Contributor Author

kkourt commented Oct 12, 2021

/test

@kkourt
Copy link
Contributor Author

kkourt commented Oct 13, 2021

/test-gke

edit: GKE was failing with, which should be fixed.

15:51:12 Failed to compile test:
15:51:12
15:51:12 # github.com/cilium/cilium/pkg/rate
15:51:12 ../pkg/rate/api_limiter.go:888:13: undefined: math.MaxInt
15:51:12 note: module requires Go 1.17
15:51:12
15:51:12 Ginkgo ran 1 suite in 20.196444393s
15:51:12 Test Suite Failed

@kkourt
Copy link
Contributor Author

kkourt commented Oct 13, 2021

/test-1.21-4.9

edit: after investigation, failure (was on policies) seemed unrelated, so redoing test.

1433181 introduced a change in
GenerateNamespaceForTest which is not needed. This patch reverts it.

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
@kkourt kkourt force-pushed the pr/kkourt/egress-gw-test-fixes branch from 6101c8a to 665a9d9 Compare October 14, 2021 09:56
@kkourt
Copy link
Contributor Author

kkourt commented Oct 14, 2021

/test

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
@kkourt kkourt force-pushed the pr/kkourt/egress-gw-test-fixes branch from 665a9d9 to 29d3737 Compare October 14, 2021 14:45
@kkourt
Copy link
Contributor Author

kkourt commented Oct 14, 2021

/test-only --focus="K8sEgress"

@kkourt
Copy link
Contributor Author

kkourt commented Oct 14, 2021

/test-only --focus="K8sEgress.*"

@kkourt
Copy link
Contributor Author

kkourt commented Oct 14, 2021

/test

@kkourt
Copy link
Contributor Author

kkourt commented Oct 14, 2021

/test-only --focus=".K8sEgressGateway."

@pchaigno
Copy link
Member

/test-only --focus=".K8sEgressGateway."

I think our script for test-only expect K8s to be the first thing in the focus string. Did the previous command not work as expected?

@kkourt
Copy link
Contributor Author

kkourt commented Oct 14, 2021

/test-only --focus=".K8sEgressGateway."

I think our script for test-only expect K8s to be the first thing in the focus string. Did the previous command not work as expected?

The K8sEgressGateway test was skipped AFAICT.

@pchaigno
Copy link
Member

The K8sEgressGateway test was skipped AFAICT.

Ah, right. That's because you need to pass the appropriate --kernel_version argument.

@kkourt kkourt force-pushed the pr/kkourt/egress-gw-test-fixes branch from 29d3737 to 2388d4e Compare October 14, 2021 17:53
@kkourt
Copy link
Contributor Author

kkourt commented Oct 14, 2021

/test

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
@kkourt
Copy link
Contributor Author

kkourt commented Oct 15, 2021

Tests are green. Will open for reviews, and will try to reproduce #17604 using a focus test.

@kkourt kkourt marked this pull request as ready for review October 15, 2021 12:50
@kkourt kkourt requested a review from a team as a code owner October 15, 2021 12:50
@pchaigno pchaigno self-requested a review October 15, 2021 12:51
@kkourt
Copy link
Contributor Author

kkourt commented Oct 15, 2021

/test-only --focus=".K8sEgressGateway." --kernel-version="net-next"

Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

LGTM! One nit below but not worth addressing IMO unless there are other comments.

test/k8sT/Egress.go Show resolved Hide resolved
@kkourt
Copy link
Contributor Author

kkourt commented Oct 15, 2021

/test-only --focus=".K8sEgressGateway." --kernel_version="net-next"

It's kernel_version, not kernel-version.

Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

👍🏻

@nbusseneau
Copy link
Member

nbusseneau commented Oct 15, 2021

I think we need to backport this to 1.10, correct?
NVM I missed the label was there already.

@kkourt
Copy link
Contributor Author

kkourt commented Oct 15, 2021

The last attempt to run the focus test, also does not seem to work:

https://jenkins.cilium.io/job/Cilium-PR-Tests-Kernel-Focus/321/console:

16:00:40  The machine with the name 'k8s1-' was not found configured for
16:00:40  this Vagrant environment.
[Pipeline] }
[Pipeline] // dir
[Pipeline] }
16:00:40  ERROR: script returned exit code 1
16:00:40  Retrying

In any case, I'll mark this as ready-to-merge. (There is an AKS failure, but the code here only modifies the Egress gateway test). Hopefully, 7e14347 will provide more info if we hit #17604 again.

@kkourt kkourt added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 15, 2021
@pchaigno
Copy link
Member

The last attempt to run the focus test, also does not seem to work:

https://jenkins.cilium.io/job/Cilium-PR-Tests-Kernel-Focus/321/console:

Weirdly enough, that build was trigered by @nbusseneau's comment 🤔 (CTRL+F for get-gh-comment-info.py: error).

@kkourt Your build is at https://jenkins.cilium.io/job/Cilium-PR-Tests-Kernel-Focus/320/console and failed because of the .* before K8sEgress in your focus string, as said here.

@pchaigno
Copy link
Member

pchaigno commented Oct 15, 2021

That one should work:
/test-only --focus="K8sEgressGateway" --kernel_version="net-next"
Started at https://jenkins.cilium.io/job/Cilium-PR-Tests-Kernel-Focus/322/.

@joamaki joamaki merged commit 25f32bc into master Oct 18, 2021
@joamaki joamaki deleted the pr/kkourt/egress-gw-test-fixes branch October 18, 2021 09:04
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.6 Oct 18, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.6 Oct 19, 2021
@jibi jibi added the feature/egress-gateway Impacts the egress IP gateway feature. label Oct 26, 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 feature/egress-gateway Impacts the egress IP gateway feature. 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
No open projects
1.10.6
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

5 participants