moved last applied spec from annotation to status by AndrewChubatiuk · Pull Request #1804 · VictoriaMetrics/operator

AndrewChubatiuk · 2026-02-14T17:13:47Z

fixes #1802
moved last applied spec from annotation to status
besides fixing given issue it should also decrease amount of requests during each reconcile since before two patch requests happened in case of spec diff (one to update annotation, another - to update status), now only one call is needed

Summary by cubic

Moved “last applied spec” from annotations to status.lastAppliedSpec for all CRDs to fix operator issue #1802 and cut reconcile API calls. Controllers now detect spec changes via status and apply updates in one step.

Refactors
- Added schemaless status.lastAppliedSpec to all CRDs and CRD schemas; change detection via LastSpecUpdated using equality.Semantic.DeepEqual.
- Removed annotation-based constants/helpers and JSON hooks (LastAppliedSpecAnnotation, parse/patch funcs) and the controller patchAnnotationPredicate; controller setups dropped WithEventFilter usage.
- Factories read Status.LastAppliedSpec for prevCR; updated deepcopy and tests.
- CRD overlays (crd.yaml and crd.specless.yaml) preserve unknown fields for status.lastAppliedSpec; small Makefile doc tweak for dynamic defaults.
Performance
- Replaced two requests (annotation patch + status) with a single status update on spec changes.

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

cubic-dev-ai

11 issues found across 53 files

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


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

<violation number="1" location="internal/controller/operator/controllers.go:361">
P0: Critical logic inversion: All `HasSpecChanges()` implementations return `equality.Semantic.DeepEqual(...)`, which is `true` when specs are **equal** (no changes). This inverts the boolean — `specChanged` will be `true` when nothing changed and `false` when the spec actually differs. The implementations should negate the result: `return !equality.Semantic.DeepEqual(&cr.Spec, cr.Status.LastAppliedSpec)`.</violation>
</file>

<file name="api/operator/v1beta1/vmalertmanager_types.go">

<violation number="1" location="api/operator/v1beta1/vmalertmanager_types.go:496">
P1: Inverted boolean logic: `HasSpecChanges()` returns `DeepEqual(...)` which is `true` when specs are **equal** (no changes), but the function name and callers expect `true` when there **are** changes. Should be negated.

Note: This pattern exists in other types too, but is currently masked by `UnmarshalJSON` always overwriting `LastAppliedSpec` with current spec. If that behavior is ever corrected to preserve the stored last-applied-spec, this inversion will cause real issues.</violation>
</file>

<file name="api/operator/v1beta1/vmauth_types.go">

<violation number="1" location="api/operator/v1beta1/vmauth_types.go:735">
P0: Inverted boolean logic: `equality.Semantic.DeepEqual` returns `true` when specs are **equal** (no changes), but the method name `HasSpecChanges` implies it should return `true` when there **are** changes. This will cause callers expecting `true` = "has changes" to skip reconciliation when changes exist and reconcile when nothing changed.</violation>
</file>

<file name="api/operator/v1beta1/vlogs_types.go">

<violation number="1" location="api/operator/v1beta1/vlogs_types.go:312">
P1: HasSpecChanges now returns true when the spec matches the last applied spec, which inverts the intended "changes" signal and will skip updates when the spec actually changed. Return the negated equality check instead.</violation>
</file>

<file name="api/operator/v1beta1/vmagent_types.go">

<violation number="1" location="api/operator/v1beta1/vmagent_types.go:244">
P2: UnmarshalJSON overwrites status.LastAppliedSpec with the current spec, discarding the stored last-applied state from the API server. This prevents detecting spec changes after the first reconcile.</violation>

<violation number="2" location="api/operator/v1beta1/vmagent_types.go:645">
P1: HasSpecChanges is inverted: it returns true when specs match and false when they differ. This will skip updates when the spec changes and trigger updates when nothing changed.</violation>
</file>

<file name="api/operator/v1/vlagent_types.go">

<violation number="1" location="api/operator/v1/vlagent_types.go:190">
P1: UnmarshalJSON overwrites Status.LastAppliedSpec on every decode, which discards the persisted last-applied spec from status and prevents detecting spec changes. Only initialize it when it’s missing or leave it untouched.</violation>

<violation number="2" location="api/operator/v1/vlagent_types.go:493">
P2: HasSpecChanges currently returns true when specs are equal, so it won’t report changes. Invert the comparison (and treat nil last-applied as changed) so the method reflects actual spec differences.</violation>
</file>

<file name="api/operator/v1/vlcluster_types.go">

<violation number="1" location="api/operator/v1/vlcluster_types.go:765">
P0: Bug: `HasSpecChanges()` returns inverted result. `equality.Semantic.DeepEqual` returns `true` when objects are **equal** (no changes), but the method name and all callers expect `true` when there **are** changes. This will cause the controller to always trigger update logic when nothing changed and skip updates when the spec actually changes.</violation>
</file>

<file name="api/operator/v1/vlsingle_types.go">

<violation number="1" location="api/operator/v1/vlsingle_types.go:192">
P1: Overwriting `Status.LastAppliedSpec` on every unmarshal erases the stored last-applied spec from the API, so reconcile can no longer detect changes between `spec` and the persisted status.</violation>
</file>

<file name="api/operator/v1beta1/vmalert_types.go">

<violation number="1" location="api/operator/v1beta1/vmalert_types.go:513">
P1: Bug: `HasSpecChanges()` returns inverted result. `equality.Semantic.DeepEqual` returns `true` when objects are **equal** (no changes), but this method should return `true` when there **are** changes. The result needs to be negated.</violation>
</file>

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

AndrewChubatiuk · 2026-02-14T18:21:05Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-02-14T18:21:10Z

@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 53 files

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="api/operator/v1beta1/vmsingle_types.go">

<violation number="1" location="api/operator/v1beta1/vmsingle_types.go:93">
P1: HasAnyStreamAggrRule now checks spec equality instead of whether stream aggregation rules exist, so callers will get true whenever the spec changed even if no aggregation rules are configured.</violation>
</file>

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

AndrewChubatiuk · 2026-02-17T12:45:42Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-02-17T12:45:52Z

@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 64 files

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


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

<violation number="1" location="internal/controller/operator/factory/vmdistributed/zone.go:105">
P1: Bug: `prevZ` reads from `cr` instead of `prevCR`, and additionally `buildVMAgentSpec` is a closure that captures `cr` — it uses `cr.Spec.ZoneCommon.VMAgent.Spec` and `cr.Spec.Zones` for merging and remote write construction. As a result, `prevAgentSpec` will always equal the current agent spec, making VMAgent change detection in `specsChanged()` completely non-functional.

Fix requires both changing `prevZ` to `&prevCR.Spec.Zones[i]` **and** parameterizing `buildVMAgentSpec` to accept the source CR (or creating a separate builder for the previous spec).</violation>
</file>

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

AndrewChubatiuk · 2026-02-17T12:58:01Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-02-17T12:58:06Z

@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 64 files

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


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

<violation number="1" location="internal/controller/operator/factory/vmdistributed/zone.go:187">
P2: Previous-zone lookup uses the current CR (`cr.Spec.Zones[i]`) instead of `prevCR.Spec.Zones[i]`, so the computed "prev" VMAgent spec isn’t actually from the previous CR. This can make specsChanged comparisons incorrect and skip/trigger upgrade steps unexpectedly. Use prevCR when selecting the previous zone.</violation>
</file>

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

<violation number="1" location="internal/controller/operator/factory/reconcile/statefulset.go:346">
P2: The local `pod` variable shadows the outer pointer, so `podStatusesToError` can never run and failures lose pod status diagnostics. Reuse the outer pointer instead of declaring a new local variable.</violation>
</file>

<file name="docs/api.md">

<violation number="1" location="docs/api.md:246">
P3: The new `VLAgentStatus` (and related `*Status`) links point to anchors that don’t exist in docs/api.md, which will render broken internal links. Add the missing status sections/headings or remove these status links to keep the docs navigation accurate.

(Based on your team's feedback about keeping docs metadata and links accurate.) [FEEDBACK_USED]</violation>
</file>

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

AndrewChubatiuk · 2026-02-18T12:52:04Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-02-18T12:52:10Z

@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 66 files

vrutkovs · 2026-02-24T11:19:32Z

 	StatusMetadata `json:",inline"`
 }

-type objectWithLastAppliedState[T, ST any] interface {


I like having this interface in general

since there's no code using it there's no value in this interface

vrutkovs · 2026-02-24T11:22:37Z

+// UpdateLastSpec compares spec with last applied spec stored, replaces old spec and returns true if it's updated
+func (cr *VMSingle) UpdateLastSpec() bool {
+	updated := cr.Status.LastAppliedSpec == nil || !equality.Semantic.DeepEqual(&cr.Spec, cr.Status.LastAppliedSpec)
+	cr.Status.LastAppliedSpec = cr.Spec.DeepCopy()


I wonder if its worth storing as JSON? This status can be corrupted and we won't be able to use it

if spec in status is corrupted it will fail during unmarshalling if it's stored as JSON, no difference

AndrewChubatiuk requested a review from vrutkovs as a code owner February 14, 2026 17:13

cubic-dev-ai Bot reviewed Feb 14, 2026

View reviewed changes

AndrewChubatiuk force-pushed the move-last-applied-spec-to-status branch from 93970b8 to a700e8a Compare February 14, 2026 18:16

cubic-dev-ai Bot reviewed Feb 14, 2026

View reviewed changes

Comment thread api/operator/v1beta1/vmsingle_types.go Outdated

AndrewChubatiuk force-pushed the move-last-applied-spec-to-status branch 2 times, most recently from a882838 to c697b75 Compare February 16, 2026 19:30

AndrewChubatiuk assigned vrutkovs Feb 17, 2026

AndrewChubatiuk force-pushed the move-last-applied-spec-to-status branch 2 times, most recently from 0c07e26 to e904206 Compare February 17, 2026 12:33

cubic-dev-ai Bot reviewed Feb 17, 2026

View reviewed changes

Comment thread internal/controller/operator/factory/vmdistributed/zone.go Outdated

AndrewChubatiuk force-pushed the move-last-applied-spec-to-status branch from e904206 to fc46987 Compare February 17, 2026 12:57

cubic-dev-ai Bot reviewed Feb 17, 2026

View reviewed changes

Comment thread internal/controller/operator/factory/vmdistributed/zone.go Outdated

Comment thread internal/controller/operator/factory/reconcile/statefulset.go Outdated

Comment thread docs/api.md Outdated

AndrewChubatiuk force-pushed the move-last-applied-spec-to-status branch 8 times, most recently from ff37878 to 6d1144f Compare February 18, 2026 12:47

cubic-dev-ai Bot reviewed Feb 18, 2026

View reviewed changes

AndrewChubatiuk force-pushed the move-last-applied-spec-to-status branch from 6d1144f to a1cd6a2 Compare February 18, 2026 13:39

AndrewChubatiuk force-pushed the move-last-applied-spec-to-status branch 3 times, most recently from 57bae4c to 73fb53c Compare February 18, 2026 18:29

vrutkovs reviewed Feb 24, 2026

View reviewed changes

AndrewChubatiuk force-pushed the move-last-applied-spec-to-status branch 2 times, most recently from ea5b860 to 1194f7c Compare February 24, 2026 13:14

moved last applied spec from annotation to status

a6b0e3d

AndrewChubatiuk force-pushed the move-last-applied-spec-to-status branch from 1194f7c to a6b0e3d Compare February 24, 2026 18:45

vrutkovs approved these changes Feb 25, 2026

View reviewed changes

AndrewChubatiuk merged commit 6554b1f into master Feb 25, 2026
6 of 7 checks passed

AndrewChubatiuk deleted the move-last-applied-spec-to-status branch February 25, 2026 09:19

Conversation

AndrewChubatiuk commented Feb 14, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by cubic

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewChubatiuk commented Feb 14, 2026

Uh oh!

cubic-dev-ai Bot commented Feb 14, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewChubatiuk commented Feb 17, 2026

Uh oh!

cubic-dev-ai Bot commented Feb 17, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewChubatiuk commented Feb 17, 2026

Uh oh!

cubic-dev-ai Bot commented Feb 17, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewChubatiuk commented Feb 18, 2026

Uh oh!

cubic-dev-ai Bot commented Feb 18, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vrutkovs Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

AndrewChubatiuk Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

vrutkovs Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

AndrewChubatiuk Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AndrewChubatiuk commented Feb 14, 2026 •

edited by cubic-dev-ai Bot

Loading