Clean up instance controller #380

gkleiman · 2019-06-22T00:32:40Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind cleanup

What this PR does / why we need it:
This PR cleans up and improves the readability of the instances controller. It improves comments, includes some refactoring code, and addresses an existing TODO.

Special notes for your reviewer:
I think that most of the code touched here should actually moved to the Reconcile method.

Does this PR introduce a user-facing change?:

NONE

zen-dog

Lgtm, though, I believe there is at least one breaking change with the default plan (see comments)

zen-dog · 2019-06-24T10:09:52Z

pkg/controller/instance/instance_controller.go

+						if !planFound {
+							// The parameter doesn't have a trigger, try to find the corresponding default plan.
+							names := []string{"update", "deploy"}
+							for _, n := range names {


Since we're refactoring this code - wdyt about extracting this loop into a separate function:

findPlan := func(names []string) (string, boolean) { for _, n := range names { if plan, found = fv.Spec.Plans[n]; found { return plan, found } return "", false }

and then calling it here (and above) with:

planName, planFound := findPlan([]string{"update", "deploy"})

I personally think that trying to understand the code for the first time would have to do an extra context switch to see what findPlan() actually does and then come back.

This would be justified if we'd findPlan() very often, but we only use it twice, so my personal opinion is that at this point extracting it into a separate function would only hurt readability.

My opinion on this matter is not very strong, so if you feel stronger about this, I can totally extract it =).

I would probably also extract it but I don't have strong opinion here :)

zen-dog · 2019-06-24T10:25:25Z

pkg/controller/instance/instance_controller.go

+									planName = n
+									planFound = true
+									break
+								}


Wait, previously (line 140 on the left) there is a default fallback planName = "deploy" if no plan was found. This is not the case on the right, correct?

The original line #140 isn't setting planName=deploy as a default fallback — it's actually only executed if ok == true, which means that deploy was found.

Line #151 might be an attempt to use deploy as a default fallback plan, but if that line is ever executed, ok will be false, so the PlanExecution will never be created.

You're right. But since we spent quite some time understanding all the ifs and elses of the logic, I believe we need to extract this portion of code into separate methods and provide a few tests for it (similar to what I did for parameterDifference. Wdyt?

To be honest I think that instead going further with the refactoring of the predicate function, I think we should get rid of it and move most of the logic to the Reconcile function, see my writeup on #422 for more details.

Instead of writing low level unit tests for something that we should change soon, I would rather start working on #422 or write a few unit tests to prepare the ground for #422.

BTW it looks like the original intention was to create a PlanExecution for deploy even if no changes were detected.

@runyontr recently created a PR that includes a change to make this happen: https://github.com/kudobuilder/kudo/pull/367/files#r297890298

zen-dog · 2019-06-24T10:27:13Z

pkg/controller/instance/instance_controller.go

-							planName = "update"
+
+							if planFound {
+								// FIXME: this can be misleading, the plan will only be used if this is the last parameter returned by `parameterDifference`.


Suggested change

// FIXME: this can be misleading, the plan will only be used if this is the last parameter returned by `parameterDifference`.

// FIXME: this can be misleading, the plan will only be used if this is the first parameter returned by `parameterDifference`.

afterward, planName (on the left) or planFound (on the right) would be set and we won't land here.

I think that neither my original comment nor your suggestion are completely accurate 😱.

This is all nested within the loop that starts on line 124 on the right. If for any changed param a trigger was defined (param.trigger != ""), then lines 142-143 will overwrite planName and planFound. This will happen regardless of whether we had previously logged (and "decided") that we were going to use a default plan. This means that the plan that we logged might actually not be used, even when it corresponds to the first parameter returned by parameterDifference.

I think that right now we don't have a test verifying which plan will be executed if parameters with different triggers have changed. We probably also haven't documented this. We should define what we want to happen... and then clean up the code a little more to reflect that and write a test to make sure that the code is correct.

I added this FIXME here to spark this discussion... but I forgot to add a comment when posting the PR. Thanks for noticing this =).

So now my question is: which plan should we run if multiple parameters have been changed triggering different triggers? cc/ @gerred @alenkacz @runyontr

We have another issue to discuss this one, let's not change this here. There's a lot of options and most of them will require a validating admission hook.

Cool, I wasn't aware of #354! — I'll change this comment to a TODO mentioning that we might need to revisit this part when #354 is resolved =).

zen-dog · 2019-06-24T10:28:52Z

pkg/controller/instance/instance_controller.go

@@ -164,7 +175,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 					if current.Status.State == kudov1alpha1.PhaseStateComplete {
 						log.Println("InstanceController: Current Plan for Instance is already done, won't change the Suspend flag.")
 					} else {
-						log.Println("InstanceController: Setting PlanExecution to Suspend")
+						log.Println("InstanceController: Suspending the PlanExecution")


Suggested change

log.Println("InstanceController: Suspending the PlanExecution")

log.Printf("InstanceController: Suspending the PlanExecution for instance %v", new.Name)

Note: this has to be log.Printf(...) instead of log.Println(...), I made the same mistake a few times during this refactoring 😳.

pkg/controller/instance/instance_controller.go

alenkacz

LGTM. Although doing any kind of refactoring while not backed by tests feels very ... dangerous :D I think we should invest in tests next time we're touching any code

alenkacz · 2019-06-25T07:11:14Z

pkg/controller/instance/instance_controller.go

-						planName = "deploy"
-					} else {
-						planName = "update"
+				// Its an upgrade!


Thanks for catching the typo!

alenkacz · 2019-06-25T07:14:55Z

pkg/controller/instance/instance_controller.go

-					// Find the right parameter in the FV
+					// Find the spec of the updated parameter.
+					//
+					// TODO: what should we do if more than one parameter was updated?


that should be addressed in #354

alenkacz · 2019-06-25T07:16:50Z

pkg/controller/instance/instance_controller.go

+						if !planFound {
+							// The parameter doesn't have a trigger, try to find the corresponding default plan.
+							names := []string{"update", "deploy"}
+							for _, n := range names {


I would probably also extract it but I don't have strong opinion here :)

gerred

I echo the thoughts here. Let's do a separate extraction PR but also make sure all of that is guarded as testable code so we can do these refactorings safely.

gerred · 2019-06-25T17:15:50Z

pkg/controller/instance/instance_controller.go

-							planName = "update"
+
+							if planFound {
+								// FIXME: this can be misleading, the plan will only be used if this is the last parameter returned by `parameterDifference`.


We have another issue to discuss this one, let's not change this here. There's a lot of options and most of them will require a validating admission hook.

gkleiman · 2019-06-26T22:20:50Z

After having spent a considerable amount of time trying to clean up this predicate function, I found a few fundamental issues that I describe in #422.

My proposal on #422 is to get rid of this predicate and to add more logic to the Reconcile method. I would honestly rather focus on that than on minor improvements to the predicate function.

However if you think that this PR is valuable, we can merge it. This should make the current implementation of InstanceController a bit easier to understand and read until #422 is resolved.

zen-dog · 2019-06-27T13:44:07Z

My proposal on #422 is to get rid of this predicate and to add more logic to the Reconcile method. I would honestly rather focus on that than on minor improvements to the predicate function.

Yeah, I agree. #422 looks like the right way to go. I'll retract my request for change and leave the decision of whether to merge it or not up to you ;)

Retracted

kensipe · 2019-06-28T03:31:59Z

@gkleiman this is based on an older version... we do not reference Frameworks anymore

NOTE: The update predicate function still inspects the status attribute and has side-effects. This is an anti-pattern that should be solved in a different patch, see #422.

gkleiman · 2019-07-09T07:38:19Z

I rebased this PR and verified that the integration tests introduced by #504 pass =).

gkleiman requested review from alenkacz, gerred, zen-dog, runyontr and fabianbaier June 22, 2019 00:32

gkleiman requested a review from kensipe as a code owner June 22, 2019 00:32

gkleiman changed the title ~~Gaston/cleanup instance controller~~ Clean up instance controller Jun 22, 2019

zen-dog previously requested changes Jun 24, 2019

View reviewed changes

alenkacz approved these changes Jun 25, 2019

View reviewed changes

gerred added this to the v0.3.0 milestone Jun 25, 2019

gerred added this to In progress in KUDO via automation Jun 25, 2019

gerred approved these changes Jun 25, 2019

View reviewed changes

gerred modified the milestones: v0.3.0, v0.3.1 Jun 26, 2019

gkleiman mentioned this pull request Jun 26, 2019

Wrong assumptions and anti-patterns in InstanceController #422

Closed

KUDO automation moved this from In progress to Reviewer approved Jun 27, 2019

Clean up instance controller.

0fb3be7

NOTE: The update predicate function still inspects the status attribute and has side-effects. This is an anti-pattern that should be solved in a different patch, see #422.

gkleiman force-pushed the gaston/cleanup-instance-controller branch from 3bb67e6 to 0fb3be7 Compare July 9, 2019 07:21

gkleiman requested a review from alenkacz July 9, 2019 07:38

alenkacz approved these changes Jul 9, 2019

View reviewed changes

Merge branch 'master' into gaston/cleanup-instance-controller

13d9db4

gerred merged commit 3eebef4 into master Jul 10, 2019

KUDO automation moved this from Reviewer approved to Done Jul 10, 2019

gerred deleted the gaston/cleanup-instance-controller branch July 10, 2019 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up instance controller #380

Clean up instance controller #380

gkleiman commented Jun 22, 2019 •

edited

Loading

zen-dog left a comment

zen-dog Jun 24, 2019

gkleiman Jun 24, 2019

alenkacz Jun 25, 2019

zen-dog Jun 24, 2019

gkleiman Jun 24, 2019

zen-dog Jun 25, 2019

gkleiman Jun 26, 2019

gkleiman Jun 26, 2019

zen-dog Jun 24, 2019

gkleiman Jun 24, 2019 •

edited

Loading

gerred Jun 25, 2019

gkleiman Jun 25, 2019

zen-dog Jun 24, 2019 •

edited

Loading

gkleiman Jun 24, 2019

alenkacz left a comment •

edited

Loading

alenkacz Jun 25, 2019

gkleiman Jun 25, 2019

alenkacz Jun 25, 2019

alenkacz Jun 25, 2019

gerred left a comment

gerred Jun 25, 2019

gkleiman commented Jun 26, 2019

zen-dog commented Jun 27, 2019

kensipe commented Jun 28, 2019

gkleiman commented Jul 9, 2019

	// FIXME: this can be misleading, the plan will only be used if this is the last parameter returned by `parameterDifference`.
	// FIXME: this can be misleading, the plan will only be used if this is the first parameter returned by `parameterDifference`.

	log.Println("InstanceController: Suspending the PlanExecution")
	log.Printf("InstanceController: Suspending the PlanExecution for instance %v", new.Name)

Clean up instance controller #380

Clean up instance controller #380

Conversation

gkleiman commented Jun 22, 2019 • edited Loading

zen-dog 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

Choose a reason for hiding this comment

gkleiman Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zen-dog Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz left a comment • 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

Choose a reason for hiding this comment

gerred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkleiman commented Jun 26, 2019

zen-dog commented Jun 27, 2019

kensipe commented Jun 28, 2019

gkleiman commented Jul 9, 2019

gkleiman commented Jun 22, 2019 •

edited

Loading

gkleiman Jun 24, 2019 •

edited

Loading

zen-dog Jun 24, 2019 •

edited

Loading

alenkacz left a comment •

edited

Loading