feat(virtctl,create): Add support for sysprep volumes #13053

0xFelix · 2024-10-11T11:51:20Z

What this PR does

This adds support for specifying a sysprep volume to virtctl create vm by
adding the --sysprep flag. A test case for specifying a sysprep volume is
added to the virtctl create vm functests.

Before this PR:

Users cannot specify a sysprep volume in VMs created with virtctl create vm.

After this PR:

Users can now specify a sysprep volume in VMs created with virtctl create vm.

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

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.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

virtctl: Users can specify a sysprep volume in VMs created with virtctl create vm

fossedihelm

Looks good overall.
I'm wondering what's the added value of the e2e test actually

fossedihelm · 2024-10-11T12:23:10Z

pkg/virtctl/create/vm/vm.go

+	SysprepVolumeFlag          = "volume-sysprep"
+
+	SysprepDisk      = "sysprepdisk"
+	SysprepConfigMap = "configMap"
+	SysprepSecret    = "secret"


I wonder why all these const are exported...

To make them reusable in vm_test.go.

e2e test should not use the productions const.
If something changes in the production code, we have to manually adjust the e2e test as well, without automatically inheriting them.
We should start decoupling them.

vm_test.go is the unit test file of vm.go.

Right! But I see it is also used in tests/create_vm file

Right! Well, we don't have any strict rules about this atm. But we should consider adding some to the coding conventions. For now I'd like to keep it.

@0xFelix @fossedihelm FYI #13058

Right, updated this PR to decouple SysprepDisk in production and test code. Will update other places in virtctl in a follow up.

fossedihelm · 2024-10-11T12:32:04Z

tests/virtctl/create_vm.go

+		Expect(vm.Spec.RunStrategy).ToNot(BeNil())
+		Expect(*vm.Spec.RunStrategy).To(Equal(runStrategy))


nit, use PointTo instead of not be nil and check equality:

Suggested change

Expect(vm.Spec.RunStrategy).ToNot(BeNil())

Expect(*vm.Spec.RunStrategy).To(Equal(runStrategy))

Expect(vm.Spec.RunStrategy).To(PointTo...

fossedihelm · 2024-10-11T12:32:20Z

tests/virtctl/create_vm.go

+		Expect(vm.Spec.Template.Spec.TerminationGracePeriodSeconds).ToNot(BeNil())
+		Expect(*vm.Spec.Template.Spec.TerminationGracePeriodSeconds).To(Equal(terminationGracePeriod))


same as above

0xFelix · 2024-10-11T12:41:23Z

I'm wondering what's the added value of the e2e test actually

To verify that the VM admitter is happy with the generated manifest. It uses DryRun so it does not create an actual VM.

fossedihelm · 2024-10-11T13:47:14Z

/lgtm

alicefr · 2024-10-14T11:11:38Z

Looks good
/approve

kubevirt-bot · 2024-10-14T11:11:47Z

[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:

~~pkg/virtctl/OWNERS~~ [alicefr]
~~tests/OWNERS~~ [alicefr]

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

fossedihelm · 2024-10-14T13:12:40Z

/lgtm
Thanks

kubevirt-commenter-bot · 2024-10-14T13:12:47Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.30-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator
/test pull-kubevirt-e2e-k8s-1.30-sig-network
/test pull-kubevirt-e2e-k8s-1.30-sig-storage
/test pull-kubevirt-e2e-k8s-1.30-sig-compute
/test pull-kubevirt-e2e-k8s-1.30-sig-operator

Add support for specifying a sysprep volume to virtctl create vm by adding the --sysprep flag. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

Add a test case for specifying a sysprep volume to the virtctl create vm functests. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

jcanocan · 2024-10-15T08:59:03Z

/lgtm

kubevirt-commenter-bot · 2024-10-15T08:59:09Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.30-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator
/test pull-kubevirt-e2e-k8s-1.30-sig-network
/test pull-kubevirt-e2e-k8s-1.30-sig-storage
/test pull-kubevirt-e2e-k8s-1.30-sig-compute
/test pull-kubevirt-e2e-k8s-1.30-sig-operator

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 Oct 11, 2024

kubevirt-bot requested review from kbidarkar and lyarwood October 11, 2024 11:51

kubevirt-bot added area/virtctl size/L labels Oct 11, 2024

dosubot bot added kind/enhancement sig/virtualization labels Oct 11, 2024

0xFelix force-pushed the virtctl-create-vm-6 branch from e6461a6 to 20bea69 Compare October 11, 2024 11:54

fossedihelm reviewed Oct 11, 2024

View reviewed changes

0xFelix force-pushed the virtctl-create-vm-6 branch from 20bea69 to 364a9d4 Compare October 11, 2024 12:57

kubevirt-bot assigned fossedihelm Oct 11, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2024

0xFelix force-pushed the virtctl-create-vm-6 branch from 364a9d4 to 4f0e1b9 Compare October 14, 2024 10:16

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2024

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2024

0xFelix added 2 commits October 15, 2024 10:13

feat(virtctl,create): Add support for sysprep volumes

727f2fe

Add support for specifying a sysprep volume to virtctl create vm by adding the --sysprep flag. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

feat(tests,virtctl): Add test for create vm with sysprep volume

6057cb0

Add a test case for specifying a sysprep volume to the virtctl create vm functests. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

0xFelix force-pushed the virtctl-create-vm-6 branch from 4f0e1b9 to 6057cb0 Compare October 15, 2024 08:19

kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 15, 2024

kubevirt-bot assigned jcanocan Oct 15, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2024

kubevirt-bot merged commit 170b7f3 into kubevirt:main Oct 15, 2024
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(virtctl,create): Add support for sysprep volumes #13053

feat(virtctl,create): Add support for sysprep volumes #13053

0xFelix commented Oct 11, 2024

fossedihelm left a comment

fossedihelm Oct 11, 2024

0xFelix Oct 11, 2024

fossedihelm Oct 11, 2024

0xFelix Oct 11, 2024

fossedihelm Oct 11, 2024

0xFelix Oct 11, 2024

enp0s3 Oct 13, 2024

0xFelix Oct 14, 2024

fossedihelm Oct 11, 2024

0xFelix Oct 11, 2024

fossedihelm Oct 11, 2024

0xFelix commented Oct 11, 2024

fossedihelm commented Oct 11, 2024

alicefr commented Oct 14, 2024

kubevirt-bot commented Oct 14, 2024

fossedihelm commented Oct 14, 2024

kubevirt-commenter-bot commented Oct 14, 2024

jcanocan commented Oct 15, 2024

kubevirt-commenter-bot commented Oct 15, 2024

		Expect(vm.Spec.RunStrategy).ToNot(BeNil())
		Expect(*vm.Spec.RunStrategy).To(Equal(runStrategy))

	Expect(vm.Spec.RunStrategy).ToNot(BeNil())
	Expect(*vm.Spec.RunStrategy).To(Equal(runStrategy))
	Expect(vm.Spec.RunStrategy).To(PointTo...

		Expect(vm.Spec.Template.Spec.TerminationGracePeriodSeconds).ToNot(BeNil())
		Expect(*vm.Spec.Template.Spec.TerminationGracePeriodSeconds).To(Equal(terminationGracePeriod))

feat(virtctl,create): Add support for sysprep volumes #13053

feat(virtctl,create): Add support for sysprep volumes #13053

Conversation

0xFelix commented Oct 11, 2024

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

fossedihelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xFelix commented Oct 11, 2024

fossedihelm commented Oct 11, 2024

alicefr commented Oct 14, 2024

kubevirt-bot commented Oct 14, 2024

fossedihelm commented Oct 14, 2024

kubevirt-commenter-bot commented Oct 14, 2024

jcanocan commented Oct 15, 2024

kubevirt-commenter-bot commented Oct 15, 2024