-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix addvolume removevolume bugs #9133
Conversation
* Fix bug of allowing to add existing volume pvc/dv with another volume name: the code checked for volume.Name instead of checking the dv/pvc name. * Fix bug of allowing to remove existing volume which wasnt hotplugged. * Add the check of add and remove volume to persistent add/remove volume code path - in case of add/remove to VM the checks should also happen. * Add and fix current UT accordingly. Signed-off-by: Shelly Kagan <skagan@redhat.com>
Signed-off-by: Shelly Kagan <skagan@redhat.com>
pkg/virt-api/rest/subresource.go
Outdated
} | ||
|
||
func volumeNameExists(volume v1.Volume, volumeName string) bool { | ||
if volume.Name == volumeName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just
return volume.Name == volumeName
pkg/virt-api/rest/subresource.go
Outdated
@@ -1052,30 +1052,92 @@ 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 { | |||
if volume.DataVolume != nil && volume.DataVolume.Hotpluggable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can
return (volume.DataVolume != nil && volume.DataVolume.Hotpluggable) || (volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.Hotpluggable)
pkg/virt-api/rest/subresource.go
Outdated
} | ||
|
||
func volumeSourceExists(volume v1.Volume, volumeName string) bool { | ||
if volume.DataVolume != nil && volume.DataVolume.Name == volumeName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can
return (volume.DataVolume != nil && volume.DataVolume.Name == volumeName) || (volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.ClaimName == volumeName)
pkg/virt-api/rest/subresource.go
Outdated
|
||
func volumeExists(volume v1.Volume, volumeName string) bool { | ||
if volumeNameExists(volume, volumeName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can
return volumeNameExists(volume, volumeName) || volumeSourceExists(volume, volumeName)
}) | ||
|
||
It("should reject hotplugging the same volume with an existing volume name", func() { | ||
dvBlock := createDataVolumeAndWaitForImport(sc, corev1.PersistentVolumeBlock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will work in a WFFC scenario. Since the code wait for the DV to be ready which will never happen. Maybe just create it and assume it is ready (blank ones usually are before you attempt to hotplug them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awels the create adds to the dv force bind
annotation so it should be fine right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it does, hmm that might introduce other weirdness but most of these are single disks so it mostly should be fine.
Signed-off-by: Shelly Kagan <skagan@redhat.com>
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks 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:
Approvers can indicate their approval by writing |
/lgtm |
/retest-required |
/retest-required |
4 similar comments
/retest-required |
/retest-required |
/retest-required |
/retest-required |
@ShellyKa13: The following tests failed, say
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. |
/retest-required |
/cherrypick release-0.53 |
@ShellyKa13: #9133 failed to apply on top of branch "release-0.53":
In response to this:
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. |
/cherrypick release-0.59 |
@ShellyKa13: new pull request created: #9197 In response to this:
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. |
/cherrypick release-0.58 |
@ShellyKa13: #9133 failed to apply on top of branch "release-0.58":
In response to this:
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. |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes # Jira: CNV-23867
CNV-24856
Special notes for your reviewer:
Release note: