Reconcile bug that fetches images several times per second

[yahor-kurachkin]

No description provided.

[Copilot]

Pull request overview

This PR addresses a reconcile loop that was causing very frequent re-reconciliation (and repeated image fetching) by reducing needless status patches, filtering reconcile triggers on ManagedCloudProfile status-only updates, and stabilizing CloudProfile-related serialized output.

Changes:

• Avoids issuing a status patch when the ManagedCloudProfile status is semantically unchanged.
• Adds a GenerationChangedPredicate to the controller watch on ManagedCloudProfile to prevent status-only updates from retriggering reconciliation.
• Stabilizes OpenStack ProviderConfig output by sorting region mappings; additionally preserves machine image expiration dates across spec regeneration.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
controllers/managedcloudprofile_controller.go	Prevents status-only update loops by skipping no-op status patches and filtering reconcile events to generation changes.
controllers/cloud_profile.go	Preserves machine image expiration dates across CloudProfile spec regeneration to reduce churn.
cloudprofilesync/ossync/provider/openstack/provider.go	Ensures deterministic ProviderConfig marshaling by sorting regions, reducing spec churn.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 24 out of 27 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

[Copilot]

🟡 Changes recommended

The new expiration-preservation logic can incorrectly retain stale ExpirationDate values when a version becomes supported again (e.g., KeepLatest changes), which is a correctness issue.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

resolveExpiration currently drops an existing expiration date when the source doesn’t provide one, contradicting intended behavior and breaking the existing expiration-preservation test.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes are targeted, improve reconcile idempotency/stability, and include corresponding unit-test updates for the new expiration-date semantics.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The updated expiration resolution logic ignores version classification and will not clear stale expiration dates for supported versions, conflicting with the intended behavior and tests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The updated expiration resolution logic does not respect version classification and can incorrectly retain an expiration date for supported versions.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

controllers/cloud_profile.go:237

• expirationDateKey uses '/' to join imageName and version, which can produce ambiguous keys if either part ever contains '/'. Using a separator that cannot occur in either value avoids accidental collisions.

cloudprofilesync/ossync/os_image_updater.go:121

• resolveExpiration's implementation ignores the version classification and will preserve an existing ExpirationDate even when the source reports the version as supported (Classification=nil/Supported), which contradicts the function comment and can leave supported versions with a stale expiration date from spec input.

// resolveExpiration decides a version's expiration date from the source's
// classification. A version the source reports as supported carries no date, so
// any previously stamped date is cleared (e.g. it returned to supported after a
// KeepLatest increase). For a deprecated version the existing date is preserved
// so the timestamp does not drift on every reconcile; otherwise the source's
// date is used. The updater never invents a date.
func (iu *ImageUpdater) resolveExpiration(src SourceImage, existing *metav1.Time) *metav1.Time {
	return cmp.Or(existing, src.ExpirationDate)
}

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The loop-prevention behavior is largely untested at the controller/provider integration points, so adding targeted regression tests is needed to prevent reintroducing reconcile churn.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

controllers/cloud_profile.go:54

• Add test coverage to ensure expiration dates on deprecated MachineImage versions are preserved across reconciles (i.e., an existing ExpirationDate is retained even if the source stamps a new one each run). This preservation is key to preventing CloudProfile spec churn, but there is no test asserting it in the controller suite.

		storedExpirations := collectExpirationDates(cloudProfile.Spec.MachineImages)
		cloudProfile.Spec = CloudProfileSpecToGardener(&mcp.Spec.CloudProfile)
		errs := make([]error, 0)
		for _, updates := range mcp.Spec.MachineImageUpdates {
			log.V(1).Info("updating machine images", "cloudProfile", cloudProfile.Name)
			if updateErr := r.updateMachineImages(ctx, log, updates, &cloudProfile.Spec); updateErr != nil {
				errs = append(errs, updateErr)
			}
		}
		applyExpirationDates(cloudProfile.Spec.MachineImages, storedExpirations)
		if mcp.Spec.KubernetesUpdate != nil {

cloudprofilesync/ossync/provider/openstack/provider.go:79

• Please add a unit test to lock in the new deterministic region ordering (e.g., feed regions in a different order and assert the resulting ProviderConfig has regions sorted by name/ID). Without a test, a future refactor could accidentally reintroduce ProviderConfig churn and the reconcile loop this change is addressing.

		// Sort regions by name so the marshaled ProviderConfig is stable across
		// reconciles; the source does not guarantee a consistent region order,
		// which would otherwise churn the CloudProfile and cause a reconcile loop.
		slices.SortFunc(entry.Regions, func(a, b openstackv1alpha1.RegionIDMapping) int {
			if c := cmp.Compare(a.Name, b.Name); c != 0 {
				return c
			}
			return cmp.Compare(a.ID, b.ID)
		})

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite