Allow VM machine type to be set dynamically#407
Allow VM machine type to be set dynamically#407carterpewpew wants to merge 1 commit intokubevirt:mainfrom
Conversation
|
Hi @carterpewpew. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
@carterpewpew PR looks great, thanks! Just one nit about the commits. We generally prefer to avoid merge commits in the history. It’s better to just rebase and address the changes there. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alromeros The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This change removes the hardcoded VM machine type and replaces it with a configurable default. A new initialization step determines the machine type based on an environment variable, falling back to the system architecture when not explicitly set. VM creation paths have been updated to consistently use this default, ensuring behavior is uniform across tests. YAML generation has also been adjusted to dynamically inject the selected machine type, avoiding duplication and making configurations easier to override. Signed-off-by: Jathavedhan M <jathavedhan.m@ibm.com>
8d10e8d to
f20f85e
Compare
Thanks for the review and the feedback! I've rebased the branch to remove the merge commit. |
|
/test all |
What this PR does / why we need it:
Adds multi-architecture support for VM machine type in the test framework. Previously, the machine type was hardcoded to
q35, which is x86_64-specific. This caused all VM and VMI creation to be rejected by the KubeVirt admission webhook on s390x with:This PR:
InitDefaultMachineType()which auto-detects the cluster architecture from node info and sets the appropriate machine type (s390-ccw-virtiofor s390x,virtfor arm64,q35for amd64)KVP_DEFAULT_MACHINE_TYPE) for manual controlq35references in Go test specs with the detected defaultRunKubectlCreateYamlCommandto substitutetype: q35in YAML manifests at apply timeCreateVMWithPVCandCreateVMIWithDataVolumethrough the YAML substitution pipeline (they previously bypassed it via rawRunKubectlCommand)Validated on an s390x OpenShift cluster: machine type correctly detected as
s390-ccw-virtio, VMs created and reached Running state without admission webhook rejections.Which issue(s) this PR fixes:
Fixes test failures on s390x clusters where all VM-based tests fail due to hardcoded
q35machine type.Special notes for your reviewer:
tests/manifests/still containtype: q35as a placeholder. This is intentional --RunKubectlCreateYamlCommandrewrites it at apply time viased. Changing the YAML files themselves was avoided to keep the diff minimal and because the sed-based substitution was already the established pattern for{{KVP_STORAGE_CLASS}}.manifests_utils.gochanges forCreateVMWithPVCandCreateVMIWithDataVolumeare necessary because they were the only two VM/VMI-creating functions using rawRunKubectlCommandinstead ofRunKubectlCreateYamlCommand, meaning neither storage class nor machine type substitution was happening for them.Release note: