GEP-33 addition : update garbage collection for images#35
GEP-33 addition : update garbage collection for images#35anton-paulovich wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ManagedCloudProfile controller's garbage collection is extended to handle capability-flavor lifecycle. ChangesGC Capability-Flavor Preservation and Cascade Deletion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
controllers/managedcloudprofile_controller.go (1)
212-216: 💤 Low valueInconsistent map access may delete untracked provider config entries.
At lines 254-255, the single-value map access
!cleanVersionsWithFlavors[mv.Version]returnstrue(delete) for entries not in the map. However, at lines 275-276, the code uses two-value access (hasRemainingFlavors, isCleanVersion := cleanVersionsWithFlavors[mv.Version]) which safely handles missing keys.If a provider config entry has
Image == ""but emptyCapabilityFlavors(a malformed but possible state), it would be deleted here even though it wasn't explicitly tracked. For consistency and defensive coding, use the same two-value pattern:🛡️ Suggested fix for consistent map access
- // Clean version entry — delete if all flavors were removed. - return !cleanVersionsWithFlavors[mv.Version] + // Clean version entry — delete only if tracked and all flavors were removed. + hasRemainingFlavors, isCleanVersion := cleanVersionsWithFlavors[mv.Version] + return isCleanVersion && !hasRemainingFlavorsAlso applies to: 226-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/managedcloudprofile_controller.go` around lines 212 - 216, The cleanVersionsWithFlavors map uses inconsistent access patterns that can cause unintended deletions. Replace the single-value map access pattern with the two-value pattern throughout the code. Specifically, where the code currently checks !cleanVersionsWithFlavors[mv.Version] (which treats missing keys as false), change it to use the two-value assignment pattern like hasRemainingFlavors, isCleanVersion := cleanVersionsWithFlavors[mv.Version] to explicitly distinguish between keys that were never tracked in the map versus keys that exist with a false value. This defensive approach ensures that provider config entries which were never explicitly tracked are not accidentally deleted due to malformed state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@controllers/managedcloudprofile_controller.go`:
- Around line 212-216: The cleanVersionsWithFlavors map uses inconsistent access
patterns that can cause unintended deletions. Replace the single-value map
access pattern with the two-value pattern throughout the code. Specifically,
where the code currently checks !cleanVersionsWithFlavors[mv.Version] (which
treats missing keys as false), change it to use the two-value assignment pattern
like hasRemainingFlavors, isCleanVersion := cleanVersionsWithFlavors[mv.Version]
to explicitly distinguish between keys that were never tracked in the map versus
keys that exist with a false value. This defensive approach ensures that
provider config entries which were never explicitly tracked are not accidentally
deleted due to malformed state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 520c50d4-6b87-478a-8233-333700588385
📒 Files selected for processing (2)
controllers/managedcloudprofile_controller.gocontrollers/managedcloudprofile_controller_test.go
There was a problem hiding this comment.
Pull request overview
Upgrades the ManagedCloudProfile garbage collection logic to understand GEP-33 “clean versions” and their backing capability flavor tags, ensuring referenced images are protected and enabling cascade-deletion of empty clean-version entries.
Changes:
- Extend GC reference detection to protect raw OCI tags backing clean versions referenced by Shoots.
- Update deletion logic to delete old capability flavors and cascade-delete clean-version entries when their flavors are gone.
- Add new reconciler tests covering protection, partial flavor cleanup, and cascade deletion.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updates the Go version directive to include a patch version. |
| controllers/managedcloudprofile_controller.go | Implements clean-version shielding via capability flavors and cascade deletion during GC. |
| controllers/managedcloudprofile_controller_test.go | Adds tests for preserving referenced flavors and cascading clean-version deletions. |
| .typos.toml | Adds an ignore-regex for a specific identifier to avoid typos false positives. |
| .golangci.yaml | Relaxes the go.mod go directive pattern to allow patch versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
controllers/managedcloudprofile_controller.go:285
deleteVersions()always performs aUpdate()call even whenversionsToDeleteis empty and there are no stale/empty clean-version entries to cascade-delete. SincereconcileGarbageCollection()now calls this method unconditionally, this can create unnecessary CloudProfile writes (resourceVersion churn) every GC run.
if err := r.Update(ctx, &cp); err != nil {
return err
}
return nil
The Garbage Collector has been upgraded to understand both legacy OCI tags and the new GEP-33 "Clean Versions". It now operates in three smart phases to keep the
CloudProfilecleanThe Shield (Protection Mapping)
Before deleting anything, the GC looks at all running Shoots to see what images they are using.
If a Shoot uses a legacy tag (e.g.,
2254.0.0-metal-sci), that tag is protected.[NEW] If a Shoot uses a clean version (e.g.,
2254.0.0), the GC automatically acts as a shield. It digs into theproviderConfig, finds all the long, raw OCI tags (Capability Flavors) backing that clean version, and protects all of themThe Sweep (Standard Cleanup)
The GC looks at the Keppel registry and identifies all tags that are older than the configured
maxAgespec.machineImagesand the newproviderConfig.capabilityFlavorsarrays.The Deep Clean (Cascade Deletion)
Previously, "Clean Version" entries (like the "folder" for
2254.0.0) would live in theCloudProfileforever, even after all of their actual OCI tags were deleted, causing infinite clutter.CloudProfileSummary by CodeRabbit
Release Notes
Bug Fixes
Tests