Skip to content

Commit

Permalink
Merge pull request #9197 from kubevirt-bot/cherry-pick-9133-to-releas…
Browse files Browse the repository at this point in the history 
…e-0.59

[release-0.59] Fix addvolume removevolume bugs
  • Loading branch information
@kubevirt-bot
kubevirt-bot committed Feb 15, 2023
2 parents a5e3a1b + 6174c0e commit a450680
Show file tree
Hide file tree
Showing 3 changed files with 258 additions and 52 deletions.
77 changes: 64 additions & 13 deletions pkg/virt-api/rest/subresource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
189 changes: 152 additions & 37 deletions pkg/virt-api/rest/subresource_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -728,9 +728,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})
Expand All @@ -755,21 +787,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(context.Background(), vmi.Name, &k8smetav1.GetOptions{}).Return(vmi, nil).AnyTimes()

if addOpts != nil {
Expand Down Expand Up @@ -816,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{
Expand Down Expand Up @@ -858,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{
Expand Down Expand Up @@ -924,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) {

Expand Down Expand Up @@ -1051,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() {
Expand Down
Loading

0 comments on commit a450680

Please sign in to comment.