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.

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

kind, check-cluster-up: Enable Kubevirt CPUManager FG when SR-IOV provider is tested #1348

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+5 −0
Open

kind, check-cluster-up: Enable Kubevirt CPUManager FG when SR-IOV provider is tested #1348

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cluster-up/cluster/kind/check-cluster-up.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ export CRI_BIN=${CRI_BIN:-$(detect_cri)}
fi
${kubectl} wait -n kubevirt kv kubevirt --for condition=Available --timeout 15m

if [[ "$KUBEVIRT_PROVIDER" =~ "sriov" ]]; then
# Some SR-IOV tests require Kubevirt CPUManager feature
${kubectl} patch kubevirts -n kubevirt kubevirt --type=json -p='[{"op": "replace", "path": "/spec/configuration/developerConfiguration/featureGates","value": ["CPUManager"]}]'
Copy link
Member

Choose a reason for hiding this comment

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

Please consider adding a feature gate to the end of the existing list, instead of replacing the whole list, as it will enable future expansion.

Copy link
Contributor Author

@ormergi ormergi Jan 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In case additional FG should be enabled I think there should still be a single patch call with all necessary FGs.
The FG names can be aggregated and then passed to the patch call.
I didnt exported the FG name to var because its the only one at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

IMO this should not come as a hard dependency of the SR-IOV provider.
Do you see a problem with directly marking the need to have CPUManager as input from the caller?

Also, the patching is odd to me too.

  • Why do you use replace and not a simple add?
  • Why do you think it is better to assume there is only one FG? It will just make it harder for the next contributor to add other FGs in general.

Copy link
Contributor Author

@ormergi ormergi Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I dont see problem with setting CPUManager feature to always on, in fact in kubevirt/kubevirt its always on.
We can go with that. Let me know what you think.
EDIT: In kubevirt/kubevirt tests, CPUManager FG is enabled unless the env architecture is s390x (it used to be always on), it might be necessary following the same logic here.
I also updated the PR title & description to express that it affects SR-IOV provider only.

Regarding the patch, I used "replace" because "add" didn't work for me in away I can add FG with one-liner.
When Kubevirt CR developerConfiguration.featureGates is not initialized, it will require patch for initializing it and another one for adding the FG.
Using "replace" the way I did enable having one-liner.

Passing the provider name is the most simple way I could find to solve it and get the lane gating as soon as possible.
We can introduce some additional env var to hold a FG list, on a follow up PR in case it will be needed (I tried to keep things simple).

Copy link
Contributor

@dhiller dhiller Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@ormergi I am not convinced that here is the right place to patch kubevirt? IMHO it should go into https://github.com/kubevirt/kubevirt/tree/main/hack/cluster-deploy.sh . WDYT?

EDIT: My reasoning behind that is that kubevirt is not a component of kubevirtci.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brianmcarey FYI ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhiller Changing deploy-cluster.sh affect cluster-sync flow, I am not sure its we should do that.
Please note "check-up-kind-sriov" calls "kind/check-cluster-up.sh" directly, and "kind/check-cluster-up.sh" deploy kubevirt (from nightly release yamls).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhiller WDYT?

fi

echo "Run latest nighly build Kubevirt conformance tests"
kubevirt_plugin="--plugin ${nightly_build_base_url}/${latest}/conformance.yaml"
SONOBUOY_EXTRA_ARGS="${SONOBUOY_EXTRA_ARGS} ${kubevirt_plugin}"
Expand Down