idle datavolumes.cdi.kubevirt.io by deleting it#719
Conversation
WalkthroughAdds CDI DataVolume awareness to the idler: RBAC updated for datavolumes and PVCs, idler deletes DataVolume owners (rather than scaling), 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 CDIAPI as "Kubernetes API (cdi.kubevirt.io)"
Controller->>DynamicClient: discover workload owners
alt owner is DataVolume
Controller->>CDIAPI: DELETE /apis/cdi.kubevirt.io/v1beta1/namespaces/{ns}/datavolumes/{name}
CDIAPI-->>DynamicClient: deletion accepted/status
DynamicClient-->>Controller: deletion response
else owner is scalable
Controller->>DynamicClient: PATCH/scale to 0 for owner resource
DynamicClient-->>Controller: scale response
end
Controller->>DynamicClient: verify resource removal (tests call DataVolumeExists/DoesNotExist)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #719 +/- ##
==========================================
- Coverage 63.29% 63.12% -0.18%
==========================================
Files 48 48
Lines 3594 3604 +10
==========================================
Hits 2275 2275
- Misses 1171 1181 +10
Partials 148 148
🚀 New features to boost your workflow:
|
MatousJobanek
force-pushed
the
datavolume-in-idler
branch
from
5ec0cd8 to
dba7e2a
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, mfrancisc, rsoaresd 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 |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controllers/idler/idler_controller_test.go (1)
991-1004: LGTM! DataVolume ownership chain properly established.The test correctly sets up the ownership chain: DataVolume → PVC → Pods, which aligns with the PR objective to handle DataVolume deletion during idling.
Consider adding minimal spec fields to the DataVolume for more realistic testing:
💡 Optional enhancement for test realism
dv := &unstructured.Unstructured{} dv.SetAPIVersion("cdi.kubevirt.io/v1beta1") dv.SetKind("DataVolume") dv.SetName(fmt.Sprintf("%s%s-datavolume", namePrefix, namespace)) dv.SetNamespace(namespace) +// Add minimal spec for realism (optional) +unstructured.SetNestedMap(dv.Object, map[string]interface{}{ + "pvc": map[string]interface{}{ + "accessModes": []interface{}{"ReadWriteOnce"}, + "resources": map[string]interface{}{ + "requests": map[string]interface{}{ + "storage": "10Gi", + }, + }, + }, + "source": map[string]interface{}{ + "blank": map[string]interface{}{}, + }, +}, "spec", nil) createObjectWithDynamicClient(t, clients.DynamicClient, dv)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controllers/idler/idler_controller.gocontrollers/idler/idler_controller_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T09:12:26.269Z
Learnt from: MatousJobanek
Repo: codeready-toolchain/member-operator PR: 694
File: controllers/idler/idler_controller_test.go:1291-1292
Timestamp: 2025-09-02T09:12:26.269Z
Learning: In the member-operator idler controller, the idleServingRuntime function only lists and deletes InferenceService resources (serving.kserve.io/v1beta1/inferenceservices), not ServingRuntime resources themselves. The function works by deleting old InferenceService objects to idle the ServingRuntime. Therefore, customListKinds in tests only requires the InferenceServiceList mapping, not ServingRuntimeList mapping.
Applied to files:
controllers/idler/idler_controller.go
⏰ 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 (7)
controllers/idler/idler_controller.go (2)
70-70: LGTM! PVC permissions added for DataVolume ownership chain.The addition of
persistentvolumeclaimsto the RBAC annotation is necessary to support the DataVolume ownership chain where DataVolume owns PVC, which in turn owns Pods.
75-80: LGTM! DataVolume and VM stop RBAC properly configured.The RBAC annotations correctly grant permissions for:
- DataVolume resources in the
cdi.kubevirt.iogroup with full lifecycle management- VM stop subresource with create/update verbs, properly documented with explanatory comments
controllers/idler/idler_controller_test.go (5)
879-879: LGTM! DataVolume field added to test payloads.The field follows the established pattern for unstructured resources in the test payload structure.
1131-1131: LGTM! DataVolume properly added to return payload.The field assignment follows the established pattern for returning test resources.
201-203: LGTM! DataVolume assertions properly integrated.The test assertions correctly verify that:
- DataVolumes are deleted when their pods are idled (consistent with DaemonSet/Job behavior)
- DataVolumes are preserved when pods haven't exceeded timeout
- Isolation is maintained across namespaces
327-327: LGTM! DataVolume assertions in error scenarios.The test correctly verifies that DataVolume deletion proceeds even when other operations fail, ensuring consistent idling behavior.
543-543: LGTM! DataVolume deletion verified in notification failure scenario.The assertion confirms that DataVolume idling proceeds independently of notification status updates.
MatousJobanek
merged commit e048e8f
into
codeready-toolchain:master
4 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.