Better Wait Handling on Installs (#1469)

Signed-off-by: Ken Sipe <kensipe@gmail.com>

pkg/kudoctl/cmd/plan.go

@@ -16,9 +16,6 @@ const (
 `
 	planStatusExample = `  # View plan status
   kubectl kudo plan status --instance=<instanceName>
-`
-	planWaitExample = `  # Wait on the current plan status to finish
-  kubectl kudo plan wait --instance=<instanceName>
 `
 	planTriggerExample = `  # Trigger an instance plan
 kubectl kudo plan trigger <planName> --instance=<instanceName>
@@ -35,7 +32,6 @@ func newPlanCmd(out io.Writer) *cobra.Command {
 
 	cmd.AddCommand(NewPlanHistoryCmd())
 	cmd.AddCommand(NewPlanStatusCmd(out))
-	cmd.AddCommand(NewPlanWaitCmd(out))
 	cmd.AddCommand(NewPlanTriggerCmd())
 
 	return cmd
@@ -81,29 +77,6 @@ func NewPlanStatusCmd(out io.Writer) *cobra.Command {
 	return cmd
 }
 
-//NewPlanWaitCmd waits on the status of an instance to complete
-func NewPlanWaitCmd(out io.Writer) *cobra.Command {
-	options := &plan.WaitOptions{Out: out, WaitTime: 300}
-	cmd := &cobra.Command{
-		Use:     "wait",
-		Short:   "Waits on a plan to finish for a particular instance.",
-		Example: planWaitExample,
-		RunE: func(cmd *cobra.Command, args []string) error {
-			return plan.Wait(options, &Settings)
-		},
-	}
-
-	cmd.Flags().StringVar(&options.Instance, "instance", "", "The instance name available from 'kubectl get instances'")
-	cmd.Flags().Int64Var(&options.WaitTime, "wait-time", 300, "Specify the max wait time in seconds for CLI to wait for the current plan to complete (default \"300\")")
-
-	if err := cmd.MarkFlagRequired("instance"); err != nil {
-		clog.Printf("Please choose the instance with '--instance=<instanceName>': %v", err)
-		os.Exit(1)
-	}
-
-	return cmd
-}
-
 // NewPlanTriggerCmd creates a command that triggers a specific plan for an instance.
 func NewPlanTriggerCmd() *cobra.Command {
 	options := &plan.TriggerOptions{}

pkg/kudoctl/util/kudo/install.go

@@ -58,12 +58,12 @@ func InstallPackage(kc *Client, resources *packages.Resources, skipInstance bool
 	}
 
 	instanceName = resources.Instance.ObjectMeta.Name
-	instanceExists, err := kc.InstanceExistsInCluster(operatorName, namespace, resources.OperatorVersion.Spec.Version, instanceName)
+	instance, err := kc.GetInstance(instanceName, namespace)
 	if err != nil {
 		return fmt.Errorf("failed to verify existing instance: %v", err)
 	}
 
-	if !instanceExists {
+	if instance == nil {
 		if _, err := kc.InstallInstanceObjToCluster(resources.Instance, namespace); err != nil {
 			return fmt.Errorf("failed to install instance %s: %v", instanceName, err)
 		}

pkg/kudoctl/util/kudo/kudo.go

@@ -207,9 +207,15 @@ func (c *Client) UpdateInstance(instanceName, namespace string, operatorVersion
 	return err
 }
 
-// WaitForInstance waits for instance to be "finished".  oldInstance is nil if there is no oldInstance.  oldInstance
-// should be provided if there was an update or upgrade.  The wait will then initially wait for the "new" plan to activate
-// then return when completed.  The error is either an error in working with kubernetes or a wait.ErrWaitTimeout
+// WaitForInstance waits for instance to be "complete".
+// It uses controller-runtime `wait.PollImmediate`, the function passed to it returns done==false if it isn't done.
+// For a situation where there is no previous state (like install), the "lastPlanStatus" will be nil until the manager
+// sets it, then it's state will be watched (see InInstanceDone for more detail)
+// For a situation where there is previous state (like update, upgrade, plan trigger) than it is important AND required
+// that the "oldInstance" be provided.  Without it, it is possible for this function to be "racy" and "flaky" meaning the
+// "current" status could be the old "done" status or the new status... it's hard to know.  If the oldInstance is provided
+// the wait will then initially wait for the "new" plan to activate then return when completed.
+// The error is either an error in working with kubernetes or a wait.ErrWaitTimeout
 func (c *Client) WaitForInstance(name, namespace string, oldInstance *v1beta1.Instance, timeout time.Duration) error {
 	// polling interval 1 sec
 	interval := 1 * time.Second