Merge branch 'master' into lblackstone/provider-namespace-diff

pulumi · Aug 1, 2019 · aa8559a · aa8559a
2 parents ead34ae + 5d13254
commit aa8559a
Show file tree

Hide file tree

Showing 6 changed files with 103 additions and 21 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,10 +6,15 @@
 - v1.14.x
 - v1.13.x
 
+### Improvements
+
+-   Improve error messages for StatefulSet. (https://github.com/pulumi/pulumi-kubernetes/pull/673)
+
 ### Bug fixes
 
 -   Detect namespace diff for first-class providers. (https://github.com/pulumi/pulumi-kubernetes/pull/674).
 -   Properly reference override values in Python Helm SDK (https://github.com/pulumi/pulumi-kubernetes/pull/676).
+-   Handle Output values in diffs. (https://github.com/pulumi/pulumi-kubernetes/pull/682).
 
 ## 0.25.3 (July 29, 2019)
 

diff --git a/pkg/await/apps_statefulset.go b/pkg/await/apps_statefulset.go
@@ -110,6 +110,8 @@ type statefulsetInitAwaiter struct {
 	pods            map[string]*unstructured.Unstructured
 	currentReplicas int64
 	targetReplicas  int64
+	currentRevision string
+	targetRevision  string
 }
 
 func makeStatefulSetInitAwaiter(c updateAwaitConfig) *statefulsetInitAwaiter {
@@ -329,6 +331,7 @@ func (sia *statefulsetInitAwaiter) processStatefulSetEvent(event watch.Event) {
 		updateRevision = rawUpdateRevision.(string)
 	}
 	sia.revisionReady = (currentRevision != "") && (currentRevision == updateRevision)
+	sia.currentRevision, sia.targetRevision = currentRevision, updateRevision
 
 	// Check if all expected replicas are ready.
 	var replicas, statusReplicas, statusReadyReplicas, statusCurrentReplicas, statusUpdatedReplicas int64
@@ -419,11 +422,13 @@ func (sia *statefulsetInitAwaiter) errorMessages() []string {
 
 	if !sia.replicasReady {
 		messages = append(messages,
-			"Failed to observe the expected number of ready replicas")
+			fmt.Sprintf(
+				"%v out of %v replicas succeeded readiness checks", sia.currentReplicas, sia.targetReplicas))
 	}
 	if !sia.revisionReady {
 		messages = append(messages,
-			".status.currentRevision does not match .status.updateRevision")
+			fmt.Sprintf("StatefulSet controller failed to advance from revision %q to revision %q",
+				sia.currentRevision, sia.targetRevision))
 	}
 
 	errorMessages := sia.aggregatePodErrors()

diff --git a/pkg/await/apps_statefulset_test.go b/pkg/await/apps_statefulset_test.go
@@ -70,7 +70,7 @@ func Test_Apps_StatefulSet(t *testing.T) {
 			expectedError: &timeoutError{
 				object: statefulsetCreating(inputNamespace, inputName, targetService),
 				subErrors: []string{
-					"Failed to observe the expected number of ready replicas",
+					"0 out of 2 replicas succeeded readiness checks",
 				}},
 		},
 		{
@@ -88,10 +88,8 @@ func Test_Apps_StatefulSet(t *testing.T) {
 			expectedError: &timeoutError{
 				object: statefulsetUpdating(inputNamespace, inputName, targetService),
 				subErrors: []string{
-					// TODO: update error message
-					// x of y replicas succeeded readiness checks
-					"Failed to observe the expected number of ready replicas",
-					".status.currentRevision does not match .status.updateRevision",
+					"0 out of 2 replicas succeeded readiness checks",
+					"StatefulSet controller failed to advance from revision \"foo-7b5cf87b78\" to revision \"foo-789c4b994f\"",
 				}},
 		},
 		{
@@ -114,7 +112,7 @@ func Test_Apps_StatefulSet(t *testing.T) {
 			expectedError: &timeoutError{
 				object: statefulsetProgressing(inputNamespace, inputName, targetService),
 				subErrors: []string{
-					"Failed to observe the expected number of ready replicas",
+					"1 out of 2 replicas succeeded readiness checks",
 				}},
 		},
 		{
@@ -137,8 +135,8 @@ func Test_Apps_StatefulSet(t *testing.T) {
 			expectedError: &timeoutError{
 				object: statefulsetUpdating(inputNamespace, inputName, targetService),
 				subErrors: []string{
-					"Failed to observe the expected number of ready replicas",
-					".status.currentRevision does not match .status.updateRevision",
+					"0 out of 2 replicas succeeded readiness checks",
+					"StatefulSet controller failed to advance from revision \"foo-7b5cf87b78\" to revision \"foo-789c4b994f\"",
 				}},
 		},
 		{
@@ -161,7 +159,7 @@ func Test_Apps_StatefulSet(t *testing.T) {
 			expectedError: &timeoutError{
 				object: statefulsetProgressing(inputNamespace, inputName, targetService),
 				subErrors: []string{
-					"Failed to observe the expected number of ready replicas",
+					"1 out of 2 replicas succeeded readiness checks",
 					"containers with unready status: [nginx] -- [ErrImagePull] manifest for nginx:busted not found",
 				}},
 		},
@@ -185,8 +183,8 @@ func Test_Apps_StatefulSet(t *testing.T) {
 			expectedError: &timeoutError{
 				object: statefulsetUpdating(inputNamespace, inputName, targetService),
 				subErrors: []string{
-					"Failed to observe the expected number of ready replicas",
-					".status.currentRevision does not match .status.updateRevision",
+					"0 out of 2 replicas succeeded readiness checks",
+					"StatefulSet controller failed to advance from revision \"foo-7b5cf87b78\" to revision \"foo-789c4b994f\"",
 					"containers with unready status: [nginx] -- [ErrImagePull] manifest for nginx:busted not found",
 				}},
 		},
@@ -236,7 +234,7 @@ func Test_Apps_StatefulSet_MultipleUpdates(t *testing.T) {
 			firstExpectedError: &timeoutError{
 				object: statefulsetFailed(),
 				subErrors: []string{
-					"Failed to observe the expected number of ready replicas",
+					"0 out of 2 replicas succeeded readiness checks",
 				}},
 			secondUpdate: func(statefulset, pods chan watch.Event, timeout chan time.Time) {
 				statefulset <- watchAddedEvent(statefulsetUpdatedAfterFailed())
@@ -295,15 +293,15 @@ func Test_Apps_StatefulSetRead(t *testing.T) {
 			description: "Read should fail if StatefulSet status empty",
 			statefulset: statefulsetAdded,
 			expectedSubErrors: []string{
-				"Failed to observe the expected number of ready replicas",
-				".status.currentRevision does not match .status.updateRevision",
+				"0 out of 2 replicas succeeded readiness checks",
+				"StatefulSet controller failed to advance from revision \"\" to revision \"\"",
 			},
 		},
 		{
 			description: "Read should fail if StatefulSet is progressing",
 			statefulset: statefulsetProgressing,
 			expectedSubErrors: []string{
-				"Failed to observe the expected number of ready replicas",
+				"1 out of 2 replicas succeeded readiness checks",
 			},
 		},
 		{

diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go
@@ -577,7 +577,11 @@ func (k *kubeProvider) Diff(
 	var isInputPatch bool
 	var patchBase *unstructured.Unstructured
 
-	tryDryRun := supportsDryRun && oldInputs.GroupVersionKind().String() == gvk.String()
+	// TODO: Skipping dry run entirely for resources with computed values is a hack. We will want to address this
+	// more granularly so that previews are as accurate as possible, but this is an easy workaround for a critical
+	// bug.
+	tryDryRun := supportsDryRun && oldInputs.GroupVersionKind().String() == gvk.String() &&
+		!hasComputedValue(newInputs) && !hasComputedValue(oldInputs)
 	if tryDryRun {
 		patch, patchBase, err = k.dryRunPatch(oldInputs, newInputs)
 

diff --git a/pkg/provider/util.go b/pkg/provider/util.go
@@ -25,11 +25,17 @@ func hasComputedValue(obj *unstructured.Unstructured) bool {
 		}
 		curr, objects = objects[0], objects[1:]
 		for _, v := range curr {
-			if _, isComputed := v.(resource.Computed); isComputed {
+			switch field := v.(type) {
+			case resource.Computed:
 				return true
-			}
-			if field, isMap := v.(map[string]interface{}); isMap {
+			case map[string]interface{}:
 				objects = append(objects, field)
+			case []interface{}:
+				for _, v := range field {
+					objects = append(objects, map[string]interface{}{"": v})
+				}
+			case []map[string]interface{}:
+				objects = append(objects, field...)
 			}
 		}
 	}

diff --git a/pkg/provider/util_test.go b/pkg/provider/util_test.go
@@ -92,6 +92,70 @@ func TestHasComputedValue(t *testing.T) {
 			}},
 			hasComputedValue: true,
 		},
+		{
+			name: "Object with nested slice of map[string]interface{} has a computed value",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"field1": 1,
+				"field2": []map[string]interface{}{
+					{"field3": resource.Computed{}},
+				},
+			}},
+			hasComputedValue: true,
+		},
+		{
+			name: "Object with nested slice of interface{} has a computed value",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"field1": 1,
+				"field2": []interface{}{
+					resource.Computed{},
+				},
+			}},
+			hasComputedValue: true,
+		},
+		{
+			name: "Object with nested slice of map[string]interface{} with nested slice of interface{} has a computed value",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"field1": 1,
+				"field2": []map[string]interface{}{
+					{"field3": []interface{}{
+						resource.Computed{},
+					}},
+				},
+			}},
+			hasComputedValue: true,
+		},
+		{
+			name: "Complex nested object with computed value",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"field1": 1,
+				"field2": []map[string]interface{}{
+					{"field3": []interface{}{
+						[]map[string]interface{}{
+							{"field4": []interface{}{
+								resource.Computed{},
+							}},
+						},
+					}},
+				},
+			}},
+			hasComputedValue: true,
+		},
+		{
+			name: "Complex nested object with no computed value",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"field1": 1,
+				"field2": []map[string]interface{}{
+					{"field3": []interface{}{
+						[]map[string]interface{}{
+							{"field4": []interface{}{
+								"field5",
+							}},
+						},
+					}},
+				},
+			}},
+			hasComputedValue: false,
+		},
 	}
 
 	for _, test := range tests {