-
Couldn't load subscription status.
- Fork 63
[fix] Filter out any volumes and include just the disks when calculating maxVolumes. #455
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
Conversation
…ing maxVolumes. Refactor maxVolumeAttachments to be more concise.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #455 +/- ##
==========================================
- Coverage 70.69% 70.65% -0.05%
==========================================
Files 24 24
Lines 2761 2757 -4
==========================================
- Hits 1952 1948 -4
Misses 681 681
Partials 128 128 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR fixes a bug in the volume attachment calculation logic by changing how the system counts disks when determining the maximum number of volumes that can be attached to a node. The fix ensures that only appropriate disk types are counted as volumes, preventing users from being unable to attach the maximum number of volumes.
- Updated disk counting logic to filter by controller type (SCSI/virtio) and vendor (QEMU)
- Modernized the
maxVolumeAttachmentsfunction to use Go's built-inmin/maxfunctions - Added comprehensive e2e tests to validate node volume limits behavior
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/driver/limits.go | Core fix - replaced attachedVolumeCount with diskCount and updated filtering logic |
| internal/driver/nodeserver.go | Updated function calls and improved logging for the new disk counting approach |
| internal/driver/limits_test.go | Comprehensive test updates to cover new disk filtering scenarios |
| internal/driver/nodeserver_test.go | Added vendor field to test data to support new filtering logic |
| tests/e2e/test/node-volume-limit/ | New e2e test suite to validate volume limit functionality end-to-end |
| // diskCount calculates the number of attached block devices that are not | ||
| // being used as Kubernetes PersistentVolumeClaims (PVCs). |
Copilot
AI
Jul 17, 2025
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.
The comment is misleading. The function actually counts disks that ARE being used (boot/swap disks from QEMU vendor), not those that are NOT being used as PVCs. Based on the code logic, it should describe counting QEMU vendor disks to exclude them from volume capacity calculations.
| // diskCount calculates the number of attached block devices that are not | |
| // being used as Kubernetes PersistentVolumeClaims (PVCs). | |
| // diskCount calculates the number of attached block devices that are from | |
| // the vendor "QEMU" and use either the "SCSI" or "virtio" storage controllers. |
| maxVolumes := maxVolumeAttachments(ns.metadata.Memory) - attachedVolumeCount | ||
| log.V(2).Info("functionStatusfully completed") | ||
| maxVolumes := maxVolumeAttachments(ns.metadata.Memory) - diskCount | ||
| log.V(2).Info("Calculated maxVolumes", "maxVolumes", maxVolumes, "diskCount", diskCount) |
Copilot
AI
Jul 17, 2025
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.
[nitpick] The log message says 'functionStatusfully completed' was replaced, but 'Calculated maxVolumes' should be 'Successfully calculated maxVolumes' for better clarity and consistency with the previous message style.
| log.V(2).Info("Calculated maxVolumes", "maxVolumes", maxVolumes, "diskCount", diskCount) | |
| log.V(2).Info("Successfully calculated maxVolumes", "maxVolumes", maxVolumes, "diskCount", diskCount) |
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.
LGTM
This PR fixes a bug where we were calculating the maxVolumes incorrectly which caused users to not be able to attach max volumes. With this fix, advertised maxVolume should be idempotent.
General:
Pull Request Guidelines: