Fix race with ActivePlan between Instance and PlanExecution controllers #722

alenkacz · 2019-08-15T08:51:17Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Based on the test I wrote in #718 it became apparent where lies the race that is one of the causes of multiple deploy plan executions.

instance is created, create event triggers reconciliation
planexecution is created
OV is created and triggers reconciliation
another planexecution is created because ActivePlan is still empty
PlanExecutionController processes added planexecutions and create link between one of them and instance leaving two deploy planexecutions in progress, only one linked to instance, the other just hanging around forever

This PR proposes to move adding the ActivePlan relation to InstanceController. To make that possible I needed to add some safeguards to the PlanExecutionController as well.

Which issue(s) this PR fixes:

This is related to #628

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

alenkacz · 2019-08-15T11:10:29Z

/retest

alenkacz · 2019-08-15T13:09:27Z

pkg/controller/instance/instance_controller.go


-//todo: REMOVE this... we do not want to create a plan outside of reconcile
-// most likely... any use of this function is wrong... all plans should be created in reconcile
-func createPlanOld(mgr manager.Manager, planName string, instance *kudov1alpha1.Instance) error {


it was real pain maintaining two, while the only difference was how they accepted the dependencies which was pretty easy to unite

alenkacz · 2019-08-15T13:09:35Z

pkg/controller/instance/instance_controller.go

-	if err := controllerutil.SetControllerReference(instance, &planExecution, mgr.GetScheme()); err != nil {
-		log.Printf("InstanceController: Error setting ControllerReference")
-		return err
+func addActivePlanReference(c client.Client, r record.EventRecorder, planExecution *kudov1alpha1.PlanExecution, instance *kudov1alpha1.Instance) error {


main change

alenkacz · 2019-08-15T13:10:25Z

pkg/controller/planexecution/planexecution_controller.go

@@ -228,6 +227,11 @@ func (r *ReconcilePlanExecution) Reconcile(request reconcile.Request) (reconcile
 		return reconcile.Result{}, err
 	}

+	if instance.Status.ActivePlan.Name != planExecution.Name || instance.Status.ActivePlan.Namespace != planExecution.Namespace {


this is needed because otherwise the plan runs until the end but then cannot update instance because that one was modified in between

in general I feel like this safeguard is correct. If we're reconciling PE that is not current active plan, we probably should not execute anything

Couldn't this be executed right after the PE is created, but before the instance controller calls addActivePlanReference?

hmm good point, I guess it could. To overcome even that case I guess adding reconcile trigger after linking activeplan could be a solution? But I still think this safeguard is correct, because we should really execute only active plan, not every plan that changed for example...

oh wait, yeah, that should be fine, I think the PE controller should also watch for instance update event and trigger reconciliation as it does for deployments etc. Would you agree? Looking at the implementation right now I am not 100% that is happening and that it was intentional but if we agree on that I can push in that direction

EDIT: Oh yeah, right now it watches only for instances that were created as part of that plan not all of them... I would probably watch and reconcile for all updates. I mean controller-runtime will deal with de-duplication and the reconcile should be idempotent anyway

gerred · 2019-08-26T11:13:38Z

pkg/controller/instance/instance_controller.go

 	gvk, _ := apiutil.GVKForObject(instance, scheme)
 	ref := corev1.ObjectReference{
 		Kind:      gvk.Kind,
 		Name:      instance.Name,
 		Namespace: instance.Namespace,
 	}
 	planExecution := kudov1alpha1.PlanExecution{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "PlanExecution",
+			APIVersion: "kudo.dev/v1alpha1",


We have this dotted in a lot of places, so I looked for some "standardization" point that already exists in our codebase generated by kubebuilder, maybe we start moving to something more like this? Or at least turn it into a constant we can reference elsewhere. Maybe this is the constant?

kudo/pkg/apis/kudo/v1alpha1/register.go

Line 33 in 4def9dc

SchemeGroupVersion = schema.GroupVersion{Group: "kudo.dev", Version: "v1alpha1"}

let's try to unify this in another PR... I'll prepare one. Btw. how do you convert the groupversion to the string?

Agree, let's unify in a separate PR

👍 we'll have to look that up. hopefully it's something we can just set as a var and not have as a function call everywhere.

gerred · 2019-08-26T12:00:14Z

pkg/controller/planexecution/planexecution_controller.go

@@ -228,6 +227,11 @@ func (r *ReconcilePlanExecution) Reconcile(request reconcile.Request) (reconcile
 		return reconcile.Result{}, err
 	}

+	if instance.Status.ActivePlan.Name != planExecution.Name || instance.Status.ActivePlan.Namespace != planExecution.Namespace {


zen-dog · 2019-08-27T12:57:21Z

pkg/controller/instance/instance_controller.go

@@ -305,57 +305,42 @@ func instanceEventFilter(mgr manager.Manager) predicate.Funcs {
 	}
 }

-func createPlan(r *ReconcileInstance, planName string, instance *kudov1alpha1.Instance) error {
+func createPlan(c client.Client, r record.EventRecorder, scheme *runtime.Scheme, planName string, instance *kudov1alpha1.Instance) error {


This method should be called createPlanAndUpdateInstance or even createPlanAndUpdateInstanceActivePlanReerence :P

zen-dog · 2019-08-27T13:14:45Z

pkg/controller/planexecution/planexecution_controller.go

@@ -228,6 +252,11 @@ func (r *ReconcilePlanExecution) Reconcile(request reconcile.Request) (reconcile
 		return reconcile.Result{}, err
 	}

+	if instance.Status.ActivePlan.Name != planExecution.Name || instance.Status.ActivePlan.Namespace != planExecution.Namespace {
+		// this can happen for newly created PlanExecution where ActivePlan was not yet set to point to this instance
+		return reconcile.Result{}, fmt.Errorf("instance %s does not have ActivePlan pointing to PlanExecution %s, %s. Instead %s, %s. Retrying", instance.Name, planExecution.Name, planExecution.Namespace, instance.Status.ActivePlan.Name, instance.Status.ActivePlan.Namespace)


You're already watching instance updates, so there is no need to re-trigger this reconciliation (it might create a loop)

zen-dog · 2019-08-27T13:23:21Z

pkg/controller/planexecution/planexecution_controller.go

 	err = c.Watch(
 		&source.Kind{Type: &kudov1alpha1.Instance{}},
 		&handler.EnqueueRequestsFromMapFunc{
-			ToRequests: mapFn,
+			ToRequests: handler.ToRequestsFunc(


That's meta...

kudo-ci · 2019-08-27T13:46:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alenkacz, gerred

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alenkacz,gerred]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Add reference to PE inside instance controller

5718c4e

alenkacz requested review from gerred, kensipe and runyontr as code owners August 15, 2019 08:51

kudo-ci added the kind/bug label Aug 15, 2019

kudo-ci requested a review from guenter August 15, 2019 08:51

kudo-ci added approved size/M labels Aug 15, 2019

alenkacz added 11 commits August 15, 2019 11:08

Support owners in createPlanOld

e70cf09

Fill GVK

55853c2

Format

3cb71c7

TypeMeta

c6d6c70

Logging

219bf80

More logging

31216b3

Log

7e83752

More

b93f869

Switch order

d801821

Logs

3a4ce80

Hardcode the values, do not rely on PE

0185fe0

alenkacz added 4 commits August 15, 2019 13:53

Retry when update fails

d171bf7

Filter out PE without reference

58f4ae8

Format

004b3cd

Have only one createPlan method

96efe48

alenkacz changed the title ~~Add reference to PE inside instance controller~~ Fix race with ActivePlan between Instance and PlanExecution controllers Aug 15, 2019

alenkacz commented Aug 15, 2019

View reviewed changes

Merge branch 'master' into av/pe-reference-in-instance-controller

e209df7

gerred approved these changes Aug 26, 2019

View reviewed changes

Register to updates for root instances

063ca40

kudo-ci added size/L and removed size/M labels Aug 27, 2019

zen-dog reviewed Aug 27, 2019

View reviewed changes

gerred approved these changes Aug 27, 2019

View reviewed changes

alenkacz added 3 commits August 28, 2019 11:31

PR fixes

7e9c866

Add log

57645d4

Fix if-else

905e61f

alenkacz merged commit 47e7da7 into master Aug 28, 2019

kudo-ci deleted the av/pe-reference-in-instance-controller branch August 28, 2019 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race with ActivePlan between Instance and PlanExecution controllers #722

Fix race with ActivePlan between Instance and PlanExecution controllers #722

alenkacz commented Aug 15, 2019 •

edited

Loading

alenkacz commented Aug 15, 2019

alenkacz Aug 15, 2019

alenkacz Aug 15, 2019

alenkacz Aug 15, 2019

alenkacz Aug 15, 2019

gkleiman Aug 15, 2019

alenkacz Aug 26, 2019

gerred Aug 26, 2019

alenkacz Aug 26, 2019 •

edited

Loading

gerred Aug 26, 2019

alenkacz Aug 27, 2019

zen-dog Aug 27, 2019

gerred Aug 27, 2019

gerred Aug 26, 2019

zen-dog Aug 27, 2019

zen-dog Aug 27, 2019

zen-dog Aug 27, 2019 •

edited

Loading

kudo-ci commented Aug 27, 2019

Fix race with ActivePlan between Instance and PlanExecution controllers #722

Fix race with ActivePlan between Instance and PlanExecution controllers #722

Conversation

alenkacz commented Aug 15, 2019 • edited Loading

alenkacz commented Aug 15, 2019

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

alenkacz Aug 26, 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

zen-dog Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

kudo-ci commented Aug 27, 2019

alenkacz commented Aug 15, 2019 •

edited

Loading

alenkacz Aug 26, 2019 •

edited

Loading

zen-dog Aug 27, 2019 •

edited

Loading