refactor: create separate reconciler for control flow Stage #2848

hiddeco · 2024-10-25T21:35:02Z

The main problem with the Stage reconciler at present is the level of complexity, which makes it hard to reason about behavior due to the sheer number of responsibilities it handles, mixing both regular Stage and control flow Stage reconciliation logic into a single controller.

This PR is the first step in a larger refactoring effort aimed at improving the Stage reconciliation logic. The effort consists of two phases:

Splitting the reconciler into two separate controllers:
- A new ControlFlowStageReconciler that handles Stages without promotion templates (i.e. "control flow" Stages)
- The existing Stage reconciler (to be renamed) that handles Stages with promotion templates
This PR implements the first part by extracting control flow Stage logic into a dedicated controller.
Rewriting the remaining Stage reconciler to follow the same patterns established in this PR:
- Clear separation of concerns and more focused reconciliation logic
- Better testability through smaller components
- A less procedural and more observant flow
This will be addressed in a follow-up PR.

The end goal is to have two well-structured controllers that each handle a specific type of Stage, making the codebase easier to understand, test, and modify.

⚠️ Though the LOC count may appear large, the majority of changes are newly added tests — the actual logic is only around 1200 lines.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

netlify · 2024-10-25T21:35:19Z

✅ Deploy Preview for docs-kargo-io ready!

Name	Link
🔨 Latest commit	`405409d`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-kargo-io/deploys/67200f12093c8d0008292369
😎 Deploy Preview	https://deploy-preview-2848.docs.kargo.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-10-25T21:39:25Z

Codecov Report

Attention: Patch coverage is 66.13419% with 212 lines in your changes missing coverage. Please review.

Project coverage is 50.01%. Comparing base (3b4e223) to head (405409d).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/controller/stages/control_flow_stages.go	76.39%	95 Missing and 2 partials ⚠️
internal/controller/stages/stages.go	0.00%	85 Missing ⚠️
internal/controller/stages/event_handlers.go	80.00%	10 Missing and 5 partials ⚠️
cmd/controlplane/controller.go	0.00%	9 Missing ⚠️
internal/indexer/indexer.go	62.50%	3 Missing ⚠️
internal/indexer/shared_field_indexer.go	81.25%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2848      +/-   ##
==========================================
+ Coverage   48.82%   50.01%   +1.18%     
==========================================
  Files         270      274       +4     
  Lines       23941    24386     +445     
==========================================
+ Hits        11690    12197     +507     
+ Misses      11620    11539      -81     
- Partials      631      650      +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hiddeco · 2024-10-25T22:10:14Z

internal/controller/stages/control_flow_stages.go

+	// This index is used to find all Freight that are directly available from
+	// a Warehouse. It is used to find Freight that can be sourced directly from
+	// the Warehouse for the control flow Stage.
+	if err := mgr.GetFieldIndexer().IndexField(


This is a matter of personal preference, but I never liked the way the registration was factored out because it hid two important things from me when I wanted to know what a reconciler cared about:

The index field that's important to the reconciler.

The actual thing that decides how the value of this field is determined.

Given this, I took the liberty of not using the abstraction but rather doing the registration by hand here.

Excellent rationale. 👍

hiddeco · 2024-10-25T22:14:37Z

internal/controller/stages/control_flow_stages.go

+// SetupWithManager sets up the control flow Stage reconciler with the given
+// controller manager. It registers the reconciler with the manager and sets up
+// watches on the required objects.
+func (r *ControlFlowStageReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error {


Even if I like to be idiomatic and puristic at times, this is how controller-runtime projects do it — and would fit more into what we have set out for ourselves with #1479.

https://xkcd.com/927/

Far from being a hill I would die on, I think the advantage of the other approach was that a caller in another package could obtain a fully-initialized reconciler after a single function call. With this approach, it requires two, which creates the possibility of obtaining a partially-initialized reconciler.

As I said, it's not a hill I would die on. Merely explaining the rationale behind the prior approach.

hiddeco · 2024-10-25T22:17:46Z

internal/controller/stages/event_handlers.go

-		)
+	// If the Stage is a control flow Stage, there is no need to reconcile it.
+	if stage.IsControlFlow() {
+		return


Previously, this would always result in a reconciliation — even if there wasn't any actual interest.

hiddeco · 2024-10-25T22:17:53Z

internal/controller/stages/event_handlers.go

@@ -478,6 +513,11 @@ func (p *phaseChangedAnalysisRunHandler[T]) Update(
 			)
 		}
 		for _, stage := range stages.Items {
+			// If the Stage is a control flow Stage, there is no need to reconcile it.
+			if stage.IsControlFlow() {
+				continue


Previously, this would always result in a reconciliation — even if there wasn't any actual interest.

hiddeco · 2024-10-25T22:22:52Z

internal/controller/stages/control_flow_stages.go

+		}
+		return ctrl.Result{}, fmt.Errorf("failed to update Stage status: %w", err)
+	}
+	return ctrl.Result{}, reconcileErr


I need to test this more thoroughly, but I believe there is no actual reason for us to requeue on an interval here because we should be informed by a watcher or change event.

Tested this, and what's expected from it is triggered just fine without it.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Add a `SharedFieldIndexer` wrapper around `client.FieldIndexer` to prevent multiple reconcilers from creating duplicate indices for the same fields. This avoids errors by ensuring each field is only indexed once, while still allowing reconcilers to manage their indices independently. Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

hiddeco added 2 commits October 25, 2024 23:34

refactor: create separate reconciler for control flow Stage

9a24d04

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

chore: remove now obsolete code from Stage reconciler

d8fd176

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

hiddeco added the kind/refactor label Oct 25, 2024

hiddeco added this to the v1.1.0 milestone Oct 25, 2024

hiddeco added priority/normal area/controller labels Oct 25, 2024

hiddeco commented Oct 25, 2024

View reviewed changes

hiddeco force-pushed the factor-out-flow-stage branch from d7b4ca4 to 33a6bce Compare October 25, 2024 23:33

hiddeco marked this pull request as ready for review October 25, 2024 23:51

hiddeco requested a review from a team as a code owner October 25, 2024 23:51

hiddeco force-pushed the factor-out-flow-stage branch from 97b6bfd to c963f03 Compare October 28, 2024 20:45

hiddeco added 4 commits October 28, 2024 22:02

refactor: make event handlers flow Stage compatible

909313b

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

chore: wire control flow Stages reconciler into main controller

ea0f065

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

chore: add tests for event handlers

1380a31

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

chore: remove now obsolete check

3bca4b0

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

hiddeco force-pushed the factor-out-flow-stage branch from c963f03 to fbc5658 Compare October 28, 2024 21:03

krancour assigned hiddeco Oct 28, 2024

krancour approved these changes Oct 28, 2024

View reviewed changes

hiddeco added 4 commits October 28, 2024 23:23

fix(stage): configure name for control flow reconciler

3925e5c

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

chore: move logline to avoid count=0 confusion

d15c956

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

chore(indexer): remove now obsolete wrappers

405409d

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

hiddeco force-pushed the factor-out-flow-stage branch from 2d4af35 to 405409d Compare October 28, 2024 22:24

hiddeco enabled auto-merge October 28, 2024 22:25

hiddeco added this pull request to the merge queue Oct 28, 2024

Merged via the queue into akuity:main with commit b2be266 Oct 28, 2024
17 checks passed

hiddeco deleted the factor-out-flow-stage branch October 28, 2024 22:45

hiddeco mentioned this pull request Oct 28, 2024

refactor: remove over-abstraction of indexers #2867

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: create separate reconciler for control flow Stage #2848

refactor: create separate reconciler for control flow Stage #2848

hiddeco commented Oct 25, 2024 •

edited

Loading

netlify bot commented Oct 25, 2024 •

edited

Loading

codecov bot commented Oct 25, 2024 •

edited

Loading

hiddeco Oct 25, 2024

krancour Oct 28, 2024

hiddeco Oct 25, 2024

krancour Oct 28, 2024

hiddeco Oct 25, 2024

hiddeco Oct 25, 2024

hiddeco Oct 25, 2024

hiddeco Oct 28, 2024

refactor: create separate reconciler for control flow Stage #2848

refactor: create separate reconciler for control flow Stage #2848

Conversation

hiddeco commented Oct 25, 2024 • edited Loading

netlify bot commented Oct 25, 2024 • edited Loading

✅ Deploy Preview for docs-kargo-io ready!

codecov bot commented Oct 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiddeco commented Oct 25, 2024 •

edited

Loading

netlify bot commented Oct 25, 2024 •

edited

Loading

codecov bot commented Oct 25, 2024 •

edited

Loading