fix status update when starting orphan mitigation (#1372)

* keep InProgressProperties when orphan mitigating

* add instance OM handling in API server

* add binding OM to API server validation

* do not clear IPP for binding OM

* add integration test

* add tests

* format code

Author: kibbles-n-bytes

pkg/apis/servicecatalog/validation/binding.go

@@ -95,8 +95,8 @@ func validateServiceBindingStatus(status *sc.ServiceBindingStatus, fldPath *fiel
 			allErrs = append(allErrs, field.Forbidden(fldPath.Child("operationStartTime"), "operationStartTime must not be present when currentOperation is not present"))
 		}
 	} else {
-		if status.OperationStartTime == nil {
-			allErrs = append(allErrs, field.Required(fldPath.Child("operationStartTime"), "operationStartTime is required when currentOperation is present"))
+		if status.OperationStartTime == nil && !status.OrphanMitigationInProgress {
+			allErrs = append(allErrs, field.Required(fldPath.Child("operationStartTime"), "operationStartTime is required when currentOperation is present and no orphan mitigation in progress"))
 		}
 		// Do not allow the binding to be ready if there is an on-going operation
 		for i, c := range status.Conditions {

pkg/apis/servicecatalog/validation/binding_test.go

@@ -412,6 +412,45 @@ func TestValidateServiceBinding(t *testing.T) {
 			create: false,
 			valid:  false,
 		},
+		{
+			name: "failed bind starting orphan mitigation",
+			binding: func() *servicecatalog.ServiceBinding {
+				b := validServiceBindingWithInProgressBind()
+				b.Status.OperationStartTime = nil
+				b.Status.OrphanMitigationInProgress = true
+				b.Status.Conditions = []servicecatalog.ServiceBindingCondition{
+					{
+						Type:   servicecatalog.ServiceBindingConditionReady,
+						Status: servicecatalog.ConditionFalse,
+					},
+					{
+						Type:   servicecatalog.ServiceBindingConditionFailed,
+						Status: servicecatalog.ConditionTrue,
+					},
+				}
+				return b
+			}(),
+			valid: true,
+		},
+		{
+			name: "in-progress orphan mitigation",
+			binding: func() *servicecatalog.ServiceBinding {
+				b := validServiceBindingWithInProgressBind()
+				b.Status.OrphanMitigationInProgress = true
+				b.Status.Conditions = []servicecatalog.ServiceBindingCondition{
+					{
+						Type:   servicecatalog.ServiceBindingConditionReady,
+						Status: servicecatalog.ConditionFalse,
+					},
+					{
+						Type:   servicecatalog.ServiceBindingConditionFailed,
+						Status: servicecatalog.ConditionTrue,
+					},
+				}
+				return b
+			}(),
+			valid: true,
+		},
 	}
 
 	for _, tc := range cases {

pkg/apis/servicecatalog/validation/instance.go

@@ -122,8 +122,8 @@ func validateServiceInstanceStatus(status *sc.ServiceInstanceStatus, fldPath *fi
 			allErrs = append(allErrs, field.Forbidden(fldPath.Child("lastOperation"), "lastOperation cannot be true when currentOperation is not present"))
 		}
 	} else {
-		if status.OperationStartTime == nil {
-			allErrs = append(allErrs, field.Required(fldPath.Child("operationStartTime"), "operationStartTime is required when currentOperation is present"))
+		if status.OperationStartTime == nil && !status.OrphanMitigationInProgress {
+			allErrs = append(allErrs, field.Required(fldPath.Child("operationStartTime"), "operationStartTime is required when currentOperation is present and no orphan mitigation in progress"))
 		}
 		// Do not allow the instance to be ready if there is an on-going operation
 		for i, c := range status.Conditions {

pkg/apis/servicecatalog/validation/instance_test.go

@@ -493,6 +493,45 @@ func TestValidateServiceInstance(t *testing.T) {
 			create: true,
 			valid:  false,
 		},
+		{
+			name: "failed provision starting orphan mitigation",
+			instance: func() *servicecatalog.ServiceInstance {
+				i := validServiceInstanceWithInProgressProvision()
+				i.Status.OperationStartTime = nil
+				i.Status.OrphanMitigationInProgress = true
+				i.Status.Conditions = []servicecatalog.ServiceInstanceCondition{
+					{
+						Type:   servicecatalog.ServiceInstanceConditionReady,
+						Status: servicecatalog.ConditionFalse,
+					},
+					{
+						Type:   servicecatalog.ServiceInstanceConditionFailed,
+						Status: servicecatalog.ConditionTrue,
+					},
+				}
+				return i
+			}(),
+			valid: true,
+		},
+		{
+			name: "in-progress orphan mitigation",
+			instance: func() *servicecatalog.ServiceInstance {
+				i := validServiceInstanceWithInProgressProvision()
+				i.Status.OrphanMitigationInProgress = true
+				i.Status.Conditions = []servicecatalog.ServiceInstanceCondition{
+					{
+						Type:   servicecatalog.ServiceInstanceConditionReady,
+						Status: servicecatalog.ConditionFalse,
+					},
+					{
+						Type:   servicecatalog.ServiceInstanceConditionFailed,
+						Status: servicecatalog.ConditionTrue,
+					},
+				}
+				return i
+			}(),
+			valid: true,
+		},
 	}
 
 	for _, tc := range cases {

pkg/controller/controller_binding.go

@@ -131,7 +131,6 @@ func (c *controller) setAndUpdateOrphanMitigation(binding *v1beta1.ServiceBindin
 	)
 	toUpdate.Status.OrphanMitigationInProgress = true
 	toUpdate.Status.OperationStartTime = nil
-	toUpdate.Status.InProgressProperties = nil
 	glog.V(5).Info(s)
 
 	c.setServiceBindingCondition(
@@ -544,6 +543,7 @@ func (c *controller) reconcileServiceBinding(binding *v1beta1.ServiceBinding) er
 		// request, so this is what the Broker knows about the state of the
 		// binding.
 		toUpdate.Status.ExternalProperties = toUpdate.Status.InProgressProperties
+		toUpdate.Status.InProgressProperties = nil
 
 		err = c.injectServiceBinding(binding, response.Credentials)
 		if err != nil {

pkg/controller/controller_binding_test.go

@@ -311,11 +311,12 @@ func TestReconcileServiceBindingWithSecretConflict(t *testing.T) {
 	assertServiceBindingOperationInProgress(t, updatedServiceBinding, v1beta1.ServiceBindingOperationBind, binding)
 
 	updatedServiceBinding = assertUpdateStatus(t, actions[1], binding).(*v1beta1.ServiceBinding)
+
 	assertServiceBindingReadyFalse(t, updatedServiceBinding, errorInjectingBindResultReason)
 	assertServiceBindingCurrentOperation(t, updatedServiceBinding, v1beta1.ServiceBindingOperationBind)
 	assertServiceBindingOperationStartTimeSet(t, updatedServiceBinding, true)
 	assertServiceBindingReconciledGeneration(t, updatedServiceBinding, binding.Status.ReconciledGeneration)
-	assertServiceBindingInProgressPropertiesParameters(t, updatedServiceBinding, nil, "")
+	assertServiceBindingInProgressPropertiesNil(t, updatedServiceBinding)
 	// External properties are updated because the bind request with the Broker was successful
 	assertServiceBindingExternalPropertiesParameters(t, updatedServiceBinding, nil, "")
 	assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, false)
@@ -1979,16 +1980,12 @@ func TestReconcileBindingWithSecretConflictFailedAfterFinalRetry(t *testing.T) {
 	assertNumberOfActions(t, actions, 1)
 
 	updatedServiceBinding := assertUpdateStatus(t, actions[0], binding).(*v1beta1.ServiceBinding)
-	assertServiceBindingReadyFalse(t, updatedServiceBinding, errorServiceBindingOrphanMitigation)
+
+	assertServiceBindingCondition(t, updatedServiceBinding, v1beta1.ServiceBindingConditionReady, v1beta1.ConditionFalse, errorServiceBindingOrphanMitigation)
 	assertServiceBindingCondition(t, updatedServiceBinding, v1beta1.ServiceBindingConditionFailed, v1beta1.ConditionTrue, errorReconciliationRetryTimeoutReason)
-	assertServiceBindingCurrentOperation(t, updatedServiceBinding, v1beta1.ServiceBindingOperationBind)
-	assertServiceBindingOperationStartTimeSet(t, updatedServiceBinding, false)
-	assertServiceBindingReconciledGeneration(t, updatedServiceBinding, binding.Status.ReconciledGeneration)
+	assertServiceBindingStartingOrphanMitigation(t, updatedServiceBinding, binding)
 	assertServiceBindingInProgressPropertiesNil(t, updatedServiceBinding)
-	// External properties are updated because the bind request with the Broker was successful
 	assertServiceBindingExternalPropertiesParameters(t, updatedServiceBinding, nil, "")
-	assertServiceBindingCondition(t, updatedServiceBinding, v1beta1.ServiceBindingConditionReady, v1beta1.ConditionFalse, errorServiceBindingOrphanMitigation)
-	assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, true)
 
 	kubeActions := fakeKubeClient.Actions()
 	assertNumberOfActions(t, kubeActions, 2)
@@ -2333,10 +2330,15 @@ func TestReconcileBindingWithSetOrphanMitigation(t *testing.T) {
 		assertServiceBindingReadyFalse(t, updatedServiceBinding)
 
 		updatedServiceBinding = assertUpdateStatus(t, actions[1], binding).(*v1beta1.ServiceBinding)
-		assertServiceBindingReadyFalse(t, updatedServiceBinding)
-		assertServiceBindingCondition(t, updatedServiceBinding, v1beta1.ServiceBindingConditionReady, v1beta1.ConditionFalse)
 
-		assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, tc.setOrphanMitigation)
+		if tc.setOrphanMitigation {
+			assertServiceBindingStartingOrphanMitigation(t, updatedServiceBinding, binding)
+		} else {
+			assertServiceBindingReadyFalse(t, updatedServiceBinding)
+			assertServiceBindingCondition(t, updatedServiceBinding, v1beta1.ServiceBindingConditionReady, v1beta1.ConditionFalse)
+			assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, tc.setOrphanMitigation)
+			assertServiceBindingExternalPropertiesNil(t, updatedServiceBinding)
+		}
 	}
 }
 

pkg/controller/controller_instance.go

@@ -1924,7 +1924,6 @@ func setServiceInstanceStartOrphanMitigation(toUpdate *v1beta1.ServiceInstance)
 	toUpdate.Status.OperationStartTime = nil
 	toUpdate.Status.AsyncOpInProgress = false
 	toUpdate.Status.OrphanMitigationInProgress = true
-	toUpdate.Status.InProgressProperties = nil
 }
 
 // shouldStartOrphanMitigation returns whether an error with the given status

pkg/controller/controller_instance_test.go

@@ -2898,14 +2898,18 @@ func TestReconcileServiceInstanceWithHTTPStatusCodeErrorOrphanMitigation(t *test
 			continue
 		}
 
-		if tc.triggersOrphanMitigation && err == nil {
-			t.Errorf("%v: Reconciler should return error so that instance is orphan mitigated", tc.name)
-			continue
-		}
-
-		if !tc.triggersOrphanMitigation && err != nil {
-			t.Errorf("%v: Reconciler should treat as terminal condition and not requeue", tc.name)
-			continue
+		if tc.triggersOrphanMitigation {
+			// TODO(mkibbe): Rework this to be an expects, not asserts
+			assertServiceInstanceStartingOrphanMitigation(t, updatedServiceInstance, errorProvisionCallFailedReason, instance)
+			if err == nil {
+				t.Errorf("%v: Reconciler should return error so that instance is orphan mitigated", tc.name)
+				continue
+			}
+		} else {
+			if err != nil {
+				t.Errorf("%v: Reconciler should treat as terminal condition and not requeue", tc.name)
+				continue
+			}
 		}
 	}
 }

pkg/controller/controller_test.go

@@ -1762,6 +1762,14 @@ func assertServiceInstanceOperationInProgressWithParameters(t *testing.T, obj ru
 	assertServiceInstanceExternalPropertiesUnchanged(t, obj, originalInstance)
 }
 
+func assertServiceInstanceStartingOrphanMitigation(t *testing.T, obj runtime.Object, readyReason string, originalInstance *v1beta1.ServiceInstance) {
+	assertServiceInstanceCurrentOperation(t, obj, v1beta1.ServiceInstanceOperationProvision)
+	assertServiceInstanceReadyFalse(t, obj, readyReason)
+	assertServiceInstanceOperationStartTimeSet(t, obj, false)
+	assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration)
+	assertServiceInstanceOrphanMitigationInProgressTrue(t, obj)
+}
+
 func assertServiceInstanceOperationSuccess(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, planName string, originalInstance *v1beta1.ServiceInstance) {
 	assertServiceInstanceOperationSuccessWithParameters(t, obj, operation, planName, nil, "", originalInstance)
 }
@@ -1813,7 +1821,6 @@ func assertServiceInstanceRequestFailingError(t *testing.T, obj runtime.Object,
 	assertServiceInstanceCondition(t, obj, v1beta1.ServiceInstanceConditionFailed, v1beta1.ConditionTrue, failureReason)
 	assertServiceInstanceOperationStartTimeSet(t, obj, false)
 	assertAsyncOpInProgressFalse(t, obj)
-	assertServiceInstanceInProgressPropertiesNil(t, obj)
 	assertServiceInstanceExternalPropertiesUnchanged(t, obj, originalInstance)
 }
 
@@ -1822,6 +1829,7 @@ func assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t *testing.T, ob
 	assertServiceInstanceCurrentOperationClear(t, obj)
 	assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Generation)
 	assertServiceInstanceOrphanMitigationInProgressFalse(t, obj)
+	assertServiceInstanceInProgressPropertiesNil(t, obj)
 }
 
 func assertServiceInstanceRequestFailingErrorStartOrphanMitigation(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1beta1.ServiceInstance) {
@@ -2148,6 +2156,14 @@ func assertServiceBindingOperationInProgressWithParameters(t *testing.T, obj run
 	assertServiceBindingExternalPropertiesUnchanged(t, obj, originalBinding)
 }
 
+func assertServiceBindingStartingOrphanMitigation(t *testing.T, obj runtime.Object, originalBinding *v1beta1.ServiceBinding) {
+	assertServiceBindingCurrentOperation(t, obj, v1beta1.ServiceBindingOperationBind)
+	assertServiceBindingReadyFalse(t, obj, errorServiceBindingOrphanMitigation)
+	assertServiceBindingOperationStartTimeSet(t, obj, false)
+	assertServiceBindingReconciledGeneration(t, obj, originalBinding.Status.ReconciledGeneration)
+	assertServiceBindingOrphanMitigationSet(t, obj, true)
+}
+
 func assertServiceBindingOperationSuccess(t *testing.T, obj runtime.Object, operation v1beta1.ServiceBindingOperation, originalBinding *v1beta1.ServiceBinding) {
 	assertServiceBindingOperationSuccessWithParameters(t, obj, operation, nil, "", originalBinding)
 }