bug fix - avoid instance update if spec not changed (#359)

api/v1/serviceinstance_validating_webhook.go

@@ -50,14 +50,15 @@ func SetAllowMultipleTenants(isAllowed bool) {
 func (si *ServiceInstance) ValidateCreate() (warnings admission.Warnings, err error) {
 	serviceinstancelog.Info("validate create", "name", si.Name)
 	if !allowMultipleTenants && len(si.Spec.SubaccountID) > 0 {
-		serviceinstancelog.Error(fmt.Errorf("invalid subaccountID property"), "the operator installation does not allow multiple subaccunts")
+		serviceinstancelog.Error(fmt.Errorf("invalid subaccountID property"), "the operator installation does not allow multiple subaccounts")
 		return nil, fmt.Errorf("setting the subaccountID property is not allowed")
 	}
 	return nil, nil
 }
 
 func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) {
 	serviceinstancelog.Info("validate update", "name", si.Name)
+
 	oldInstance := old.(*ServiceInstance)
 	if oldInstance.Spec.SubaccountID != si.Spec.SubaccountID {
 		return nil, fmt.Errorf("changing the subaccountID for an existing instance is not allowed")

config/samples/services_v1_serviceinstance.yaml

@@ -4,4 +4,4 @@ metadata:
   name: sample-instance-1
 spec:
   serviceOfferingName: service-manager
-  servicePlanName: subaccount-admin
+  servicePlanName: subaccount-audit

controllers/base_controller.go

@@ -202,7 +202,7 @@ func setInProgressConditions(ctx context.Context, operationType smClientTypes.Op
 		Status:             metav1.ConditionFalse,
 		Reason:             getConditionReason(operationType, smClientTypes.INPROGRESS),
 		Message:            message,
-		ObservedGeneration: object.GetGeneration(),
+		ObservedGeneration: object.GetObservedGeneration(),
 	}
 	meta.SetStatusCondition(&conditions, lastOpCondition)
 	meta.SetStatusCondition(&conditions, getReadyCondition(object))
@@ -230,14 +230,14 @@ func setSuccessConditions(operationType smClientTypes.OperationCategory, object
 		Status:             metav1.ConditionTrue,
 		Reason:             getConditionReason(operationType, smClientTypes.SUCCEEDED),
 		Message:            message,
-		ObservedGeneration: object.GetGeneration(),
+		ObservedGeneration: object.GetObservedGeneration(),
 	}
 	readyCondition := metav1.Condition{
 		Type:               api.ConditionReady,
 		Status:             metav1.ConditionTrue,
 		Reason:             Provisioned,
 		Message:            message,
-		ObservedGeneration: object.GetGeneration(),
+		ObservedGeneration: object.GetObservedGeneration(),
 	}
 	meta.SetStatusCondition(&conditions, lastOpCondition)
 	meta.SetStatusCondition(&conditions, readyCondition)
@@ -257,7 +257,7 @@ func setCredRotationInProgressConditions(reason, message string, object api.SAPB
 		Status:             metav1.ConditionTrue,
 		Reason:             reason,
 		Message:            message,
-		ObservedGeneration: object.GetGeneration(),
+		ObservedGeneration: object.GetObservedGeneration(),
 	}
 	meta.SetStatusCondition(&conditions, credRotCondition)
 	object.SetConditions(conditions)
@@ -286,7 +286,7 @@ func setFailureConditions(operationType smClientTypes.OperationCategory, errorMe
 		Status:             metav1.ConditionFalse,
 		Reason:             reason,
 		Message:            message,
-		ObservedGeneration: object.GetGeneration(),
+		ObservedGeneration: object.GetObservedGeneration(),
 	}
 	meta.SetStatusCondition(&conditions, lastOpCondition)
 
@@ -295,7 +295,7 @@ func setFailureConditions(operationType smClientTypes.OperationCategory, errorMe
 		Status:             metav1.ConditionTrue,
 		Reason:             reason,
 		Message:            message,
-		ObservedGeneration: object.GetGeneration(),
+		ObservedGeneration: object.GetObservedGeneration(),
 	}
 	meta.SetStatusCondition(&conditions, failedCondition)
 	meta.SetStatusCondition(&conditions, getReadyCondition(object))
@@ -310,7 +310,7 @@ func setBlockedCondition(ctx context.Context, message string, object api.SAPBTPR
 	lastOpCondition.Reason = Blocked
 }
 
-func isDelete(object metav1.ObjectMeta) bool {
+func isMarkedForDeletion(object metav1.ObjectMeta) bool {
 	return !object.DeletionTimestamp.IsZero()
 }
 
@@ -400,5 +400,5 @@ func getReadyCondition(object api.SAPBTPResource) metav1.Condition {
 		reason = Provisioned
 	}
 
-	return metav1.Condition{Type: api.ConditionReady, Status: status, Reason: reason, ObservedGeneration: object.GetGeneration()}
+	return metav1.Condition{Type: api.ConditionReady, Status: status, Reason: reason}
 }

controllers/servicebinding_controller.go

@@ -92,7 +92,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return ctrl.Result{}, err
 	}
 
-	if isDelete(serviceBinding.ObjectMeta) {
+	if isMarkedForDeletion(serviceBinding.ObjectMeta) {
 		return r.delete(ctx, serviceBinding, serviceInstance.Spec.SubaccountID)
 	}
 
@@ -348,7 +348,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *ser
 		freshStatus := servicesv1.ServiceBindingStatus{
 			Conditions: serviceBinding.GetConditions(),
 		}
-		if isDelete(serviceBinding.ObjectMeta) {
+		if isMarkedForDeletion(serviceBinding.ObjectMeta) {
 			freshStatus.BindingID = serviceBinding.Status.BindingID
 		}
 		serviceBinding.Status = freshStatus
@@ -451,7 +451,7 @@ func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *servic
 
 	if !isFailed(binding) {
 		if _, err := r.getSecret(ctx, binding.Namespace, binding.Spec.SecretName); err != nil {
-			if apierrors.IsNotFound(err) && !isDelete(binding.ObjectMeta) {
+			if apierrors.IsNotFound(err) && !isMarkedForDeletion(binding.ObjectMeta) {
 				log.Info(fmt.Sprintf("secret not found recovering binding %s", binding.Name))
 				binding.Status.BindingID = ""
 				binding.Status.Ready = metav1.ConditionFalse
@@ -911,7 +911,7 @@ func (r *ServiceBindingReconciler) recover(ctx context.Context, serviceBinding *
 }
 
 func isStaleServiceBinding(binding *servicesv1.ServiceBinding) bool {
-	if isDelete(binding.ObjectMeta) {
+	if isMarkedForDeletion(binding.ObjectMeta) {
 		return false
 	}
 
@@ -995,7 +995,7 @@ func bindingAlreadyOwnedByInstance(instance *servicesv1.ServiceInstance, binding
 }
 
 func serviceNotUsable(instance *servicesv1.ServiceInstance) bool {
-	if isDelete(instance.ObjectMeta) {
+	if isMarkedForDeletion(instance.ObjectMeta) {
 		return true
 	}
 	if len(instance.Status.Conditions) != 0 {

controllers/serviceinstance_controller.go

@@ -69,7 +69,6 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 	serviceInstance = serviceInstance.DeepCopy()
-	serviceInstance.SetObservedGeneration(serviceInstance.Generation)
 
 	if len(serviceInstance.GetConditions()) == 0 {
 		err := r.init(ctx, serviceInstance)
@@ -78,7 +77,17 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		}
 	}
 
-	if isDelete(serviceInstance.ObjectMeta) {
+	if isFinalState(ctx, serviceInstance) {
+		if len(serviceInstance.Status.HashedSpec) == 0 {
+			updateHashedSpecValue(serviceInstance)
+			return ctrl.Result{}, r.Client.Status().Update(ctx, serviceInstance)
+		}
+		return ctrl.Result{}, nil
+	}
+
+	if isMarkedForDeletion(serviceInstance.ObjectMeta) {
+		// delete updates the generation
+		serviceInstance.SetObservedGeneration(serviceInstance.Generation)
 		return r.deleteInstance(ctx, serviceInstance)
 	}
 
@@ -95,14 +104,8 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		}
 	}
 
-	if isFinalState(serviceInstance) {
-		log.Info(fmt.Sprintf("Final state, spec did not change, and we are not in progress - ignoring... Generation is - %v", serviceInstance.Generation))
-		if len(serviceInstance.Status.HashedSpec) == 0 {
-			updateHashedSpecValue(serviceInstance)
-			return ctrl.Result{}, r.Client.Status().Update(ctx, serviceInstance)
-		}
-		return ctrl.Result{}, nil
-	}
+	log.Info(fmt.Sprintf("instance is not in final state, handling... (generation: %d, observedGen: %d", serviceInstance.Generation, serviceInstance.Status.ObservedGeneration))
+	serviceInstance.SetObservedGeneration(serviceInstance.Generation)
 
 	smClient, err := r.getSMClient(ctx, serviceInstance, serviceInstance.Spec.SubaccountID)
 	if err != nil {
@@ -342,7 +345,7 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *s
 		setInProgressConditions(ctx, serviceInstance.Status.OperationType, statusErr.Error(), serviceInstance)
 		// if failed to read operation status we cleanup the status to trigger re-sync from SM
 		freshStatus := servicesv1.ServiceInstanceStatus{Conditions: serviceInstance.GetConditions(), ObservedGeneration: serviceInstance.Generation}
-		if isDelete(serviceInstance.ObjectMeta) {
+		if isMarkedForDeletion(serviceInstance.ObjectMeta) {
 			freshStatus.InstanceID = serviceInstance.Status.InstanceID
 		}
 		serviceInstance.Status = freshStatus
@@ -512,24 +515,37 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte
 	return ctrl.Result{Requeue: isTransient}, r.updateStatus(ctx, object)
 }
 
-func isFinalState(serviceInstance *servicesv1.ServiceInstance) bool {
-	// succeeded condition represents last operation, and it is constantly synced with generation
-	succeededCondition := meta.FindStatusCondition(serviceInstance.GetConditions(), api.ConditionSucceeded)
-	if succeededCondition == nil || succeededCondition.ObservedGeneration != serviceInstance.Generation {
+func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstance) bool {
+	log := GetLogger(ctx)
+	if isMarkedForDeletion(serviceInstance.ObjectMeta) {
+		log.Info("instance is not in final state, it is marked for deletion")
+		return false
+	}
+	if len(serviceInstance.Status.OperationURL) > 0 {
+		log.Info(fmt.Sprintf("instance is not in final state, async operation is in progress (%s)", serviceInstance.Status.OperationURL))
+		return false
+	}
+	if serviceInstance.Generation != serviceInstance.GetObservedGeneration() {
+		log.Info(fmt.Sprintf("instance is not in final state, generation: %d, observedGen: %d", serviceInstance.Generation, serviceInstance.GetObservedGeneration()))
 		return false
 	}
 
 	// succeeded=false for current generation, and without failed=true --> transient error retry
 	if isInProgress(serviceInstance) {
+		log.Info("instance is not in final state, sync operation is in progress")
 		return false
 	}
 
-	// for cases of instance update while polling for create/update
-	if getSpecHash(serviceInstance) != serviceInstance.Status.HashedSpec {
+	if sharingUpdateRequired(serviceInstance) {
+		log.Info("instance is not in final state, need to sync sharing status")
+		if len(serviceInstance.Status.HashedSpec) == 0 {
+			updateHashedSpecValue(serviceInstance)
+		}
 		return false
 	}
 
-	return !sharingUpdateRequired(serviceInstance)
+	log.Info(fmt.Sprintf("instance is in final state (generation: %d)", serviceInstance.Generation))
+	return true
 }
 
 // TODO unit test

controllers/serviceinstance_controller_test.go

@@ -34,7 +34,6 @@ const (
 	testNamespace            = "ic-test-namespace"
 	fakeOfferingName         = "offering-a"
 	fakePlanName             = "plan-a"
-	testSubaccountID         = "subaccountID"
 )
 
 var _ = Describe("ServiceInstance controller", func() {
@@ -669,7 +668,6 @@ var _ = Describe("ServiceInstance controller", func() {
 				deleteInstance(ctx, serviceInstance, false)
 				waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, DeleteInProgress, "")
 			})
-
 			When("polling ends with success", func() {
 				BeforeEach(func() {
 					fakeClient.StatusReturns(&smclientTypes.Operation{
@@ -742,6 +740,28 @@ var _ = Describe("ServiceInstance controller", func() {
 		})
 	})
 
+	Describe("full reconcile", func() {
+		When("instance hashedSpec is not initialized", func() {
+			BeforeEach(func() {
+				serviceInstance = createInstance(ctx, instanceSpec, true)
+			})
+			It("should not send update request and update the hashed spec", func() {
+				hashed := serviceInstance.Status.HashedSpec
+				serviceInstance.Status.HashedSpec = ""
+				Expect(k8sClient.Status().Update(ctx, serviceInstance)).To(Succeed())
+
+				Eventually(func() bool {
+					err := k8sClient.Get(ctx, types.NamespacedName{Name: serviceInstance.Name, Namespace: serviceInstance.Namespace}, serviceInstance)
+					if err != nil {
+						return false
+					}
+					cond := meta.FindStatusCondition(serviceInstance.GetConditions(), api.ConditionSucceeded)
+					return serviceInstance.Status.HashedSpec == hashed && cond != nil && cond.Reason == Created
+				}, timeout, interval).Should(BeTrue())
+			})
+		})
+	})
+
 	Context("Recovery", func() {
 		When("instance exists in SM", func() {
 			recoveredInstance := smclientTypes.ServiceInstance{
@@ -1042,7 +1062,7 @@ var _ = Describe("ServiceInstance controller", func() {
 							ExternalName: "name",
 						}}
 					instance.SetGeneration(2)
-					Expect(isFinalState(instance)).To(BeFalse())
+					Expect(isFinalState(ctx, instance)).To(BeFalse())
 				})
 
 				When("Succeeded is false", func() {
@@ -1068,11 +1088,34 @@ var _ = Describe("ServiceInstance controller", func() {
 							},
 						}
 						instance.SetGeneration(1)
-						Expect(isFinalState(instance)).To(BeFalse())
+						Expect(isFinalState(ctx, instance)).To(BeFalse())
 					})
 				})
 			})
 
+			When("async operation in progress", func() {
+				It("should return false", func() {
+					var instance = &v1.ServiceInstance{
+						Status: v1.ServiceInstanceStatus{
+							Conditions: []metav1.Condition{
+								{
+									Type:               api.ConditionReady,
+									Status:             metav1.ConditionTrue,
+									ObservedGeneration: 1,
+								},
+							},
+							HashedSpec:         "929e78f4449f8036ce39da3cc3e7eaea",
+							OperationURL:       "/operations/somepollingurl",
+							ObservedGeneration: 2,
+						},
+						Spec: v1.ServiceInstanceSpec{
+							ExternalName: "name",
+						}}
+					instance.SetGeneration(2)
+					Expect(isFinalState(ctx, instance)).To(BeFalse())
+				})
+			})
+
 			When("in progress", func() {
 				It("should return false", func() {
 					var instance = &v1.ServiceInstance{
@@ -1095,7 +1138,7 @@ var _ = Describe("ServiceInstance controller", func() {
 							ExternalName: "name",
 						}}
 					instance.SetGeneration(2)
-					Expect(isFinalState(instance)).To(BeFalse())
+					Expect(isFinalState(ctx, instance)).To(BeFalse())
 				})
 			})
 
@@ -1121,7 +1164,7 @@ var _ = Describe("ServiceInstance controller", func() {
 							ExternalName: "name",
 						}}
 					instance.SetGeneration(2)
-					Expect(isFinalState(instance)).To(BeFalse())
+					Expect(isFinalState(ctx, instance)).To(BeFalse())
 				})
 			})
 
@@ -1153,7 +1196,7 @@ var _ = Describe("ServiceInstance controller", func() {
 							Shared:       pointer.Bool(true),
 						}}
 					instance.SetGeneration(2)
-					Expect(isFinalState(instance)).To(BeFalse())
+					Expect(isFinalState(ctx, instance)).To(BeFalse())
 				})
 			})
 
@@ -1177,14 +1220,15 @@ var _ = Describe("ServiceInstance controller", func() {
 									Status: metav1.ConditionTrue,
 								},
 							},
-							HashedSpec: "929e78f4449f8036ce39da3cc3e7eaea",
+							HashedSpec:         "929e78f4449f8036ce39da3cc3e7eaea",
+							ObservedGeneration: 2,
 						},
 						Spec: v1.ServiceInstanceSpec{
 							ExternalName: "name",
 							Shared:       pointer.Bool(true),
 						}}
 					instance.SetGeneration(2)
-					Expect(isFinalState(instance)).To(BeTrue())
+					Expect(isFinalState(ctx, instance)).To(BeTrue())
 				})
 			})
 		})