fix: reject setting serial disk number for lun disks - #18882
Conversation
Signed-off-by: dsanatar <dsanatar@redhat.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The error message "Serial definition is not supported for LUN disks" is duplicated in both the validation layer and the virtctl addvolume path; consider centralizing this string (or using a shared helper) so future changes keep these errors consistent.
- In
validateSerialDevice, you hardcode the SCSI bus check alongside the LUN disk type; if LUN disks are already constrained to SCSI elsewhere, you could simplify the condition to just the disk type, or alternatively document in a comment why the bus check is needed here to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The error message "Serial definition is not supported for LUN disks" is duplicated in both the validation layer and the virtctl addvolume path; consider centralizing this string (or using a shared helper) so future changes keep these errors consistent.
- In `validateSerialDevice`, you hardcode the SCSI bus check alongside the LUN disk type; if LUN disks are already constrained to SCSI elsewhere, you could simplify the condition to just the disk type, or alternatively document in a comment why the bus check is needed here to avoid confusion.
## Individual Comments
### Comment 1
<location path="pkg/storage/admitters/disks_test.go" line_range="366-375" />
<code_context>
+ Expect(causes[0].Field).To(Equal("fake[0].serial"))
+ })
+
+ It("should accept serial set for scsi disk", func() {
+ vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{
+ Name: "scsi-disk",
+ Serial: "testserial",
+ DiskDevice: v1.DiskDevice{
+ Disk: &v1.DiskTarget{
+ Bus: v1.DiskBusSCSI,
+ },
+ },
+ })
+
+ causes := ValidateDisks(k8sfield.NewPath("fake"), vmi.Spec.Domain.Devices.Disks)
+ Expect(causes).To(BeEmpty())
+ })
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a positive test for LUN without serial to show the constraint is only on serial presence
We already cover the rejected case (LUN with serial) and accepted case (SCSI disk with serial). Please also add a test for a SCSI LUN with an empty serial to confirm LUNs are still valid when `serial` is omitted, and to make the validation rule’s intent explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| It("should accept serial set for scsi disk", func() { | ||
| vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{ | ||
| Name: "scsi-disk", | ||
| Serial: "testserial", | ||
| DiskDevice: v1.DiskDevice{ | ||
| Disk: &v1.DiskTarget{ | ||
| Bus: v1.DiskBusSCSI, | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
suggestion (testing): Add a positive test for LUN without serial to show the constraint is only on serial presence
We already cover the rejected case (LUN with serial) and accepted case (SCSI disk with serial). Please also add a test for a SCSI LUN with an empty serial to confirm LUNs are still valid when serial is omitted, and to make the validation rule’s intent explicit.
|
@dsanatar: The following tests failed, say
DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
What this PR does
LibVirt does not allow specifying a disk serial number for LUN disks and are currently silently ignored when set.
This PR adds new check at the disk spec validation layer to reject VMs that are configured with LUN disks containing a serial. Add similar rejection to the
addvolumeAPI call so this specific disk configuration cannot be hotplugged.Before this PR:
After this PR:
References
Why we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Links to places where the discussion took place:
Special notes for your reviewer
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note