virctl: Add dry-run option to addvolume and removevolume commands

Signed-off-by: fossedihelm <[email protected]>
SchSeba · Dec 9, 2021 · 89f78e5 · 89f78e5
1 parent 8619fd5
commit 89f78e5
Show file tree

Hide file tree

Showing 12 changed files with 480 additions and 116 deletions.
diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json
@@ -11055,6 +11055,14 @@
       "description": "Disk represents the hotplug disk that will be plugged into the running VMI",
       "$ref": "#/definitions/v1.Disk"
      },
+     "dryRun": {
+      "description": "When present, indicates that modifications should not be persisted. An invalid or unrecognized dryRun directive will result in an error response and no further processing of the request. Valid values are: - All: all dry run stages will be processed",
+      "type": "array",
+      "items": {
+       "type": "string"
+      },
+      "x-kubernetes-list-type": "atomic"
+     },
      "name": {
       "description": "Name represents the name that will be used to map the disk to the corresponding volume. This overrides any name set inside the Disk struct itself.",
       "type": "string"
@@ -13344,6 +13352,14 @@
      "name"
     ],
     "properties": {
+     "dryRun": {
+      "description": "When present, indicates that modifications should not be persisted. An invalid or unrecognized dryRun directive will result in an error response and no further processing of the request. Valid values are: - All: all dry run stages will be processed",
+      "type": "array",
+      "items": {
+       "type": "string"
+      },
+      "x-kubernetes-list-type": "atomic"
+     },
      "name": {
       "description": "Name represents the name that maps to both the disk and volume that should be removed",
       "type": "string"

diff --git a/pkg/virt-api/rest/subresource.go b/pkg/virt-api/rest/subresource.go
@@ -1216,8 +1216,9 @@ func (app *SubresourceAPIApp) vmiVolumePatch(name, namespace string, volumeReque
 		return errors.NewConflict(v1.Resource("virtualmachineinstance"), name, err)
 	}
 
+	dryRunOption := app.getDryRunOption(volumeRequest)
 	log.Log.Object(vmi).V(4).Infof("Patching VMI: %s", patch)
-	if _, err := app.virtCli.VirtualMachineInstance(vmi.Namespace).Patch(vmi.Name, types.JSONPatchType, []byte(patch), &k8smetav1.PatchOptions{}); err != nil {
+	if _, err := app.virtCli.VirtualMachineInstance(vmi.Namespace).Patch(vmi.Name, types.JSONPatchType, []byte(patch), &k8smetav1.PatchOptions{DryRun: dryRunOption}); err != nil {
 		log.Log.Object(vmi).V(1).Errorf("unable to patch vmi: %v", err)
 		if errors.IsInvalid(err) {
 			if statErr, ok := err.(*errors.StatusError); ok {
@@ -1240,8 +1241,9 @@ func (app *SubresourceAPIApp) vmVolumePatchStatus(name, namespace string, volume
 		return errors.NewConflict(v1.Resource("virtualmachine"), name, err)
 	}
 
+	dryRunOption := app.getDryRunOption(volumeRequest)
 	log.Log.Object(vm).V(4).Infof("Patching VM: %s", patch)
-	if err := app.statusUpdater.PatchStatus(vm, types.JSONPatchType, []byte(patch), &k8smetav1.PatchOptions{}); err != nil {
+	if err := app.statusUpdater.PatchStatus(vm, types.JSONPatchType, []byte(patch), &k8smetav1.PatchOptions{DryRun: dryRunOption}); err != nil {
 		log.Log.Object(vm).V(1).Errorf("unable to patch vm status: %v", err)
 		if errors.IsInvalid(err) {
 			if statErr, ok := err.(*errors.StatusError); ok {
@@ -1253,6 +1255,16 @@ func (app *SubresourceAPIApp) vmVolumePatchStatus(name, namespace string, volume
 	return nil
 }
 
+func (app *SubresourceAPIApp) getDryRunOption(volumeRequest *v1.VirtualMachineVolumeRequest) []string {
+	var dryRunOption []string
+	if volumeRequest.AddVolumeOptions != nil && volumeRequest.AddVolumeOptions.DryRun != nil && volumeRequest.AddVolumeOptions.DryRun[0] == k8smetav1.DryRunAll {
+		dryRunOption = volumeRequest.AddVolumeOptions.DryRun
+	} else if volumeRequest.RemoveVolumeOptions != nil && volumeRequest.RemoveVolumeOptions.DryRun != nil && volumeRequest.RemoveVolumeOptions.DryRun[0] == k8smetav1.DryRunAll {
+		dryRunOption = volumeRequest.RemoveVolumeOptions.DryRun
+	}
+	return dryRunOption
+}
+
 // VMAddVolumeRequestHandler handles the subresource for hot plugging a volume and disk.
 func (app *SubresourceAPIApp) VMAddVolumeRequestHandler(request *restful.Request, response *restful.Response) {
 	app.addVolumeRequestHandler(request, response, false)

diff --git a/pkg/virt-api/rest/subresource_test.go b/pkg/virt-api/rest/subresource_test.go
diff --git a/pkg/virt-operator/resource/generate/components/validations_generated.go b/pkg/virt-operator/resource/generate/components/validations_generated.go
@@ -5649,6 +5649,16 @@ var CRDsValidation map[string]string = map[string]string{
                     required:
                     - name
                     type: object
+                  dryRun:
+                    description: 'When present, indicates that modifications should
+                      not be persisted. An invalid or unrecognized dryRun directive
+                      will result in an error response and no further processing of
+                      the request. Valid values are: - All: all dry run stages will
+                      be processed'
+                    items:
+                      type: string
+                    type: array
+                    x-kubernetes-list-type: atomic
                   name:
                     description: Name represents the name that will be used to map
                       the disk to the corresponding volume. This overrides any name
@@ -5706,6 +5716,16 @@ var CRDsValidation map[string]string = map[string]string{
                   be removed. The details within this field specify how to add the
                   volume
                 properties:
+                  dryRun:
+                    description: 'When present, indicates that modifications should
+                      not be persisted. An invalid or unrecognized dryRun directive
+                      will result in an error response and no further processing of
+                      the request. Valid values are: - All: all dry run stages will
+                      be processed'
+                    items:
+                      type: string
+                    type: array
+                    x-kubernetes-list-type: atomic
                   name:
                     description: Name represents the name that maps to both the disk
                       and volume that should be removed
@@ -16184,6 +16204,16 @@ var CRDsValidation map[string]string = map[string]string{
                                 required:
                                 - name
                                 type: object
+                              dryRun:
+                                description: 'When present, indicates that modifications
+                                  should not be persisted. An invalid or unrecognized
+                                  dryRun directive will result in an error response
+                                  and no further processing of the request. Valid
+                                  values are: - All: all dry run stages will be processed'
+                                items:
+                                  type: string
+                                type: array
+                                x-kubernetes-list-type: atomic
                               name:
                                 description: Name represents the name that will be
                                   used to map the disk to the corresponding volume.
@@ -16243,6 +16273,16 @@ var CRDsValidation map[string]string = map[string]string{
                               volume should be removed. The details within this field
                               specify how to add the volume
                             properties:
+                              dryRun:
+                                description: 'When present, indicates that modifications
+                                  should not be persisted. An invalid or unrecognized
+                                  dryRun directive will result in an error response
+                                  and no further processing of the request. Valid
+                                  values are: - All: all dry run stages will be processed'
+                                items:
+                                  type: string
+                                type: array
+                                x-kubernetes-list-type: atomic
                               name:
                                 description: Name represents the name that maps to
                                   both the disk and volume that should be removed

diff --git a/pkg/virtctl/vm/vm.go b/pkg/virtctl/vm/vm.go
@@ -194,6 +194,7 @@ func NewAddVolumeCommand(clientConfig clientcmd.ClientConfig) *cobra.Command {
 	cmd.MarkFlagRequired(volumeNameArg)
 	cmd.Flags().StringVar(&serial, "serial", "", "serial number you want to assign to the disk")
 	cmd.Flags().BoolVar(&persist, "persist", false, "if set, the added volume will be persisted in the VM spec (if it exists)")
+	cmd.Flags().BoolVar(&dryRun, "dry-run", false, dryRunCommandUsage)
 
 	return cmd
 }
@@ -213,6 +214,7 @@ func NewRemoveVolumeCommand(clientConfig clientcmd.ClientConfig) *cobra.Command
 	cmd.Flags().StringVar(&volumeName, volumeNameArg, "", "name used in volumes section of spec")
 	cmd.MarkFlagRequired(volumeNameArg)
 	cmd.Flags().BoolVar(&persist, "persist", false, "if set, the added volume will be persisted in the VM spec (if it exists)")
+	cmd.Flags().BoolVar(&dryRun, "dry-run", false, dryRunCommandUsage)
 	return cmd
 }
 
@@ -283,7 +285,7 @@ func usageRemoveVolume() string {
 	return usage
 }
 
-func addVolume(vmiName, volumeName, namespace string, virtClient kubecli.KubevirtClient) error {
+func addVolume(vmiName, volumeName, namespace string, virtClient kubecli.KubevirtClient, dryRunOption *[]string) error {
 	volumeSource, err := getVolumeSourceFromVolume(volumeName, namespace, virtClient)
 	if err != nil {
 		return fmt.Errorf("error adding volume, %v", err)
@@ -298,6 +300,7 @@ func addVolume(vmiName, volumeName, namespace string, virtClient kubecli.Kubevir
 			},
 		},
 		VolumeSource: volumeSource,
+		DryRun:       *dryRunOption,
 	}
 	if serial != "" {
 		hotplugRequest.Disk.Serial = serial
@@ -316,15 +319,17 @@ func addVolume(vmiName, volumeName, namespace string, virtClient kubecli.Kubevir
 	return nil
 }
 
-func removeVolume(vmiName, volumeName, namespace string, virtClient kubecli.KubevirtClient) error {
+func removeVolume(vmiName, volumeName, namespace string, virtClient kubecli.KubevirtClient, dryRunOption *[]string) error {
 	var err error
 	if !persist {
 		err = virtClient.VirtualMachineInstance(namespace).RemoveVolume(vmiName, &v1.RemoveVolumeOptions{
-			Name: volumeName,
+			Name:   volumeName,
+			DryRun: *dryRunOption,
 		})
 	} else {
 		err = virtClient.VirtualMachine(namespace).RemoveVolume(vmiName, &v1.RemoveVolumeOptions{
-			Name: volumeName,
+			Name:   volumeName,
+			DryRun: *dryRunOption,
 		})
 	}
 	if err != nil {
@@ -452,9 +457,9 @@ func (o *Command) Run(args []string) error {
 		fmt.Printf("%s\n", string(data))
 		return nil
 	case COMMAND_ADDVOLUME:
-		return addVolume(args[0], volumeName, namespace, virtClient)
+		return addVolume(args[0], volumeName, namespace, virtClient, &dryRunOption)
 	case COMMAND_REMOVEVOLUME:
-		return removeVolume(args[0], volumeName, namespace, virtClient)
+		return removeVolume(args[0], volumeName, namespace, virtClient, &dryRunOption)
 	}
 
 	fmt.Printf("VM %s was scheduled to %s\n", vmiName, o.command)

diff --git a/pkg/virtctl/vm/vm_test.go b/pkg/virtctl/vm/vm_test.go
@@ -449,15 +449,20 @@ var _ = Describe("VirtualMachine", func() {
 			table.Entry("removevolume name, missing required volume-name", "removevolume", "required flag(s)", "testvmi"),
 			table.Entry("removevolume name, invalid extra parameter", "removevolume", "unknown flag", "testvmi", "--volume-name=blah", "--invalid=test"),
 		)
-
-		It("addvolume should fail when no source is found", func() {
+		table.DescribeTable("should fail addvolume when no source is found according to option", func(args ...string) {
 			kubecli.MockKubevirtClientInstance.EXPECT().CdiClient().Return(cdiClient)
 			kubecli.MockKubevirtClientInstance.EXPECT().CoreV1().Return(coreClient.CoreV1())
-			cmd := tests.NewVirtctlCommand("addvolume", "testvmi", "--volume-name=testvolume")
-			res := cmd.Execute()
+			commandAndArgs := make([]string, 0)
+			commandAndArgs = append(commandAndArgs, "addvolume", "testvmi")
+			commandAndArgs = append(commandAndArgs, args...)
+			cmdAdd := tests.NewRepeatableVirtctlCommand(commandAndArgs...)
+			res := cmdAdd()
 			Expect(res).ToNot(BeNil())
 			Expect(res.Error()).To(ContainSubstring("Volume testvolume is not a DataVolume or PersistentVolumeClaim"))
-		})
+		},
+			table.Entry("with default", "--volume-name=testvolume"),
+			table.Entry("with dry-run arg", "--volume-name=testvolume", "--dry-run"),
+		)
 
 		table.DescribeTable("should call correct endpoint", func(commandName, vmiName, volumeName string, useDv bool, expectFunc func(vmiName, volumeName string, useDv bool), args ...string) {
 			if commandName == "addvolume" {
@@ -478,23 +483,38 @@ var _ = Describe("VirtualMachine", func() {
 			cmd := tests.NewVirtctlCommand(commandAndArgs...)
 			Expect(cmd.Execute()).To(BeNil())
 		},
-			table.Entry("addvolume dv, no persist shoud call VMI endpoint", "addvolume", "testvmi", "testvolume", true, expectVMIEndpointAddVolume),
-			table.Entry("addvolume pvc, no persist shoud call VMI endpoint", "addvolume", "testvmi", "testvolume", false, expectVMIEndpointAddVolume),
-			table.Entry("addvolume dv, with persist shoud call VM endpoint", "addvolume", "testvmi", "testvolume", true, expectVMEndpointAddVolume, "--persist"),
-			table.Entry("addvolume pvc, with persist shoud call VM endpoint", "addvolume", "testvmi", "testvolume", false, expectVMEndpointAddVolume, "--persist"),
-			table.Entry("removevolume dv, no persist shoud call VMI endpoint", "removevolume", "testvmi", "testvolume", true, expectVMIEndpointRemoveVolume),
-			table.Entry("removevolume pvc, no persist shoud call VMI endpoint", "removevolume", "testvmi", "testvolume", false, expectVMIEndpointRemoveVolume),
-			table.Entry("removevolume dv, with persist shoud call VM endpoint", "removevolume", "testvmi", "testvolume", true, expectVMEndpointRemoveVolume, "--persist"),
-			table.Entry("removevolume pvc, with persist shoud call VM endpoint", "removevolume", "testvmi", "testvolume", false, expectVMEndpointRemoveVolume, "--persist"),
+			table.Entry("addvolume dv, no persist should call VMI endpoint", "addvolume", "testvmi", "testvolume", true, expectVMIEndpointAddVolume),
+			table.Entry("addvolume pvc, no persist should call VMI endpoint", "addvolume", "testvmi", "testvolume", false, expectVMIEndpointAddVolume),
+			table.Entry("addvolume dv, with persist should call VM endpoint", "addvolume", "testvmi", "testvolume", true, expectVMEndpointAddVolume, "--persist"),
+			table.Entry("addvolume pvc, with persist should call VM endpoint", "addvolume", "testvmi", "testvolume", false, expectVMEndpointAddVolume, "--persist"),
+			table.Entry("removevolume dv, no persist should call VMI endpoint", "removevolume", "testvmi", "testvolume", true, expectVMIEndpointRemoveVolume),
+			table.Entry("removevolume pvc, no persist should call VMI endpoint", "removevolume", "testvmi", "testvolume", false, expectVMIEndpointRemoveVolume),
+			table.Entry("removevolume dv, with persist should call VM endpoint", "removevolume", "testvmi", "testvolume", true, expectVMEndpointRemoveVolume, "--persist"),
+			table.Entry("removevolume pvc, with persist should call VM endpoint", "removevolume", "testvmi", "testvolume", false, expectVMEndpointRemoveVolume, "--persist"),
+
+			table.Entry("addvolume dv, no persist with dry-run should call VMI endpoint", "addvolume", "testvmi", "testvolume", true, expectVMIEndpointAddVolume, "--dry-run"),
+			table.Entry("addvolume pvc, no persist with dry-run should call VMI endpoint", "addvolume", "testvmi", "testvolume", false, expectVMIEndpointAddVolume, "--dry-run"),
+			table.Entry("addvolume dv, with persist with dry-run should call VM endpoint", "addvolume", "testvmi", "testvolume", true, expectVMEndpointAddVolume, "--persist", "--dry-run"),
+			table.Entry("addvolume pvc, with persist with dry-run should call VM endpoint", "addvolume", "testvmi", "testvolume", false, expectVMEndpointAddVolume, "--persist", "--dry-run"),
+			table.Entry("removevolume dv, no persist with dry-run should call VMI endpoint", "removevolume", "testvmi", "testvolume", true, expectVMIEndpointRemoveVolume, "--dry-run"),
+			table.Entry("removevolume pvc, no persist with dry-run should call VMI endpoint", "removevolume", "testvmi", "testvolume", false, expectVMIEndpointRemoveVolume, "--dry-run"),
+			table.Entry("removevolume dv, with persist with dry-run should call VM endpoint", "removevolume", "testvmi", "testvolume", true, expectVMEndpointRemoveVolume, "--persist", "--dry-run"),
+			table.Entry("removevolume pvc, with persist with dry-run should call VM endpoint", "removevolume", "testvmi", "testvolume", false, expectVMEndpointRemoveVolume, "--persist", "--dry-run"),
 		)
 
-		It("removevolume should report error if call returns error", func() {
+		table.DescribeTable("removevolume should report error if call returns error according to option", func(args ...string) {
 			expectVMIEndpointRemoveVolumeError("testvmi", "testvolume")
-			cmd := tests.NewVirtctlCommand("removevolume", "testvmi", "--volume-name=testvolume")
-			res := cmd.Execute()
+			commandAndArgs := make([]string, 0)
+			commandAndArgs = append(commandAndArgs, "removevolume", "testvmi")
+			commandAndArgs = append(commandAndArgs, args...)
+			cmdAdd := tests.NewRepeatableVirtctlCommand(commandAndArgs...)
+			res := cmdAdd()
 			Expect(res).ToNot(BeNil())
 			Expect(res.Error()).To(ContainSubstring("error removing"))
-		})
+		},
+			table.Entry("with default", "--volume-name=testvolume"),
+			table.Entry("with dry-run arg", "--volume-name=testvolume", "--dry-run"),
+		)
 	})
 
 	AfterEach(func() {

diff --git a/staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go b/staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go
diff --git a/staging/src/kubevirt.io/api/core/v1/types.go b/staging/src/kubevirt.io/api/core/v1/types.go
@@ -1903,13 +1903,29 @@ type AddVolumeOptions struct {
 	Disk *Disk `json:"disk"`
 	// VolumeSource represents the source of the volume to map to the disk.
 	VolumeSource *HotplugVolumeSource `json:"volumeSource"`
+	// When present, indicates that modifications should not be
+	// persisted. An invalid or unrecognized dryRun directive will
+	// result in an error response and no further processing of the
+	// request. Valid values are:
+	// - All: all dry run stages will be processed
+	// +optional
+	// +listType=atomic
+	DryRun []string `json:"dryRun,omitempty"`
 }
 
 // RemoveVolumeOptions is provided when dynamically hot unplugging volume and disk
 type RemoveVolumeOptions struct {
 	// Name represents the name that maps to both the disk and volume that
 	// should be removed
 	Name string `json:"name"`
+	// When present, indicates that modifications should not be
+	// persisted. An invalid or unrecognized dryRun directive will
+	// result in an error response and no further processing of the
+	// request. Valid values are:
+	// - All: all dry run stages will be processed
+	// +optional
+	// +listType=atomic
+	DryRun []string `json:"dryRun,omitempty"`
 }
 
 type TokenBucketRateLimiter struct {