Add "virtctl create clone" command #9596

iholder101 · 2023-04-10T08:54:27Z

What this PR does / why we need it:
This PR adds a virtctl create clone command.

Here's the full usage details:

> ./cluster-up/virtctl.sh create clone --help
Create a clone object manifest

Usage:
  virtctl create clone [flags]

Examples:
  # Create a manifest for a clone with a random name:
  virtctl create clone --source-name sourceVM --target-name targetVM
  
  # Create a manifest for a clone with a specified name:
  virtctl create clone --name my-clone --source-name sourceVM --target-name targetVM

  # Create a manifest for a clone with a randomized target name (target name is omitted):
  virtctl create --source-name sourceVM

  # Create a manifest for a clone with specified source / target types. The default type is VM.
  virtctl create clone --source-name sourceVM --source-type vm --target-name targetVM --target-type vm

  # Create a manifest for a clone with a source type snapshot to a target type VM:
  virtctl create clone --source-name mySnapshot --source-type snapshot --target-name targetVM
  
  # Create a manifest for a clone with label filters:
  virtctl create clone --source-name sourceVM --label-filter "*" --label-filter "!some/key" 
  
  # Create a manifest for a clone with annotation filters:
  virtctl create clone --source-name sourceVM --annotation-filter "*" --annotation-filter "!some/key"

  # Create a manifest for a clone with new MAC addresses:
  virtctl create clone --source-name sourceVM --new-mac-address interface1:00-11-22 --new-mac-address interface2:00-11-33

  # Create a manifest for a clone with new SMBIOS serial:
  virtctl create clone --source-name sourceVM --new-smbios-serial "new-serial"

  # Create a manifest for a clone and use it to create a resource with kubectl virt
  virtctl create clone --source-name sourceVM | kubectl create -f -

Flags:
      --annotation-filter stringArray   Specify clone's annotation filters. Can be provided multiple times.
  -h, --help                            help for clone
      --label-filter stringArray        Specify clone's label filters. Can be provided multiple times.
      --name string                     Specify the name of the clone. If not specified, name would be randomized.
      --new-mac-address stringArray     Specify clone's new mac addresses. For example: 'interfaceName0:newAddress0'
      --new-smbios-serial string        Specify the clone's new smbios serial
      --source-name string              Specify the clone's source name.
      --source-type string              Specify the clone's source type. Default type is VM. Supported types: vm, vmsnapshot
      --target-name string              Specify the clone's target name.
      --target-type string              Specify the clone's target type. Default type is VM. Supported types: vm

Release note:

Add "virtctl create clone" command

iholder101 · 2023-04-10T08:55:35Z

/cc @fabiand @lyarwood @0xFelix

jean-edouard

/approve

kubevirt-bot · 2023-04-10T17:37:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jean-edouard

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~~ [jean-edouard]

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

alicefr · 2023-04-11T07:20:17Z

pkg/virtctl/create/clone/clone.go

+  {{ProgramName}} create clone --name my-clone --source sourceVM --target targetVM
+
+  # The above is a shortcut to:
+  {{ProgramName}} create clone --name my-clone --source name:sourceVM,apiGroup:kubevirt.io,kind:VirtualMachine --target name:targetVM,apiGroup:kubevirt.io,kind:VirtualMachine


Why do we need to specify the apiGroup and kind:VirtualMachine? Couldn't we simply specify an additional flag --type and there you map type to the k8s object? This version seems a bit lengthy and not very straightforward to me.

IMO, it will be nice to understand from the help option which kind of k8s objects I can clone

Why do we need to specify the apiGroup and kind:VirtualMachine? Couldn't we simply specify an additional flag --type and there you map type to the k8s object? This version seems a bit lengthy and not very straightforward to me.

TBH I'm in a dilemma here :)

The rationale for the flags as they are now is that it's fully generic in the sense that when the clone API would expand and change in the future, for example by supporting more source and target types, virtctl create clone command wouldn't have to be changed. If we would go with something like --type, then every supported type would need to be added to the mapping once supported, which might lead to inconsistencies and for harder time when expanding the API.

In addition, there's a need to specify a type for both the source and the target, which might be of different types. For example, the source could be a vmSnapshot while the target is a VM. TBH, I'm not sure how this can be mapped by order ,e.g. virtctl create clone --source sourceSnapshot --type snapshot --target targetVM --type vm, how can we know that the first --type refers to the source in this case?

Instead, we can simply omit the apiGroup and map it out automatically, e.g. virtctl create clone --source name:sourceSnapshot,type:snapshot --target name:targetVM,type:vm, but this is not a lot nicer and suffers from the disadvantage I laid out above.

Last point is that at least I've made VMs as the default, so cloning VMs can be done much for elegantly (virtctl create clone --source sourceVM --target targetVM).

But on the other hand - you're right - it is lengthy as for now.
What do you think? I'd appreciate your feedback here.

IMO, it will be nice to understand from the help option which kind of k8s objects I can clone

Same concern as above. While I think this is very valuable, I'm a bit concerned about not having one single source of truth. Updating in many places once a new type is supported gets me a bit uncomfortable.

WDYT about adding a link to the relevant user-guide entry instead?

In addition, there's a need to specify a type for both the source and the target, which might be of different types. For example, the source could be a vmSnapshot while the target is a VM. TBH, I'm not sure how this can be mapped by order ,e.g. virtctl create clone --source sourceSnapshot --type snapshot --target targetVM --type vm, how can we know that the first --type refers to the source in this case?

For this, if we can avoid the lengthy option, I still think a --type-source and --type-target are still more elegant :). If nothing is specified, you can always assume VirtualMachine.

The rationale for the flags as they are now is that it's fully generic in the sense that when the clone API would expand and change in the future, for example by supporting more source and target types, virtctl create clone command wouldn't have to be changed. If we would go with something like --type, then every supported type would need to be added to the mapping once supported, which might lead to inconsistencies and for harder time when expanding the API.

Ok, yes this is the real question. Do you expect many k8s object types to be snapshottable/clonable?
Additionally, if we have control of the type, we could validate a certain field. At least, here you are adding a new mac address option. This flag is only valid if the target is a VM. I'm not sure if the target (at least) can be an arbitrary k8s object. Same thing for the smbios

IMO, it will be nice to understand from the help option which kind of k8s objects I can clone

Same concern as above. While I think this is very valuable, I'm a bit concerned about not having one single source of truth. Updating in many places once a new type is supported gets me a bit uncomfortable.

WDYT about adding a link to the relevant user-guide entry instead?

Well, if you have it in the code, for me that's the most reliable source of the truth :). I'd avoid putting links, they get outdated quickly. Let's try to understand what we prefer if an implicit or explicit list of the clonable types :).
I don't want necessarily block this PR, only understand the reasoning behind

Ok, yes this is the real question. Do you expect many k8s object types to be snapshottable?

What I currently have in mind is:

VMI should be supported as both source and target types

We were once talking about supporting PVC as the source types. The main idea was that a VM state could reside in the PVC in some format, along with the VM manifest and perhaps a configuration file. We can also utilize the Export API for that.

It's hard to know which more object would need to be supported in the future. For example, maybe a vmpool could be cloned too?

But you're right, maybe we're not talking about a lot of new supported types.

I think I'm convinced :)
Thank you for this discussion

It's hard to know which more object would need to be supported in the future. For example, maybe a vmpool could be cloned too?

Maybe but if we really realize that the list starts to grow too much and we want to give more flexibility, we can always add an arbitrary type with the logic you currently have, but I would avoid it if not necessary

alicefr · 2023-04-11T07:26:09Z

pkg/virtctl/create/clone/clone_test.go

+				flags = addFlag(flags, clone.TargetFlag, targetVM)
+			}
+
+			expectErr := !addSourceFlag || !addTargetFlag


nit: I'd simply move this condition in the if statement below for readability

done :)
thanks!

alicefr · 2023-04-11T09:44:44Z

/hold
Putting on hold to address the comments in #9596 (comment)

iholder101 · 2023-04-11T11:27:47Z

/hold Putting on hold to address the comments in #9596 (comment)

It's now addressed :)
Can you PTAL?

alicefr · 2023-04-11T11:56:03Z

pkg/virtctl/create/clone/clone.go

+
+	mustBeSpecifiedErrFmt = "%s must be specified"
+
+	supportedSourceTypes = "vm, snapshot"


nit: I'd use vmsnapshot to differentiate from a snapshot of a pvc

alicefr · 2023-04-11T12:07:11Z

pkg/virtctl/create/clone/clone.go

+  {{ProgramName}} create clone --source-name sourceVM --annotation-filter "*" --annotation-filter "!some/key"
+
+  # Create a manifest for a clone with new MAC addresses:
+  {{ProgramName}} create clone --source-name sourceVM --new-mac-address interfaceName:interface1,newMacAddress:00-11-22 --new-mac-address interfaceName:interface2,newMacAddress:00-11-33


nit: a possible alternative could be to drop the param name like: --new-mac-address interface1:00-11-22 --new-mac-address interface2:00-11-33 . Do we already have some similar options in virtctl?

Great advice. Thanks!
done

alicefr · 2023-04-11T12:09:48Z

@iholder101 it looks good to me, I let a couple of nit comments, feel free to ignore them if you like. Just remove the hold when you feel ready.

/lgtm

Signed-off-by: Itamar Holder <iholder@redhat.com>

Also, replace literal lists with nil ones. Signed-off-by: Itamar Holder <iholder@redhat.com>

This file uses reflection and isn't straight-forward to figure out. I've added documentation so that it would be easier for others to understand. Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 · 2023-04-11T13:22:45Z

@iholder101 it looks good to me, I let a couple of nit comments, feel free to ignore them if you like. Just remove the hold when you feel ready.

/lgtm

Addressed your comments.
Would you mind re-LGTMing? Thanks a lot for your fruitful review!

/unhold

alicefr · 2023-04-11T13:33:06Z

/lgtm

iholder101 · 2023-04-11T14:05:54Z

/test pull-kubevirt-prom-rules-verify

kubevirt-bot · 2023-04-11T14:17:01Z

@iholder101: The following tests 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	`7a9ef38`	link	false	`/test pull-kubevirt-fossa`
pull-kubevirt-e2e-arm64	`7a9ef38`	link	false	`/test pull-kubevirt-e2e-arm64`

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.

lyarwood

Apologies I sat on these review comments for most of the day, missing that this had already been approved. I've got a few nits and comments, all addressable in follow ups if you think they are worth it.

lyarwood · 2023-04-11T16:41:42Z

pkg/virtctl/create/clone/clone.go

+
+	cmd := &cobra.Command{
+		Use:     Clone,
+		Short:   "Create a clone object manifest",


nit - VirtualMachineClone

Please see #9596 (comment)

lyarwood · 2023-04-11T16:44:34Z

pkg/virtctl/create/clone/clone.go

+	}
+
+	switch sourceOrTargetType {
+	case "vm", "VM", "VirtualMachine", "virtualmachine":


nit - Could we document these in the help output somehow? You're only listing vm and vmsnapshot at the moment.

Will do in a follow-up PR 👍

lyarwood · 2023-04-11T16:49:53Z

pkg/virtctl/create/clone/clone_test.go

+			Entry("snapshot source, vm target", "snapshot", snapshotKind, snapshotApiGroup, "vm", vmKind, vmApiGroup),
+			Entry("VirtualMachineSnapshot source, vm target", "VirtualMachineSnapshot", snapshotKind, snapshotApiGroup, "vm", vmKind, vmApiGroup),
+			Entry("vmsnapshot source, vm target", "vmsnapshot", snapshotKind, snapshotApiGroup, "vm", vmKind, vmApiGroup),
+			Entry("VMSnapshot source, vm target", "VMSnapshot", snapshotKind, snapshotApiGroup, "vm", vmKind, vmApiGroup),


A snapshot source and target isn't valid right? AFAICT that isn't being rejected by the command at least at the moment, seems like an easy win.

It is being rejected here.

Do you think we need another DescribeTable for source/target kinds that are invalid?

I think that would be useful coverage yeah.

lyarwood · 2023-04-11T16:53:33Z

pkg/virtctl/create/clone/clone.go

+)
+
+const (
+	Clone = "clone"


supernit - this is more of a nit against the naming of the CRD itself but this command creates VirtualMachineClone manifests. create clone works as a command here, it's just a shame we didn't call the actual resource Clone IMHO given the generic type support you included.

it's just a shame we didn't call the actual resource Clone

I couldn't agree more.
Actually, I still plan to chance the CRD's name one day (this can be easily done by now since the API is still alpha). That's the reason in this PR I decided to refer to it as a "clone object" instead of the full CRD name.

Thanks for raising this up!

0xFelix

Added some comments which could be addressed in a follow up.

Good work! 👍

0xFelix · 2023-04-18T07:54:17Z

pkg/virtctl/create/clone/clone.go

+	const emptyValue = ""
+	const supportsMultipleFlags = "Can be provided multiple times."
+
+	cmd.Flags().StringVar(&c.name, NameFlag, emptyValue, "Specify the name of the clone. If not specified, name would be randomized.")


Instead of using emptyValue you could use the corresponding struct field (e.g. c.name). If not initialized otherwise it is already empty.

TBH, I don't agree that this makes the situation better. From my perspective, emptyValue (or nil) as a default value is much clearer than providing an uninitialized struct field. It's confusing.

In addition, in your suggested approach we rely on the fact that the fields are indeed uninitialized. That's an assumption that might lead to bugs in the future if the assumption changes for some reason.

Is there any added value to your approach that I'm missing? WDYT?

It's useful e.g. when you want to provide default values from a function which initializes the struct. But in your case it does not add that much value so let's keep it for now.

0xFelix · 2023-04-18T07:55:26Z

pkg/virtctl/create/clone/clone.go

+	cmd.Flags().StringVar(&c.sourceType, SourceTypeFlag, emptyValue, "Specify the clone's source type. Default type is VM. Supported types: "+supportedSourceTypes)
+	cmd.Flags().StringVar(&c.targetType, TargetTypeFlag, emptyValue, "Specify the clone's target type. Default type is VM. Supported types: "+supportedTargetTypes)
+	cmd.Flags().StringArrayVar(&c.labelFilters, LabelFilterFlag, nil, "Specify clone's label filters. "+supportsMultipleFlags)
+	cmd.Flags().StringArrayVar(&c.annotationFilters, AnnotationFilterFlag, nil, "Specify clone's annotation filters. "+supportsMultipleFlags)


Similar to above: nil vs. c.annotationFilters

0xFelix · 2023-04-18T07:56:04Z

pkg/virtctl/create/clone/clone.go

+	cmd.Flags().StringArrayVar(&c.newMacAddresses, NewMacAddressesFlag, nil, "Specify clone's new mac addresses. For example: 'interfaceName0:newAddress0'")
+	cmd.Flags().StringVar(&c.newSmbiosSerial, NewSMBiosSerialFlag, emptyValue, "Specify the clone's new smbios serial")
+
+	_ = cmd.MarkFlagRequired(SourceNameFlag)


What about panicking instead of ignoring the error?

0xFelix · 2023-04-18T07:57:07Z

pkg/virtctl/create/clone/clone.go

+}
+
+func withNewMacAddresses(c *createClone, cloneSpec *cloneSpec) error {
+	const flag = NewMacAddressesFlag


Unused const?

0xFelix · 2023-04-18T07:58:06Z

pkg/virtctl/create/clone/clone.go

+  {{ProgramName}} create clone --name my-clone --source-name sourceVM --target-name targetVM
+
+  # Create a manifest for a clone with a randomized target name (target name is omitted):
+  {{ProgramName}} create --source-name sourceVM


Missing clone after create.

0xFelix · 2023-04-18T07:59:58Z

pkg/virtctl/create/clone/clone.go

+  {{ProgramName}} create clone --source-name sourceVM --label-filter "*" --label-filter "!some/key" 
+
+  # Create a manifest for a clone with annotation filters:
+  {{ProgramName}} create clone --source-name sourceVM --annotation-filter "*" --annotation-filter "!some/key"


! as arg might result in unwanted behavior by the user's shell. I would recommend using single quotes ('!some/key') to make sure the command works as expected.

0xFelix · 2023-04-18T08:06:59Z

pkg/virtctl/create/clone/clone_test.go

+		)
+	})
+
+	It("new mac addresses", func() {


A test case for a wrong arg to the new mac addresses flag would be good.

0xFelix · 2023-04-18T08:07:21Z

pkg/virtctl/create/clone/clone_test.go

+		})
+	})
+
+	Context("source and target", func() {


You could add a negative test case for an empty source name.

0xFelix · 2023-04-18T08:08:05Z

pkg/virtctl/create/instancetype/instancetype.go

@@ -96,8 +96,8 @@ func NewCommand() *cobra.Command {
 	cmd.Flags().StringArrayVar(&c.gpus, GPUFlag, c.gpus, "Specify the list of vGPUs to passthrough. Can be provided multiple times.")
 	cmd.Flags().StringArrayVar(&c.hostDevices, HostDeviceFlag, c.hostDevices, "Specify list of HostDevices to passthrough. Can be provided multiple times.")

-	cmd.MarkFlagRequired(CPUFlag)
-	cmd.MarkFlagRequired(MemoryFlag)
+	_ = cmd.MarkFlagRequired(CPUFlag)


Panick here too?

iholder101 · 2023-04-20T15:44:45Z

Hey @0xFelix and @lyarwood!
Here's a PR to address your comments: #9637.

Thanks for your reviews!

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. size/XL labels Apr 10, 2023

kubevirt-bot requested review from acardace and stu-gott April 10, 2023 08:54

kubevirt-bot requested review from 0xFelix, fabiand and lyarwood April 10, 2023 08:55

jean-edouard reviewed Apr 10, 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 10, 2023

alicefr reviewed Apr 11, 2023

View reviewed changes

iholder101 force-pushed the virtctl_create/clone branch from e0b984e to 22d779d Compare April 11, 2023 08:02

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 11, 2023

iholder101 force-pushed the virtctl_create/clone branch 2 times, most recently from f26347d to eb0a920 Compare April 11, 2023 11:24

iholder101 force-pushed the virtctl_create/clone branch from eb0a920 to d6c55f0 Compare April 11, 2023 11:30

alicefr reviewed Apr 11, 2023

View reviewed changes

kubevirt-bot assigned alicefr Apr 11, 2023

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

iholder101 added 4 commits April 11, 2023 16:18

Introduce virtctl create clone command

8422c9b

Signed-off-by: Itamar Holder <iholder@redhat.com>

Unit test: virtctl create clone

dc89863

Signed-off-by: Itamar Holder <iholder@redhat.com>

Refactor: remove unused params, add explcit error dismissing

260ca3e

Also, replace literal lists with nil ones. Signed-off-by: Itamar Holder <iholder@redhat.com>

Document params.go

7a9ef38

This file uses reflection and isn't straight-forward to figure out. I've added documentation so that it would be easier for others to understand. Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 force-pushed the virtctl_create/clone branch from d6c55f0 to 7a9ef38 Compare April 11, 2023 13:19

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

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 11, 2023

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

kubevirt-bot merged commit 115e71f into kubevirt:main Apr 11, 2023

lyarwood reviewed Apr 11, 2023

View reviewed changes

0xFelix reviewed Apr 18, 2023

View reviewed changes

iholder101 mentioned this pull request Apr 20, 2023

virtctl create clone: minor improvements #9637

Merged

enp0s3 mentioned this pull request Jun 13, 2023

nominate iholder101 as core approver #9900

Merged


		mustBeSpecifiedErrFmt = "%s must be specified"

		supportedSourceTypes = "vm, snapshot"

Add "virtctl create clone" command #9596

Add "virtctl create clone" command #9596

Conversation

iholder101 commented Apr 10, 2023 • edited

iholder101 commented Apr 10, 2023

jean-edouard left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Apr 10, 2023

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

alicefr Apr 11, 2023 • edited

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

alicefr commented Apr 11, 2023

iholder101 commented Apr 11, 2023

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

alicefr commented Apr 11, 2023

iholder101 commented Apr 11, 2023

alicefr commented Apr 11, 2023

iholder101 commented Apr 11, 2023

kubevirt-bot commented Apr 11, 2023 • edited

lyarwood 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

lyarwood Apr 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xFelix 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

iholder101 commented Apr 20, 2023

iholder101 commented Apr 10, 2023 •

edited

alicefr Apr 11, 2023 •

edited

kubevirt-bot commented Apr 11, 2023 •

edited

lyarwood Apr 11, 2023 •

edited