feat(virtctl): Add boot order support to create vm #9629

0xFelix · 2023-04-19T07:36:18Z

What this PR does / why we need it:

This adds a new param bootorder to flags of virtctl create vm,
which allows to specify the boot order of added volumes.

Support for fields of type *uint is added to params.go for this.
Also some refactorings are applied to virtctl create vm.

The param is added to the following flags:

volume-containerdisk
volume-datasource
volume-clone-pvc
volume-pvc

The method getInferFromVolume is added to vm.go to allow inferring
instancetypes or preferences from the disk with the lowest boot order or
if no boot order was specified the first volume.

Release note:

virtctl: Allow to specify the boot order of volumes when creating VMs

By moving the generation of the the default VM name to setDefaults() the usage output of virtctl create vm stays the same between runs. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

This moves the newVM func to be a method of createVM to be more consistent with the rest of the code. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

This adds support for *uint fields to params.go. It will be useful in the next commit. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

0xFelix · 2023-04-19T07:36:32Z

/cc @lyarwood

lyarwood · 2023-04-19T15:14:26Z

pkg/virtctl/create/vm/vm.go

+
+// getInferFromVolume returns the volume to infer the instancetype or preference from.
+// It returns either the disk with the lowest boot order or the first volume in the VM spec.
+func (c *createVM) getInferFromVolume(flag string, vm *v1.VirtualMachine) (string, error) {


This is cool but out of scope for the commit and PR right?

IMO it is in scope because without #9631 we always want the infer flags to infer from the volume the VM booted from. Without this method this isn't possible.

0xFelix · 2023-04-19T15:44:22Z

/retest

This adds a new param 'bootorder' which allows to specify the boot order of added volumes. The param is added to the following flags: - volume-containerdisk - volume-datasource - volume-clone-pvc - volume-pvc Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

The method getInferFromVolume is added to vm.go to allow inferring instancetypes or preferences from the disk with the lowest boot order or if no boot order was specified the first volume. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

lyarwood · 2023-04-19T15:58:33Z

/lgtm

0xFelix · 2023-04-20T09:14:33Z

/retest

kubevirt-bot · 2023-04-20T09:24:06Z

@0xFelix: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-fossa	`6c21c2f`	link	false	`/test pull-kubevirt-fossa`

Instructions 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/test-infra repository. I understand the commands that are listed here.

alicefr · 2023-04-21T06:52:00Z

@0xFelix do we specify values like not ordered like '2' and then '4' or values greater than the total number of disks?

alicefr · 2023-04-21T06:54:38Z

pkg/virtctl/create/vm/vm.go

@@ -243,6 +258,9 @@ func (c *createVM) usage() string {
  # Create a manifest for a VirtualMachine with a cloned DataSource and inferred instancetype and preference
  {{ProgramName}} create vm --volume-datasource=src:my-annotated-ds --infer-instancetype --infer-preference

+  # Create a manifest for a VirtualMachine with multiple volumes and specified boot order
+  {{ProgramName}} create vm --volume-containerdisk=src:my.registry/my-image:my-tag --volume-datasource=src:my-ds,bootorder:1


Can't we simply use the volume name instead on the need to specify the type of volume? The name should be unique

The flags are not only setting the boot order of a volume but also add the volume itself. So I don't think this can be simplified any more.

0xFelix · 2023-04-21T12:50:04Z

@alicefr As we discovered offline it is not necessary to order the boot orders or make sure the highest boot order does not exceed the count of disks.

The only required validations are:

boot orders must be > 0
boot orders must not be duplicated

The description here is correct: http://kubevirt.io/api-reference/v0.59.0/definitions.html#_v1_disk

alicefr · 2023-04-24T12:59:43Z

/approve

kubevirt-bot · 2023-04-24T12:59:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alicefr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-commenter-bot · 2023-04-24T15:00:23Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

0xFelix added 3 commits April 18, 2023 10:53

feat(virtctl): Keep usage output of virtctl create vm stable

b1021a2

By moving the generation of the the default VM name to setDefaults() the usage output of virtctl create vm stays the same between runs. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

cleanup(virtctl): Move newVM func to be a method of createVM

28c7b73

This moves the newVM func to be a method of createVM to be more consistent with the rest of the code. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

feat(virtctl,create): Add *uint support to params.go

af37330

This adds support for *uint fields to params.go. It will be useful in the next commit. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Apr 19, 2023

kubevirt-bot requested review from kbidarkar and vladikr April 19, 2023 07:36

kubevirt-bot added the size/L label Apr 19, 2023

kubevirt-bot requested a review from lyarwood April 19, 2023 07:36

0xFelix force-pushed the virtctl-create-vm-bootorder branch from 6e578ce to 25affe2 Compare April 19, 2023 09:10

0xFelix mentioned this pull request Apr 19, 2023

feat(virtctl): Allow to infer from specified volume in create vm #9631

Merged

lyarwood reviewed Apr 19, 2023

View reviewed changes

0xFelix added 2 commits April 19, 2023 17:52

0xFelix force-pushed the virtctl-create-vm-bootorder branch from 25affe2 to 6c21c2f Compare April 19, 2023 15:53

kubevirt-bot assigned lyarwood Apr 19, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2023

alicefr reviewed Apr 21, 2023

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2023

kubevirt-bot merged commit a8d1c0b into kubevirt:main Apr 24, 2023
35 of 36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(virtctl): Add boot order support to create vm #9629

feat(virtctl): Add boot order support to create vm #9629

0xFelix commented Apr 19, 2023 •

edited

0xFelix commented Apr 19, 2023

lyarwood Apr 19, 2023

0xFelix Apr 19, 2023

0xFelix commented Apr 19, 2023

lyarwood commented Apr 19, 2023 •

edited

0xFelix commented Apr 20, 2023

kubevirt-bot commented Apr 20, 2023 •

edited

alicefr commented Apr 21, 2023

alicefr Apr 21, 2023

0xFelix Apr 21, 2023

0xFelix commented Apr 21, 2023

alicefr commented Apr 24, 2023

kubevirt-bot commented Apr 24, 2023

kubevirt-commenter-bot commented Apr 24, 2023

feat(virtctl): Add boot order support to create vm #9629

feat(virtctl): Add boot order support to create vm #9629

Conversation

0xFelix commented Apr 19, 2023 • edited

0xFelix commented Apr 19, 2023

lyarwood Apr 19, 2023

Choose a reason for hiding this comment

0xFelix Apr 19, 2023

Choose a reason for hiding this comment

0xFelix commented Apr 19, 2023

lyarwood commented Apr 19, 2023 • edited

0xFelix commented Apr 20, 2023

kubevirt-bot commented Apr 20, 2023 • edited

alicefr commented Apr 21, 2023

alicefr Apr 21, 2023

Choose a reason for hiding this comment

0xFelix Apr 21, 2023

Choose a reason for hiding this comment

0xFelix commented Apr 21, 2023

alicefr commented Apr 24, 2023

kubevirt-bot commented Apr 24, 2023

kubevirt-commenter-bot commented Apr 24, 2023

0xFelix commented Apr 19, 2023 •

edited

lyarwood commented Apr 19, 2023 •

edited

kubevirt-bot commented Apr 20, 2023 •

edited