Ensure finalizer is removed when 409 Conflict returned on status update

pkg/controller/controller_instance.go

@@ -1211,6 +1211,22 @@ func (c *controller) updateServiceInstanceReferences(toUpdate *v1beta1.ServiceIn
 // Note: objects coming from informers should never be mutated; the instance
 // passed to this method should always be a deep copy.
 func (c *controller) updateServiceInstanceStatus(instance *v1beta1.ServiceInstance) (*v1beta1.ServiceInstance, error) {
+	return c.updateServiceInstanceStatusWithRetries(instance, nil)
+}
+
+// updateServiceInstanceStatusWithRetries updates the status
+// and automatically retries if a 409 Conflict error is
+// returned by the API server.
+// If a conflict occurs, the function overrides the new
+// version's status with the status on the ServiceInstance passed
+// to it; it also runs the provided postConflictUpdateFunc,
+// allowing the caller to make additional changes to the
+// new version of the instance - to other parts of the object
+// (e.g. finalizers).
+func (c *controller) updateServiceInstanceStatusWithRetries(
+	instance *v1beta1.ServiceInstance,
+	postConflictUpdateFunc func(*v1beta1.ServiceInstance)) (*v1beta1.ServiceInstance, error) {
+
 	pcb := pretty.NewInstanceContextBuilder(instance)
 
 	const interval = 100 * time.Millisecond
@@ -1232,6 +1248,9 @@ func (c *controller) updateServiceInstanceStatus(instance *v1beta1.ServiceInstan
 				return false, err
 			}
 			instanceToUpdate.Status = instance.Status
+			if postConflictUpdateFunc != nil {
+				postConflictUpdateFunc(instanceToUpdate)
+			}
 			return false, nil
 		}
 
@@ -1632,11 +1651,8 @@ func (c *controller) prepareServiceInstanceLastOperationRequest(instance *v1beta
 // updating of a ServiceInstance that has successfully finished graceful
 // deletion.
 func (c *controller) processServiceInstanceGracefulDeletionSuccess(instance *v1beta1.ServiceInstance) error {
-	finalizers := sets.NewString(instance.Finalizers...)
-	finalizers.Delete(v1beta1.FinalizerServiceCatalog)
-	instance.Finalizers = finalizers.List()
-
-	if _, err := c.updateServiceInstanceStatus(instance); err != nil {
+	c.removeFinalizer(instance)
+	if _, err := c.updateServiceInstanceStatusWithRetries(instance, c.removeFinalizer); err != nil {
 		return err
 	}
 
@@ -1646,6 +1662,12 @@ func (c *controller) processServiceInstanceGracefulDeletionSuccess(instance *v1b
 	return nil
 }
 
+func (c *controller) removeFinalizer(instance *v1beta1.ServiceInstance) {
+	finalizers := sets.NewString(instance.Finalizers...)
+	finalizers.Delete(v1beta1.FinalizerServiceCatalog)
+	instance.Finalizers = finalizers.List()
+}
+
 // handleServiceInstanceReconciliationError is a helper function that handles
 // on error whether the error represents an operation error and should update
 // the ServiceInstance resource.

pkg/controller/controller_instance_test.go

@@ -2161,6 +2161,70 @@ func TestReconcileServiceInstanceDeleteDoesNotInvokeClusterServiceBroker(t *test
 	assertNumEvents(t, events, 0)
 }
 
+// TestFinalizerClearedWhen409ConflictEncounteredOnStatusUpdate verfies that the finalizer
+// is removed even when the status update gets back a 409 Conflict from the API server
+// because the controller is working with an old version of the ServiceInstance
+func TestFinalizerClearedWhen409ConflictEncounteredOnStatusUpdate(t *testing.T) {
+	fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, noFakeActions())
+
+	sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker())
+	sharedInformers.ClusterServiceClasses().Informer().GetStore().Add(getTestClusterServiceClass())
+	sharedInformers.ClusterServicePlans().Informer().GetStore().Add(getTestClusterServicePlan())
+
+	instance := getTestServiceInstanceWithRefs()
+	instance.ResourceVersion = "1"
+	instance.ObjectMeta.DeletionTimestamp = &metav1.Time{}
+	instance.ObjectMeta.Finalizers = []string{v1beta1.FinalizerServiceCatalog}
+	instance.Generation = 1
+	instance.Status.ReconciledGeneration = 0
+	instance.Status.ObservedGeneration = 0
+	instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned
+	instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusNotRequired
+
+	newerInstance := instance.DeepCopy()
+	newerInstance.ResourceVersion = "2"
+
+	fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) {
+		return true, newerInstance, nil
+	})
+	fakeCatalogClient.AddReactor("update", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) {
+		updateAction := action.(clientgotesting.UpdateAction)
+		object := updateAction.GetObject()
+		instance := object.(*v1beta1.ServiceInstance)
+
+		if instance.ResourceVersion == "1" {
+			return true, nil, apierrors.NewConflict(action.GetResource().GroupResource(), instance.Name, errors.New("object has changed"))
+		}
+		return false, nil, nil
+	})
+
+	if err := reconcileServiceInstance(t, testController, instance); err != nil {
+		t.Fatalf("This should not fail : %v", err)
+	}
+
+	brokerActions := fakeClusterServiceBrokerClient.Actions()
+	assertNumberOfClusterServiceBrokerActions(t, brokerActions, 0)
+
+	// Verify no core kube actions occurred
+	kubeActions := fakeKubeClient.Actions()
+	assertNumberOfActions(t, kubeActions, 0)
+
+	actions := fakeCatalogClient.Actions()
+	// The actions should be:
+	// 0. Update the instance status (with finalizer removed); the server responds with 409 Conflict
+	// 1. Fetch the fresh instance
+	// 2. Update the fresh instance. Finalizer should be removed again.
+	assertNumberOfActions(t, actions, 3)
+
+	assertUpdateStatus(t, actions[0], instance)
+	assertGet(t, actions[1], instance)
+	updatedServiceInstance := assertUpdateStatus(t, actions[2], instance)
+	assertEmptyFinalizers(t, updatedServiceInstance)
+
+	events := getRecordedEvents(testController)
+	assertNumEvents(t, events, 0)
+}
+
 // TestReconcileServiceInstanceWithFailedCondition tests reconciling an instance that
 // has a status condition set to Failed.
 // Instances with Failed condition are retriable after updating the spec.