kind,check-cluster-up: Enable Kubevirt CPUManager FG when env supports it #1348
Conversation
kubevirt-bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test check-up-kind-sriov |
ormergi
force-pushed
the
check-kind-up-cpumanager
branch
from
3476f78 to
ae4e2e5
Compare
ormergi
force-pushed
the
check-kind-up-cpumanager
branch
from
ae4e2e5 to
92b836a
Compare
Signed-off-by: Or Mergi <[email protected]>
ormergi
force-pushed
the
check-kind-up-cpumanager
branch
from
92b836a to
6a5991f
Compare
|
SR-IOV lane is green and ran the tests that failed all over https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirtci/1348/check-up-kind-sriov/1880962417102426112 |
… constantly failing (kubevirt#3878)" This reverts commit 86cf6b7. The PR kubevirt/kubevirtci#1348 fixes the issue and stabilize the lane. Signed-off-by: Or Mergi <[email protected]>
ormergi
changed the title
| @@ -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"]}]' | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
replaceand not a simpleadd? - 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.
Done |
|
/test check-up-kind-sriov |
Thank you. |
What this PR does / why we need it:
The "check-up-kind-sriov" is turned optional and not gating because it constantly failing, due to following tests failures:
The mentioned tests causing the lane to fail following removal of programmatic skips in kubevirt/kubevirt tests kubevirt/kubevirt#13144, affecting the mentioned tests.
Previously the mentioned tests were skipped silently (bad) and now, following the programmatic skip removal, fail loudly.
The root cause for the failures (or previous skips) is tests depends on Kubevirt's CPUManager feature but its not enabled at all, see below notes section for more details *.
This PR fixes the lane by enabling Kubevirt's CPUManager features for environments that supports it, i.e.: when SR-IOV provider is used (the SR-IOV provider create cluster with Kubernetes CPU manager on).
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
The failing tests, creates VMs with dedicated-CPUs option, Kubevirt will label such VM's virt-launcher pod with node-selector signifying
cpumanager=truelabel.The end result is the tested VMs fail to become ready on time due to impossible scheduling; VMs has
cpumanager=turenode selector, but no node hascpumanager=truelabel.Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: