v1alpha2 Workflow Controller #874

chrisdoherty4 · 2024-01-29T16:23:09Z

Introduce basic functionality for a v1alpha2 Workflow Controller. This implementation is independent of the existing controller as we won't support the v1alpha1 API when we move to v1alpha2. The first commit moves deprecated code into /internal/deprecated and isn't that interesting: the work is mostly in the second commit.

The implementation renders the template actions onto the Workflow only which will aid in E2E tests with Tink Server. Logic such as remediating Workflow states is deferred to a later PR.

codecov · 2024-01-31T15:35:44Z

Codecov Report

Attention: Patch coverage is 60.58824% with 67 lines in your changes are missing coverage. Please review.

Project coverage is 50.61%. Comparing base (8f0edac) to head (8a89f80).

Files	Patch %	Lines
internal/workflow/internal/reconcile.go	63.15%	14 Missing and 7 partials ⚠️
internal/deprecated/workflow/reconciler.go	77.01%	17 Missing and 3 partials ⚠️
internal/workflow/reconciler.go	0.00%	19 Missing ⚠️
internal/workflow/internal/template.go	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #874      +/-   ##
==========================================
- Coverage   51.23%   50.61%   -0.63%     
==========================================
  Files          33       36       +3     
  Lines        1458     1549      +91     
==========================================
+ Hits          747      784      +37     
- Misses        665      712      +47     
- Partials       46       53       +7

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

jacobweinstock

Thanks for the work on this, @chrisdoherty4. Left some comments for discussion.

jacobweinstock · 2024-02-17T16:22:20Z

Makefile

@@ -49,21 +49,21 @@ help: ## Print this help
 VERSION ?= $(shell git rev-parse --short HEAD)

 # Define all the binaries we build for this project that get packaged into containers.
-BINARIES := tink-server tink-agent tink-worker tink-controller virtual-worker
+BINARIES := tink-server tink-agent tink-worker tink-controller tink-controller-v1alpha2 virtual-worker


not too excited we have to use tink-controller-v1alpha2 :(

jacobweinstock · 2024-02-17T16:23:21Z

api/v1alpha2/conditions.go

@@ -30,7 +30,7 @@ type Condition struct {
 	Status ConditionStatus `json:"status"`

 	// LastTransition is the last time the condition transitioned from one status to another.
-	LastTransition *metav1.Time `json:"lastTransitionTime"`
+	LastTransition metav1.Time `json:"lastTransitionTime"`


I know you love pointers ;) so why move away from one here?

This isn't optional because we expect the LastTransition to be updated even if we're populating the first status.

jacobweinstock · 2024-02-17T16:26:05Z

cmd/tink-controller-v1alpha2/main.go

@@ -0,0 +1,179 @@
+package main


is v1alpha2 so different from a cmd/cli perspective that we have to break it out entirely? Could a flag be used in the existing tink-controller?

I'm open to using a single binary. This temporary extra binary just felt simpler to me. There won't be a transition period where both APIs are supported so eventually tink-controller-v1alpha2 will become tink-controller.

jacobweinstock · 2024-02-17T16:27:25Z

cmd/tink-controller-v1alpha2/Dockerfile

@@ -0,0 +1,10 @@
+FROM alpine:3.15


any reason not to use scratch?

I don't know. The goal of the PR is to get something functional so I've not experimented.

jacobweinstock · 2024-02-17T16:29:13Z

cmd/tink-controller-v1alpha2/main.go

+	v := viper.New()
+	v.AutomaticEnv()
+	v.SetConfigName("tink-controller")
+	v.AddConfigPath("/etc/tinkerbell")


do we need a default location for a config file? does this provide much value, you think?

Probably not; this is copy-n-paste from the existing tink-controller. If we maintain the temporary v1alpha2 binary, I'll reduce this down to bare minimum so we don't end up with cruft down the road.

jacobweinstock · 2024-02-17T16:40:00Z

internal/workflow/internal/reconcile_test.go

@@ -0,0 +1,135 @@
+package internal_test


im not fond of internal_test files that use dot notation import for the package it's testing. I don't see the value. Not anything to hold up the PR, just wanted to voice it. : )

Do you mean specifically for internal packages, or do you mean generally the _test idiom with dot import?

Either way, dot imports are just a convenience thing, and this is the only acceptable usage.

jacobweinstock · 2024-02-17T16:41:58Z

internal/workflow/reconciler.go


-func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
+func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (result reconcile.Result, rerr error) {


are we gaining any value by having named return values? I don't see them being used, feels unnecessary.

The named returns are included for the defer to work because we change the return variable if patching fails. I don't think the code needs to work this way, and honestly I'm not fond of the defer approach - defering the patch is a common method for always patching.

oh its the rerr that is used not result. But obviously we cant just have a single named return. i see now.

Well, that's a good point: _.

jacobweinstock · 2024-02-17T16:43:07Z

internal/workflow/reconciler.go

-				// Mark the workflow as timed out
-				stored.Status.State = v1alpha1.WorkflowStateTimeout
-			}
+	// Always attempt to patch.


Why always attempt a patch?

The rationale is that you do as much as possible to bring the resource to the desired state and you shouldn't update the .Status state if nothing changed so always patching ensures we always try to persist the updates.

jacobweinstock · 2024-02-17T22:20:06Z

Hey @chrisdoherty4 , feel free to merge as is. We can address these before the official removal of v1alpha1.

Signed-off-by: Chris Doherty <[email protected]>

chrisdoherty4 force-pushed the feature/controllerv2 branch 9 times, most recently from d927835 to eb5eb09 Compare January 31, 2024 15:32

chrisdoherty4 force-pushed the feature/controllerv2 branch 3 times, most recently from c0c4a06 to f22864f Compare January 31, 2024 15:49

chrisdoherty4 requested a review from jacobweinstock January 31, 2024 15:50

chrisdoherty4 assigned jacobweinstock Jan 31, 2024

chrisdoherty4 marked this pull request as ready for review January 31, 2024 15:50

jacobweinstock reviewed Feb 17, 2024

View reviewed changes

jacobweinstock previously approved these changes Feb 17, 2024

View reviewed changes

chrisdoherty4 dismissed jacobweinstock’s stale review via cf8692a February 28, 2024 18:27

chrisdoherty4 force-pushed the feature/controllerv2 branch from f22864f to cf8692a Compare February 28, 2024 18:27

chrisdoherty4 added 2 commits February 28, 2024 12:32

Deprecate /internal/workflow

0c87805

Signed-off-by: Chris Doherty <[email protected]>

Initial v1alpha2 Workflow controller

8a89f80

Signed-off-by: Chris Doherty <[email protected]>

chrisdoherty4 force-pushed the feature/controllerv2 branch from cf8692a to 8a89f80 Compare February 28, 2024 18:32

chrisdoherty4 requested a review from jacobweinstock February 28, 2024 18:36

jacobweinstock approved these changes Feb 28, 2024

View reviewed changes

chrisdoherty4 added the ready-to-merge Signal to Mergify to merge the PR. label Feb 28, 2024

mergify bot merged commit 1e1b1a3 into tinkerbell:main Feb 28, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1alpha2 Workflow Controller #874

v1alpha2 Workflow Controller #874

chrisdoherty4 commented Jan 29, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

jacobweinstock left a comment

jacobweinstock Feb 17, 2024

jacobweinstock Feb 17, 2024

chrisdoherty4 Feb 28, 2024

jacobweinstock Feb 17, 2024

chrisdoherty4 Feb 17, 2024

jacobweinstock Feb 17, 2024

chrisdoherty4 Feb 17, 2024

jacobweinstock Feb 17, 2024

chrisdoherty4 Feb 17, 2024 •

edited

Loading

jacobweinstock Feb 17, 2024

chrisdoherty4 Feb 17, 2024 •

edited

Loading

jacobweinstock Feb 17, 2024

chrisdoherty4 Feb 28, 2024

jacobweinstock Feb 28, 2024

chrisdoherty4 Feb 28, 2024 •

edited

Loading

jacobweinstock Feb 17, 2024

chrisdoherty4 Feb 28, 2024

jacobweinstock commented Feb 17, 2024


		func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
		func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (result reconcile.Result, rerr error) {

v1alpha2 Workflow Controller #874

v1alpha2 Workflow Controller #874

Conversation

chrisdoherty4 commented Jan 29, 2024 • edited Loading

codecov bot commented Jan 31, 2024 • edited Loading

Codecov Report

jacobweinstock left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdoherty4 Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdoherty4 Feb 17, 2024 • edited Loading

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

chrisdoherty4 Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobweinstock commented Feb 17, 2024

chrisdoherty4 commented Jan 29, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

chrisdoherty4 Feb 17, 2024 •

edited

Loading

chrisdoherty4 Feb 17, 2024 •

edited

Loading

chrisdoherty4 Feb 28, 2024 •

edited

Loading