chore: remove flux from kubernetes controller#2068
Merged
jakobmoellerdev merged 6 commits intoMar 26, 2026
Conversation
…ling Replace FluxCD runtime/conditions helpers with standard k8s.io/apimachinery/pkg/api/meta for condition management across all four controllers (Repository, Component, Resource, Deployer). - Add own condition type and reason constants (ReadyCondition, ReconcilingCondition, StalledCondition, SucceededReason, ProgressingWithRetryReason) with identical string values for backward compatibility - Create thin wrappers around apimeta in status/conditions.go - Add semantic requeue helper (status.RequeueResult) that derives ctrl.Result from condition state - Set EffectiveOCMConfig immediately after GetEffectiveConfig() succeeds so the deferred patch always persists correct status data - Remove all fluxcd/pkg/runtime/conditions imports from Go sources Signed-off-by: Jakob Möller <contact@jakob-moeller.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughReplaces FluxCD condition/patch/event helpers with local types and Kubernetes apimachinery equivalents across controllers, tests, and utilities. Adds local NamespacedObjectKindReference and condition helpers, changes status patching to deep-copy + conditional Status().Patch, removes manager.events.address config and Flux event/meta/runtime deps. Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant StatusPkg as status (helpers)
participant K8sAPI as Kubernetes API (client.Status())
participant ControllerMgr as Manager (event recorder)
Reconciler->>StatusPkg: compute EffectiveOCMConfig
alt EffectiveOCMConfig differs from Status
Reconciler->>Reconciler: set Status.EffectiveOCMConfig
Reconciler-->>Reconciler: return error (force status persist)
end
Reconciler->>StatusPkg: UpdateBeforePatch(obj, recorder, requeue, err)
Note right of Reconciler: deferred execution before final patch
Reconciler->>K8sAPI: Status().Patch(ctx, obj, client.MergeFrom(old)) when status changed
K8sAPI-->>Reconciler: patch result
Reconciler->>ControllerMgr: mgr.GetEventRecorderFor(...) used for events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
c9be67d to
3a39ffb
Compare
3a39ffb to
5ce966f
Compare
Replace the three remaining FluxCD packages with local implementations: - Replace meta.NamespacedObjectKindReference with identical local struct in api/v1alpha1/common_types.go (CRD-compatible, same JSON tags) - Replace eventv1.EventSeverityInfo/Error with own constants - Replace patch.SerialPatcher with local StatusPatcher using client.MergeFrom + client.Status().Patch() - Replace events.Recorder with standard mgr.GetEventRecorderFor() (drops Flux-specific --events-addr webhook forwarding) - Run go mod tidy to remove fluxcd from go.mod entirely BREAKING CHANGE: OCMConfiguration.NamespacedObjectKindReference type changed from github.com/fluxcd/pkg/apis/meta to local v1alpha1 type. The serialized JSON format is unchanged. Signed-off-by: Jakob Möller <contact@jakob-moeller.com>
- Replace StatusPatcher with equality.Semantic.DeepEqual checks and client.MergeFrom patches - Simplify status update logic in controllers by handling observed generation and status patching directly BREAKING CHANGE: StatusPatcher utility was removed, affecting custom patching logic. Signed-off-by: Jakob Möller <contact@jakob-moeller.com>
Signed-off-by: Jakob Möller <contact@jakob-moeller.com>
5ce966f to
ccfab50
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kubernetes/controller/internal/controller/deployer/deployer_controller_test.go (1)
997-1018:⚠️ Potential issue | 🟠 MajorRefresh the
MergeFrombase inside eachEventuallyretry.Lines 997 and 1274 capture
oldbefore the retry loop. If another reconcile updates the resource first, every retry reuses the same stale object/base pair, soStatus().Patch(...)repeats the conflict instead of recovering. Fetch the current resource inside the retry closure and use that as the merge base.Safer pattern
Eventually(func(ctx context.Context) error { current := &v1alpha1.Resource{} if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(resourceObj), current); err != nil { return err } old := current.DeepCopy() current.Status.Component = /* ... */ current.Status.Resource = /* ... */ status.MarkReady(recorder, current, "applied mock resource") current.SetObservedGeneration(current.GetGeneration()) return k8sClient.Status().Patch(ctx, current, client.MergeFrom(old)) }).WithContext(ctx).Should(Succeed())Also applies to: 1274-1296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/controller/internal/controller/deployer/deployer_controller_test.go` around lines 997 - 1018, The retry closure captures a stale "old" snapshot (old/ resourceObj) before Eventually, causing MergeFrom conflicts; update the closure used by Eventually to fetch the latest object via k8sClient.Get (into a local current variable), set current.Status.Component/Resource, call status.MarkReady(recorder, current, ...), take old := current.DeepCopy() and then call k8sClient.Status().Patch(ctx, current, client.MergeFrom(old)); apply the same change to the other Eventually block that currently reuses the precomputed old.
🧹 Nitpick comments (2)
kubernetes/controller/internal/controller/component/component_controller_unit_test.go (1)
398-404: Minor: Duplicate nil check.Line 403 duplicates the nil check already performed on line 399.
🔧 Remove duplicate assertion
cond := apimeta.FindStatusCondition(updatedComponent.GetConditions(), v1alpha1.ReadyCondition) g.Expect(cond).ToNot(BeNil()) g.Expect(cond.Reason).To(Equal(v1alpha1.ResolutionInProgress), "expected component ready-condition reason to be %s, got %s", v1alpha1.ResolutionInProgress, cond.Reason) - g.Expect(cond).ToNot(BeNil()) g.Expect(cond.Message).To(ContainSubstring("resolution in progress"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/controller/internal/controller/component/component_controller_unit_test.go` around lines 398 - 404, The test contains a duplicated nil-check on the resolved condition: remove the redundant g.Expect(cond).ToNot(BeNil()) that appears after the Reason assertion so the code uses a single nil-check (the one immediately after cond := apimeta.FindStatusCondition(updatedComponent.GetConditions(), v1alpha1.ReadyCondition)); keep the Reason and Message assertions (cond.Reason and cond.Message) as-is.kubernetes/controller/internal/controller/resource/resource_controller.go (1)
281-286: Consider clarifying the intent of the "effective ocm config changed" error.Returning an error here is intentional to trigger the deferred patch for persisting the updated
EffectiveOCMConfig. However, this pattern results in the reconciliation being retried via the error path rather than a clean requeue, and the error message may appear alarming in logs/events.This is a stylistic choice and not a bug since the deferred patch will correctly persist the config change, but you might consider:
- Using a sentinel error type to distinguish this from actual failures
- Or documenting this pattern with a comment explaining it's an intentional early-exit mechanism
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/controller/internal/controller/resource/resource_controller.go` around lines 281 - 286, Replace the generic error return used to trigger the deferred patch with a clear sentinel error and/or an explanatory comment: where the code currently checks equality.Semantic.DeepEqual(resource.Status.EffectiveOCMConfig, configs) and sets resource.Status.EffectiveOCMConfig = configs, add a short comment stating this is an intentional early-exit to persist the updated EffectiveOCMConfig via the deferred patch, and replace fmt.Errorf("effective ocm config changed") with a package-level sentinel error (e.g., ErrEffectiveOCMConfigChanged) so callers and logs can distinguish this intentional control flow from real failures; ensure code that handles reconciliation treats this sentinel appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kubernetes/controller/internal/controller/deployer/deployer_controller.go`:
- Around line 292-297: The defer currently only patches the status subresource
and so metadata mutations (finalizers, labels, annotations) made to the deployer
are lost; update the defer func (which currently calls
status.UpdateBeforePatch(deployer, r.EventRecorder, 0, err)) to first detect
metadata changes by comparing deployer.Finalizers, deployer.GetAnnotations(),
and deployer.GetLabels() to old.Finalizers/old annotations/old labels using
equality.Semantic.DeepEqual and, if any differ, call r.GetClient().Patch(ctx,
deployer, client.MergeFrom(old)) (joining any error with errors.Join), then
retain the existing check that patches the status subresource with
r.GetClient().Status().Patch(ctx, deployer, client.MergeFrom(old)) when
deployer.Status changed.
---
Outside diff comments:
In
`@kubernetes/controller/internal/controller/deployer/deployer_controller_test.go`:
- Around line 997-1018: The retry closure captures a stale "old" snapshot (old/
resourceObj) before Eventually, causing MergeFrom conflicts; update the closure
used by Eventually to fetch the latest object via k8sClient.Get (into a local
current variable), set current.Status.Component/Resource, call
status.MarkReady(recorder, current, ...), take old := current.DeepCopy() and
then call k8sClient.Status().Patch(ctx, current, client.MergeFrom(old)); apply
the same change to the other Eventually block that currently reuses the
precomputed old.
---
Nitpick comments:
In
`@kubernetes/controller/internal/controller/component/component_controller_unit_test.go`:
- Around line 398-404: The test contains a duplicated nil-check on the resolved
condition: remove the redundant g.Expect(cond).ToNot(BeNil()) that appears after
the Reason assertion so the code uses a single nil-check (the one immediately
after cond := apimeta.FindStatusCondition(updatedComponent.GetConditions(),
v1alpha1.ReadyCondition)); keep the Reason and Message assertions (cond.Reason
and cond.Message) as-is.
In `@kubernetes/controller/internal/controller/resource/resource_controller.go`:
- Around line 281-286: Replace the generic error return used to trigger the
deferred patch with a clear sentinel error and/or an explanatory comment: where
the code currently checks
equality.Semantic.DeepEqual(resource.Status.EffectiveOCMConfig, configs) and
sets resource.Status.EffectiveOCMConfig = configs, add a short comment stating
this is an intentional early-exit to persist the updated EffectiveOCMConfig via
the deferred patch, and replace fmt.Errorf("effective ocm config changed") with
a package-level sentinel error (e.g., ErrEffectiveOCMConfigChanged) so callers
and logs can distinguish this intentional control flow from real failures;
ensure code that handles reconciliation treats this sentinel appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d1338bd-da16-40ed-af68-5f014d50f18a
⛔ Files ignored due to path filters (1)
kubernetes/controller/go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
kubernetes/controller/api/v1alpha1/common_types.gokubernetes/controller/api/v1alpha1/condition_types.gokubernetes/controller/api/v1alpha1/interfaces.gokubernetes/controller/api/v1alpha1/zz_generated.deepcopy.gokubernetes/controller/chart/README.mdkubernetes/controller/chart/templates/manager/manager.yamlkubernetes/controller/chart/values.schema.jsonkubernetes/controller/chart/values.yamlkubernetes/controller/cmd/main.gokubernetes/controller/go.modkubernetes/controller/internal/configuration/config_test.gokubernetes/controller/internal/controller/component/component_controller.gokubernetes/controller/internal/controller/component/component_controller_test.gokubernetes/controller/internal/controller/component/component_controller_unit_test.gokubernetes/controller/internal/controller/deployer/deployer_controller.gokubernetes/controller/internal/controller/deployer/deployer_controller_test.gokubernetes/controller/internal/controller/repository/repository_controller.gokubernetes/controller/internal/controller/repository/repository_controller_test.gokubernetes/controller/internal/controller/resource/predicates.gokubernetes/controller/internal/controller/resource/predicates_test.gokubernetes/controller/internal/controller/resource/resource_controller.gokubernetes/controller/internal/controller/resource/resource_controller_test.gokubernetes/controller/internal/event/event.gokubernetes/controller/internal/event/event_test.gokubernetes/controller/internal/ocm/ocm_test.gokubernetes/controller/internal/resolution/service_test.gokubernetes/controller/internal/setup/integration_test.gokubernetes/controller/internal/status/conditions.gokubernetes/controller/internal/status/identifiable_contract.gokubernetes/controller/internal/status/mutate_condition_status.gokubernetes/controller/internal/status/register_status_defer.gokubernetes/controller/internal/status/requeue.gokubernetes/controller/internal/test/component.gokubernetes/controller/internal/test/k8s.gokubernetes/controller/internal/test/ocmrepository.gokubernetes/controller/internal/test/resource.gokubernetes/controller/internal/util/k8s.go
💤 Files with no reviewable changes (5)
- kubernetes/controller/chart/values.yaml
- kubernetes/controller/chart/values.schema.json
- kubernetes/controller/chart/templates/manager/manager.yaml
- kubernetes/controller/go.mod
- kubernetes/controller/chart/README.md
The defer block in the deployer Reconcile function only called Status().Apply(), which persists the status subresource but not object metadata. Finalizers added (applySetPruneFinalizer, resourceWatchFinalizer) and removed in reconcileDeletionTimestamp were never written back to the API server, causing new deployers to silently miss cleanup finalizers and deleting deployers to get stuck. Fix by capturing a DeepCopy of the deployer before reconciliation and, in the defer, issuing a MergeFrom patch when Finalizers differ. Adds integration tests that verify both finalizer addition (deployer reaches Ready with both finalizers present) and removal (deployer is fully deleted without getting stuck). Signed-off-by: Jakob Möller <contact@jakob-moeller.com>
frewilhelm
reviewed
Mar 26, 2026
fabianburth
approved these changes
Mar 26, 2026
fabianburth
left a comment
Contributor
There was a problem hiding this comment.
One comment, not critical. LGTM
frewilhelm
approved these changes
Mar 26, 2026
b084c3f
into
open-component-model:main
39 of 45 checks passed
This was referenced Mar 27, 2026
5 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
this drops flux from the controller dependencies completely.
Which issue(s) this PR fixes
resolves status update issues we had due to the status patcher and drops our dependency in code to flux
this also fixes up a bug where the status was not patched in one go, and where the status was patched after the conditions were set.