Report partial errors on create and update (#428)

* Report partial errors

* test partial errors

* Fix apply error

Use a different name than `err` when calling `applyModuleOperation `, otherwise `err` will be overwritten by the call to `PublishViewSteps`.

* More logging

* Detect more error cases for views

This definitely could be refactored/improved (it's not really the right place to be eliding the error), but it does correctly handle the `ReplaceDestroyBeforeCreate` case when delete succeeds but create fails (we assume).

* clean up viewStepStatusCheck

---------

Co-authored-by: Justin Van Patten <[email protected]>

Author: mjeffryes

pkg/modprovider/module.go

@@ -325,15 +325,8 @@ func (h *moduleHandler) applyModuleOperation(
 	}
 
 	if applyErr != nil {
-		// TODO[pulumi/pulumi-terraform-module#342] Possibly wrap partial errors in initializationError. This
-		// does not quite work as expected yet as views get recorded into state as pending_operations. They
-		// need to be recorded as finalized operations because they did complete.
-		if 1+2 == 4 {
-			applyErr = h.initializationError(moduleOutputs, applyErr.Error())
-		}
-
-		// Instead, log and propagate the error for now. This will forget partial TF state but fail Pulumi.
-		logger.Log(ctx, tfsandbox.Error, fmt.Sprintf("partial failure in apply: %v", applyErr))
+		// we have a partial error, wrap it with ErrorResourceInitFailed
+		applyErr = h.initializationError(moduleOutputs, applyErr.Error())
 	}
 
 	hasOutputFieldMappings := inferredModule != nil &&
@@ -486,7 +479,7 @@ func (h *moduleHandler) Update(
 
 	//q.Q("Update", req.GetPreview())
 
-	moduleOutputs, views, err := h.applyModuleOperation(
+	moduleOutputs, views, applyErr := h.applyModuleOperation(
 		ctx,
 		urn,
 		moduleInputs,
@@ -499,18 +492,21 @@ func (h *moduleHandler) Update(
 		req.GetPreview(),
 		executor,
 	)
-	// TODO[pulumi/pulumi-terraform-module#342] partial error handling needs to modify this.
-	if err != nil {
-		return nil, err
+
+	// Publish views even if applyErr != nil as is the case of partial failures.
+	if views != nil {
+		_, err = statusClient.PublishViewSteps(ctx, &pulumirpc.PublishViewStepsRequest{
+			Token: req.ResourceStatusToken,
+			Steps: views,
+		})
+		if err != nil {
+			logger.Log(ctx, tfsandbox.Debug, fmt.Sprintf("error publishing view steps after Update: %v", err))
+			return nil, err
+		}
 	}
 
-	_, err = statusClient.PublishViewSteps(ctx, &pulumirpc.PublishViewStepsRequest{
-		Token: req.ResourceStatusToken,
-		Steps: views,
-	})
-	if err != nil {
-		logger.Log(ctx, tfsandbox.Debug, fmt.Sprintf("error publishing view steps after Update: %v", err))
-		return nil, err
+	if applyErr != nil {
+		return nil, applyErr
 	}
 
 	props, err := plugin.MarshalProperties(moduleOutputs, h.marshalOpts())

pkg/modprovider/views.go

@@ -154,38 +154,46 @@ func viewStepOp(changeKind tfsandbox.ChangeKind, _ bool /*drift*/) []pulumirpc.V
 
 // Starting with very basic error checks for starters. It should be possible to extract more information from TF.
 func viewStepStatusCheck(
+	op pulumirpc.ViewStep_Op,
 	changeKind tfsandbox.ChangeKind,
 	finalState *tfsandbox.ResourceState, // may be nil when planning or failed to create
 ) error {
 
-	switch {
+	switch changeKind {
 
 	// Planned a create but there is no final state. Resource creation must have failed. Neither TF state nor TF
 	// plan contains the correct error message, so using a generic message for now before TF errors can be properly
 	// correlated to a resource by address.
-	case changeKind == tfsandbox.Create && finalState == nil:
-		return fmt.Errorf("failed to create")
+	case tfsandbox.Create:
+		if finalState == nil {
+			return fmt.Errorf("resource operation failed: %s missing final state", op.String())
+		}
+
+	// All these operations when successful imply the resource must exist in the final state.
+	case tfsandbox.NoOp, tfsandbox.Update:
+		if finalState == nil {
+			return fmt.Errorf("resource operation failed: %s missing final state", op.String())
+		}
 
-	default:
-		return nil
+		// Replace expects a final state to be present, so a missing final state implies the create step failed.
+		// A replace has three view steps, but we only want to return an error on the create step
+		// when the final state is missing. (That must mean the delete succeeded.)
+		// It is possible that both the create and delete steps could have failed,
+		// but we can't easily tell that here. It should be ok to tell the engine that these view steps succeeded,
+		// and count on a subsequent refresh to correct state.
+	case tfsandbox.Replace, tfsandbox.ReplaceDestroyBeforeCreate:
+		if op == pulumirpc.ViewStep_CREATE_REPLACEMENT && finalState == nil {
+			return fmt.Errorf("resource operation failed: %s missing final state", op.String())
+		}
 
+	// These operations if successful imply the resource must not exist in the final state.
+	case tfsandbox.Delete, tfsandbox.Forget:
+		if finalState != nil {
+			return fmt.Errorf("resource operation failed: %s left resource in state", op.String())
+		}
 	}
 
-	// All these operations when successful imply the resource must exist in the final state.
-	// case tfsandbox.NoOp, tfsandbox.Update, tfsandbox.Create,
-	// 	tfsandbox.Replace, tfsandbox.ReplaceDestroyBeforeCreate:
-	// 	if finalState == nil {
-	// 		return errors.New("resource operation failed")
-	// 	}
-
-	// // These operations if successful imply the resource must not exist in the final state.
-	// case tfsandbox.Delete, tfsandbox.Forget:
-	// 	if finalState != nil {
-	// 		return errors.New("resource operation failed")
-	// 	}
-	// }
-
-	// return nil
+	return nil
 }
 
 // A resource that has not changed and therefore has no Plan entry in TF needs a ViewStep.
@@ -262,7 +270,7 @@ func viewStepsForResource(
 		}
 
 		if !preview {
-			if err := viewStepStatusCheck(rplan.ChangeKind(), finalState); err != nil {
+			if err := viewStepStatusCheck(op, rplan.ChangeKind(), finalState); err != nil {
 				step.Error = err.Error()
 			}
 		}

tests/acc_test.go

@@ -245,8 +245,6 @@ func TestLambdaMemorySizeDiff(t *testing.T) {
 func TestPartialApply(t *testing.T) {
 	t.Parallel()
 
-	t.Skip("TODO[pulumi/pulumi#19635]")
-
 	var debugOpts debug.LoggingOptions
 
 	// To enable debug logging in this test, un-comment:
@@ -279,6 +277,10 @@ func TestPartialApply(t *testing.T) {
 	// Generate package
 	pulumiPackageAdd(t, integrationTest, localProviderBinPath, localMod, "localmod")
 
+	t.Logf("################################################################################")
+	t.Logf("step 1 - partial failure in create")
+	t.Logf("################################################################################")
+
 	_, err = integrationTest.CurrentStack().Up(
 		integrationTest.Context(),
 		optup.Diff(),
@@ -298,7 +300,7 @@ func TestPartialApply(t *testing.T) {
 	mustFindDeploymentResourceByType(t, integrationTest, "localmod:tf:aws_iam_role")
 
 	t.Logf("################################################################################")
-	t.Logf("step 2")
+	t.Logf("step 2 - complete create")
 	t.Logf("################################################################################")
 
 	integrationTest.SetConfig(t, "step", "2")
@@ -311,11 +313,53 @@ func TestPartialApply(t *testing.T) {
 	)
 	changes2 := *upRes2.Summary.ResourceChanges
 	assert.Equal(t, map[string]int{
-		"update": 1,
+		"update": 2,
 		"create": 1,
-		"same":   2,
+		"same":   1,
 	}, changes2)
 	assert.Contains(t, upRes2.Outputs, "roleArn")
+
+	t.Logf("State: %s", string(integrationTest.ExportStack(t).Deployment))
+
+	t.Logf("################################################################################")
+	t.Logf("step 3 - partial failure in update")
+	t.Logf("################################################################################")
+
+	integrationTest.SetConfig(t, "step", "3")
+
+	_, err = integrationTest.CurrentStack().Up(
+		integrationTest.Context(),
+		optup.Diff(),
+		optup.ErrorProgressStreams(testWriter),
+		optup.ProgressStreams(testWriter),
+		optup.DebugLogging(debugOpts),
+	)
+
+	assert.Errorf(t, err, "expected error on up")
+
+	t.Logf("State: %s", string(integrationTest.ExportStack(t).Deployment))
+
+	t.Logf("################################################################################")
+	t.Logf("step 4 - complete update")
+	t.Logf("################################################################################")
+
+	integrationTest.SetConfig(t, "step", "4")
+
+	upRes4 := integrationTest.Up(t,
+		optup.Diff(),
+		optup.ErrorProgressStreams(testWriter),
+		optup.ProgressStreams(testWriter),
+		optup.DebugLogging(debugOpts),
+	)
+	changes4 := *upRes4.Summary.ResourceChanges
+	assert.Equal(t, map[string]int{
+		"update": 2,
+		"create": 1,
+		"same":   1,
+	}, changes4)
+	assert.Contains(t, upRes4.Outputs, "roleArn")
+
+	t.Logf("State: %s", string(integrationTest.ExportStack(t).Deployment))
 }
 
 // Sanity check that we can provision two instances of the same module side-by-side, in particular

tests/testdata/programs/ts/partial-apply/index.ts

@@ -6,9 +6,10 @@ const prefix = config.get('prefix') ?? pulumi.getStack();
 const step = config.getNumber('step') ?? 1;
 
 const mod = new localmod.Module('test-localmod', {
-    name_prefix: prefix,
-    should_fail: step === 1 ? true : false,
+  name_prefix: prefix,
+  description: `Step ${step}`,
+  should_fail: step % 2 === 1 ? true : false,
 });
 
-export const roleArn =  mod.role_arn;
+export const roleArn = mod.role_arn;
 

tests/testdata/programs/ts/partial-apply/local_module/main.tf

@@ -1,5 +1,6 @@
 resource "aws_iam_role" "this" {
   name_prefix = var.name_prefix
+  description = var.description
   assume_role_policy = jsonencode({
     Version = "2012-10-17"
     Statement = [

tests/testdata/programs/ts/partial-apply/local_module/variables.tf

@@ -3,6 +3,11 @@ variable "name_prefix" {
   description = "Prefix to use for the name of the IAM role"
 }
 
+variable "description" {
+  type        = string
+  description = "description of the IAM role"
+}
+
 variable "should_fail" {
   type        = bool
   description = "Whether the module should fail"