Instance controller refactoring #850

alenkacz · 2019-09-20T19:43:36Z

What this PR does / why we need it:
This is an implementation of controller based on KEP-18.

Some highlights:

start reviewing with the Reconcile function https://github.com/kudobuilder/kudo/pull/850/files#diff-45e4638e2c718ccd5d08635baec770b1R98
plan execution CRD, controller and everything around that was removed
this currently lacks unit tests, open to hear where would you add more coverage, I think the coverage problem already improved significantly with introduction of Plan execution logic extraction covered by first unit tests #759 which this one just builds on top of
documentation and flow diagrams will follow in another PR (I have them on paper so far only :))
with this PR we temporarily don't really support custom plan execution and for the correct plan execution we need the admission webhook which will come also in another PR

Fixes #422

gerred · 2019-09-24T03:03:17Z

pkg/controller/instance/plan_execution_engine.go

@@ -232,7 +245,7 @@ func prepareKubeResources(plan *activePlan, meta *executionMetadata, renderer ku
 	}

 	for _, phase := range plan.Spec.Phases {
-		phaseState, _ := getPhaseFromStatus(phase.Name, plan.State)
+		phaseState, _ := getPhaseFromStatus(phase.Name, plan.PlanStatus)


check error or remove it from the return signature of getPhaseFromStatus

I dislike the ignored error as well at first glance... however the error for an invalid name and we are getting the name for the phases. It is hard to see that this could error unless the plans change from underneath...

this is a private function that is use in only this use case... we should remove the error return from the function. I would also be inclined to from the prefix get -> phaseFromStatus. all of this is minor.

gerred · 2019-09-24T03:05:13Z

pkg/controller/instance/plan_execution_engine.go

 		}
 	}
 	return nil
 }

+func prettyPrint(i interface{}) string {
+	s, _ := json.MarshalIndent(i, "", "  ")


this could fail for custom marshalers, make sure if this errors to set some reasonable default at least ("{}"?) (so you can preserve your return signature)

gerred · 2019-09-24T03:07:21Z

pkg/controller/instance/plan_execution_engine.go

 	}

 	return newState, nil
 }

 func executeStep(step v1alpha1.Step, state *v1alpha1.StepStatus, resources []runtime.Object, c client.Client) error {
-	if isInProgress(state.State) {
-		state.State = v1alpha1.PhaseStateInProgress
+	if isInProgress(state.Status) {


General comment: I'm wondering if instead of doing all of this inline setting of status fields everywhere, we could create a set of transformer functions that take a state and the next step, executes it, and returns the state of the PE at that point. Could make everything a lot more fluent and set the stage for various transformations.

oh yeah, that was my original intentioon behing this executePlan refactoring but at that point I did not do it because it's hard to "compute the next step" - you need to figure out if it's update or create (that's easy) and then what to update (so compute the diff) which is harder. I think moving in this direction would make the test really testable though. Are you ok with me pursuing that direction in the next phase? as it's rather more a testability improvement in the execution engine than the Instance controller thing...

I'm ok with pursuing this in the next phase and agree 👍

kensipe

I'm looking to have a number of passes for review.
This pass is looking at plan-execution changes:

Beyond the suggestion provided... there is a label for PlanExecutionAnnotation
https://github.com/kudobuilder/kudo/blob/master/pkg/util/kudo/labels.go#L14
It isn't clear how it is used now... it is used in instance/apply_convention.go
which is an applyConventionsToTemplates. I don't think this is needed any longer (meaning the PEAnnonation)

kensipe · 2019-09-26T19:05:58Z

pkg/kudoctl/cmd/plan/plan_history.go

 		labelSelector = fmt.Sprintf("instance=%s", options.Instance)
 	} else {
-		fmt.Printf("History of plan-executions for instance \"%s\" in namespace \"%s\" to operator-version \"%s\":\n", options.Instance, options.Namespace, args[0])
+		fmt.Printf("History of plan-executions for instance \"%s\" in namespace \"%s\" for operator-version \"%s\":\n", options.Instance, options.Namespace, args[0])


Suggested change

fmt.Printf("History of plan-executions for instance \"%s\" in namespace \"%s\" for operator-version \"%s\":\n", options.Instance, options.Namespace, args[0])

fmt.Printf("History of plan executions for instance \"%s\" in namespace \"%s\" for operator-version \"%s\":\n", options.Instance, options.Namespace, args[0])

Looking at plan history cmd more... I'm not sure any of this makes sense. you mentioned in our walk thru that this is slated for the next release... which I completely support. I wonder if we should just remove these commands or stub them out. they will not be functional.

the following:

planExecutionsGVR := schema.GroupVersionResource{ Group: "kudo.dev", Version: "v1alpha1", Resource: "planexecutions", }

is not functional

What Ken said! ^^

kensipe · 2019-09-26T20:13:37Z

pkg/controller/instance/instance_controller.go

 		func(a handler.MapObject) []reconcile.Request {
 			requests := make([]reconcile.Request, 0)
-			// We want to query and queue up operators Instances
 			instances := &kudov1alpha1.InstanceList{}
 			// we are listing all instances here, which could come with some performance penalty
 			// a possible optimization is to introduce filtering based on operatorversion (or operator)


I'm less concerned with the optimization concern (which we should do... we should put a todo: here)
I'm more concerned that when the watch condition is triggered for 1 instance we perform this operation on ALL instances. Why do we not work solely with the instance associated with the trigger?

We want to handle edge and level... we need to handle level in a different way IMO... one that isn't driven by events in the system.

ok... after further review... I see that we loop thru all instances... but we only operator on the one defined by "a". my concern isn't valid... I still suggest the //TODO: but it isn't a big deal.

This is nothing new in this PR, this was already in before. I don't like TODOs in code, I am happy to file an issue though :) #860

kensipe · 2019-09-26T20:49:53Z

pkg/apis/kudo/v1alpha1/instance_types.go

+			for i2, p := range v.Phases {
+				planStatus.Phases[i2].Status = ExecutionPending
+				for i3 := range p.Steps {
+					i.Status.PlanStatus[n1].Phases[i2].Steps[i3].Status = ExecutionPending


it is unfortunate that i is instance here... and we have i2 and i3 as indexes. I would encourage expanding i into instance in this case. and use i, j,k as indexes

Agree. it's a nit, but still: using n1, i2, i3 for indexes/keys is confusing. short var names are good until they aren't ;)

If I rename this one I would have to rename all of them in the whole file :/ and I kind of like to use i for it, feels go-ish... I renamed the index to planIndex let me know if that helps

@alenkacz i've also seen Idx to go (heh heh) shorter in a lot of Go, so you could have:

planIdx, phaseIdx, stepIdx

this is a rule/situation where we can favor clarity over conciseness, because as it stands it would be really easy to refactor this, not thinking about indexes, and create a runtime out of bounds error. making those index names look like that embeds the semantic right in the access pattern.

pkg/apis/kudo/v1alpha1/instance_types.go

pkg/controller/instance/instance_controller.go

kensipe · 2019-09-26T21:01:37Z

pkg/controller/instance/instance_controller.go

+
+// getInstance retrieves the instance by namespaced name
+// returns nil, nil when instance is not found (not found is not considered an error)
+func (r *Reconciler) getInstance(request ctrl.Request) (instance *kudov1alpha1.Instance, err error) {


it looks like this should take a ctx context.Context as an argument. This function is called once from Reconcile and each step creates it's own context (which works of course) but the point of context is to carry the context throughout the execution.

same with getOperatorVersion

additionally, would it be helpful to create our own Context struct embedding conext.Context here?

Also, to counter this, including myself, we should question ourselves every time we think we need a context somewhere because context typically ends up being very viral in your codebase. I don't think it's an antipattern but should be very carefully considered as a whole before we do that.

I am not sure, let's talk about this... right now we don't really use the context at all, we always use context.TODO() ... until we start doing something meaningful with it I don't see much advantage of having it as dependency as it's only bloating the number of parameters that method has... Having it as dependency and then always passing in context.TODO() for now that feels a bit meaningless

gerred · 2019-09-26T21:05:07Z

pkg/controller/instance/instance_controller.go

-	// after have an active plan (which may be complete)
-	return instance.Status.ActivePlan.Name == ""
+	if len(missingRequiredParameters) != 0 {
+		return nil, &executionError{err: fmt.Errorf("parameters are missing when evaluating template: %s", strings.Join(missingRequiredParameters, ",")), fatal: true, eventName: kudo.String("Missing parameter")}


we should consider creating a Kubernetes event here as well.

we actually do! :) the execution error contains name of the event and that is then emitted "automatically" in handleError so the error type already holds the event information

We do. this executionError will bubble up and land in handleError method where an event will be generated.

pkg/controller/instance/plan_execution_engine.go

gerred · 2019-09-26T21:09:40Z

👍 to @kensipe's comments, and I need another pass after taking some more time to think about it all. This is a great PR. I have a lot of little things I want to think about in the full context before I suggest anything that actually blocks this PR from landing. In general, with maybe disabling the Plan History command for the moment (not having tried it or delved into @kensipe's comments there), I'm open to landing this and continuing to increment on a lot of those nits and see how it evolves with Tasker.

kensipe

I have a number of recommendations / nits... but nothing that should stop this.

nice work

kensipe · 2019-09-26T21:02:05Z

pkg/controller/instance/instance_controller.go

 	ov = &kudov1alpha1.OperatorVersion{}
-	err = c.Get(ctx,
+	err = r.Get(context.TODO(),


seems like it should be passed in

kensipe · 2019-09-26T23:17:10Z

pkg/controller/instance/instance_controller.go

-	if err != nil {
-		return nil, err
-	}
+type executionError struct {


executionError is confusing to me... many things could be an execution error... I'm not sure when to use it and how it is different from InstanceError. The places it is used it looks like a PlanExecutionError. perhaps it is fine... but I had to hunt for it's use to understand it which means like some challenge in readability.

and... although we collapse planexec into instance... I could still see having a planexecution package which would allow for planexec.NewError etc. or plan... like plan.NewError

kensipe

after more thought... I really don't like the broken CLI commands for plan. While I dislike adding more to a large PR... we can't merge this and then release... we will need to do something regarding plan commands.

kensipe · 2019-09-27T02:10:10Z

I'm ok with merge with plan cmd fixes... which could be removing them... or commenting them out.. or responding with a "coming soon message"

Co-Authored-By: Ken Sipe <[email protected]>

gerred · 2019-09-27T12:40:18Z

I'm ok with merge with plan cmd fixes... which could be removing them... or commenting them out.. or responding with a "coming soon message"

what if we just don't actually register them with the command handler so we can iterate on it and it's tests, then introduce it?

zen-dog

Lgtm! First batch of nits/comments out, more to come

P.S. don't forget to publish the events ;)

zen-dog · 2019-09-27T09:33:09Z

pkg/apis/kudo/v1alpha1/instance_types.go

+//       v                       v
+//+------+------+        +-------+--------+
+//| Fatal error |        |    Complete    |
+//+-------------+        +----------------+


👏 👏 👏

zen-dog · 2019-09-27T10:24:48Z

pkg/apis/kudo/v1alpha1/instance_types.go

+// EnsurePlanStatusInitialized initializes plan status for all plans this instance supports
+// it does not trigger run of any plan
+// it either initializes everything for a fresh instance without any status or tries to adjust status after OV was updated
+func (i *Instance) EnsurePlanStatusInitialized(ov *OperatorVersion) {


We talked about it offline, so it's merely a reminder: we need to unit tests this method. It's not very wild but merging Json manually deserves a few tests.

Also, as mentioned offline, we might wanna use apimachinery patch module to do a 2/3 way merge automatically.

zen-dog · 2019-09-27T11:32:52Z

pkg/apis/kudo/v1alpha1/instance_types.go

-const PhaseStatePending PhaseState = "PENDING"
+// isUpgradePlan returns true if this could be an upgrade plan - this is just an approximation because deploy plan can be used for both
+func isUpgradePlan(planName string) bool {
+	return planName == "deploy" || planName == "upgrade"


nit: we should put these magic strings into a const() at least.

zen-dog · 2019-09-27T11:49:05Z

pkg/apis/kudo/v1alpha1/instance_types.go

+			for i2, p := range v.Phases {
+				planStatus.Phases[i2].Status = ExecutionPending
+				for i3 := range p.Steps {
+					i.Status.PlanStatus[n1].Phases[i2].Steps[i3].Status = ExecutionPending


Agree. it's a nit, but still: using n1, i2, i3 for indexes/keys is confusing. short var names are good until they aren't ;)

zen-dog · 2019-09-27T11:50:00Z

pkg/apis/kudo/v1alpha1/instance_types.go


-// PhaseState captures the state of the rollout.
-type PhaseState string
+	// TODO in the future when we again support manual plan execution, snapshot should be saved only manually executed plans


Could you elaborate on why this should be the case?

zen-dog · 2019-09-27T12:10:24Z

pkg/apis/kudo/v1alpha1/instance_types.go

+
+	// new instance, need to run deploy plan
+	if i.NoPlanEverExecuted() {
+		return kudo.String("deploy"), nil


magic strings ;)

what's your point here? :DD

okay okay, I'll do better :)

zen-dog · 2019-09-27T12:15:35Z

pkg/apis/kudo/v1alpha1/instance_types.go

+
+	// did the instance change so that we need to run deploy/upgrade/update plan?
+	instanceSnapshot, err := i.getSnapshotedSpec()
+	if err != nil {


getSnapshotedSpec returns nil, nil when no annotations or no snapshotAnnotation was found but you only check the err != nil here (which could lead to a npe below)

pkg/apis/kudo/v1alpha1/instance_types.go

pkg/controller/instance/instance_controller.go

Co-Authored-By: Aleksey Dukhovniy <[email protected]>

…do into av/instance-controller-v2

zen-dog

Part 2 done. I left a few more nits but nothing that should prevent this PR from landing! 🚢 Great work!

zen-dog · 2019-09-27T14:47:34Z

pkg/apis/kudo/v1alpha1/instance_types.go

+	// TODO in the future when we again support manual plan execution, snapshot should be saved only manually executed plans
+	err := i.SaveSnapshot()
+	if err != nil {
+		return err


Should we wrap it into an InstanceError to make it visible on the event bus?

zen-dog · 2019-09-27T14:49:55Z

pkg/controller/instance/instance_controller.go

+// Automatically generate RBAC rules to allow the Controller to read and write Deployments
+// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=kudo.dev,resources=instances,verbs=get;list;watch;create;update;patch;delete
+func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {


This is one good looking Reconcile method! 👏

zen-dog · 2019-09-27T15:23:37Z

pkg/controller/instance/instance_controller.go

 	if err != nil {
-		log.Printf("InstanceController: Error creating PlanExecution")
-		return err
+		fmt.Printf("Pre-error instance %v", instance)


suggestion: remove this line - logging will happen in r. handleError

Suggested change

fmt.Printf("Pre-error instance %v", instance)

zen-dog · 2019-09-27T15:40:48Z

pkg/controller/instance/instance_controller.go

-	// after have an active plan (which may be complete)
-	return instance.Status.ActivePlan.Name == ""
+	if len(missingRequiredParameters) != 0 {
+		return nil, &executionError{err: fmt.Errorf("parameters are missing when evaluating template: %s", strings.Join(missingRequiredParameters, ",")), fatal: true, eventName: kudo.String("Missing parameter")}


We do. this executionError will bubble up and land in handleError method where an event will be generated.

zen-dog · 2019-09-27T15:52:07Z

pkg/kudoctl/cmd/plan/plan_history.go

 		labelSelector = fmt.Sprintf("instance=%s", options.Instance)
 	} else {
-		fmt.Printf("History of plan-executions for instance \"%s\" in namespace \"%s\" to operator-version \"%s\":\n", options.Instance, options.Namespace, args[0])
+		fmt.Printf("History of plan-executions for instance \"%s\" in namespace \"%s\" for operator-version \"%s\":\n", options.Instance, options.Namespace, args[0])


What Ken said! ^^

zen-dog · 2019-09-28T12:11:05Z

pkg/util/health/ready.go


-		if plan.Status.State == kudov1alpha1.PhaseStateComplete {
+		if obj.Status.AggregatedStatus.Status == kudov1alpha1.ExecutionComplete {


wdyt?

Suggested change

if obj.Status.AggregatedStatus.Status == kudov1alpha1.ExecutionComplete {

if isFinished(obj.Status.AggregatedStatus.Status){

kensipe

/lgtm

alenkacz added 8 commits September 17, 2019 13:49

Cleanup controller registration to the runtime 0.2.0

7e4605c

Formatting

7a9e240

More cleanup

639e55e

Skeleton of the new instance controller

eb920c9

Progress

f894bfd

Merge

46d754d

Implement plan execution in instance controller

ea1c146

Merge

a31c100

alenkacz requested review from gerred, kensipe and zen-dog as code owners September 20, 2019 19:43

alenkacz added 2 commits September 20, 2019 21:45

Typo

673bc9b

Remove dead code, better registration

4dcfc66

alenkacz requested a review from jbarrick-mesosphere as a code owner September 20, 2019 20:05

alenkacz added 2 commits September 22, 2019 09:37

Fix CLI commands and make linter happy

a927c61

Fix integration test, implement deploy plan trigger

57b37b5

gerred reviewed Sep 24, 2019

View reviewed changes

alenkacz added 7 commits September 24, 2019 19:36

Upgrade

123f496

Update

d6d0a40

OV reconciliation and update fix

b2e23c0

Go back to maps from slice of statuses

0213ef6

Fix when the plan snapshot has to be stored

c3fc262

Make the last test pass

7f4712e

Format

fb739b0

alenkacz changed the title ~~WIP: Instance controller refactoring~~ Instance controller refactoring Sep 25, 2019

alenkacz added 3 commits September 26, 2019 15:33

Add docs and missing events

0e81033

Extra comment

a315770

Fix format

7c48dca

kensipe reviewed Sep 26, 2019

View reviewed changes

pkg/apis/kudo/v1alpha1/instance_types.go Outdated Show resolved Hide resolved

kensipe reviewed Sep 26, 2019

View reviewed changes

pkg/controller/instance/instance_controller.go Outdated Show resolved Hide resolved

kensipe reviewed Sep 26, 2019

View reviewed changes

gerred reviewed Sep 26, 2019

View reviewed changes

pkg/controller/instance/plan_execution_engine.go Outdated Show resolved Hide resolved

kensipe approved these changes Sep 26, 2019

View reviewed changes

kensipe reviewed Sep 27, 2019

View reviewed changes

kensipe self-requested a review September 27, 2019 02:08

alenkacz and others added 2 commits September 27, 2019 08:45

Update pkg/apis/kudo/v1alpha1/instance_types.go

04b50ab

Co-Authored-By: Ken Sipe <[email protected]>

Some PR feedback

5a089bd

zen-dog reviewed Sep 27, 2019

View reviewed changes

alenkacz and others added 7 commits September 27, 2019 14:47

Remaining PR comments

3d0aa1b

Fix plan history CLI commands

f24a5cc

Update pkg/apis/kudo/v1alpha1/instance_types.go

b38a248

Co-Authored-By: Aleksey Dukhovniy <[email protected]>

Update pkg/apis/kudo/v1alpha1/instance_types.go

d02a3f6

Co-Authored-By: Aleksey Dukhovniy <[email protected]>

Merge branch 'av/instance-controller-v2' of github.com:kudobuilder/ku…

9c867fa

…do into av/instance-controller-v2

updating golden file and fixing segfault in instance controller

2eee2ee

Merge branch 'master' into av/instance-controller-v2

f90e0d1

zen-dog approved these changes Sep 28, 2019

View reviewed changes

alenkacz and others added 3 commits September 30, 2019 09:18

PR review comments

33c5666

adding required godocs

b2f841f

Merge branch 'master' into av/instance-controller-v2

9da4810

kensipe approved these changes Sep 30, 2019

View reviewed changes

alenkacz merged commit 37dd630 into master Sep 30, 2019

alenkacz deleted the av/instance-controller-v2 branch October 30, 2019 09:07

alenkacz mentioned this pull request Dec 16, 2019

Do not deepcopy, instance can be nil #1185

Merged

	fmt.Printf("History of plan-executions for instance \"%s\" in namespace \"%s\" for operator-version \"%s\":\n", options.Instance, options.Namespace, args[0])
	fmt.Printf("History of plan executions for instance \"%s\" in namespace \"%s\" for operator-version \"%s\":\n", options.Instance, options.Namespace, args[0])


		if plan.Status.State == kudov1alpha1.PhaseStateComplete {
		if obj.Status.AggregatedStatus.Status == kudov1alpha1.ExecutionComplete {

	if obj.Status.AggregatedStatus.Status == kudov1alpha1.ExecutionComplete {
	if isFinished(obj.Status.AggregatedStatus.Status){

Instance controller refactoring #850

Instance controller refactoring #850

Conversation

alenkacz commented Sep 20, 2019 • 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

Choose a reason for hiding this comment

kensipe 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

gerred Sep 27, 2019 • 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

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 commented Sep 26, 2019

kensipe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

kensipe commented Sep 27, 2019

gerred commented Sep 27, 2019

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

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

kensipe left a comment

Choose a reason for hiding this comment

alenkacz commented Sep 20, 2019 •

edited

Loading

gerred Sep 27, 2019 •

edited

Loading