Fix stack state message regression (#107)

* WIP fixes for #102

* Update CRD and avoid clobbering last successful commit

* Record actual git commit instead of using spec value

* Delete working directory after reconciliation - should help with #104

* Address PR comments

* Improve tests

* Adding more tests to cover lifecycle of stack with success and failure

deploy/crds/pulumi.com_stacks.yaml

@@ -101,6 +101,12 @@ spec:
             lastUpdate:
               description: LastUpdate contains details of the status of the last update.
               properties:
+                lastAttemptedCommit:
+                  description: Last commit attempted
+                  type: string
+                lastSuccessfulCommit:
+                  description: Last commit successfully applied
+                  type: string
                 permalink:
                   description: Permalink is the Pulumi Console URL of the stack operation.
                   type: string

go.sum

@@ -789,8 +789,6 @@ github.com/prometheus/prometheus v0.0.0-20180315085919-58e2a31db8de/go.mod h1:oA
 github.com/prometheus/prometheus v1.8.2-0.20200110114423-1e64d757f711/go.mod h1:7U90zPoLkWjEIQcy/rweQla82OCTUzxVHE51G3OhJbI=
 github.com/prometheus/prometheus v2.3.2+incompatible/go.mod h1:oAIUtOny2rjMX0OWN5vPR5/q/twIROJvdqnQKDdil/s=
 github.com/prometheus/tsdb v0.7.1/go.mod h1:qhTCs0VvXwvX/y3TZrWD7rabWM+ijKTux40TwIPHuXU=
-github.com/pulumi/pulumi/sdk/v2 v2.12.1 h1:rEQHyjaGSGybqqeKLMJZH034UemMPGw2AWzpGcn1tF4=
-github.com/pulumi/pulumi/sdk/v2 v2.12.1/go.mod h1:WQ4WaHMA7mduVHAJi87iIqbWvqsuBUYccBiKK+FrayI=
 github.com/pulumi/pulumi/sdk/v2 v2.15.0 h1:gTiohXl5dyw3z/aKbuhrN50KQMYFFKnGwebPWvOIvs8=
 github.com/pulumi/pulumi/sdk/v2 v2.15.0/go.mod h1:Z9ifPo/Q0+hUpAyguVx2gp5Sx+CBumnWvYQDhrM8l3E=
 github.com/rcrowley/go-metrics v0.0.0-20181016184325-3113b8401b8a/go.mod h1:bCqnVzQkZxMG4s8nGwiZ5l3QUCyqpo9Y+/ZMZ9VjZe4=

pkg/apis/pulumi/v1alpha1/stack_types.go

@@ -107,7 +107,11 @@ type StackOutputs map[string]apiextensionsv1.JSON
 // StackUpdateState is the status of a stack update
 type StackUpdateState struct {
 	// State is the state of the stack update - one of `succeeded` or `failed`
-	State string `json:"state,omitempty"`
+	State StackUpdateStateMessage `json:"state,omitempty"`
+	// Last commit attempted
+	LastAttemptedCommit string `json:"lastAttemptedCommit,omitempty"`
+	// Last commit successfully applied
+	LastSuccessfulCommit string `json:"lastSuccessfulCommit,omitempty"`
 	// Permalink is the Pulumi Console URL of the stack operation.
 	Permalink Permalink `json:"permalink,omitempty"`
 }
@@ -153,8 +157,14 @@ const (
 	StackNotFound StackUpdateStatus = 4
 )
 
-// FailedStackStateMessage is a const to indicate stack failure in the status.
-const FailedStackStateMessage = "failed"
+type StackUpdateStateMessage string
+
+const (
+	// SucceededStackStateMessage is a const to indicate success in stack status state.
+	SucceededStackStateMessage StackUpdateStateMessage = "succeeded"
+	// FailedStackStateMessage is a const to indicate stack failure in stack status state.
+	FailedStackStateMessage StackUpdateStateMessage = "failed"
+)
 
 // Permalink is the Pulumi Service URL of the stack operation.
 type Permalink string

pkg/controller/stack/stack_controller.go

@@ -25,7 +25,6 @@ import (
 	"github.com/pulumi/pulumi/sdk/v2/go/x/auto/optup"
 	giturls "github.com/whilp/git-urls"
 	git "gopkg.in/src-d/go-git.v4"
-	"gopkg.in/src-d/go-git.v4/plumbing"
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
@@ -174,6 +173,14 @@ func (r *ReconcileStack) Reconcile(request reconcile.Request) (reconcile.Result,
 		return reconcile.Result{}, err
 	}
 
+	currentCommit, err := revisionAtWorkingDir(sess.workdir)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
+	// Delete the working directory after the reconciliation is completed (regardless of success or failure).
+	defer sess.CleanupPulumiWorkdir()
+
 	// Step 2. If there are extra environment variables, read them in now and use them for subsequent commands.
 	err = sess.SetEnvs(stack.Envs, request.Namespace)
 	if err != nil {
@@ -206,24 +213,12 @@ func (r *ReconcileStack) Reconcile(request reconcile.Request) (reconcile.Result,
 			}
 			time.Sleep(2 * time.Second) // arbitrary sleep after finalizer add to avoid stale obj for permalink
 			// Add default permalink for the stack in the Pulumi Service.
-			sess.addDefaultPermalink(instance)
+			if err := sess.addDefaultPermalink(instance); err != nil {
+				return reconcile.Result{}, err
+			}
 		}
 	}
 
-	// Run the Pulumi update iff the desired state has not already been
-	// reached, or if it is marked to be deleted.
-	err = sess.getLatestResource(instance, request.NamespacedName)
-	if err != nil {
-		sess.logger.Error(err, "Failed to get latest Stack before updating Stack", "Stack.Name", instance.Spec.Stack)
-		return reconcile.Result{}, err
-	}
-	isStackMarkedToBeDeleted = instance.GetDeletionTimestamp() != nil
-	// Don't run rest of loop if already at desired state, unless marked for deletion.
-	if !isStackMarkedToBeDeleted && (instance.Status.LastUpdate != nil && instance.Status.LastUpdate.State == instance.Spec.Commit) {
-		reqLogger.Info("Stack already at desired state", "Stack.Commit", instance.Spec.Commit)
-		return reconcile.Result{}, nil
-	}
-
 	// Step 3. If a stack refresh is requested, run it now.
 	if sess.stack.Refresh {
 		permalink, err := sess.RefreshStack(sess.stack.ExpectNoRefreshChanges)
@@ -237,12 +232,10 @@ func (r *ReconcileStack) Reconcile(request reconcile.Request) (reconcile.Result,
 			return reconcile.Result{}, err
 		}
 		if instance.Status.LastUpdate == nil {
-			instance.Status.LastUpdate = &pulumiv1alpha1.StackUpdateState{
-				Permalink: permalink,
-			}
-		} else {
-			instance.Status.LastUpdate.Permalink = permalink
+			instance.Status.LastUpdate = &pulumiv1alpha1.StackUpdateState{}
 		}
+		instance.Status.LastUpdate.Permalink = permalink
+
 		err = sess.updateResourceStatus(instance)
 		if err != nil {
 			reqLogger.Error(err, "Failed to update Stack status for refresh", "Stack.Name", stack.Stack)
@@ -269,10 +262,10 @@ func (r *ReconcileStack) Reconcile(request reconcile.Request) (reconcile.Result,
 		if err != nil {
 			reqLogger.Error(err, "Failed to update Stack", "Stack.Name", stack.Stack)
 			// Update Stack status with failed state
-			instance.Status.LastUpdate = &pulumiv1alpha1.StackUpdateState{
-				State:     pulumiv1alpha1.FailedStackStateMessage,
-				Permalink: permalink,
-			}
+			instance.Status.LastUpdate.LastAttemptedCommit = currentCommit
+			instance.Status.LastUpdate.State = pulumiv1alpha1.FailedStackStateMessage
+			instance.Status.LastUpdate.Permalink = permalink
+
 			if err2 := sess.updateResourceStatus(instance); err2 != nil {
 				msg := "Failed to update status for a failed Stack update"
 				err3 := errors.Wrapf(err, err2.Error())
@@ -299,14 +292,11 @@ func (r *ReconcileStack) Reconcile(request reconcile.Request) (reconcile.Result,
 		return reconcile.Result{}, err
 	}
 	instance.Status.Outputs = outs
-	if instance.Status.LastUpdate == nil {
-		instance.Status.LastUpdate = &pulumiv1alpha1.StackUpdateState{
-			State:     instance.Spec.Commit,
-			Permalink: permalink,
-		}
-	} else {
-		instance.Status.LastUpdate.State = instance.Spec.Commit
-		instance.Status.LastUpdate.Permalink = permalink
+	instance.Status.LastUpdate = &pulumiv1alpha1.StackUpdateState{
+		State:                pulumiv1alpha1.SucceededStackStateMessage,
+		LastAttemptedCommit:  currentCommit,
+		LastSuccessfulCommit: currentCommit,
+		Permalink:            permalink,
 	}
 	err = sess.updateResourceStatus(instance)
 	if err != nil {
@@ -377,12 +367,12 @@ func (sess *reconcileStackSession) addFinalizer(stack *pulumiv1alpha1.Stack) err
 }
 
 type reconcileStackSession struct {
-	logger      logr.Logger
-	kubeClient  client.Client
-	accessToken string
-	stack       pulumiv1alpha1.StackSpec
-	autoStack   *auto.Stack
-	workdir     string
+	logger        logr.Logger
+	kubeClient    client.Client
+	accessToken   string
+	stack         pulumiv1alpha1.StackSpec
+	autoStack     *auto.Stack
+	workdir       string
 }
 
 // blank assignment to verify that reconcileStackSession implements pulumiv1alpha1.StackController.
@@ -399,25 +389,6 @@ func newReconcileStackSession(
 	}
 }
 
-// gitCloneAndCheckoutCommit clones the Git repository and checkouts the specified commit hash or branch.
-func gitCloneAndCheckoutCommit(url, hash, branch, path string) error {
-	repo, err := git.PlainClone(path, false, &git.CloneOptions{URL: url})
-	if err != nil {
-		return err
-	}
-
-	w, err := repo.Worktree()
-	if err != nil {
-		return err
-	}
-
-	return w.Checkout(&git.CheckoutOptions{
-		Hash:   plumbing.NewHash(hash),
-		Branch: plumbing.ReferenceName(branch),
-		Force:  true,
-	})
-}
-
 // SetEnvs populates the environment the stack run with values
 // from an array of Kubernetes ConfigMaps in a Namespace.
 func (sess *reconcileStackSession) SetEnvs(configMapNames []string, namespace string) error {
@@ -531,6 +502,7 @@ func (sess *reconcileStackSession) SetupPulumiWorkdir(gitAuth *auto.GitAuth) err
 	if sess.accessToken != "" {
 		w.SetEnvVar("PULUMI_ACCESS_TOKEN", sess.accessToken)
 	}
+	sess.workdir = w.WorkDir()
 
 	// Create a new stack if the stack does not already exist, or fall back to
 	// selecting the existing stack. If the stack does not exist, it will be created and selected.
@@ -555,6 +527,27 @@ func (sess *reconcileStackSession) SetupPulumiWorkdir(gitAuth *auto.GitAuth) err
 	return nil
 }
 
+func (sess *reconcileStackSession) CleanupPulumiWorkdir() {
+	if sess.workdir != "" {
+		if err := os.RemoveAll(sess.workdir); err != nil {
+			sess.logger.Error(err, "Failed to delete working dir: %s", sess.workdir)
+		}
+	}
+}
+
+// Determine the actual commit information from the working directory (Spec commit etc. is optional).
+func revisionAtWorkingDir(workingDir string) (string, error) {
+	gitRepo, err := git.PlainOpen(workingDir)
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to resolve git repository from working directory: %s", workingDir)
+	}
+	headRef, err := gitRepo.Head()
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to determine revision for git repository at %s", workingDir)
+	}
+	return headRef.Hash().String(), nil
+}
+
 func (sess *reconcileStackSession) InstallProjectDependencies(ctx context.Context, workspace auto.Workspace) error {
 	project, err := workspace.ProjectSettings(ctx)
 	if err != nil {

test/stack_controller_test.go

@@ -157,7 +157,9 @@ var _ = Describe("Stack Controller", func() {
 				return false
 			}
 			if fetched.Status.LastUpdate != nil {
-				return fetched.Status.LastUpdate.State == stack.Spec.Commit
+				return fetched.Status.LastUpdate.LastSuccessfulCommit == stack.Spec.Commit &&
+					fetched.Status.LastUpdate.LastAttemptedCommit == stack.Spec.Commit &&
+					fetched.Status.LastUpdate.State == pulumiv1alpha1.SucceededStackStateMessage
 			}
 			return false
 		}, timeout, interval).Should(BeTrue())
@@ -207,24 +209,82 @@ var _ = Describe("Stack Controller", func() {
 				return false
 			}
 			if original.Status.LastUpdate != nil {
-				return original.Status.LastUpdate.State == stack.Spec.Commit
+				return original.Status.LastUpdate.LastSuccessfulCommit == stack.Spec.Commit &&
+					original.Status.LastUpdate.LastAttemptedCommit == stack.Spec.Commit &&
+					original.Status.LastUpdate.State == pulumiv1alpha1.SucceededStackStateMessage
 			}
 			return false
 		}, timeout, interval).Should(BeTrue())
 
-		// Update the stack commit to a different commit.
-		original.Spec.Commit = commit
-		Expect(k8sClient.Update(ctx, original)).Should(Succeed())
+		// Update the stack config (this time to cause a failure)
+		original.Spec.Config["aws:region"] = "us-nonexistent-1"
+		Expect(k8sClient.Update(ctx, original)).Should(Succeed(), "%+v", original)
 
-		// Check that the stack updated
+		// Check that the stack tried to update but failed
+		configChanged := &pulumiv1alpha1.Stack{}
+		Eventually(func() bool {
+			err := k8sClient.Get(ctx, types.NamespacedName{Name: stack.Name, Namespace: namespace}, configChanged)
+			if err != nil {
+				return false
+			}
+			if configChanged.Status.LastUpdate != nil {
+				return configChanged.Status.LastUpdate.LastSuccessfulCommit == stack.Spec.Commit &&
+					configChanged.Status.LastUpdate.LastAttemptedCommit == stack.Spec.Commit &&
+					configChanged.Status.LastUpdate.State == pulumiv1alpha1.FailedStackStateMessage
+			}
+			return false
+		})
+
+		// Update the stack commit to a different commit. Need retries because of
+		// competing retries within the operator due to failure.
+		Eventually(func() bool {
+			if err := k8sClient.Get(ctx, types.NamespacedName{Name: stack.Name, Namespace: namespace}, configChanged); err != nil {
+				return false
+			}
+			configChanged.Spec.Commit = commit
+			if err := k8sClient.Update(ctx, configChanged); err != nil {
+				return false
+			}
+			return true
+		}, timeout, interval).Should(BeTrue(), "%#v", configChanged)
+
+		// Check that the stack update was attempted but failed
 		fetched := &pulumiv1alpha1.Stack{}
 		Eventually(func() bool {
 			err := k8sClient.Get(ctx, types.NamespacedName{Name: stack.Name, Namespace: namespace}, fetched)
 			if err != nil {
 				return false
 			}
 			if fetched.Status.LastUpdate != nil {
-				return fetched.Status.LastUpdate.State == commit
+				return fetched.Status.LastUpdate.LastSuccessfulCommit == stack.Spec.Commit &&
+					fetched.Status.LastUpdate.LastAttemptedCommit == commit &&
+					fetched.Status.LastUpdate.State == pulumiv1alpha1.FailedStackStateMessage
+			}
+			return false
+		}, timeout, interval).Should(BeTrue())
+
+		Eventually(func() bool {
+			if err := k8sClient.Get(ctx, types.NamespacedName{Name: stack.Name, Namespace: namespace}, fetched); err != nil {
+				return false
+			}
+			// Update the stack config to now be valid
+			fetched.Spec.Config["aws:region"] = "us-east-2"
+			if err := k8sClient.Update(ctx, fetched); err != nil {
+				return false
+			}
+			return true
+		}, timeout, interval).Should(BeTrue())
+
+		// Check that the stack update attempted but failed
+		Eventually(func() bool {
+			err := k8sClient.Get(ctx, types.NamespacedName{Name: stack.Name, Namespace: namespace}, fetched)
+			if err != nil {
+				return false
+			}
+			if fetched.Status.LastUpdate != nil {
+				return fetched.Status.LastUpdate.LastSuccessfulCommit == commit &&
+					fetched.Status.LastUpdate.LastAttemptedCommit == commit &&
+					fetched.Status.LastUpdate.State == pulumiv1alpha1.SucceededStackStateMessage
 			}
 			return false
 		}, timeout, interval).Should(BeTrue())