Make Cilium CLI performance tests not depend on Cilium #38245
Merged
Conversation
This file contains hidden or 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
|
/test |
|
/scale-egw |
Artyop
approved these changes
Mar 18, 2025
The blamed commit extended the CLI performance tests to support measuring the throughput with multiple parallel streams. However, the test name is longer than the maximum length previously considered when pretty-printing the summary of the results, hence causing the table to not be aligned. Let's get this fixed. Fixes: dc5ceda ("cli/perf: add throughput tests with multiple streams") Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Move the namespace-annotations parameter to the set of common flags, given that it is not specific to the connectivity tests only. Additionally, let's convert it to a plain map of strings, rather than json format, for ease of configuration. This change does not raise backward-compatibility concerns given that the flag is hidden. While being there, let's also mark the deployment-pod-annotations flag hidden both for the test and perf commands, which seems to have been lost when moving the flag to the common list. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Introduce a new namespace-labels flag which allows to configure extra labels applied to the test namespace(s). Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Generalize the performance tests initialization phase to not require Cilium to be running on the target cluster to simplify reuse, given that the measurements are not Cilium specific. While being there, let's also extract perf-specific logic into separate methods, for better separation. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
…ilium" This reverts commit 0fedeef. This workaround is no longer necessary now that the initialization of the performance tests only sets up the bits that don't depend on Cilium being running in the target cluster. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
6018f57 to
4886c8f
Compare
|
@giorio94 sorry Marco I missclicked the rebase |
|
/test |
|
@asauber Gentle ping 🙏 |
eb14604 to
4886c8f
Compare
|
Accidentally pushed extra commits intended for a follow-up PR. Reverted to the previous state now. |
|
/test |
pchaigno
approved these changes
Mar 21, 2025
marseel
reviewed
Mar 21, 2025
Yeah, makes sense. I would avoid going through CI once more for that only change though, unless anything else needs to addressed as well. |
|
@cilium/cli Gentle ping 🙏 |
mhofstetter
reviewed
Mar 21, 2025
mhofstetter
approved these changes
Mar 21, 2025
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.
Generalize the performance tests initialization phase to not require Cilium to be running on the target cluster to simplify reuse, given that the measurements are not Cilium specific. Additionally, let's improve configurability of test namespace labels and annotations.