Simplify waiting for the instance to be done #1636

zen-dog · 2020-08-06T12:01:45Z

Summary:
previous logic relied on figuring out the plan in progress and the plan status from the instance status. However, since 1.15 this can be simplified by using the Spec.PlanExecution which already has the relevant information.

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com

Summary: previous logic relied on figuring out the plan in progress and the plan status from the instance status. However, since 1.15 this can be simplified by using the `Spec.PlanExecution` which already has the relevant information. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

alenkacz

Still have couple of questions before I am ready to approve :)

alenkacz · 2020-08-06T12:51:02Z

pkg/kudoctl/cmd/plan/plan_status.go

@@ -120,7 +120,7 @@ func status(kc *kudo.Client, options *Options, ns string) error {
 		} else {
 			break
 		}
-		done, err := kc.IsInstanceDone(instance, nil)
+		done, err := kc.IsInstanceDone(instance, instance.Spec.PlanExecution.PlanName)


why are you passing in instance as well as field on that instance? 🤔 should we just pass in an instance?

Good eye ;) basically, because I don't know how this method will be used:

the passed instance has to be the one returned by Create/Update/Patch method because it has the PlanExecution.PlanName field set to the right plan

if you simply fetch the instance later, the plan might be already done (which is not a big deal), or there might be already another plan (from a later update) in progress

Passing a plan explicitly was an attempt to make the usage of this method slightly safer.

alenkacz · 2020-08-06T12:52:18Z

pkg/kudoctl/packages/install/package.go

@@ -73,12 +73,13 @@ func Package(
 		return nil
 	}

-	if err := install.Instance(client, resources.Instance); err != nil {
+	installed, err := install.Instance(client, resources.Instance)


hmm why is this better than what was here before? 🤔

I'm ok with this, tbh, it enforces the "always use the returned object if you create one" pattern... It allows you to update the object later one:

pod := corev1.Pod{ ... } pod := client.Create(pod) pod.metadata.Labels["something"] = "addedlabel" pod := client.Update(pod)

This won't work if you try to do:

pod := corev1.Pod{ ... } client.Create(pod) pod.metadata.Labels["something"] = "addedlabel" client.Update(pod)

because of the added fields from the api-server...

see my comment above

ANeumann82

Looks generally good, needs a bit of cleanup though

ANeumann82 · 2020-08-06T13:23:31Z

pkg/kudoctl/util/kudo/kudo.go

-	status := lastPlanStatus.Status
-	if status.IsFinished() {
-		clog.V(2).Printf("plan status for %q is finished\n", instance.Name)
+func (c *Client) IsInstanceDone(instance *v1beta1.Instance, plan string) (bool, error) {


I think this func should move to the "instance_helpers.go" and have the instance as the receiver. I'm not sure why it's part of the kudo.Client..

And then it should probably be something like

func (i *v1beta1.Instance) IsPlanDone(plan string) (bool, error) {

Maybe with an additional

func ( i *v1beta1.Instance) IsCurrentPlanDone() (bool, error) {

ANeumann82 · 2020-08-06T13:28:47Z

pkg/kudoctl/packages/install/package.go

@@ -73,12 +73,13 @@ func Package(
 		return nil
 	}

-	if err := install.Instance(client, resources.Instance); err != nil {
+	installed, err := install.Instance(client, resources.Instance)


I'm ok with this, tbh, it enforces the "always use the returned object if you create one" pattern... It allows you to update the object later one:

pod := corev1.Pod{ ... } pod := client.Create(pod) pod.metadata.Labels["something"] = "addedlabel" pod := client.Update(pod)

This won't work if you try to do:

pod := corev1.Pod{ ... } client.Create(pod) pod.metadata.Labels["something"] = "addedlabel" client.Update(pod)

because of the added fields from the api-server...

ANeumann82 · 2020-08-06T13:30:16Z

pkg/kudoctl/util/kudo/kudo.go

 // IsInstanceDone provides a check on instance to see if it is "finished" without retries
 // oldInstance is nil if there is no previous instance


This comment should probably be adjusted. I have no idea what "finished" means in the context here... Also, oldInstance isn't a parameter anymore

porridge

TBH I'm not a big fan of using spec for holding status information, and still hope this can be changed at some point. This change seems to move us further away from that...

zen-dog · 2020-08-07T09:08:12Z

TBH I'm not a big fan of using spec for holding status information, and still hope this can be changed at some point. This change seems to move us further away from that...

The introduced logic relies on Spec.PlanExecution.PlanName which expresses the user intent to run a plan (e.g. from kudoctl plan trigger...). This field belongs certainly to a Spec and not to Status. The Spec.PlanExecution.Status field on the other hand is only used for logging.

porridge · 2020-08-10T05:28:45Z

TBH I'm not a big fan of using spec for holding status information, and still hope this can be changed at some point. This change seems to move us further away from that...

The introduced logic relies on Spec.PlanExecution.PlanName which expresses the user intent to run a plan (e.g. from kudoctl plan trigger...). This field belongs certainly to a Spec and not to Status.

Well, if it was purely intent, then we would not be able to determine state from it, but yet we do :-)
Since the value is controlled by the webhook which in turn looks at status, the boundary between state and status has become quite fuzzy. This is one of my concerns...

…o instance Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82

lgtm 🚢

zen-dog · 2020-10-07T15:06:03Z

We discussed this offline with @ANeumann82 a while ago and I don't remember the exact reasoning but we were both not happy with this PR. Something about handling the "edginess" of waiting for a plan to be over.

zen-dog requested review from alenkacz, gerred, kensipe and nfnt as code owners August 6, 2020 12:01

zen-dog requested review from ANeumann82 and porridge August 6, 2020 12:01

alenkacz reviewed Aug 6, 2020

View reviewed changes

ANeumann82 reviewed Aug 6, 2020

View reviewed changes

porridge reviewed Aug 6, 2020

View reviewed changes

Renamed "IsInstanceDone" to "IsPlanDone" and moved from kudo client t…

2f28bcc

…o instance Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 approved these changes Aug 20, 2020

View reviewed changes

zen-dog closed this Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify waiting for the instance to be done #1636

Simplify waiting for the instance to be done #1636

zen-dog commented Aug 6, 2020

alenkacz left a comment

alenkacz Aug 6, 2020

zen-dog Aug 6, 2020

alenkacz Aug 6, 2020

ANeumann82 Aug 6, 2020

zen-dog Aug 6, 2020

ANeumann82 left a comment

ANeumann82 Aug 6, 2020

ANeumann82 Aug 6, 2020

ANeumann82 Aug 6, 2020

porridge left a comment

zen-dog commented Aug 7, 2020

porridge commented Aug 10, 2020

ANeumann82 left a comment

zen-dog commented Oct 7, 2020

		// IsInstanceDone provides a check on instance to see if it is "finished" without retries
		// oldInstance is nil if there is no previous instance

Simplify waiting for the instance to be done #1636

Simplify waiting for the instance to be done #1636

Conversation

zen-dog commented Aug 6, 2020

alenkacz 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

ANeumann82 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

porridge left a comment

Choose a reason for hiding this comment

zen-dog commented Aug 7, 2020

porridge commented Aug 10, 2020

ANeumann82 left a comment

Choose a reason for hiding this comment

zen-dog commented Oct 7, 2020