Connectivity test concurrent run #2496
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viktor-kurchenko
force-pushed
the
pr/vk/conn/tests/concurrency
branch
from
fde3e78
to
5777f53
Compare
viktor-kurchenko
force-pushed
the
pr/vk/conn/tests/concurrency
branch
from
5777f53
to
44c3f3c
Compare
|
@viktor-kurchenko could you take a look at kind test failures? |
There was a problem hiding this comment.
let's add a matrix for the kind job and run with both --test-concurrency=1 and --test-concurrency=5: https://github.com/cilium/cilium-cli/blob/main/.github/workflows/kind.yaml#L24
marseel
approved these changes
Apr 23, 2024
nebril
requested changes
Apr 23, 2024
michi-covalent
added a commit
that referenced
this pull request
Apr 25, 2024
This commit changes the behavior of --test-namespace flag. Instead of only deleting the namespace that matches --test-namespace flag exactly, delete all the namespaces with the prefix specified in --test-namespace flag. This is in preparation to support running connectivity tests in parallel using multiple namespaces (#2496). Signed-off-by: Michi Mutsuzaki <[email protected]>
viktor-kurchenko
force-pushed
the
pr/vk/conn/tests/concurrency
branch
from
44c3f3c
to
d0158de
Compare
viktor-kurchenko
force-pushed
the
pr/vk/conn/tests/concurrency
branch
from
d0158de
to
bdfbbc4
Compare
viktor-kurchenko
force-pushed
the
pr/vk/conn/tests/concurrency
branch
from
c118ec6
to
871489f
Compare
asauber
requested changes
May 10, 2024
asauber
requested changes
May 10, 2024
viktor-kurchenko
force-pushed
the
pr/vk/conn/tests/concurrency
branch
from
b3f8469
to
e011654
Compare
viktor-kurchenko
force-pushed
the
pr/vk/conn/tests/concurrency
branch
from
e011654
to
9fe19fb
Compare
viktor-kurchenko
force-pushed
the
pr/vk/conn/tests/concurrency
branch
from
9fe19fb
to
13e5f28
Compare
joestringer
approved these changes
May 13, 2024
There was a problem hiding this comment.
LGTM for my codeowners. I think the new policy lock retains the same assumptions we have today where only one piece of software should be manipulating policies at the time that you do the connectivity test (ie, only the CLI does it). And that should be good enough for now. It would be cool to explore better mechanisms for ensuring that datapath updates have occurred for a specific policy, but for now we don't really have a solution to that problem and it's not worth holding back this effort until we do that.
.github/workflows/kind.yaml
Outdated
Comment on lines
28
to
31
| fail-fast: false | ||
| matrix: | ||
| include: | ||
| # run connectivity tests explicitly without concurrency | ||
| - test-concurrency: 1 |
There was a problem hiding this comment.
Would it make sense to retain fail-fast for test-concurrency: 1 for quicker iteration and keep it off for higher concurrency?
There was a problem hiding this comment.
Good catch! I've disabled fail-fast for the debugging purpose.
I think it's better to remove it at all for the workflow.
viktor-kurchenko
force-pushed
the
pr/vk/conn/tests/concurrency
branch
from
13e5f28
to
7e58779
Compare
asauber
approved these changes
May 14, 2024
New `test-concurrency` input param added for the connectivity test command to provide namespace count for concurrent tests execution. The hidden flag will be removed after internal testing. Signed-off-by: viktor-kurchenko <[email protected]>
This PR introduces connectivity test concurrent run: - Parameters struct extended with ExternalDeploymentPort. - ConnectivityTest.Cleanup method implemented for instance reusability. - ConnectivityTest instantiated concerning test-concurrency input param. - GetTestSuites function implemented to return test suite functions. - Run function refactored to properly setup and run ConnectivityTest instances. Signed-off-by: viktor-kurchenko <[email protected]>
Package global policy mutex added to guarantee proper policy provisioning and deletion in case of concurrent run. Signed-off-by: viktor-kurchenko <[email protected]>
Signed-off-by: viktor-kurchenko <[email protected]>
The MultiError struct implemented and used instead of `hashicorp/go-multierror` library. The MultiError allows running many goroutine concurrently and returns a joined error if at least one goroutine returns an error. Signed-off-by: viktor-kurchenko <[email protected]>
The kind workflow improved with matrix to test `test-concurrency` parameter. Signed-off-by: viktor-kurchenko <[email protected]>
Signed-off-by: viktor-kurchenko <[email protected]>
The check.EchoServerHostPort constant moved into check.Parameters to make it unique for each test namespace in case of concurrent run. Signed-off-by: viktor-kurchenko <[email protected]>
ClusterMesh connectivity tests setup fixed to support test-concurrency param. Kind workflow (clustermesh part) updated with test-concurrency. Signed-off-by: viktor-kurchenko <[email protected]>
viktor-kurchenko
force-pushed
the
pr/vk/conn/tests/concurrency
branch
from
7e58779
to
9f98ef0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces connectivity test concurrent run.
The new
test-concurrencyinput param can be used to specify a count of K8S test namespaces.So, most connectivity tests will run concurrently across test namespaces.
The new parameter is marked
hiddenbecause of the following reasons:debugandverbosemodes). I want to address this in a separate PR.Command run example:
cilium connectivity test --test-concurrency=5Local test in a 4 node kind cluster result:
--test-concurrency=1--test-concurrency=5