idle datavolumes.cdi.kubevirt.io by deleting it #719
Conversation
WalkthroughAdds CDI DataVolume awareness to the idler: RBAC updated for datavolumes, idler treats DataVolume as a deletable owner in scaleOwnerToZero, and tests plus assertion helpers were extended to create, track, and verify DataVolume lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Controller
participant DynamicClient as "Dynamic Client"
participant API as "Kubernetes API (cdi.kubevirt.io)"
Controller->>DynamicClient: discover workload owners
alt owner is DataVolume
Controller->>API: DELETE /apis/cdi.kubevirt.io/v1beta1/namespaces/{ns}/datavolumes/{name}
API-->>DynamicClient: deletion accepted / status
DynamicClient-->>Controller: deletion response
else owner is scalable (Deployment/ReplicaSet/etc.)
Controller->>API: PATCH/scale to 0
API-->>Controller: scale response
end
Controller->>DynamicClient: verify resource removal (tests call DataVolumeExists/DoesNotExist)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controllers/idler/idler_controller.go(1 hunks)controllers/idler/idler_controller_test.go(6 hunks)controllers/idler/owners.go(1 hunks)controllers/idler/owners_test.go(2 hunks)test/idler_assertion.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (9)
controllers/idler/idler_controller.go (2)
160-171: LGTM! DataVolume test configuration follows established patterns.The test configuration correctly treats DataVolume like DaemonSet and Job (exists/does not exist assertions rather than scaling), which aligns with the deletion behavior in
owners.go.
449-454: The DataVolume API versioncdi.kubevirt.io/v1beta1is correct. This is the current stable API version used throughout KubeVirt CDI documentation and examples. No newer v1 stable version exists.test/idler_assertion.go (1)
301-315: LGTM! Assertion helpers follow established patterns.The DataVolume assertion methods correctly mirror the implementation for Job and DaemonSet assertions, using the dynamic client with appropriate error handling.
controllers/idler/idler_controller_test.go (2)
999-1007: LGTM! DataVolume test payload creation is comprehensive.The DataVolume is properly created with:
- Correct API version (
cdi.kubevirt.io/v1beta1)- Proper namespace and naming
- Associated controlled pods via owner references
This follows the established pattern for other resource types in the test suite.
203-205: LGTM! Test assertions properly verify DataVolume lifecycle.The test assertions comprehensively cover DataVolume behavior in both idled and running scenarios, following the same pattern as DaemonSet and Job resources.
Also applies to: 333-333, 551-551
controllers/idler/owners.go (1)
63-64: LGTM! DataVolume correctly handled via deletion.DataVolumes, like DaemonSets and Jobs, don't support scaling operations and are appropriately deleted instead. This implementation aligns with the test expectations and the resource's capabilities.
controllers/idler/owners_test.go (3)
160-171: LGTM! DataVolume test configuration follows the correct pattern.The test configuration correctly models DataVolume as a delete-rather-than-scale resource, consistent with DaemonSet and Job. The assertion hooks properly verify existence during scale-up and deletion during scale-down.
449-454: No action required—the DataVolume APIResource is correctly usingcdi.kubevirt.io/v1beta1, which is the current stable API version for KubeVirt CDI DataVolumes.
443-443: The hard-coded"kubevirt.io/v1"is intentional and appropriate for this test fixture. The variablevmGVRdoes not exist in the codebase, so there was no previous dynamic reference. ThenoAAPResourceList()function builds test data for fake discovery responses, where well-known KubeVirt and CDI API versions are appropriately hard-coded as static test fixtures. The dynamically discovered resources (lines 455+) correctly usegvk.GroupVersion().String()andgvr.GroupVersion().String(), which is a deliberate distinction, not an inconsistency.Likely an incorrect or invalid review comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #719 +/- ##
==========================================
+ Coverage 82.53% 82.58% +0.04%
==========================================
Files 48 48
Lines 3596 3606 +10
==========================================
+ Hits 2968 2978 +10
Misses 477 477
Partials 151 151
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
MatousJobanek
force-pushed
the
datavolume-in-idler
branch
from
5ec0cd8 to
dba7e2a
Compare
|
2 participants
SANDBOX-1560
paired with: codeready-toolchain/toolchain-e2e#1241
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.