diff --git a/pkg/virt-api/rest/subresource.go b/pkg/virt-api/rest/subresource.go index 9cd31518492f..2ec9751255c7 100644 --- a/pkg/virt-api/rest/subresource.go +++ b/pkg/virt-api/rest/subresource.go @@ -1052,30 +1052,71 @@ func addVolumeRequestExists(request v1.VirtualMachineVolumeRequest, name string) return request.AddVolumeOptions != nil && request.AddVolumeOptions.Name == name } -func generateVMIVolumeRequestPatch(vmi *v1.VirtualMachineInstance, volumeRequest *v1.VirtualMachineVolumeRequest) (string, error) { +func volumeHotpluggable(volume v1.Volume) bool { + return (volume.DataVolume != nil && volume.DataVolume.Hotpluggable) || (volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.Hotpluggable) +} - volumeVerb := "add" - diskVerb := "add" +func volumeNameExists(volume v1.Volume, volumeName string) bool { + return volume.Name == volumeName +} - if len(vmi.Spec.Volumes) > 0 { - volumeVerb = "replace" +func volumeSourceName(volumeSource *v1.HotplugVolumeSource) string { + if volumeSource.DataVolume != nil { + return volumeSource.DataVolume.Name } - - if len(vmi.Spec.Domain.Devices.Disks) > 0 { - diskVerb = "replace" + if volumeSource.PersistentVolumeClaim != nil { + return volumeSource.PersistentVolumeClaim.ClaimName } + return "" +} +func volumeSourceExists(volume v1.Volume, volumeName string) bool { + return (volume.DataVolume != nil && volume.DataVolume.Name == volumeName) || + (volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.ClaimName == volumeName) +} + +func volumeExists(volume v1.Volume, volumeName string) bool { + return volumeNameExists(volume, volumeName) || volumeSourceExists(volume, volumeName) +} + +func verifyVolumeOption(volumes []v1.Volume, volumeRequest *v1.VirtualMachineVolumeRequest) error { foundRemoveVol := false - for _, volume := range vmi.Spec.Volumes { - if volumeRequest.AddVolumeOptions != nil && volume.Name == volumeRequest.AddVolumeOptions.Name { - return "", fmt.Errorf("Unable to add volume [%s] because it already exists", volume.Name) - } else if volumeRequest.RemoveVolumeOptions != nil && volume.Name == volumeRequest.RemoveVolumeOptions.Name { + for _, volume := range volumes { + if volumeRequest.AddVolumeOptions != nil { + volSourceName := volumeSourceName(volumeRequest.AddVolumeOptions.VolumeSource) + if volumeNameExists(volume, volumeRequest.AddVolumeOptions.Name) { + return fmt.Errorf("Unable to add volume [%s] because volume with that name already exists", volumeRequest.AddVolumeOptions.Name) + } + if volumeSourceExists(volume, volSourceName) { + return fmt.Errorf("Unable to add volume source [%s] because it already exists", volSourceName) + } + } else if volumeRequest.RemoveVolumeOptions != nil && volumeExists(volume, volumeRequest.RemoveVolumeOptions.Name) { + if !volumeHotpluggable(volume) { + return fmt.Errorf("Unable to remove volume [%s] because it is not hotpluggable", volume.Name) + } foundRemoveVol = true + break } } if volumeRequest.RemoveVolumeOptions != nil && !foundRemoveVol { - return "", fmt.Errorf("Unable to remove volume [%s] because it does not exist", volumeRequest.RemoveVolumeOptions.Name) + return fmt.Errorf("Unable to remove volume [%s] because it does not exist", volumeRequest.RemoveVolumeOptions.Name) + } + + return nil +} + +func generateVMIVolumeRequestPatch(vmi *v1.VirtualMachineInstance, volumeRequest *v1.VirtualMachineVolumeRequest) (string, error) { + + volumeVerb := "add" + diskVerb := "add" + + if len(vmi.Spec.Volumes) > 0 { + volumeVerb = "replace" + } + + if len(vmi.Spec.Domain.Devices.Disks) > 0 { + diskVerb = "replace" } vmiCopy := vmi.DeepCopy() @@ -1235,6 +1276,11 @@ func (app *SubresourceAPIApp) vmiVolumePatch(name, namespace string, volumeReque return errors.NewConflict(v1.Resource("virtualmachineinstance"), name, fmt.Errorf(vmiNotRunning)) } + err := verifyVolumeOption(vmi.Spec.Volumes, volumeRequest) + if err != nil { + return errors.NewConflict(v1.Resource("virtualmachineinstance"), name, err) + } + patch, err := generateVMIVolumeRequestPatch(vmi, volumeRequest) if err != nil { return errors.NewConflict(v1.Resource("virtualmachineinstance"), name, err) @@ -1260,6 +1306,11 @@ func (app *SubresourceAPIApp) vmVolumePatchStatus(name, namespace string, volume return statErr } + err := verifyVolumeOption(vm.Spec.Template.Spec.Volumes, volumeRequest) + if err != nil { + return errors.NewConflict(v1.Resource("virtualmachine"), name, err) + } + patch, err := generateVMVolumeRequestPatch(vm, volumeRequest) if err != nil { return errors.NewConflict(v1.Resource("virtualmachine"), name, err) diff --git a/pkg/virt-api/rest/subresource_test.go b/pkg/virt-api/rest/subresource_test.go index 55981c918d05..b646c1b28180 100644 --- a/pkg/virt-api/rest/subresource_test.go +++ b/pkg/virt-api/rest/subresource_test.go @@ -727,9 +727,41 @@ var _ = Describe("VirtualMachineInstance Subresources", func() { request.Request.Body = newRemoveVolumeBody(removeOpts) } + vmi := api.NewMinimalVMI(request.PathParameter("name")) + vmi.Namespace = k8smetav1.NamespaceDefault + vmi.Status.Phase = v1.Running + vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{ + Name: "existingvol", + }) + vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{ + Name: "hotpluggedPVC", + }) + vmi.Spec.Volumes = append(vmi.Spec.Volumes, v1.Volume{ + Name: "existingvol", + VolumeSource: v1.VolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{PersistentVolumeClaimVolumeSource: k8sv1.PersistentVolumeClaimVolumeSource{ + ClaimName: "testpvcdiskclaim", + }}, + }, + }) + vmi.Spec.Volumes = append(vmi.Spec.Volumes, v1.Volume{ + Name: "hotpluggedPVC", + VolumeSource: v1.VolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ + PersistentVolumeClaimVolumeSource: k8sv1.PersistentVolumeClaimVolumeSource{ + ClaimName: "hotpluggedPVC", + }, + Hotpluggable: true, + }, + }, + }) + if isVM { vm := newMinimalVM(request.PathParameter("name")) vm.Namespace = k8smetav1.NamespaceDefault + vm.Spec.Template = &v1.VirtualMachineInstanceTemplateSpec{ + Spec: vmi.Spec, + } patchedVM := vm.DeepCopy() patchedVM.Status.VolumeRequests = append(patchedVM.Status.VolumeRequests, v1.VirtualMachineVolumeRequest{AddVolumeOptions: addOpts, RemoveVolumeOptions: removeOpts}) @@ -754,20 +786,6 @@ var _ = Describe("VirtualMachineInstance Subresources", func() { app.VMRemoveVolumeRequestHandler(request, response) } } else { - vmi := api.NewMinimalVMI(request.PathParameter("name")) - vmi.Namespace = k8smetav1.NamespaceDefault - vmi.Status.Phase = v1.Running - vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{ - Name: "existingvol", - }) - vmi.Spec.Volumes = append(vmi.Spec.Volumes, v1.Volume{ - Name: "existingvol", - VolumeSource: v1.VolumeSource{ - PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{PersistentVolumeClaimVolumeSource: k8sv1.PersistentVolumeClaimVolumeSource{ - ClaimName: "testpvcdiskclaim", - }}, - }, - }) vmiClient.EXPECT().Get(vmi.Name, &k8smetav1.GetOptions{}).Return(vmi, nil).AnyTimes() @@ -815,10 +833,10 @@ var _ = Describe("VirtualMachineInstance Subresources", func() { Disk: &v1.Disk{}, }, nil, false, http.StatusBadRequest, true), Entry("VM with a valid remove volume request", nil, &v1.RemoveVolumeOptions{ - Name: "vol1", + Name: "hotpluggedPVC", }, true, http.StatusAccepted, true), Entry("VMI with a valid remove volume request", nil, &v1.RemoveVolumeOptions{ - Name: "existingvol", + Name: "hotpluggedPVC", }, false, http.StatusAccepted, true), Entry("VMI with a invalid remove volume request missing a name", nil, &v1.RemoveVolumeOptions{}, false, http.StatusBadRequest, true), Entry("VMI with a valid remove volume request but no feature gate", nil, &v1.RemoveVolumeOptions{ @@ -857,11 +875,11 @@ var _ = Describe("VirtualMachineInstance Subresources", func() { DryRun: getDryRunOption(), }, nil, false, http.StatusBadRequest, true), Entry("VM with a valid remove volume request with DryRun", nil, &v1.RemoveVolumeOptions{ - Name: "vol1", + Name: "hotpluggedPVC", DryRun: getDryRunOption(), }, true, http.StatusAccepted, true), Entry("VMI with a valid remove volume request with DryRun", nil, &v1.RemoveVolumeOptions{ - Name: "existingvol", + Name: "hotpluggedPVC", DryRun: getDryRunOption(), }, false, http.StatusAccepted, true), Entry("VMI with a invalid remove volume request missing a name with DryRun", nil, &v1.RemoveVolumeOptions{ @@ -923,24 +941,6 @@ var _ = Describe("VirtualMachineInstance Subresources", func() { }, "[{ \"op\": \"test\", \"path\": \"/spec/volumes\", \"value\": [{\"name\":\"existingvol\",\"persistentVolumeClaim\":{\"claimName\":\"testpvcdiskclaim\"}}]}, { \"op\": \"test\", \"path\": \"/spec/domain/devices/disks\", \"value\": [{\"name\":\"existingvol\"}]}, { \"op\": \"replace\", \"path\": \"/spec/volumes\", \"value\": []}, { \"op\": \"replace\", \"path\": \"/spec/domain/devices/disks\", \"value\": []}]", false), - Entry("remove volume that doesn't exist", - &v1.VirtualMachineVolumeRequest{ - RemoveVolumeOptions: &v1.RemoveVolumeOptions{ - Name: "non-existent", - }, - }, - "", - true), - Entry("add a volume that already exists", - &v1.VirtualMachineVolumeRequest{ - AddVolumeOptions: &v1.AddVolumeOptions{ - Name: "existingvol", - Disk: &v1.Disk{}, - VolumeSource: &v1.HotplugVolumeSource{}, - }, - }, - "", - true), ) DescribeTable("Should generate expected vm patch", func(volumeRequest *v1.VirtualMachineVolumeRequest, existingVolumeRequests []v1.VirtualMachineVolumeRequest, expectedPatch string, expectError bool) { @@ -1050,6 +1050,122 @@ var _ = Describe("VirtualMachineInstance Subresources", func() { "", true), ) + + DescribeTable("Should verify volume option", func(volumeRequest *v1.VirtualMachineVolumeRequest, existingVolumes []v1.Volume, expectedError string) { + err := verifyVolumeOption(existingVolumes, volumeRequest) + if expectedError != "" { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal(expectedError)) + } else { + Expect(err).ToNot(HaveOccurred()) + } + }, + Entry("add volume name which already exists should fail", + &v1.VirtualMachineVolumeRequest{ + AddVolumeOptions: &v1.AddVolumeOptions{ + Name: "vol1", + Disk: &v1.Disk{}, + VolumeSource: &v1.HotplugVolumeSource{}, + }, + }, + []v1.Volume{ + { + Name: "vol1", + VolumeSource: v1.VolumeSource{ + DataVolume: &v1.DataVolumeSource{ + Name: "dv1", + }, + }, + }, + }, + "Unable to add volume [vol1] because volume with that name already exists"), + Entry("add volume source which already exists should fail(existing dv)", + &v1.VirtualMachineVolumeRequest{ + AddVolumeOptions: &v1.AddVolumeOptions{ + Name: "dv1", + Disk: &v1.Disk{}, + VolumeSource: &v1.HotplugVolumeSource{ + DataVolume: &v1.DataVolumeSource{ + Name: "dv1", + }, + }, + }, + }, + []v1.Volume{ + { + Name: "vol1", + VolumeSource: v1.VolumeSource{ + DataVolume: &v1.DataVolumeSource{ + Name: "dv1", + }, + }, + }, + }, + "Unable to add volume source [dv1] because it already exists"), + Entry("add volume which source already exists should fail(existing pvc)", + &v1.VirtualMachineVolumeRequest{ + AddVolumeOptions: &v1.AddVolumeOptions{ + Name: "pvc1", + Disk: &v1.Disk{}, + VolumeSource: &v1.HotplugVolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{PersistentVolumeClaimVolumeSource: k8sv1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc1", + }}, + }, + }, + }, + []v1.Volume{ + { + Name: "vol1", + VolumeSource: v1.VolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{PersistentVolumeClaimVolumeSource: k8sv1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc1", + }}, + }, + }, + }, + "Unable to add volume source [pvc1] because it already exists"), + Entry("remove volume which doesnt exist should fail", + &v1.VirtualMachineVolumeRequest{ + RemoveVolumeOptions: &v1.RemoveVolumeOptions{ + Name: "vol1", + }, + }, + []v1.Volume{}, + "Unable to remove volume [vol1] because it does not exist"), + Entry("remove volume which wasnt hotplugged should fail(existing dv)", + &v1.VirtualMachineVolumeRequest{ + RemoveVolumeOptions: &v1.RemoveVolumeOptions{ + Name: "dv1", + }, + }, + []v1.Volume{ + { + Name: "vol1", + VolumeSource: v1.VolumeSource{ + DataVolume: &v1.DataVolumeSource{ + Name: "dv1", + }, + }, + }, + }, + "Unable to remove volume [vol1] because it is not hotpluggable"), + Entry("remove volume which wasnt hotplugged should fail(existing cloudInit)", + &v1.VirtualMachineVolumeRequest{ + RemoveVolumeOptions: &v1.RemoveVolumeOptions{ + Name: "cloudinitdisk", + }, + }, + []v1.Volume{ + { + Name: "cloudinitdisk", + VolumeSource: v1.VolumeSource{ + CloudInitNoCloud: &v1.CloudInitNoCloudSource{}, + }, + }, + }, + "Unable to remove volume [cloudinitdisk] because it is not hotpluggable"), + ) }) Context("Memory dump Subresource api", func() { diff --git a/tests/storage/hotplug.go b/tests/storage/hotplug.go index debc3efcd539..48eefb6e1fbf 100644 --- a/tests/storage/hotplug.go +++ b/tests/storage/hotplug.go @@ -59,6 +59,7 @@ import ( const ( dataMessage = "data/message" addingVolumeRunningVM = "Adding volume to running VM" + addingVolumeAgain = "Adding the same volume again with different name" verifyingVolumeDiskInVM = "Verifying the volume and disk are in the VM and VMI" removingVolumeFromVM = "removing volume from VM" verifyingVolumeNotExist = "Verifying the volume no longer exists in VM" @@ -532,7 +533,7 @@ var _ = SIGDescribe("Hotplug", func() { DescribeTable("Should add volumes on an offline VM", func(addVolumeFunc addVolumeFunction, removeVolumeFunc removeVolumeFunction) { By("Adding test volumes") addVolumeFunc(vm.Name, vm.Namespace, testNewVolume1, "madeup", v1.DiskBusSCSI, false, "") - addVolumeFunc(vm.Name, vm.Namespace, testNewVolume2, "madeup", v1.DiskBusSCSI, false, "") + addVolumeFunc(vm.Name, vm.Namespace, testNewVolume2, "madeup2", v1.DiskBusSCSI, false, "") By("Verifying the volumes have been added to the template spec") verifyVolumeAndDiskVMAdded(virtClient, vm, testNewVolume1, testNewVolume2) By("Removing new volumes from VM") @@ -854,9 +855,48 @@ var _ = SIGDescribe("Hotplug", func() { }, }, false, "")) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("conflicts with an existing volume of the same name on the vmi template")) + Expect(err.Error()).To(ContainSubstring("Unable to add volume [disk0] because volume with that name already exists")) }) + It("should reject hotplugging the same volume with an existing volume name", func() { + dvBlock := createDataVolumeAndWaitForImport(sc, corev1.PersistentVolumeBlock) + vmi, err := virtClient.VirtualMachineInstance(vm.Namespace).Get(vm.Name, &metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + tests.WaitForSuccessfulVMIStartWithTimeout(vmi, 240) + + By(addingVolumeRunningVM) + addPVCVolumeVMI(vmi.Name, vmi.Namespace, "testvolume", dvBlock.Name, v1.DiskBusSCSI, false, "") + + By(verifyingVolumeDiskInVM) + vmi, err = virtClient.VirtualMachineInstance(vm.Namespace).Get(vm.Name, &metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + verifyVolumeAndDiskVMIAdded(virtClient, vmi, "testvolume") + verifyVolumeStatus(vmi, v1.VolumeReady, "", "testvolume") + + By(addingVolumeAgain) + err = virtClient.VirtualMachineInstance(vmi.Namespace).AddVolume(vmi.Name, getAddVolumeOptions(dvBlock.Name, v1.DiskBusSCSI, &v1.HotplugVolumeSource{ + DataVolume: &v1.DataVolumeSource{ + Name: dvBlock.Name, + }, + }, false, "")) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("Unable to add volume source [%s] because it already exists", dvBlock.Name))) + }) + + DescribeTable("should reject removing a volume", func(volName, expectedErr string) { + vmi, err := virtClient.VirtualMachineInstance(vm.Namespace).Get(vm.Name, &metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + tests.WaitForSuccessfulVMIStartWithTimeout(vmi, 240) + + By(removingVolumeFromVM) + err = virtClient.VirtualMachine(vm.Namespace).RemoveVolume(vm.Name, &v1.RemoveVolumeOptions{Name: volName}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(expectedErr)) + }, + Entry("which wasn't hotplugged", "disk0", "Unable to remove volume [disk0] because it is not hotpluggable"), + Entry("which doesn't exist", "doesntexist", "Unable to remove volume [doesntexist] because it does not exist"), + ) + It("should allow hotplugging both a filesystem and block volume", func() { dvBlock := createDataVolumeAndWaitForImport(sc, corev1.PersistentVolumeBlock) dvFileSystem := createDataVolumeAndWaitForImport(sc, corev1.PersistentVolumeFilesystem)