Skip to content

[v1.16] eks: Set egressMasqueradeInterfaces, concurrency #40468

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 14, 2025
Merged

[v1.16] eks: Set egressMasqueradeInterfaces, concurrency #40468

merged 2 commits into from
Jul 14, 2025
+8 −1

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented Jul 10, 2025 •
edited
Loading

This is a v1.16 backport of the egressMasqueradeInterfaces setting and the concurrency fix for ci-eks workflow to enable cluster-external traffic and to move back well under 1h of runtime.

Fixes: #40462

Backported setting egressMasqueradeInterfaces and concurrent test runs to fix ci-eks workflow.

@jrajahalme jrajahalme added the dont-merge/preview-only Only for preview or testing, don't merge it. label Jul 10, 2025
@jrajahalme jrajahalme requested review from a team as code owners July 10, 2025 15:36
@jrajahalme jrajahalme added the release-note/ci This PR makes changes to the CI. label Jul 10, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.16 This PR represents a backport for Cilium 1.16.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. labels Jul 10, 2025
@jrajahalme
Copy link
Member Author

/ci-eks

@jrajahalme jrajahalme mentioned this pull request Jul 10, 2025
Closed
@jrajahalme
Copy link
Member Author

/ci-eks

@jrajahalme
Copy link
Member Author

External traffic issue is fixed by setting egressMasqueradeInterfaces, but now the workflow timed out, so the concurrency restoration is also needed. Cherry-picked that into the 2nd commit.

@jrajahalme jrajahalme added kind/bug/CI This is a bug in the testing code. area/CI Continuous Integration testing issue or flake and removed dont-merge/preview-only Only for preview or testing, don't merge it. labels Jul 10, 2025
@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme force-pushed the test-eks-masquarade-setting branch from 901af1a to 3e47e1a Compare July 10, 2025 18:09
@jrajahalme jrajahalme changed the title eks: Set egressMasqueradeInterfaces [v1.16] eks: Set egressMasqueradeInterfaces, concurrency Jul 10, 2025
@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme force-pushed the test-eks-masquarade-setting branch from 3e47e1a to 82f8b50 Compare July 11, 2025 04:05
@jrajahalme
Copy link
Member Author

/test

@viktor-kurchenko
Copy link
Contributor

@jrajahalme can we set test_concurrency: 1?
I think EKS on v1.16 branch might not have enough resources.

@mathpl mathpl mentioned this pull request Jul 11, 2025
Closed
8 tasks
@jrajahalme jrajahalme force-pushed the test-eks-masquarade-setting branch from 82f8b50 to 7957568 Compare July 12, 2025 13:37
@jrajahalme
Copy link
Member Author

@viktor-kurchenko ci-eks was cancelled due to a timeout without any concurrency. And yes, with concurrency set to 2 it can't deploy the conn-disrupt-test pods. I tried only using the concurrency for the connectivity test before the conn-disrupt-test, but meybe we should just increase the timeout instead?

@jrajahalme jrajahalme force-pushed the test-eks-masquarade-setting branch from 7957568 to 8d6cd74 Compare July 12, 2025 13:41
@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme force-pushed the test-eks-masquarade-setting branch from 8d6cd74 to e799acb Compare July 12, 2025 14:12
@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

@viktor-kurchenko Do you think running the full test suite after key rotation adds any value? I'd think no-interrupted-connections" test should be enough, as the full test suite has already been run before the key rotation? Changing full-testtofalse` would speed up this workflow a lot!

michi-covalent and others added 2 commits July 12, 2025 17:17
@michi-covalent @jrajahalme
eks: Set egressMasqueradeInterfaces
8ea4a9b 
[ upstream commit 72369a6 ]

The egressMasqueradeInterfaces Helm value incorrectly gets set to "eth+"
instead of "eth+ ens+" because of the version check ">=1.17.0" added in
#36887. versioncheck.Version does
not ignore alpha/beta/rc/snapshot pre-release versions [^1]. Explicitly
set egressMasqueradeInterfaces to 'eth+ ens+' as a workaround.

[^1]: https://github.com/cilium/cilium/blob/2a349d5c5353cf0235414579885122425025dbdb/pkg/versioncheck/check.go#L44-L71

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@julianwiedmann @jrajahalme
gh: eks: restore concurrent execution of connectivity tests
9dfc85c 
Run connectivity tests before key rotation with test-concurrency=2 to
speed them up, but remove `cilium-test-2` namespace before setting up
conn-disrupt-test to avoid running out of resources. After key rotation,
only run the `no-interrupted-connections` test instead repeating the full
test suite.

These changes help avoid the workflow timing out.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme force-pushed the test-eks-masquarade-setting branch from e799acb to 9dfc85c Compare July 12, 2025 15:17
@jrajahalme
Copy link
Member Author

/test

@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 Jul 12, 2025
@viktor-kurchenko
Copy link
Contributor

@viktor-kurchenko Do you think running the full test suite after key rotation adds any value? I'd think no-interrupted-connections" test should be enough, as the full test suite has already been run before the key rotation? Changing full-testtofalse` would speed up this workflow a lot!

@jrajahalme I don't have a strong opinion here, I think it's better to ask somebody form the IPsec team.
My point about resources was about the difference between EKS setup for the:

So, I think if we can increase node count or decrease the tests concurrency to 1, we won't need the Delete test namespaces to make space for conn-disrupt-test pods step.

@sayboras sayboras added this pull request to the merge queue Jul 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 14, 2025
@sayboras sayboras added this pull request to the merge queue Jul 14, 2025
Merged via the queue into v1.16 with commit 7539967 Jul 14, 2025
69 of 70 checks passed
@sayboras sayboras deleted the test-eks-masquarade-setting branch July 14, 2025 11:02
@aanm aanm mentioned this pull request Jul 14, 2025
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viktor-kurchenko viktor-kurchenko viktor-kurchenko approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake backport/1.16 This PR represents a backport for Cilium 1.16.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jrajahalme @viktor-kurchenko @sayboras @michi-covalent @julianwiedmann