Implement OS image version offset and update pause for CloudPro… - #53
Conversation
…file machine images Signed-off-by: C5421281 <yahor.kurachkin@sap.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new Glance version slicing can panic on out-of-range offsets, and paused updates can introduce duplicate machine image entries in the reconciled CloudProfile.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends CloudProfile sync behavior by (1) allowing a configurable “version offset” when selecting OS image versions from OpenStack Glance, and (2) adding a per-image “paused” switch to stop automatic machine image updates while keeping existing CloudProfile images.
Changes:
- Add
pausedtoMachineImageUpdateand implement reconcile logic to preserve existing images when updates are paused. - Add
versionOffsetto Glance source configuration and wire it through to the Glance source implementation. - Regenerate/update the CRD schema to include the new fields.
File summaries
| File | Description |
|---|---|
crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml |
CRD schema updated to include paused and versionOffset. |
controllers/cloud_profile.go |
Reconcile logic updated to support pausing machine image updates and forwarding versionOffset to Glance. |
cloudprofilesync/ossync/source/glance/os_source.go |
Glance source updated to apply a version offset when selecting the “latest N” versions. |
api/v1alpha1/managedcloudprofile.go |
API types updated with paused and versionOffset fields. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
b4d4d64 to
1be85c6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The Glance VersionOffset logic is currently skipped for some valid inputs (len(versions) <= keepLatest) and the paused-update test does not cover the new “keep stored CloudProfile versions” branch.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
cloudprofilesync/ossync/source/glance/os_source.go:221
- Glance VersionOffset is ignored when len(versions) <= keepLatest because the slicing logic only runs under
len(versions) > g.keepLatest; with offset>0 this returns the newest versions instead of skipping them.
if g.keepLatest > 0 && len(versions) > g.keepLatest {
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The current Glance VersionOffset slicing is incorrectly gated and can ignore offsets in valid boundary cases, leading to publishing the wrong version window.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
cloudprofilesync/ossync/source/glance/os_source.go:225
- The VersionOffset windowing logic is gated by
len(versions) > g.keepLatest, so whenVersionOffset > 0andlen(versions) <= keepLatest(or== keepLatest) the offset is ignored and the newest versions are still returned. This contradicts the intended "skip newest N" behavior and can publish the wrong GardenLinux versions.
if g.keepLatest > 0 && len(versions) > g.keepLatest {
// Skip the newest `offset` versions, then keep the next `keepLatest`.
// Clamp both bounds so an offset that runs past the available versions
// never slices out of range.
lo := min(g.offset, len(versions))
crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml:676
- The CRD schema for
machineImageUpdates[].pausedis missing a description, while other boolean fields in the CRD (e.g.insecure) include one. Adding the description improves kubectl explain / API docs and keeps the CRD aligned with the Go type comment.
paused:
type: boolean
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
1be85c6 to
03626c3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The Glance version slicing currently ignores versionOffset when len(versions) <= keepLatest, which breaks the intended behavior and will fail the newly added test case.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
cloudprofilesync/ossync/source/glance/os_source.go:225
- VersionOffset is intended to apply even when len(versions) <= keepLatest (as covered by the new test case), but this block only runs when len(versions) > keepLatest, so offset is ignored when keepLatest covers all available versions.
if g.keepLatest > 0 && len(versions) > g.keepLatest {
// Skip the newest `offset` versions, then keep the next `keepLatest`.
// Clamp both bounds so an offset that runs past the available versions
// never slices out of range.
lo := min(g.offset, len(versions))
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Signed-off-by: C5421281 <yahor.kurachkin@sap.com>
03626c3 to
fe69f29
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new Glance version-windowing behavior/documentation is internally inconsistent around keepLatest=0 semantics (and can mislead API users), so the intended configuration contract should be clarified/fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
controllers/managedcloudprofile_controller_test.go:335
- This test verifies that updates are skipped when Paused=true, but it doesn’t exercise the intended behavior of preserving previously reconciled CloudProfile machineImages across reconciles (i.e., keeping the stored CloudProfile versions even if the MCP spec/source would otherwise change them). Consider adding a follow-up reconcile scenario where the CloudProfile already contains a different version, then Paused is enabled and the version is asserted unchanged.
It("keeps existing images when the update is paused", func(ctx SpecContext) {
keptVersion := "4242.0.0"
var mcp v1alpha1.ManagedCloudProfile
mcp.Name = "test-paused"
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
No description provided.