reconcile: unify recreating object handling, updated metadata comparing logic

[AndrewChubatiuk]

unify reconcile logic, fix issue with recreated resource finalizer removal, removed 3-way merge logic for labels

---

Summary by cubic

Unifies reconciliation across Kubernetes objects and fixes stuck updates when resources are deleted and recreated. Adds a safer 3‑way metadata merge, standardizes scrape object management, introduces an Ingress reconciler, and ensures sequential rollouts.

• Bug Fixes

  • Recreating a resource after deletion no longer hangs; finalizers are cleaned and the object is recreated on the next loop.
  • StatefulSet/Deployment/DaemonSet rollouts are now sequential; removed nested retry that caused parallel pod updates.
  • Consistent recreate-and-retry handling applied to ConfigMap, Secret, Service, ServiceAccount, HPA, PDB, RBAC, Deployments/DaemonSets/StatefulSets, HTTPRoute, and Ingress.

• Refactors

  • 3‑way merge for annotations (mergeMaps) preserves third‑party metadata and removes only operator-managed keys.
  • Unified reconcile signatures to newObj/prevObj with retry-on-conflict; renamed HandleSTSUpdate to StatefulSet; added Ingress reconciler.
  • Scrape CRDs now built via small helpers with prev‑object support; service funcs stop returning objects; tests updated.

^{Written for commit ea86bf3. Summary will update on new commits.}

[cubic-dev-ai]

11 issues found across 46 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="internal/controller/operator/factory/vtcluster/select.go">

<violation number="1" location="internal/controller/operator/factory/vtcluster/select.go:75">
P1: buildVTSelectScrape dereferences cr.Spec.Select without guarding for nil, but it is called with prevCR even when prevCR.Spec.Select may be nil. This can panic during reconcile when a previous CR version lacks the Select spec.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/configmap.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/configmap.go:41">
P2: The `updated` flag is never set to false on the no-change fast path, so the function always reports an update even when nothing changed. This can trigger unnecessary downstream logic in callers that depend on the boolean.</violation>
</file>

<file name="internal/controller/operator/factory/vtcluster/storage.go">

<violation number="1" location="internal/controller/operator/factory/vtcluster/storage.go:54">
P2: Guard against `prevCR.Spec.Storage == nil` in buildVTStorageScrape to avoid a nil dereference when reconciling from a previously unset Storage spec.</violation>
</file>

<file name="internal/controller/operator/factory/vtcluster/insert.go">

<violation number="1" location="internal/controller/operator/factory/vtcluster/insert.go:264">
P2: buildVTInsertScrape dereferences cr.Spec.Insert without guarding against nil, so a previous CR without Insert spec will panic when building prevSvs.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/hpa.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/hpa.go:41">
P2: Updating HPA labels via direct assignment drops 3rd‑party/unmanaged labels. Use the same 3‑way merge logic for labels (like `mergeMaps`/`areMapsEqual`) to preserve external labels and avoid churn when extra labels exist.</violation>
</file>

<file name="internal/controller/operator/factory/vmcluster/vmcluster.go">

<violation number="1" location="internal/controller/operator/factory/vmcluster/vmcluster.go:438">
P2: VMStorage’s VMServiceScrape no longer includes the vmbackupmanager port. VMServiceScrape filters out non-"http" ports unless explicitly listed, so this change drops scraping for vmbackupmanager when VMBackup is enabled.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/rbac.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/rbac.go:42">
P0: Critical bug: `existingObj.Subjects` and `existingObj.RoleRef` are never updated with values from `newObj` before calling `rclient.Update`. Changes to Subjects and RoleRef are detected but never persisted.</violation>

<violation number="2" location="internal/controller/operator/factory/reconcile/rbac.go:43">
P0: Critical bug: `existingObj.Rules` is never updated with `newObj.Rules` before calling `rclient.Update`. The comparison detects differences in Rules, but the update only applies Labels and Annotations. Add `existingObj.Rules = newObj.Rules` before the Update call.</violation>
</file>

<file name="internal/controller/operator/factory/vlcluster/vlselect.go">

<violation number="1" location="internal/controller/operator/factory/vlcluster/vlselect.go:75">
P2: Guard against prevCR.Spec.VLSelect being nil before dereferencing in buildVLSelectScrape to avoid a panic when the previous CR omitted VLSelect.</violation>
</file>

<file name="internal/controller/operator/factory/vtcluster/vmauth_lb.go">

<violation number="1" location="internal/controller/operator/factory/vtcluster/vmauth_lb.go:232">
P2: Guard against a nil Service in buildVMAuthScrape. When enabling scraping after a previously disabled load balancer, `prevSvc` is nil and this call can panic.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/daemonset.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/daemonset.go:57">
P2: Use the previous pod template annotations (not object-level annotations) when merging template annotations; otherwise removed template annotations may be retained because `mergeMaps` never sees them in `prevTemplateAnnotations`.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

7 issues found across 47 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="internal/controller/operator/factory/reconcile/secret.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/secret.go:41">
P2: Overwriting `existingObj.Labels` removes third‑party labels from the Secret on every update. Use the same 3‑way merge strategy as annotations (and the previous mergeObjectMetadataIntoNew behavior) so unmanaged labels are preserved.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/configmap.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/configmap.go:44">
P2: Labels are overwritten instead of 3‑way merged, so any third‑party labels on the ConfigMap will be dropped on every update. Use the same merge strategy as annotations (with previous labels) to preserve non-operator metadata.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/pdb.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/pdb.go:42">
P2: Directly assigning `existingObj.Labels = newObj.Labels` removes labels added outside the operator. Use the same 3‑way merge strategy as annotations (with prev labels) so third‑party labels are preserved and updates are idempotent.</violation>
</file>

<file name="internal/controller/operator/factory/vtcluster/vmauth_lb.go">

<violation number="1" location="internal/controller/operator/factory/vtcluster/vmauth_lb.go:233">
P2: Guard against nil MatchLabels before assigning to it; a custom selector with MatchExpressions but no MatchLabels will cause a panic here.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/statefulset_pvc_expand.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/statefulset_pvc_expand.go:142">
P2: The arguments to `areMapsEqual` are reversed. `src` is the existing PVC and `dst` is the desired PVC, but the call passes desired first. This makes the equality check fail when existing annotations include extra third‑party keys, causing unnecessary updates that wipe those annotations. Swap the arguments to match the expected `(existing, desired, prev)` order.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/service_account.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/service_account.go:39">
P2: Overwriting ServiceAccount labels removes any existing third‑party labels; merge labels the same way annotations are merged to avoid deleting unmanaged keys.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/deploy.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/deploy.go:67">
P2: Overwriting newObj.Labels with existingObj.Labels drops desired label updates, so label changes in the new deployment are never reconciled.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[AndrewChubatiuk]

@cubic-dev-ai review this PR

[cubic-dev-ai]

@cubic-dev-ai review this PR

@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

3 issues found across 47 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="internal/controller/operator/factory/vmauth/vmauth.go">

<violation number="1" location="internal/controller/operator/factory/vmauth/vmauth.go:456">
P2: Guard prev ingress creation when the previous spec didn't include an Ingress; otherwise buildIngressConfig will panic on nil Spec.Ingress when Ingress is newly enabled.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/deploy.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/deploy.go:67">
P2: This assignment discards the desired labels before Update, so label changes will never be applied (the reconcile keeps reattempting). Keep the desired labels from newObj instead of overwriting them with existing labels.</violation>
</file>

<file name="internal/controller/operator/factory/vmcluster/vmcluster.go">

<violation number="1" location="internal/controller/operator/factory/vmcluster/vmcluster.go:246">
P2: Initialize MatchLabels before assigning to it; VMServiceScrape can return a selector with nil MatchLabels when only MatchExpressions are defined.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[AndrewChubatiuk]

@cubic-dev-ai review this PR

[cubic-dev-ai]

@cubic-dev-ai review this PR

@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

No issues found across 47 files

[cubic-dev-ai]

2 issues found across 7 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="internal/controller/operator/factory/reconcile/service.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/service.go:84">
P1: `recreateService` is now called with the desired Service object, so finalizer removal patches the live Service with the desired object’s finalizers/owner refs. Because the desired Service only includes the operator finalizer, this can inadvertently strip third‑party finalizers from the existing Service. Use `existingObj` for recreate/delete operations.</violation>
</file>

<file name="internal/controller/operator/factory/reconcile/statefulset.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/statefulset.go:108">
P2: The HPA safeguard is ineffective: assigning `newObj.Spec.Replicas` to itself does not preserve the existing replica count, so updates can override HPA-controlled scaling. Use the current StatefulSet’s replicas instead to avoid resetting HPA-managed values.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[AndrewChubatiuk]

@cubic-dev-ai review this PR

[cubic-dev-ai]

@cubic-dev-ai review this PR

@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

1 issue found across 48 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="internal/controller/operator/factory/reconcile/ingress.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/ingress.go:46">
P3: Compute the spec diff before overwriting existingObj.Spec; otherwise spec_diff is always empty and the log is misleading.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[AndrewChubatiuk]

@cubic-dev-ai review this PR

[cubic-dev-ai]

@cubic-dev-ai review this PR

@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

No issues found across 48 files

[AndrewChubatiuk]

@cubic-dev-ai review this PR

[cubic-dev-ai]

@cubic-dev-ai review this PR

@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

1 issue found across 50 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="internal/controller/operator/factory/reconcile/secret.go">

<violation number="1" location="internal/controller/operator/factory/reconcile/secret.go:36">
P2: `Secret.StringData` is a write-only convenience field in Kubernetes. It is cleared (empty) when reading resources from the API. Comparing the local `newObj.StringData` against the fetched `existingObj.StringData` (which is always empty) is incorrect.

If `newObj.StringData` is populated, this check will always return `false`, forcing an unnecessary update (and potentially an infinite reconcile loop if `Data` matches). Remove this comparison and rely on `Data` (which acts as the canonical source of truth) or ensure `newObj` is normalized to `Data` before comparison.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}