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

connectivity: disrupt deployment improved #2679

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

viktor-kurchenko
Copy link
Contributor

@viktor-kurchenko viktor-kurchenko commented Jul 12, 2024

Deploy conn-disrupt test actors only in the first test namespace in case of tests concurrent run to avoid resource wasting. Conn-disrupt tests always run at the beginning, sequentially and in the first test namespace.

Fixed: #2678

Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

let's add a test for this in one of the workflows. https://github.com/cilium/cilium-cli/blob/main/.github/workflows/kind.yaml is already using --test-concurrency flag, so maybe do this conn disrupt thingy there.

edit: btw kind workflow doesn't seem too happy 👀 https://github.com/cilium/cilium-cli/actions/runs/9911162365/job/27383203458?pr=2679

@viktor-kurchenko
Copy link
Contributor Author

let's add a test for this in one of the workflows. https://github.com/cilium/cilium-cli/blob/main/.github/workflows/kind.yaml is already using --test-concurrency flag, so maybe do this conn disrupt thingy there.

edit: btw kind workflow doesn't seem too happy 👀 https://github.com/cilium/cilium-cli/actions/runs/9911162365/job/27383203458?pr=2679

I've fixed the error.
Also, kind workflow already contains disrupt tests: here and here.

I think we can do the same for performance tests.

@viktor-kurchenko viktor-kurchenko marked this pull request as ready for review July 12, 2024 17:59
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

i guess we still need to specify --test-concurrency flag here? 💭

--test-concurrency=5 \

Deploy conn-disrupt test actors only in the first test namespace in
case of tests concurrent run to avoid resource wasting.
Conn-disrupt tests always run at the beginning, sequentially and in
the first test namespace.
Kind workflow improved to start disrupt tests without concurrency and
continue with concurrency.

Signed-off-by: viktor-kurchenko <[email protected]>
@viktor-kurchenko
Copy link
Contributor Author

i guess we still need to specify --test-concurrency flag here? 💭

--test-concurrency=5 \

I've removed --test-concurrency=5 from the first run conn tests run.

Also, please note that --test-namespace test-namespace is renamed to --test-namespace test-namespace-1 otherwise we need to start disrupt tests again for the second run.

Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

for future improvement, maybe it's better to create a namespace cilium-test-1 instead of cilium-test when test-concurrency is set to 1 to get rid of the special case where there is no -$index suffix in the namespace name.

michi-covalent added a commit to cilium/cilium that referenced this pull request Jul 12, 2024
@michi-covalent
Copy link
Contributor

ok i'm merging this. i tried this cilium-cli against the cilium repo, and there are some failures https://github.com/cilium/cilium/actions/runs/9914828717 let's investigate before enabling --test-concurrency flag in cilium repo 🧑‍🔬

@michi-covalent michi-covalent merged commit 9ffcbb4 into main Jul 13, 2024
13 checks passed
@michi-covalent michi-covalent deleted the pr/vk/disrupt/tests/fix branch July 13, 2024 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

connectivity: --test-concurrency flag doesn't work with --include-conn-disrupt-test
4 participants