From 66b51b592eb069e96b31b8ecbb01c47baf7c45ca Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Tue, 20 Feb 2018 20:58:48 +1100 Subject: [PATCH 01/22] Add ObservedGeneration and Provisioned into ServiceInstanceStatus --- pkg/apis/servicecatalog/types.go | 12 +++ pkg/apis/servicecatalog/v1beta1/types.go | 12 +++ pkg/controller/controller_instance.go | 100 +++++++++++++++-------- 3 files changed, 90 insertions(+), 34 deletions(-) diff --git a/pkg/apis/servicecatalog/types.go b/pkg/apis/servicecatalog/types.go index b1139d3ab52..be18bb37f26 100644 --- a/pkg/apis/servicecatalog/types.go +++ b/pkg/apis/servicecatalog/types.go @@ -537,8 +537,20 @@ type ServiceInstanceStatus struct { // ReconciledGeneration is the 'Generation' of the serviceInstanceSpec that // was last processed by the controller. The reconciled generation is updated // even if the controller failed to process the spec. + // Deprecated: use ObservedGeneration instead ReconciledGeneration int64 + // ObservedGeneration is the 'Generation' of the serviceInstanceSpec that + // was last processed by the controller. The observed generation is updated + // whenever the status is updated regardless of operation result + ObservedGeneration int64 + + // Provisioned is the flag that marks whether the instance has been + // successfully provisioned (i.e. it exists on the broker's side and can be + // updated or deprovisioned). The flag should be reset after successful + // deprovisioning. + Provisioned bool + // OperationStartTime is the time at which the current operation began. OperationStartTime *metav1.Time diff --git a/pkg/apis/servicecatalog/v1beta1/types.go b/pkg/apis/servicecatalog/v1beta1/types.go index d8479559632..34d869e1f40 100644 --- a/pkg/apis/servicecatalog/v1beta1/types.go +++ b/pkg/apis/servicecatalog/v1beta1/types.go @@ -601,8 +601,20 @@ type ServiceInstanceStatus struct { // ReconciledGeneration is the 'Generation' of the serviceInstanceSpec that // was last processed by the controller. The reconciled generation is updated // even if the controller failed to process the spec. + // Deprecated: use ObservedGeneration instead ReconciledGeneration int64 `json:"reconciledGeneration"` + // ObservedGeneration is the 'Generation' of the serviceInstanceSpec that + // was last processed by the controller. The observed generation is updated + // whenever the status is updated regardless of operation result + ObservedGeneration int64 `json:"observedGeneration"` + + // Provisioned is the flag that marks whether the instance has been + // successfully provisioned (i.e. it exists on the broker's side and can be + // updated or deprovisioned). The flag should be reset after successful + // deprovisioning. + Provisioned bool `json:"provisioned"` + // OperationStartTime is the time at which the current operation began. OperationStartTime *metav1.Time `json:"operationStartTime,omitempty"` diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index fc21cfe4543..c279b871473 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -228,6 +228,16 @@ func (c *controller) reconcileServiceInstanceKey(key string) error { // error is returned to indicate that the instance has not been fully // processed and should be resubmitted at a later time. func (c *controller) reconcileServiceInstance(instance *v1beta1.ServiceInstance) error { + handled, err := c.initObservedGeneration(instance) + if err != nil { + return err + } + if handled { + // Update the status and finish the iteration + // The updates instance will be automatically added to the instance queue + // and be processed again + return nil + } reconciliationAction := getReconciliationActionForServiceInstance(instance) switch reconciliationAction { case reconcileAdd: @@ -244,6 +254,23 @@ func (c *controller) reconcileServiceInstance(instance *v1beta1.ServiceInstance) } } +// initObservedGeneration implements ObservedGeneration initialization based on +// ReconciledGeneration for status API migration +func (c *controller) initObservedGeneration(instance *v1beta1.ServiceInstance) (bool, error) { + if instance.Status.ObservedGeneration == 0 && instance.Status.ReconciledGeneration == 0 { + instance.Status.ObservedGeneration = instance.Status.ReconciledGeneration + // Before we implement https://github.com/kubernetes-incubator/service-catalog/issues/1715 + // and switch to non-terminal errors, the "Failed":"True" is a sign that the provisioning failed + instance.Status.Provisioned = !isServiceInstanceFailed(instance) + _, err := c.updateServiceInstanceStatus(instance) + if err != nil { + return false, err + } + return true, nil + } + return false, nil +} + // reconcileServiceInstanceAdd is responsible for handling the provisioning // of new service instances. func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstance) error { @@ -313,7 +340,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan ) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorProvisionCallFailedReason, msg) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, "ClusterServiceBrokerReturnedFailure", msg) - return c.processProvisionFailure(instance, readyCond, failedCond, shouldStartOrphanMitigation(httpErr.StatusCode)) + return c.processProvisionFailure(instance, readyCond, failedCond, shouldStartOrphanMitigation(httpErr.StatusCode), true) } reason := errorErrorCallingProvisionReason @@ -324,7 +351,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan msg := fmt.Sprintf("Communication with the ClusterServiceBroker timed out; operation will not be retried: %v", urlErr) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, msg) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, reason, msg) - return c.processProvisionFailure(instance, readyCond, failedCond, true) + return c.processProvisionFailure(instance, readyCond, failedCond, true, true) } // All other errors should be retried, unless the @@ -335,7 +362,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan if c.reconciliationRetryDurationExceeded(instance.Status.OperationStartTime) { msg := "Stopping reconciliation retries because too much time has elapsed" failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, errorReconciliationRetryTimeoutReason, msg) - return c.processProvisionFailure(instance, readyCond, failedCond, false) + return c.processProvisionFailure(instance, readyCond, failedCond, false, true) } return c.processServiceInstanceOperationError(instance, readyCond) @@ -345,7 +372,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan return c.processProvisionAsyncResponse(instance, response) } - return c.processProvisionSuccess(instance, response.DashboardURL) + return c.processProvisionSuccess(instance, response.DashboardURL, true) } // reconcileServiceInstanceUpdate is responsible for handling updating the plan @@ -410,7 +437,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns if httpErr, ok := osb.IsHTTPError(err); ok { msg := fmt.Sprintf("ClusterServiceBroker returned a failure for update call; update will not be retried: %v", httpErr) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorUpdateInstanceCallFailedReason, msg) - return c.processUpdateServiceInstanceFailure(instance, readyCond) + return c.processUpdateServiceInstanceFailure(instance, readyCond, true) } reason := errorErrorCallingUpdateInstanceReason @@ -418,7 +445,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns if urlErr, ok := err.(*url.Error); ok && urlErr.Timeout() { msg := fmt.Sprintf("Communication with the ClusterServiceBroker timed out; update will not be retried: %v", urlErr) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, msg) - return c.processUpdateServiceInstanceFailure(instance, readyCond) + return c.processUpdateServiceInstanceFailure(instance, readyCond, true) } msg := fmt.Sprintf("The update call failed and will be retried: Error communicating with broker for updating: %s", err) @@ -431,7 +458,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns msg = "Stopping reconciliation retries because too much time has elapsed" readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorReconciliationRetryTimeoutReason, msg) - return c.processUpdateServiceInstanceFailure(instance, readyCond) + return c.processUpdateServiceInstanceFailure(instance, readyCond, true) } readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, msg) @@ -442,7 +469,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns return c.processUpdateServiceInstanceAsyncResponse(instance, response) } - return c.processUpdateServiceInstanceSuccess(instance) + return c.processUpdateServiceInstanceSuccess(instance, true) } // reconcileServiceInstanceDelete is responsible for handling any instance whose @@ -484,7 +511,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns msg := fmt.Sprintf("ServiceInstance has invalid DeprovisionStatus field: %v", instance.Status.DeprovisionStatus) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionUnknown, errorInvalidDeprovisionStatusReason, msg) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, errorInvalidDeprovisionStatusReason, msg) - return c.processDeprovisionFailure(instance, readyCond, failedCond) + return c.processDeprovisionFailure(instance, readyCond, failedCond, true) } // We don't want to delete the instance if there are any bindings associated. @@ -535,7 +562,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns if c.reconciliationRetryDurationExceeded(instance.Status.OperationStartTime) { msg := "Stopping reconciliation retries because too much time has elapsed" failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, errorReconciliationRetryTimeoutReason, msg) - return c.processDeprovisionFailure(instance, readyCond, failedCond) + return c.processDeprovisionFailure(instance, readyCond, failedCond, true) } return c.processServiceInstanceOperationError(instance, readyCond) @@ -545,7 +572,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns return c.processDeprovisionAsyncResponse(instance, response) } - return c.processDeprovisionSuccess(instance) + return c.processDeprovisionSuccess(instance, true) } func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) error { @@ -578,7 +605,7 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro // If the operation was for delete and we receive a http.StatusGone, // this is considered a success as per the spec if osb.IsGoneError(err) && deleting { - if err := c.processDeprovisionSuccess(instance); err != nil { + if err := c.processDeprovisionSuccess(instance, false); err != nil { return c.handleServiceInstancePollingError(instance, err) } return c.finishPollingServiceInstance(instance) @@ -642,11 +669,11 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro var err error switch { case deleting: - err = c.processDeprovisionSuccess(instance) + err = c.processDeprovisionSuccess(instance, false) case provisioning: - err = c.processProvisionSuccess(instance, nil) + err = c.processProvisionSuccess(instance, nil, false) default: - err = c.processUpdateServiceInstanceSuccess(instance) + err = c.processUpdateServiceInstanceSuccess(instance, false) } if err != nil { return c.handleServiceInstancePollingError(instance, err) @@ -682,11 +709,11 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro message := "Provision call failed: " + description readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, message) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, reason, message) - err = c.processProvisionFailure(instance, readyCond, failedCond, false) + err = c.processProvisionFailure(instance, readyCond, failedCond, false, false) default: msg := "Update call failed: " + description readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorUpdateInstanceCallFailedReason, msg) - err = c.processUpdateServiceInstanceFailure(instance, readyCond) + err = c.processUpdateServiceInstanceFailure(instance, readyCond, false) } if err != nil { return c.handleServiceInstancePollingError(instance, err) @@ -729,14 +756,14 @@ func (c *controller) processServiceInstancePollingFailureRetryTimeout(instance * var err error switch { case deleting: - err = c.processDeprovisionFailure(instance, readyCond, failedCond) + err = c.processDeprovisionFailure(instance, readyCond, failedCond, false) case provisioning: // always finish polling instance, as triggering OM will return an error c.finishPollingServiceInstance(instance) - return c.processProvisionFailure(instance, readyCond, failedCond, true) + return c.processProvisionFailure(instance, readyCond, failedCond, true, false) default: readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorReconciliationRetryTimeoutReason, msg) - err = c.processUpdateServiceInstanceFailure(instance, readyCond) + err = c.processUpdateServiceInstanceFailure(instance, readyCond, false) } if err != nil { return c.handleServiceInstancePollingError(instance, err) @@ -1084,7 +1111,7 @@ func (c *controller) updateServiceInstanceCondition( // 2 - any error that occurred func (c *controller) recordStartOfServiceInstanceOperation(toUpdate *v1beta1.ServiceInstance, operation v1beta1.ServiceInstanceOperation, inProgressProperties *v1beta1.ServiceInstancePropertiesState) (*v1beta1.ServiceInstance, error) { currentReconciledGeneration := toUpdate.Status.ReconciledGeneration - clearServiceInstanceCurrentOperation(toUpdate) + clearServiceInstanceCurrentOperation(toUpdate, true) toUpdate.Status.ReconciledGeneration = currentReconciledGeneration toUpdate.Status.CurrentOperation = operation @@ -1159,7 +1186,7 @@ func (c *controller) checkForRemovedClassAndPlan(instance *v1beta1.ServiceInstan // clearServiceInstanceCurrentOperation sets the fields of the instance's Status // to indicate that there is no current operation being performed. The Status // is *not* recorded in the registry. -func clearServiceInstanceCurrentOperation(toUpdate *v1beta1.ServiceInstance) { +func clearServiceInstanceCurrentOperation(toUpdate *v1beta1.ServiceInstance, updateObservedGeneration bool) { toUpdate.Status.CurrentOperation = "" toUpdate.Status.OperationStartTime = nil toUpdate.Status.AsyncOpInProgress = false @@ -1167,6 +1194,9 @@ func clearServiceInstanceCurrentOperation(toUpdate *v1beta1.ServiceInstance) { toUpdate.Status.LastOperation = nil toUpdate.Status.InProgressProperties = nil toUpdate.Status.ReconciledGeneration = toUpdate.Generation + if updateObservedGeneration { + toUpdate.Status.ObservedGeneration = toUpdate.Generation + } } // rollbackReconciledGenerationOnDeletion resets the ReconciledGeneration if a @@ -1441,12 +1471,13 @@ func (c *controller) processServiceInstanceOperationError(instance *v1beta1.Serv // processProvisionSuccess handles the logging and updating of a // ServiceInstance that has successfully been provisioned at the broker. -func (c *controller) processProvisionSuccess(instance *v1beta1.ServiceInstance, dashboardURL *string) error { +func (c *controller) processProvisionSuccess(instance *v1beta1.ServiceInstance, dashboardURL *string, updateObservedGeneration bool) error { + instance.Status.Provisioned = true setServiceInstanceDashboardURL(instance, dashboardURL) setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, v1beta1.ConditionTrue, successProvisionReason, successProvisionMessage) instance.Status.ExternalProperties = instance.Status.InProgressProperties currentReconciledGeneration := instance.Status.ReconciledGeneration - clearServiceInstanceCurrentOperation(instance) + clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) if _, err := c.updateServiceInstanceStatus(instance); err != nil { @@ -1459,7 +1490,7 @@ func (c *controller) processProvisionSuccess(instance *v1beta1.ServiceInstance, // processProvisionFailure handles the logging and updating of a // ServiceInstance that hit a terminal failure during provision reconciliation. -func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition, shouldMitigateOrphan bool) error { +func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition, shouldMitigateOrphan bool, updateObservedGeneration bool) error { currentReconciledGeneration := instance.Status.ReconciledGeneration if failedCond == nil { return fmt.Errorf("failedCond must not be nil") @@ -1487,7 +1518,7 @@ func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance, err = fmt.Errorf(failedCond.Message) } else { - clearServiceInstanceCurrentOperation(instance) + clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) } @@ -1517,11 +1548,11 @@ func (c *controller) processProvisionAsyncResponse(instance *v1beta1.ServiceInst // processUpdateServiceInstanceSuccess handles the logging and updating of a // ServiceInstance that has successfully been updated at the broker. -func (c *controller) processUpdateServiceInstanceSuccess(instance *v1beta1.ServiceInstance) error { +func (c *controller) processUpdateServiceInstanceSuccess(instance *v1beta1.ServiceInstance, updateObservedGeneration bool) error { setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, v1beta1.ConditionTrue, successUpdateInstanceReason, successUpdateInstanceMessage) instance.Status.ExternalProperties = instance.Status.InProgressProperties currentReconciledGeneration := instance.Status.ReconciledGeneration - clearServiceInstanceCurrentOperation(instance) + clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) if _, err := c.updateServiceInstanceStatus(instance); err != nil { @@ -1534,12 +1565,12 @@ func (c *controller) processUpdateServiceInstanceSuccess(instance *v1beta1.Servi // processUpdateServiceInstanceFailure handles the logging and updating of a // ServiceInstance that hit a terminal failure during update reconciliation. -func (c *controller) processUpdateServiceInstanceFailure(instance *v1beta1.ServiceInstance, readyCond *v1beta1.ServiceInstanceCondition) error { +func (c *controller) processUpdateServiceInstanceFailure(instance *v1beta1.ServiceInstance, readyCond *v1beta1.ServiceInstanceCondition, updateObservedGeneration bool) error { c.recorder.Event(instance, corev1.EventTypeWarning, readyCond.Reason, readyCond.Message) currentReconciledGeneration := instance.Status.ReconciledGeneration setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, readyCond.Status, readyCond.Reason, readyCond.Message) - clearServiceInstanceCurrentOperation(instance) + clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) if _, err := c.updateServiceInstanceStatus(instance); err != nil { @@ -1567,7 +1598,7 @@ func (c *controller) processUpdateServiceInstanceAsyncResponse(instance *v1beta1 // processDeprovisionSuccess handles the logging and updating of // a ServiceInstance that has successfully been deprovisioned at the broker. -func (c *controller) processDeprovisionSuccess(instance *v1beta1.ServiceInstance) error { +func (c *controller) processDeprovisionSuccess(instance *v1beta1.ServiceInstance, updateObservedGeneration bool) error { mitigatingOrphan := instance.Status.OrphanMitigationInProgress reason := successDeprovisionReason @@ -1577,8 +1608,9 @@ func (c *controller) processDeprovisionSuccess(instance *v1beta1.ServiceInstance msg = successOrphanMitigationMessage } + instance.Status.Provisioned = false setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, v1beta1.ConditionFalse, reason, msg) - clearServiceInstanceCurrentOperation(instance) + clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) instance.Status.ExternalProperties = nil instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusSucceeded @@ -1601,7 +1633,7 @@ func (c *controller) processDeprovisionSuccess(instance *v1beta1.ServiceInstance // processDeprovisionFailure handles the logging and updating of a // ServiceInstance that hit a terminal failure during deprovision // reconciliation. -func (c *controller) processDeprovisionFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition) error { +func (c *controller) processDeprovisionFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition, updateObservedGeneration bool) error { if failedCond == nil { return fmt.Errorf("failedCond must not be nil") } @@ -1623,7 +1655,7 @@ func (c *controller) processDeprovisionFailure(instance *v1beta1.ServiceInstance c.recorder.Event(instance, corev1.EventTypeWarning, failedCond.Reason, failedCond.Message) } - clearServiceInstanceCurrentOperation(instance) + clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusFailed if _, err := c.updateServiceInstanceStatus(instance); err != nil { From f15cda5d198dc822a93f400b8fa80937026dc83a Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 21 Feb 2018 12:03:31 +1100 Subject: [PATCH 02/22] Fix tests --- pkg/controller/controller_instance.go | 2 +- pkg/controller/controller_instance_test.go | 90 ++++++++++++++-------- pkg/controller/controller_test.go | 10 +++ test/integration/clientset_test.go | 2 + 4 files changed, 72 insertions(+), 32 deletions(-) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index c279b871473..dfd6eb648e5 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -257,7 +257,7 @@ func (c *controller) reconcileServiceInstance(instance *v1beta1.ServiceInstance) // initObservedGeneration implements ObservedGeneration initialization based on // ReconciledGeneration for status API migration func (c *controller) initObservedGeneration(instance *v1beta1.ServiceInstance) (bool, error) { - if instance.Status.ObservedGeneration == 0 && instance.Status.ReconciledGeneration == 0 { + if instance.Status.ObservedGeneration == 0 && instance.Status.ReconciledGeneration != 0 { instance.Status.ObservedGeneration = instance.Status.ReconciledGeneration // Before we implement https://github.com/kubernetes-incubator/service-catalog/issues/1715 // and switch to non-terminal errors, the "Failed":"True" is a sign that the provisioning failed diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index caa6a01bce0..2b651f44cd2 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -1494,6 +1494,8 @@ func TestReconcileServiceInstanceDelete(t *testing.T) { // as that implies a previous success. instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, @@ -1563,6 +1565,8 @@ func TestReconcileServiceInstanceDeleteBlockedByCredentials(t *testing.T) { // as that implies a previous success. instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, @@ -1668,6 +1672,8 @@ func TestReconcileServiceInstanceDeleteAsynchronous(t *testing.T) { // as that implies a previous success. instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, @@ -1747,6 +1753,8 @@ func TestReconcileServiceInstanceDeleteFailedProvisionWithRequest(t *testing.T) instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = false fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil @@ -1791,44 +1799,44 @@ func TestReconcileServiceInstanceDeleteFailedProvisionWithRequest(t *testing.T) // an instance will be finalized under various conditions defined by test parameters func TestReconsileServiceInstanceDeleteWithParameters(t *testing.T) { cases := []struct { - name string - externalProperties *v1beta1.ServiceInstancePropertiesState - deprovisionStatus v1beta1.ServiceInstanceDeprovisionStatus - serviceBinding *v1beta1.ServiceBinding - generation int64 - reconceiledGeneration int64 + name string + externalProperties *v1beta1.ServiceInstancePropertiesState + deprovisionStatus v1beta1.ServiceInstanceDeprovisionStatus + serviceBinding *v1beta1.ServiceBinding + generation int64 + reconciledGeneration int64 }{ { - name: "With a failed to provision instance and without making a provision request", - externalProperties: &v1beta1.ServiceInstancePropertiesState{}, - deprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired, - serviceBinding: nil, - generation: 1, - reconceiledGeneration: 0, + name: "With a failed to provision instance and without making a provision request", + externalProperties: &v1beta1.ServiceInstancePropertiesState{}, + deprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired, + serviceBinding: nil, + generation: 1, + reconciledGeneration: 0, }, { - name: "With a failed to provision instance, with inactive binding, and without making a provision request", - externalProperties: &v1beta1.ServiceInstancePropertiesState{}, - deprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired, - serviceBinding: getTestServiceInactiveBinding(), - generation: 1, - reconceiledGeneration: 0, + name: "With a failed to provision instance, with inactive binding, and without making a provision request", + externalProperties: &v1beta1.ServiceInstancePropertiesState{}, + deprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired, + serviceBinding: getTestServiceInactiveBinding(), + generation: 1, + reconciledGeneration: 0, }, { - name: "With a deprovisioned instance and without making a deprovision request", - externalProperties: nil, - deprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusSucceeded, - serviceBinding: nil, - generation: 2, - reconceiledGeneration: 1, + name: "With a deprovisioned instance and without making a deprovision request", + externalProperties: nil, + deprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusSucceeded, + serviceBinding: nil, + generation: 2, + reconciledGeneration: 1, }, { - name: "With a deprovisioned instance, with inactive binding, and without making a deprovision request", - externalProperties: nil, - deprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusSucceeded, - serviceBinding: getTestServiceInactiveBinding(), - generation: 2, - reconceiledGeneration: 1, + name: "With a deprovisioned instance, with inactive binding, and without making a deprovision request", + externalProperties: nil, + deprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusSucceeded, + serviceBinding: getTestServiceInactiveBinding(), + generation: 2, + reconciledGeneration: 1, }, } @@ -1850,7 +1858,9 @@ func TestReconsileServiceInstanceDeleteWithParameters(t *testing.T) { instance.Status.DeprovisionStatus = tc.deprovisionStatus instance.Generation = tc.generation - instance.Status.ReconciledGeneration = tc.reconceiledGeneration + instance.Status.ReconciledGeneration = tc.reconciledGeneration + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil @@ -1899,6 +1909,8 @@ func TestReconcileServiceInstanceDeleteWhenAlreadyDeprovisionedUnsuccessfully(t instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil @@ -1947,6 +1959,8 @@ func TestReconcileServiceInstanceDeleteFailedUpdate(t *testing.T) { } instance.Generation = 2 instance.Status.ReconciledGeneration = 2 + instance.Status.ObservedGeneration = 2 + instance.Status.Provisioned = true instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { @@ -2006,6 +2020,8 @@ func TestReconcileServiceInstanceDeleteDoesNotInvokeClusterServiceBroker(t *test instance.ObjectMeta.Finalizers = []string{v1beta1.FinalizerServiceCatalog} instance.Generation = 1 instance.Status.ReconciledGeneration = 0 + instance.Status.ObservedGeneration = 0 + instance.Status.Provisioned = false instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusNotRequired fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { @@ -3366,6 +3382,8 @@ func TestReconcileInstanceDeleteUsingOriginatingIdentity(t *testing.T) { // ReconciledGeneration set as that implies a previous success. instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired if tc.includeUserInfo { instance.Spec.UserInfo = testUserInfo @@ -4028,6 +4046,8 @@ func TestReconcileServiceInstanceUpdateParameters(t *testing.T) { instance := getTestServiceInstanceWithRefs() instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired oldParameters := map[string]interface{}{ @@ -4155,6 +4175,8 @@ func TestReconcileServiceInstanceDeleteParameters(t *testing.T) { instance := getTestServiceInstanceWithRefs() instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired oldParameters := map[string]interface{}{ @@ -4328,6 +4350,8 @@ func TestReconcileServiceInstanceUpdatePlan(t *testing.T) { instance := getTestServiceInstanceWithRefs() instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired oldParameters := map[string]interface{}{ @@ -4766,6 +4790,8 @@ func TestReconcileServiceInstanceUpdateAsynchronous(t *testing.T) { instance := getTestServiceInstanceWithRefs() instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ @@ -5233,6 +5259,8 @@ func TestReconcileServiceInstanceDeleteWithNonExistentPlan(t *testing.T) { // as that implies a previous success. instance.Generation = 2 instance.Status.ReconciledGeneration = 1 + instance.Status.ObservedGeneration = 1 + instance.Status.Provisioned = true instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: "old-plan-name", ClusterServicePlanExternalID: "old-plan-id", diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 38a788b74b2..d38593c9357 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -687,6 +687,8 @@ func getTestServiceInstanceUpdatingPlan() *v1beta1.ServiceInstance { }, // It's been provisioned successfully. ReconciledGeneration: 1, + ObservedGeneration: 1, + Provisioned: true, DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, } @@ -707,6 +709,8 @@ func getTestServiceInstanceUpdatingParameters() *v1beta1.ServiceInstance { }, // It's been provisioned successfully. ReconciledGeneration: 1, + ObservedGeneration: 1, + Provisioned: true, DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, } @@ -727,6 +731,8 @@ func getTestServiceInstanceUpdatingParametersOfDeletedPlan() *v1beta1.ServiceIns }, // It's been provisioned successfully. ReconciledGeneration: 1, + ObservedGeneration: 1, + Provisioned: true, DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, } @@ -770,6 +776,8 @@ func getTestServiceInstanceAsyncUpdating(operation string) *v1beta1.ServiceInsta operationStartTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) instance.Status = v1beta1.ServiceInstanceStatus{ ReconciledGeneration: 1, + ObservedGeneration: 1, + Provisioned: true, Conditions: []v1beta1.ServiceInstanceCondition{{ Type: v1beta1.ServiceInstanceConditionReady, Status: v1beta1.ConditionFalse, @@ -812,6 +820,8 @@ func getTestServiceInstanceAsyncDeprovisioning(operation string) *v1beta1.Servic OperationStartTime: &operationStartTime, CurrentOperation: v1beta1.ServiceInstanceOperationDeprovision, ReconciledGeneration: 1, + ObservedGeneration: 1, + Provisioned: true, ExternalProperties: &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, diff --git a/test/integration/clientset_test.go b/test/integration/clientset_test.go index 27320b9ddb0..c5d7cba6b4a 100644 --- a/test/integration/clientset_test.go +++ b/test/integration/clientset_test.go @@ -923,6 +923,8 @@ func testInstanceClient(sType server.StorageType, client servicecatalogclient.In } instanceServer.Status = v1beta1.ServiceInstanceStatus{ ReconciledGeneration: instanceServer.Generation, + ObservedGeneration: instanceServer.Generation, + Provisioned: true, Conditions: []v1beta1.ServiceInstanceCondition{readyConditionTrue}, DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired, } From c033bb5139b096e695efc3baa26d761617d59c91 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 21 Feb 2018 13:22:04 +1100 Subject: [PATCH 03/22] Add test for API migration path --- pkg/controller/controller_instance_test.go | 46 ++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index 2b651f44cd2..3106e33f787 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -5306,3 +5306,49 @@ func TestReconcileServiceInstanceDeleteWithNonExistentPlan(t *testing.T) { t.Fatal(err) } } + +// TestReconcileServiceInstanceUpdateMissingObservedGeneration tests reconciling a +// ServiceInstance with ObservedGeneration missing (while Reconciled Generation set) +// i.e. API version migration testing +func TestReconcileServiceInstanceUpdateMissingObservedGeneration(t *testing.T) { + _, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Response: &osb.UpdateInstanceResponse{}, + }, + }) + + sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker()) + sharedInformers.ClusterServiceClasses().Informer().GetStore().Add(getTestClusterServiceClass()) + sharedInformers.ClusterServicePlans().Informer().GetStore().Add(getTestClusterServicePlan()) + + instance := getTestServiceInstanceWithRefs() + instance.Generation = 2 + instance.Status.ReconciledGeneration = 1 + // Missing ObservedGeneration and Provisioned after updating Service Catalog + instance.Status.ObservedGeneration = 0 + instance.Status.Provisioned = false + instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired + + instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ + ClusterServicePlanExternalName: testClusterServicePlanName, + ClusterServicePlanExternalID: testClusterServicePlanGUID, + } + + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("This should not fail : %v", err) + } + + brokerActions := fakeClusterServiceBrokerClient.Actions() + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 0) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance).(*v1beta1.ServiceInstance) + if updatedServiceInstance.Status.ObservedGeneration == 0 || updatedServiceInstance.Status.ObservedGeneration != instance.Status.ReconciledGeneration { + t.Fatalf("Unexpected ObservedGeneration value: %d", updatedServiceInstance.Status.ObservedGeneration) + } + if !updatedServiceInstance.Status.Provisioned { + t.Fatalf("The instance was expected to have Provisioned flag set") + } +} From d0efd0fd6742d2ee2619496c576ae39712dc60b8 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 21 Feb 2018 14:05:17 +1100 Subject: [PATCH 04/22] Add checking ObservedGeneration to tests --- pkg/controller/controller_instance.go | 24 ++++---- pkg/controller/controller_instance_test.go | 14 ++--- pkg/controller/controller_test.go | 67 ++++++++++++++++++++-- 3 files changed, 84 insertions(+), 21 deletions(-) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index dfd6eb648e5..9f0a3d5a317 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -196,9 +196,9 @@ func getReconciliationActionForServiceInstance(instance *v1beta1.ServiceInstance return reconcilePoll case instance.ObjectMeta.DeletionTimestamp != nil || instance.Status.OrphanMitigationInProgress: return reconcileDelete - case instance.Status.ReconciledGeneration != 0: + case instance.Status.Provisioned: return reconcileUpdate - default: // instance.Status.ReconciledGeneration == 0 + default: // instance.Status.Provisioned == false return reconcileAdd } } @@ -281,8 +281,8 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan return nil } - if instance.Status.ReconciledGeneration == instance.Generation { - glog.V(4).Info(pcb.Message("Not processing event because reconciled generation showed there is no work to do")) + if isServiceInstanceProcessedAlready(instance) { + glog.V(4).Info(pcb.Message("Not processing event because status showed there is no work to do")) return nil } @@ -385,8 +385,8 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns return nil } - if instance.Status.ReconciledGeneration == instance.Generation { - glog.V(4).Info(pcb.Message("Not processing event because reconciled generation showed there is no work to do")) + if isServiceInstanceProcessedAlready(instance) { + glog.V(4).Info(pcb.Message("Not processing event because status showed there is no work to do")) return nil } @@ -743,6 +743,13 @@ func isServiceInstanceFailed(instance *v1beta1.ServiceInstance) bool { return false } +// isServiceInstanceProcessedAlready returns true if there is no further processing +// needed for the instance based on ObservedGeneration +func isServiceInstanceProcessedAlready(instance *v1beta1.ServiceInstance) bool { + return instance.Status.ObservedGeneration >= instance.Generation && + (isServiceInstanceReady(instance) || isServiceInstanceFailed(instance)) +} + // processServiceInstancePollingFailureRetryTimeout marks the instance as having // failed polling due to its reconciliation retry duration expiring func (c *controller) processServiceInstancePollingFailureRetryTimeout(instance *v1beta1.ServiceInstance, readyCond *v1beta1.ServiceInstanceCondition) error { @@ -1155,10 +1162,7 @@ func (c *controller) checkForRemovedClassAndPlan(instance *v1beta1.ServiceInstan return nil } - isProvisioning := false - if instance.Status.ReconciledGeneration == 0 { - isProvisioning = true - } + isProvisioning := !instance.Status.Provisioned // Regardless of what's been deleted, you can always update // parameters (ie, not change plans) diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index 3106e33f787..f0bdeb17b4c 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -1345,7 +1345,7 @@ func TestReconcileServiceInstanceAsynchronous(t *testing.T) { assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) - assertServiceInstanceAsyncInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) + assertServiceInstanceAsyncStartInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) assertServiceInstanceDashboardURL(t, updatedServiceInstance, testDashboardURL) // verify no kube resources created. @@ -1412,7 +1412,7 @@ func TestReconcileServiceInstanceAsynchronousNoOperation(t *testing.T) { assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) - assertServiceInstanceAsyncInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, "", testClusterServicePlanName, testClusterServicePlanGUID, instance) + assertServiceInstanceAsyncStartInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, "", testClusterServicePlanName, testClusterServicePlanGUID, instance) assertServiceInstanceDashboardURL(t, updatedServiceInstance, testDashboardURL) // verify no kube resources created. @@ -1721,7 +1721,7 @@ func TestReconcileServiceInstanceDeleteAsynchronous(t *testing.T) { assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) - assertServiceInstanceAsyncInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) + assertServiceInstanceAsyncStartInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -2134,7 +2134,7 @@ func TestPollServiceInstanceInProgressProvisioningWithOperation(t *testing.T) { assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceAsyncInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) + assertServiceInstanceAsyncStartInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) assertServiceInstanceConditionHasLastOperationDescription(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, lastOperationDescription) // verify no kube resources created. @@ -2308,7 +2308,7 @@ func TestPollServiceInstanceInProgressDeprovisioningWithOperationNoFinalizer(t * assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceAsyncInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) + assertServiceInstanceAsyncStillInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) assertServiceInstanceConditionHasLastOperationDescription(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, lastOperationDescription) // verify no kube resources created. @@ -4829,7 +4829,7 @@ func TestReconcileServiceInstanceUpdateAsynchronous(t *testing.T) { assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, instance) updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) - assertServiceInstanceAsyncInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) + assertServiceInstanceAsyncStartInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) // verify no kube resources created. // One single action comes from getting namespace uid @@ -4891,7 +4891,7 @@ func TestPollServiceInstanceAsyncInProgressUpdating(t *testing.T) { assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceAsyncInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) + assertServiceInstanceAsyncStillInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) assertServiceInstanceConditionHasLastOperationDescription(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, lastOperationDescription) // verify no kube resources created. diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index d38593c9357..36447b8571d 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -758,7 +758,8 @@ func getTestServiceInstanceAsyncProvisioning(operation string) *v1beta1.ServiceI ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, }, - DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, + ObservedGeneration: instance.Generation, + DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, } if operation != "" { instance.Status.LastOperation = &operation @@ -776,7 +777,7 @@ func getTestServiceInstanceAsyncUpdating(operation string) *v1beta1.ServiceInsta operationStartTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) instance.Status = v1beta1.ServiceInstanceStatus{ ReconciledGeneration: 1, - ObservedGeneration: 1, + ObservedGeneration: 2, Provisioned: true, Conditions: []v1beta1.ServiceInstanceCondition{{ Type: v1beta1.ServiceInstanceConditionReady, @@ -820,7 +821,7 @@ func getTestServiceInstanceAsyncDeprovisioning(operation string) *v1beta1.Servic OperationStartTime: &operationStartTime, CurrentOperation: v1beta1.ServiceInstanceOperationDeprovision, ReconciledGeneration: 1, - ObservedGeneration: 1, + ObservedGeneration: 2, Provisioned: true, ExternalProperties: &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, @@ -1937,6 +1938,28 @@ func assertServiceInstanceReconciledGeneration(t *testing.T, obj runtime.Object, } } +func assertServiceInstanceObservedGeneration(t *testing.T, obj runtime.Object, observedGeneration int64) { + instance, ok := obj.(*v1beta1.ServiceInstance) + if !ok { + fatalf(t, "Couldn't convert object %+v into a *v1beta1.ServiceInstance", obj) + } + + if e, a := observedGeneration, instance.Status.ObservedGeneration; e != a { + fatalf(t, "unexpected observed generation: expected %v, got %v", e, a) + } +} + +func assertServiceInstanceProvisioned(t *testing.T, obj runtime.Object, provisioned bool) { + instance, ok := obj.(*v1beta1.ServiceInstance) + if !ok { + fatalf(t, "Couldn't convert object %+v into a *v1beta1.ServiceInstance", obj) + } + + if e, a := provisioned, instance.Status.Provisioned; e != a { + fatalf(t, "unexpected provisioned flag: expected %v, got %v", e, a) + } +} + func assertServiceInstanceOperationStartTimeSet(t *testing.T, obj runtime.Object, isOperationStartTimeSet bool) { instance, ok := obj.(*v1beta1.ServiceInstance) if !ok { @@ -2049,6 +2072,8 @@ func assertServiceInstanceErrorBeforeRequest(t *testing.T, obj runtime.Object, r assertServiceInstanceCurrentOperationClear(t, obj) assertServiceInstanceOperationStartTimeSet(t, obj, false) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) + // TODO: should be originalInstance.Generation? + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) assertServiceInstanceInProgressPropertiesNil(t, obj) @@ -2074,6 +2099,7 @@ func assertServiceInstanceOperationInProgressWithParameters(t *testing.T, obj ru assertServiceInstanceCurrentOperation(t, obj, operation) assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) switch operation { @@ -2092,6 +2118,7 @@ func assertServiceInstanceStartingOrphanMitigation(t *testing.T, obj runtime.Obj assertServiceInstanceReadyFalse(t, obj, startingInstanceOrphanMitigationReason) assertServiceInstanceOperationStartTimeSet(t, obj, false) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) assertServiceInstanceOrphanMitigationInProgressTrue(t, obj) assertServiceInstanceDeprovisionStatus(t, obj, v1beta1.ServiceInstanceDeprovisionStatusRequired) } @@ -2124,6 +2151,7 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti assertServiceInstanceCurrentOperationClear(t, obj) assertServiceInstanceOperationStartTimeSet(t, obj, false) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) if operation == v1beta1.ServiceInstanceOperationDeprovision { @@ -2170,6 +2198,7 @@ func assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t *testing.T, ob assertServiceInstanceRequestFailingError(t, obj, operation, readyReason, failureReason, originalInstance) assertServiceInstanceCurrentOperationClear(t, obj) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) assertServiceInstanceInProgressPropertiesNil(t, obj) } @@ -2178,6 +2207,7 @@ func assertServiceInstanceRequestFailingErrorStartOrphanMitigation(t *testing.T, assertServiceInstanceRequestFailingError(t, obj, operation, readyReason, failureReason, originalInstance) assertServiceInstanceCurrentOperation(t, obj, v1beta1.ServiceInstanceOperationProvision) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) assertServiceInstanceOrphanMitigationInProgressTrue(t, obj) } @@ -2197,6 +2227,7 @@ func assertServiceInstanceRequestRetriableErrorWithParameters(t *testing.T, obj assertServiceInstanceCurrentOperation(t, obj, operation) assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) switch operation { case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) @@ -2208,7 +2239,34 @@ func assertServiceInstanceRequestRetriableErrorWithParameters(t *testing.T, obj assertServiceInstanceDeprovisionStatus(t, obj, originalInstance.Status.DeprovisionStatus) } -func assertServiceInstanceAsyncInProgress(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, operationKey string, planName, planID string, originalInstance *v1beta1.ServiceInstance) { +func assertServiceInstanceAsyncStartInProgress(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, operationKey string, planName, planID string, originalInstance *v1beta1.ServiceInstance) { + reason := "" + switch operation { + case v1beta1.ServiceInstanceOperationProvision: + reason = asyncProvisioningReason + case v1beta1.ServiceInstanceOperationUpdate: + reason = asyncUpdatingInstanceReason + case v1beta1.ServiceInstanceOperationDeprovision: + reason = asyncDeprovisioningReason + } + assertServiceInstanceReadyFalse(t, obj, reason) + assertServiceInstanceLastOperation(t, obj, operationKey) + assertServiceInstanceCurrentOperation(t, obj, operation) + assertServiceInstanceOperationStartTimeSet(t, obj, true) + assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + switch operation { + case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: + assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) + assertServiceInstanceInProgressPropertiesParameters(t, obj, nil, "") + case v1beta1.ServiceInstanceOperationDeprovision: + assertServiceInstanceInProgressPropertiesNil(t, obj) + } + assertAsyncOpInProgressTrue(t, obj) + assertServiceInstanceDeprovisionStatus(t, obj, originalInstance.Status.DeprovisionStatus) +} + +func assertServiceInstanceAsyncStillInProgress(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, operationKey string, planName, planID string, originalInstance *v1beta1.ServiceInstance) { reason := "" switch operation { case v1beta1.ServiceInstanceOperationProvision: @@ -2223,6 +2281,7 @@ func assertServiceInstanceAsyncInProgress(t *testing.T, obj runtime.Object, oper assertServiceInstanceCurrentOperation(t, obj, operation) assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Status.ObservedGeneration) switch operation { case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) From 18169124f367fbee147a8ef16422d936b77133fc Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 21 Feb 2018 14:52:30 +1100 Subject: [PATCH 05/22] Add Provisioned flag checks to unit tests --- pkg/controller/controller_test.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 36447b8571d..7f5b23b9c38 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -2072,8 +2072,8 @@ func assertServiceInstanceErrorBeforeRequest(t *testing.T, obj runtime.Object, r assertServiceInstanceCurrentOperationClear(t, obj) assertServiceInstanceOperationStartTimeSet(t, obj, false) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) - // TODO: should be originalInstance.Generation? - assertServiceInstanceObservedGeneration(t, obj, originalInstance.Status.ReconciledGeneration) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Status.ObservedGeneration) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) assertServiceInstanceInProgressPropertiesNil(t, obj) @@ -2100,6 +2100,7 @@ func assertServiceInstanceOperationInProgressWithParameters(t *testing.T, obj ru assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) switch operation { @@ -2119,6 +2120,7 @@ func assertServiceInstanceStartingOrphanMitigation(t *testing.T, obj runtime.Obj assertServiceInstanceOperationStartTimeSet(t, obj, false) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) assertServiceInstanceOrphanMitigationInProgressTrue(t, obj) assertServiceInstanceDeprovisionStatus(t, obj, v1beta1.ServiceInstanceDeprovisionStatusRequired) } @@ -2133,16 +2135,20 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti readyStatus v1beta1.ConditionStatus deprovisionStatus v1beta1.ServiceInstanceDeprovisionStatus ) + var provisioned bool switch operation { case v1beta1.ServiceInstanceOperationProvision: + provisioned = true reason = successProvisionReason readyStatus = v1beta1.ConditionTrue deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired case v1beta1.ServiceInstanceOperationUpdate: + provisioned = true reason = successUpdateInstanceReason readyStatus = v1beta1.ConditionTrue deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired case v1beta1.ServiceInstanceOperationDeprovision: + provisioned = false reason = successDeprovisionReason readyStatus = v1beta1.ConditionFalse deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusSucceeded @@ -2152,6 +2158,7 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti assertServiceInstanceOperationStartTimeSet(t, obj, false) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Generation) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceProvisioned(t, obj, provisioned) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) if operation == v1beta1.ServiceInstanceOperationDeprovision { @@ -2199,6 +2206,7 @@ func assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t *testing.T, ob assertServiceInstanceCurrentOperationClear(t, obj) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Generation) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) assertServiceInstanceInProgressPropertiesNil(t, obj) } @@ -2208,6 +2216,7 @@ func assertServiceInstanceRequestFailingErrorStartOrphanMitigation(t *testing.T, assertServiceInstanceCurrentOperation(t, obj, v1beta1.ServiceInstanceOperationProvision) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) assertServiceInstanceOrphanMitigationInProgressTrue(t, obj) } @@ -2228,6 +2237,7 @@ func assertServiceInstanceRequestRetriableErrorWithParameters(t *testing.T, obj assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) switch operation { case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) @@ -2255,6 +2265,7 @@ func assertServiceInstanceAsyncStartInProgress(t *testing.T, obj runtime.Object, assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) switch operation { case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) @@ -2282,6 +2293,7 @@ func assertServiceInstanceAsyncStillInProgress(t *testing.T, obj runtime.Object, assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Status.ObservedGeneration) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) switch operation { case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) From 50a81bb963b784ab815286b0f2203f5ed6a1df1d Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 21 Feb 2018 14:52:58 +1100 Subject: [PATCH 06/22] Update generated files --- .../v1beta1/zz_generated.conversion.go | 4 ++++ pkg/openapi/openapi_generated.go | 18 ++++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go b/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go index 486046797c3..58f8df0d0e0 100644 --- a/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go +++ b/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go @@ -1005,6 +1005,8 @@ func autoConvert_v1beta1_ServiceInstanceStatus_To_servicecatalog_ServiceInstance out.DashboardURL = (*string)(unsafe.Pointer(in.DashboardURL)) out.CurrentOperation = servicecatalog.ServiceInstanceOperation(in.CurrentOperation) out.ReconciledGeneration = in.ReconciledGeneration + out.ObservedGeneration = in.ObservedGeneration + out.Provisioned = in.Provisioned out.OperationStartTime = (*v1.Time)(unsafe.Pointer(in.OperationStartTime)) out.InProgressProperties = (*servicecatalog.ServiceInstancePropertiesState)(unsafe.Pointer(in.InProgressProperties)) out.ExternalProperties = (*servicecatalog.ServiceInstancePropertiesState)(unsafe.Pointer(in.ExternalProperties)) @@ -1025,6 +1027,8 @@ func autoConvert_servicecatalog_ServiceInstanceStatus_To_v1beta1_ServiceInstance out.DashboardURL = (*string)(unsafe.Pointer(in.DashboardURL)) out.CurrentOperation = ServiceInstanceOperation(in.CurrentOperation) out.ReconciledGeneration = in.ReconciledGeneration + out.ObservedGeneration = in.ObservedGeneration + out.Provisioned = in.Provisioned out.OperationStartTime = (*v1.Time)(unsafe.Pointer(in.OperationStartTime)) out.InProgressProperties = (*ServiceInstancePropertiesState)(unsafe.Pointer(in.InProgressProperties)) out.ExternalProperties = (*ServiceInstancePropertiesState)(unsafe.Pointer(in.ExternalProperties)) diff --git a/pkg/openapi/openapi_generated.go b/pkg/openapi/openapi_generated.go index e9454462e83..1591d95c1ae 100644 --- a/pkg/openapi/openapi_generated.go +++ b/pkg/openapi/openapi_generated.go @@ -1495,11 +1495,25 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, "reconciledGeneration": { SchemaProps: spec.SchemaProps{ - Description: "ReconciledGeneration is the 'Generation' of the serviceInstanceSpec that was last processed by the controller. The reconciled generation is updated even if the controller failed to process the spec.", + Description: "ReconciledGeneration is the 'Generation' of the serviceInstanceSpec that was last processed by the controller. The reconciled generation is updated even if the controller failed to process the spec. Deprecated: use ObservedGeneration instead", Type: []string{"integer"}, Format: "int64", }, }, + "observedGeneration": { + SchemaProps: spec.SchemaProps{ + Description: "ObservedGeneration is the 'Generation' of the serviceInstanceSpec that was last processed by the controller. The observed generation is updated whenever the status is updated regardless of operation result", + Type: []string{"integer"}, + Format: "int64", + }, + }, + "provisioned": { + SchemaProps: spec.SchemaProps{ + Description: "Provisioned is the flag that marks whether the instance has been successfully provisioned (i.e. it exists on the broker's side and can be updated or deprovisioned). The flag should be reset after successful deprovisioning.", + Type: []string{"boolean"}, + Format: "", + }, + }, "operationStartTime": { SchemaProps: spec.SchemaProps{ Description: "OperationStartTime is the time at which the current operation began.", @@ -1526,7 +1540,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, }, }, - Required: []string{"conditions", "asyncOpInProgress", "orphanMitigationInProgress", "reconciledGeneration", "deprovisionStatus"}, + Required: []string{"conditions", "asyncOpInProgress", "orphanMitigationInProgress", "reconciledGeneration", "observedGeneration", "provisioned", "deprovisionStatus"}, }, }, Dependencies: []string{ From fd2c9e7a0d3a1aa5c1ccecd976c683fec45e8c68 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 21 Feb 2018 14:57:57 +1100 Subject: [PATCH 07/22] Determine reconciled generation based on ObservedGeneration and Ready/Failed conditions --- test/util/util.go | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/test/util/util.go b/test/util/util.go index 3f838395b11..4db3d4e13d8 100644 --- a/test/util/util.go +++ b/test/util/util.go @@ -216,7 +216,8 @@ func WaitForInstanceReconciledGeneration(client v1beta1servicecatalog.Servicecat return false, fmt.Errorf("error getting Instance %v/%v: %v", namespace, name, err) } - if instance.Status.ReconciledGeneration == reconciledGeneration { + if instance.Status.ObservedGeneration >= reconciledGeneration && + (isServiceInstanceReady(instance) || isServiceInstanceFailed(instance)) { return true, nil } @@ -225,6 +226,30 @@ func WaitForInstanceReconciledGeneration(client v1beta1servicecatalog.Servicecat ) } +// isServiceInstanceReady returns whether the given instance has a ready condition +// with status true. +func isServiceInstanceReady(instance *v1beta1.ServiceInstance) bool { + for _, cond := range instance.Status.Conditions { + if cond.Type == v1beta1.ServiceInstanceConditionReady { + return cond.Status == v1beta1.ConditionTrue + } + } + + return false +} + +// isServiceInstanceFailed returns whether the instance has a failed condition with +// status true. +func isServiceInstanceFailed(instance *v1beta1.ServiceInstance) bool { + for _, condition := range instance.Status.Conditions { + if condition.Type == v1beta1.ServiceInstanceConditionFailed && condition.Status == v1beta1.ConditionTrue { + return true + } + } + + return false +} + // WaitForBindingCondition waits for the status of the named binding to contain // a condition whose type and status matches the supplied one and then returns // back the last binding condition of the same type requested during polling if found. From 4091566ba5f34d3a6654f62735e422446209c599 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 21 Feb 2018 15:03:51 +1100 Subject: [PATCH 08/22] Fix typo in comment --- pkg/controller/controller_instance.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 9f0a3d5a317..3590246ed99 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -234,7 +234,7 @@ func (c *controller) reconcileServiceInstance(instance *v1beta1.ServiceInstance) } if handled { // Update the status and finish the iteration - // The updates instance will be automatically added to the instance queue + // The updated instance will be automatically added back to the queue // and be processed again return nil } From 4fcefee277599f46cf96526c8f557ccf366b878f Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Thu, 22 Feb 2018 11:26:01 +1100 Subject: [PATCH 09/22] Fix test for reconciled generation --- test/integration/controller_instance_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/controller_instance_test.go b/test/integration/controller_instance_test.go index cf3deb2f742..2fd33f653c2 100644 --- a/test/integration/controller_instance_test.go +++ b/test/integration/controller_instance_test.go @@ -566,7 +566,7 @@ func TestUpdateServiceInstanceChangePlans(t *testing.T) { t.Fatalf("error updating Instance: %v", err) } - if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Status.ReconciledGeneration+1); err != nil { + if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Generation); err != nil { t.Fatalf("error waiting for instance to reconcile: %v", err) } @@ -837,7 +837,7 @@ func TestUpdateServiceInstanceUpdateParameters(t *testing.T) { t.Fatalf("error updating Instance: %v", err) } - if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Status.ReconciledGeneration+1); err != nil { + if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Generation); err != nil { t.Fatalf("error waiting for instance to reconcile: %v", err) } From 200d0e442b2dc7a0276fdf3f02397c2d1a4b6a81 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Thu, 22 Feb 2018 12:50:02 +1100 Subject: [PATCH 10/22] Update cached instance after update succeeds --- test/integration/controller_instance_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/integration/controller_instance_test.go b/test/integration/controller_instance_test.go index 2fd33f653c2..367d6857368 100644 --- a/test/integration/controller_instance_test.go +++ b/test/integration/controller_instance_test.go @@ -562,9 +562,11 @@ func TestUpdateServiceInstanceChangePlans(t *testing.T) { ct.instance.Spec.ClusterServicePlanName = otherPlanID } - if _, err := ct.client.ServiceInstances(testNamespace).Update(ct.instance); err != nil { + updatedInstance, err := ct.client.ServiceInstances(testNamespace).Update(ct.instance) + if err != nil { t.Fatalf("error updating Instance: %v", err) } + ct.instance = updatedInstance if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Generation); err != nil { t.Fatalf("error waiting for instance to reconcile: %v", err) @@ -833,9 +835,11 @@ func TestUpdateServiceInstanceUpdateParameters(t *testing.T) { ct.instance.Spec.UpdateRequests++ } - if _, err := ct.client.ServiceInstances(testNamespace).Update(ct.instance); err != nil { + updatedInstance, err := ct.client.ServiceInstances(testNamespace).Update(ct.instance) + if err != nil { t.Fatalf("error updating Instance: %v", err) } + ct.instance = updatedInstance if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Generation); err != nil { t.Fatalf("error waiting for instance to reconcile: %v", err) From 8990e750b44bd6a029b056822e204a46cd9daa8c Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Thu, 22 Feb 2018 13:54:08 +1100 Subject: [PATCH 11/22] Add debugging information for the test --- test/integration/controller_instance_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/integration/controller_instance_test.go b/test/integration/controller_instance_test.go index 367d6857368..dec2feff68e 100644 --- a/test/integration/controller_instance_test.go +++ b/test/integration/controller_instance_test.go @@ -34,6 +34,7 @@ import ( // avoid error `servicecatalog/v1beta1 is not enabled` _ "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/install" + "encoding/json" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" "github.com/kubernetes-incubator/service-catalog/test/util" ) @@ -569,6 +570,15 @@ func TestUpdateServiceInstanceChangePlans(t *testing.T) { ct.instance = updatedInstance if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Generation); err != nil { + latestInstance, err := ct.client.ServiceInstances(testNamespace).Get(testInstanceName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("unable to retrieve instance: %v", err) + } + instanceJson, err := json.Marshal(latestInstance) + if err != nil { + t.Fatalf("unable to marshal instance: %v", err) + } + t.Logf("instance: %s", string(instanceJson)) t.Fatalf("error waiting for instance to reconcile: %v", err) } From cc99ccee2e80b0a2625f50eb00ce574188845315 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Thu, 22 Feb 2018 15:27:29 +1100 Subject: [PATCH 12/22] Bugfix: set Failed condition in case of terminal failure during update --- pkg/controller/controller_instance.go | 23 ++++++++++++++-------- pkg/controller/controller_instance_test.go | 2 +- pkg/controller/controller_test.go | 2 +- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 3590246ed99..41fdef1dafa 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -437,7 +437,8 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns if httpErr, ok := osb.IsHTTPError(err); ok { msg := fmt.Sprintf("ClusterServiceBroker returned a failure for update call; update will not be retried: %v", httpErr) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorUpdateInstanceCallFailedReason, msg) - return c.processUpdateServiceInstanceFailure(instance, readyCond, true) + failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, errorUpdateInstanceCallFailedReason, msg) + return c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond, true) } reason := errorErrorCallingUpdateInstanceReason @@ -445,7 +446,8 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns if urlErr, ok := err.(*url.Error); ok && urlErr.Timeout() { msg := fmt.Sprintf("Communication with the ClusterServiceBroker timed out; update will not be retried: %v", urlErr) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, msg) - return c.processUpdateServiceInstanceFailure(instance, readyCond, true) + failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, reason, msg) + return c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond, true) } msg := fmt.Sprintf("The update call failed and will be retried: Error communicating with broker for updating: %s", err) @@ -458,7 +460,8 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns msg = "Stopping reconciliation retries because too much time has elapsed" readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorReconciliationRetryTimeoutReason, msg) - return c.processUpdateServiceInstanceFailure(instance, readyCond, true) + failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, errorReconciliationRetryTimeoutReason, msg) + return c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond, true) } readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, msg) @@ -711,9 +714,11 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, reason, message) err = c.processProvisionFailure(instance, readyCond, failedCond, false, false) default: - msg := "Update call failed: " + description - readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorUpdateInstanceCallFailedReason, msg) - err = c.processUpdateServiceInstanceFailure(instance, readyCond, false) + reason := errorUpdateInstanceCallFailedReason + message := "Update call failed: " + description + readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, message) + failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, reason, message) + err = c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond, false) } if err != nil { return c.handleServiceInstancePollingError(instance, err) @@ -770,7 +775,7 @@ func (c *controller) processServiceInstancePollingFailureRetryTimeout(instance * return c.processProvisionFailure(instance, readyCond, failedCond, true, false) default: readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorReconciliationRetryTimeoutReason, msg) - err = c.processUpdateServiceInstanceFailure(instance, readyCond, false) + err = c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond, false) } if err != nil { return c.handleServiceInstancePollingError(instance, err) @@ -1569,11 +1574,13 @@ func (c *controller) processUpdateServiceInstanceSuccess(instance *v1beta1.Servi // processUpdateServiceInstanceFailure handles the logging and updating of a // ServiceInstance that hit a terminal failure during update reconciliation. -func (c *controller) processUpdateServiceInstanceFailure(instance *v1beta1.ServiceInstance, readyCond *v1beta1.ServiceInstanceCondition, updateObservedGeneration bool) error { +func (c *controller) processUpdateServiceInstanceFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition, updateObservedGeneration bool) error { c.recorder.Event(instance, corev1.EventTypeWarning, readyCond.Reason, readyCond.Message) currentReconciledGeneration := instance.Status.ReconciledGeneration setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, readyCond.Status, readyCond.Reason, readyCond.Message) + setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionFailed, failedCond.Status, failedCond.Reason, failedCond.Message) + clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index f0bdeb17b4c..68d26e61ab8 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -4559,7 +4559,7 @@ func TestReconcileServiceInstanceWithUpdateFailure(t *testing.T) { assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, instance) updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) - assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, "ClusterServiceBrokerReturnedFailure", instance) + assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, errorUpdateInstanceCallFailedReason, instance) events := getRecordedEvents(testController) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 7f5b23b9c38..75d02b5e0b0 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -2193,7 +2193,7 @@ func assertServiceInstanceRequestFailingError(t *testing.T, obj runtime.Object, case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationDeprovision: assertServiceInstanceCondition(t, obj, v1beta1.ServiceInstanceConditionFailed, v1beta1.ConditionTrue, failureReason) case v1beta1.ServiceInstanceOperationUpdate: - assertServiceInstanceConditionsCount(t, obj, 1) + assertServiceInstanceCondition(t, obj, v1beta1.ServiceInstanceConditionFailed, v1beta1.ConditionTrue, failureReason) } assertServiceInstanceOperationStartTimeSet(t, obj, false) assertAsyncOpInProgressFalse(t, obj) From 77c79bad2753a05a69cdcca87f0c3c31596452f4 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Thu, 22 Feb 2018 16:35:23 +1100 Subject: [PATCH 13/22] Remove debugging information --- test/integration/controller_instance_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/test/integration/controller_instance_test.go b/test/integration/controller_instance_test.go index dec2feff68e..367d6857368 100644 --- a/test/integration/controller_instance_test.go +++ b/test/integration/controller_instance_test.go @@ -34,7 +34,6 @@ import ( // avoid error `servicecatalog/v1beta1 is not enabled` _ "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/install" - "encoding/json" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" "github.com/kubernetes-incubator/service-catalog/test/util" ) @@ -570,15 +569,6 @@ func TestUpdateServiceInstanceChangePlans(t *testing.T) { ct.instance = updatedInstance if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Generation); err != nil { - latestInstance, err := ct.client.ServiceInstances(testNamespace).Get(testInstanceName, metav1.GetOptions{}) - if err != nil { - t.Fatalf("unable to retrieve instance: %v", err) - } - instanceJson, err := json.Marshal(latestInstance) - if err != nil { - t.Fatalf("unable to marshal instance: %v", err) - } - t.Logf("instance: %s", string(instanceJson)) t.Fatalf("error waiting for instance to reconcile: %v", err) } From c46c96faa1f9c99b20835ba66a2277345d668932 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Tue, 27 Feb 2018 15:30:50 +1100 Subject: [PATCH 14/22] Update ObservedGeneration at the start of operation only --- pkg/apis/servicecatalog/types.go | 22 ++-- pkg/apis/servicecatalog/v1beta1/types.go | 22 ++-- .../v1beta1/zz_generated.conversion.go | 4 +- pkg/controller/controller_instance.go | 100 +++++++++--------- pkg/controller/controller_instance_test.go | 41 ++++--- pkg/controller/controller_test.go | 42 ++++---- pkg/openapi/openapi_generated.go | 14 +-- test/integration/clientset_test.go | 2 +- 8 files changed, 135 insertions(+), 112 deletions(-) diff --git a/pkg/apis/servicecatalog/types.go b/pkg/apis/servicecatalog/types.go index be18bb37f26..ada7e3a6212 100644 --- a/pkg/apis/servicecatalog/types.go +++ b/pkg/apis/servicecatalog/types.go @@ -545,12 +545,6 @@ type ServiceInstanceStatus struct { // whenever the status is updated regardless of operation result ObservedGeneration int64 - // Provisioned is the flag that marks whether the instance has been - // successfully provisioned (i.e. it exists on the broker's side and can be - // updated or deprovisioned). The flag should be reset after successful - // deprovisioning. - Provisioned bool - // OperationStartTime is the time at which the current operation began. OperationStartTime *metav1.Time @@ -563,6 +557,9 @@ type ServiceInstanceStatus struct { // broker knows about. ExternalProperties *ServiceInstancePropertiesState + // ProvisionStatus describes whether the instance is in the provisioned state. + ProvisionStatus ServiceInstanceProvisionStatus + // DeprovisionStatus describes what has been done to deprovision the // ServiceInstance. DeprovisionStatus ServiceInstanceDeprovisionStatus @@ -665,6 +662,19 @@ const ( ServiceInstanceDeprovisionStatusFailed ServiceInstanceDeprovisionStatus = "Failed" ) +// ServiceInstanceProvisionStatus is the status of provisioning a +// ServiceInstance +type ServiceInstanceProvisionStatus string + +const ( + // ServiceInstanceProvisionStatusProvisioned indicates that the instance + // was provisioned. + ServiceInstanceProvisionStatusProvisioned ServiceInstanceProvisionStatus = "Provisioned" + // ServiceInstanceProvisionStatusNotProvisioned indicates that the instance + // was not ever provisioned or was deprovisioned. + ServiceInstanceProvisionStatusNotProvisioned ServiceInstanceProvisionStatus = "NotProvisioned" +) + // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // ServiceBindingList is a list of ServiceBindings. diff --git a/pkg/apis/servicecatalog/v1beta1/types.go b/pkg/apis/servicecatalog/v1beta1/types.go index 34d869e1f40..c39927b05a1 100644 --- a/pkg/apis/servicecatalog/v1beta1/types.go +++ b/pkg/apis/servicecatalog/v1beta1/types.go @@ -609,12 +609,6 @@ type ServiceInstanceStatus struct { // whenever the status is updated regardless of operation result ObservedGeneration int64 `json:"observedGeneration"` - // Provisioned is the flag that marks whether the instance has been - // successfully provisioned (i.e. it exists on the broker's side and can be - // updated or deprovisioned). The flag should be reset after successful - // deprovisioning. - Provisioned bool `json:"provisioned"` - // OperationStartTime is the time at which the current operation began. OperationStartTime *metav1.Time `json:"operationStartTime,omitempty"` @@ -627,6 +621,9 @@ type ServiceInstanceStatus struct { // broker knows about. ExternalProperties *ServiceInstancePropertiesState `json:"externalProperties,omitempty"` + // ProvisionStatus describes whether the instance is in the provisioned state. + ProvisionStatus ServiceInstanceProvisionStatus `json:"provisioned"` + // DeprovisionStatus describes what has been done to deprovision the // ServiceInstance. DeprovisionStatus ServiceInstanceDeprovisionStatus `json:"deprovisionStatus"` @@ -729,6 +726,19 @@ const ( ServiceInstanceDeprovisionStatusFailed ServiceInstanceDeprovisionStatus = "Failed" ) +// ServiceInstanceProvisionStatus is the status of provisioning a +// ServiceInstance +type ServiceInstanceProvisionStatus string + +const ( + // ServiceInstanceProvisionStatusProvisioned indicates that the instance + // was provisioned. + ServiceInstanceProvisionStatusProvisioned ServiceInstanceProvisionStatus = "Provisioned" + // ServiceInstanceProvisionStatusNotProvisioned indicates that the instance + // was not ever provisioned or was deprovisioned. + ServiceInstanceProvisionStatusNotProvisioned ServiceInstanceProvisionStatus = "NotProvisioned" +) + // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // ServiceBindingList is a list of ServiceBindings. diff --git a/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go b/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go index 58f8df0d0e0..da0cc079f69 100644 --- a/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go +++ b/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go @@ -1006,10 +1006,10 @@ func autoConvert_v1beta1_ServiceInstanceStatus_To_servicecatalog_ServiceInstance out.CurrentOperation = servicecatalog.ServiceInstanceOperation(in.CurrentOperation) out.ReconciledGeneration = in.ReconciledGeneration out.ObservedGeneration = in.ObservedGeneration - out.Provisioned = in.Provisioned out.OperationStartTime = (*v1.Time)(unsafe.Pointer(in.OperationStartTime)) out.InProgressProperties = (*servicecatalog.ServiceInstancePropertiesState)(unsafe.Pointer(in.InProgressProperties)) out.ExternalProperties = (*servicecatalog.ServiceInstancePropertiesState)(unsafe.Pointer(in.ExternalProperties)) + out.ProvisionStatus = servicecatalog.ServiceInstanceProvisionStatus(in.ProvisionStatus) out.DeprovisionStatus = servicecatalog.ServiceInstanceDeprovisionStatus(in.DeprovisionStatus) return nil } @@ -1028,10 +1028,10 @@ func autoConvert_servicecatalog_ServiceInstanceStatus_To_v1beta1_ServiceInstance out.CurrentOperation = ServiceInstanceOperation(in.CurrentOperation) out.ReconciledGeneration = in.ReconciledGeneration out.ObservedGeneration = in.ObservedGeneration - out.Provisioned = in.Provisioned out.OperationStartTime = (*v1.Time)(unsafe.Pointer(in.OperationStartTime)) out.InProgressProperties = (*ServiceInstancePropertiesState)(unsafe.Pointer(in.InProgressProperties)) out.ExternalProperties = (*ServiceInstancePropertiesState)(unsafe.Pointer(in.ExternalProperties)) + out.ProvisionStatus = ServiceInstanceProvisionStatus(in.ProvisionStatus) out.DeprovisionStatus = ServiceInstanceDeprovisionStatus(in.DeprovisionStatus) return nil } diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 41fdef1dafa..912b032bef1 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -196,9 +196,9 @@ func getReconciliationActionForServiceInstance(instance *v1beta1.ServiceInstance return reconcilePoll case instance.ObjectMeta.DeletionTimestamp != nil || instance.Status.OrphanMitigationInProgress: return reconcileDelete - case instance.Status.Provisioned: + case instance.Status.ProvisionStatus == v1beta1.ServiceInstanceProvisionStatusProvisioned: return reconcileUpdate - default: // instance.Status.Provisioned == false + default: // instance.Status.ProvisionStatus == "NotProvisioned" return reconcileAdd } } @@ -261,7 +261,13 @@ func (c *controller) initObservedGeneration(instance *v1beta1.ServiceInstance) ( instance.Status.ObservedGeneration = instance.Status.ReconciledGeneration // Before we implement https://github.com/kubernetes-incubator/service-catalog/issues/1715 // and switch to non-terminal errors, the "Failed":"True" is a sign that the provisioning failed - instance.Status.Provisioned = !isServiceInstanceFailed(instance) + provisioned := !isServiceInstanceFailed(instance) + if provisioned { + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned + } else { + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned + } + _, err := c.updateServiceInstanceStatus(instance) if err != nil { return false, err @@ -314,7 +320,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan return c.handleServiceInstanceReconciliationError(instance, err) } - if instance.Status.CurrentOperation == "" { + if instance.Status.ObservedGeneration != instance.Generation { instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationProvision, inProgressProperties) if err != nil { // There has been an update to the instance. Start reconciliation @@ -340,7 +346,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan ) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorProvisionCallFailedReason, msg) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, "ClusterServiceBrokerReturnedFailure", msg) - return c.processProvisionFailure(instance, readyCond, failedCond, shouldStartOrphanMitigation(httpErr.StatusCode), true) + return c.processProvisionFailure(instance, readyCond, failedCond, shouldStartOrphanMitigation(httpErr.StatusCode)) } reason := errorErrorCallingProvisionReason @@ -351,7 +357,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan msg := fmt.Sprintf("Communication with the ClusterServiceBroker timed out; operation will not be retried: %v", urlErr) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, msg) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, reason, msg) - return c.processProvisionFailure(instance, readyCond, failedCond, true, true) + return c.processProvisionFailure(instance, readyCond, failedCond, true) } // All other errors should be retried, unless the @@ -362,7 +368,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan if c.reconciliationRetryDurationExceeded(instance.Status.OperationStartTime) { msg := "Stopping reconciliation retries because too much time has elapsed" failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, errorReconciliationRetryTimeoutReason, msg) - return c.processProvisionFailure(instance, readyCond, failedCond, false, true) + return c.processProvisionFailure(instance, readyCond, failedCond, false) } return c.processServiceInstanceOperationError(instance, readyCond) @@ -372,7 +378,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan return c.processProvisionAsyncResponse(instance, response) } - return c.processProvisionSuccess(instance, response.DashboardURL, true) + return c.processProvisionSuccess(instance, response.DashboardURL) } // reconcileServiceInstanceUpdate is responsible for handling updating the plan @@ -418,7 +424,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns return c.handleServiceInstanceReconciliationError(instance, err) } - if instance.Status.CurrentOperation == "" { + if instance.Status.ObservedGeneration != instance.Generation { instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationUpdate, inProgressProperties) if err != nil { // There has been an update to the instance. Start reconciliation @@ -438,7 +444,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns msg := fmt.Sprintf("ClusterServiceBroker returned a failure for update call; update will not be retried: %v", httpErr) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorUpdateInstanceCallFailedReason, msg) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, errorUpdateInstanceCallFailedReason, msg) - return c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond, true) + return c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond) } reason := errorErrorCallingUpdateInstanceReason @@ -447,7 +453,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns msg := fmt.Sprintf("Communication with the ClusterServiceBroker timed out; update will not be retried: %v", urlErr) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, msg) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, reason, msg) - return c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond, true) + return c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond) } msg := fmt.Sprintf("The update call failed and will be retried: Error communicating with broker for updating: %s", err) @@ -461,7 +467,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns msg = "Stopping reconciliation retries because too much time has elapsed" readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorReconciliationRetryTimeoutReason, msg) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, errorReconciliationRetryTimeoutReason, msg) - return c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond, true) + return c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond) } readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, msg) @@ -472,7 +478,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns return c.processUpdateServiceInstanceAsyncResponse(instance, response) } - return c.processUpdateServiceInstanceSuccess(instance, true) + return c.processUpdateServiceInstanceSuccess(instance) } // reconcileServiceInstanceDelete is responsible for handling any instance whose @@ -514,7 +520,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns msg := fmt.Sprintf("ServiceInstance has invalid DeprovisionStatus field: %v", instance.Status.DeprovisionStatus) readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionUnknown, errorInvalidDeprovisionStatusReason, msg) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, errorInvalidDeprovisionStatusReason, msg) - return c.processDeprovisionFailure(instance, readyCond, failedCond, true) + return c.processDeprovisionFailure(instance, readyCond, failedCond) } // We don't want to delete the instance if there are any bindings associated. @@ -539,7 +545,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns instance.Status.OperationStartTime = &now } } else { - if instance.Status.CurrentOperation != v1beta1.ServiceInstanceOperationDeprovision { + if instance.Status.ObservedGeneration != instance.Generation { instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationDeprovision, nil) if err != nil { // There has been an update to the instance. Start reconciliation @@ -565,7 +571,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns if c.reconciliationRetryDurationExceeded(instance.Status.OperationStartTime) { msg := "Stopping reconciliation retries because too much time has elapsed" failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, errorReconciliationRetryTimeoutReason, msg) - return c.processDeprovisionFailure(instance, readyCond, failedCond, true) + return c.processDeprovisionFailure(instance, readyCond, failedCond) } return c.processServiceInstanceOperationError(instance, readyCond) @@ -575,7 +581,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns return c.processDeprovisionAsyncResponse(instance, response) } - return c.processDeprovisionSuccess(instance, true) + return c.processDeprovisionSuccess(instance) } func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) error { @@ -608,7 +614,7 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro // If the operation was for delete and we receive a http.StatusGone, // this is considered a success as per the spec if osb.IsGoneError(err) && deleting { - if err := c.processDeprovisionSuccess(instance, false); err != nil { + if err := c.processDeprovisionSuccess(instance); err != nil { return c.handleServiceInstancePollingError(instance, err) } return c.finishPollingServiceInstance(instance) @@ -672,11 +678,11 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro var err error switch { case deleting: - err = c.processDeprovisionSuccess(instance, false) + err = c.processDeprovisionSuccess(instance) case provisioning: - err = c.processProvisionSuccess(instance, nil, false) + err = c.processProvisionSuccess(instance, nil) default: - err = c.processUpdateServiceInstanceSuccess(instance, false) + err = c.processUpdateServiceInstanceSuccess(instance) } if err != nil { return c.handleServiceInstancePollingError(instance, err) @@ -712,13 +718,13 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro message := "Provision call failed: " + description readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, message) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, reason, message) - err = c.processProvisionFailure(instance, readyCond, failedCond, false, false) + err = c.processProvisionFailure(instance, readyCond, failedCond, false) default: reason := errorUpdateInstanceCallFailedReason message := "Update call failed: " + description readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, reason, message) failedCond := newServiceInstanceFailedCondition(v1beta1.ConditionTrue, reason, message) - err = c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond, false) + err = c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond) } if err != nil { return c.handleServiceInstancePollingError(instance, err) @@ -752,7 +758,7 @@ func isServiceInstanceFailed(instance *v1beta1.ServiceInstance) bool { // needed for the instance based on ObservedGeneration func isServiceInstanceProcessedAlready(instance *v1beta1.ServiceInstance) bool { return instance.Status.ObservedGeneration >= instance.Generation && - (isServiceInstanceReady(instance) || isServiceInstanceFailed(instance)) + (isServiceInstanceReady(instance) || isServiceInstanceFailed(instance)) && !instance.Status.OrphanMitigationInProgress } // processServiceInstancePollingFailureRetryTimeout marks the instance as having @@ -768,14 +774,14 @@ func (c *controller) processServiceInstancePollingFailureRetryTimeout(instance * var err error switch { case deleting: - err = c.processDeprovisionFailure(instance, readyCond, failedCond, false) + err = c.processDeprovisionFailure(instance, readyCond, failedCond) case provisioning: // always finish polling instance, as triggering OM will return an error c.finishPollingServiceInstance(instance) - return c.processProvisionFailure(instance, readyCond, failedCond, true, false) + return c.processProvisionFailure(instance, readyCond, failedCond, true) default: readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorReconciliationRetryTimeoutReason, msg) - err = c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond, false) + err = c.processUpdateServiceInstanceFailure(instance, readyCond, failedCond) } if err != nil { return c.handleServiceInstancePollingError(instance, err) @@ -1123,8 +1129,8 @@ func (c *controller) updateServiceInstanceCondition( // 2 - any error that occurred func (c *controller) recordStartOfServiceInstanceOperation(toUpdate *v1beta1.ServiceInstance, operation v1beta1.ServiceInstanceOperation, inProgressProperties *v1beta1.ServiceInstancePropertiesState) (*v1beta1.ServiceInstance, error) { currentReconciledGeneration := toUpdate.Status.ReconciledGeneration - clearServiceInstanceCurrentOperation(toUpdate, true) - + clearServiceInstanceCurrentOperation(toUpdate) + toUpdate.Status.ObservedGeneration = toUpdate.Generation toUpdate.Status.ReconciledGeneration = currentReconciledGeneration toUpdate.Status.CurrentOperation = operation now := metav1.Now() @@ -1167,7 +1173,7 @@ func (c *controller) checkForRemovedClassAndPlan(instance *v1beta1.ServiceInstan return nil } - isProvisioning := !instance.Status.Provisioned + isProvisioning := instance.Status.ProvisionStatus != v1beta1.ServiceInstanceProvisionStatusProvisioned // Regardless of what's been deleted, you can always update // parameters (ie, not change plans) @@ -1195,17 +1201,15 @@ func (c *controller) checkForRemovedClassAndPlan(instance *v1beta1.ServiceInstan // clearServiceInstanceCurrentOperation sets the fields of the instance's Status // to indicate that there is no current operation being performed. The Status // is *not* recorded in the registry. -func clearServiceInstanceCurrentOperation(toUpdate *v1beta1.ServiceInstance, updateObservedGeneration bool) { +func clearServiceInstanceCurrentOperation(toUpdate *v1beta1.ServiceInstance) { toUpdate.Status.CurrentOperation = "" toUpdate.Status.OperationStartTime = nil toUpdate.Status.AsyncOpInProgress = false toUpdate.Status.OrphanMitigationInProgress = false toUpdate.Status.LastOperation = nil toUpdate.Status.InProgressProperties = nil + // TODO nilebox: update ReconciledGeneration only when operation is finished toUpdate.Status.ReconciledGeneration = toUpdate.Generation - if updateObservedGeneration { - toUpdate.Status.ObservedGeneration = toUpdate.Generation - } } // rollbackReconciledGenerationOnDeletion resets the ReconciledGeneration if a @@ -1480,13 +1484,13 @@ func (c *controller) processServiceInstanceOperationError(instance *v1beta1.Serv // processProvisionSuccess handles the logging and updating of a // ServiceInstance that has successfully been provisioned at the broker. -func (c *controller) processProvisionSuccess(instance *v1beta1.ServiceInstance, dashboardURL *string, updateObservedGeneration bool) error { - instance.Status.Provisioned = true +func (c *controller) processProvisionSuccess(instance *v1beta1.ServiceInstance, dashboardURL *string) error { + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned setServiceInstanceDashboardURL(instance, dashboardURL) setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, v1beta1.ConditionTrue, successProvisionReason, successProvisionMessage) instance.Status.ExternalProperties = instance.Status.InProgressProperties currentReconciledGeneration := instance.Status.ReconciledGeneration - clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) + clearServiceInstanceCurrentOperation(instance) rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) if _, err := c.updateServiceInstanceStatus(instance); err != nil { @@ -1499,7 +1503,7 @@ func (c *controller) processProvisionSuccess(instance *v1beta1.ServiceInstance, // processProvisionFailure handles the logging and updating of a // ServiceInstance that hit a terminal failure during provision reconciliation. -func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition, shouldMitigateOrphan bool, updateObservedGeneration bool) error { +func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition, shouldMitigateOrphan bool) error { currentReconciledGeneration := instance.Status.ReconciledGeneration if failedCond == nil { return fmt.Errorf("failedCond must not be nil") @@ -1527,7 +1531,7 @@ func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance, err = fmt.Errorf(failedCond.Message) } else { - clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) + clearServiceInstanceCurrentOperation(instance) rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) } @@ -1557,11 +1561,11 @@ func (c *controller) processProvisionAsyncResponse(instance *v1beta1.ServiceInst // processUpdateServiceInstanceSuccess handles the logging and updating of a // ServiceInstance that has successfully been updated at the broker. -func (c *controller) processUpdateServiceInstanceSuccess(instance *v1beta1.ServiceInstance, updateObservedGeneration bool) error { +func (c *controller) processUpdateServiceInstanceSuccess(instance *v1beta1.ServiceInstance) error { setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, v1beta1.ConditionTrue, successUpdateInstanceReason, successUpdateInstanceMessage) instance.Status.ExternalProperties = instance.Status.InProgressProperties currentReconciledGeneration := instance.Status.ReconciledGeneration - clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) + clearServiceInstanceCurrentOperation(instance) rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) if _, err := c.updateServiceInstanceStatus(instance); err != nil { @@ -1574,14 +1578,14 @@ func (c *controller) processUpdateServiceInstanceSuccess(instance *v1beta1.Servi // processUpdateServiceInstanceFailure handles the logging and updating of a // ServiceInstance that hit a terminal failure during update reconciliation. -func (c *controller) processUpdateServiceInstanceFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition, updateObservedGeneration bool) error { +func (c *controller) processUpdateServiceInstanceFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition) error { c.recorder.Event(instance, corev1.EventTypeWarning, readyCond.Reason, readyCond.Message) currentReconciledGeneration := instance.Status.ReconciledGeneration setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, readyCond.Status, readyCond.Reason, readyCond.Message) setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionFailed, failedCond.Status, failedCond.Reason, failedCond.Message) - clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) + clearServiceInstanceCurrentOperation(instance) rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) if _, err := c.updateServiceInstanceStatus(instance); err != nil { @@ -1609,7 +1613,7 @@ func (c *controller) processUpdateServiceInstanceAsyncResponse(instance *v1beta1 // processDeprovisionSuccess handles the logging and updating of // a ServiceInstance that has successfully been deprovisioned at the broker. -func (c *controller) processDeprovisionSuccess(instance *v1beta1.ServiceInstance, updateObservedGeneration bool) error { +func (c *controller) processDeprovisionSuccess(instance *v1beta1.ServiceInstance) error { mitigatingOrphan := instance.Status.OrphanMitigationInProgress reason := successDeprovisionReason @@ -1619,9 +1623,9 @@ func (c *controller) processDeprovisionSuccess(instance *v1beta1.ServiceInstance msg = successOrphanMitigationMessage } - instance.Status.Provisioned = false + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, v1beta1.ConditionFalse, reason, msg) - clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) + clearServiceInstanceCurrentOperation(instance) instance.Status.ExternalProperties = nil instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusSucceeded @@ -1644,7 +1648,7 @@ func (c *controller) processDeprovisionSuccess(instance *v1beta1.ServiceInstance // processDeprovisionFailure handles the logging and updating of a // ServiceInstance that hit a terminal failure during deprovision // reconciliation. -func (c *controller) processDeprovisionFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition, updateObservedGeneration bool) error { +func (c *controller) processDeprovisionFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition) error { if failedCond == nil { return fmt.Errorf("failedCond must not be nil") } @@ -1666,7 +1670,7 @@ func (c *controller) processDeprovisionFailure(instance *v1beta1.ServiceInstance c.recorder.Event(instance, corev1.EventTypeWarning, failedCond.Reason, failedCond.Message) } - clearServiceInstanceCurrentOperation(instance, updateObservedGeneration) + clearServiceInstanceCurrentOperation(instance) instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusFailed if _, err := c.updateServiceInstanceStatus(instance); err != nil { diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index 68d26e61ab8..f1456f07892 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -1495,7 +1495,7 @@ func TestReconcileServiceInstanceDelete(t *testing.T) { instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, @@ -1566,7 +1566,7 @@ func TestReconcileServiceInstanceDeleteBlockedByCredentials(t *testing.T) { instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, @@ -1673,7 +1673,7 @@ func TestReconcileServiceInstanceDeleteAsynchronous(t *testing.T) { instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, @@ -1754,7 +1754,7 @@ func TestReconcileServiceInstanceDeleteFailedProvisionWithRequest(t *testing.T) instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = false + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil @@ -1860,7 +1860,7 @@ func TestReconsileServiceInstanceDeleteWithParameters(t *testing.T) { instance.Generation = tc.generation instance.Status.ReconciledGeneration = tc.reconciledGeneration instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil @@ -1910,7 +1910,7 @@ func TestReconcileServiceInstanceDeleteWhenAlreadyDeprovisionedUnsuccessfully(t instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil @@ -1960,7 +1960,7 @@ func TestReconcileServiceInstanceDeleteFailedUpdate(t *testing.T) { instance.Generation = 2 instance.Status.ReconciledGeneration = 2 instance.Status.ObservedGeneration = 2 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { @@ -1986,12 +1986,9 @@ func TestReconcileServiceInstanceDeleteFailedUpdate(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -2021,7 +2018,7 @@ func TestReconcileServiceInstanceDeleteDoesNotInvokeClusterServiceBroker(t *test instance.Generation = 1 instance.Status.ReconciledGeneration = 0 instance.Status.ObservedGeneration = 0 - instance.Status.Provisioned = false + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusNotRequired fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { @@ -2727,6 +2724,7 @@ func TestReconcileServiceInstanceSuccessOnFinalRetry(t *testing.T) { ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, } + instance.Status.ObservedGeneration = instance.Generation startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) instance.Status.OperationStartTime = &startTime @@ -2788,6 +2786,7 @@ func TestReconcileServiceInstanceFailureOnFinalRetry(t *testing.T) { instance.Status.CurrentOperation = v1beta1.ServiceInstanceOperationProvision startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) instance.Status.OperationStartTime = &startTime + instance.Status.ObservedGeneration = instance.Generation if err := testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("Should have returned no error because the retry duration has elapsed: %v", err) @@ -3383,7 +3382,7 @@ func TestReconcileInstanceDeleteUsingOriginatingIdentity(t *testing.T) { instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired if tc.includeUserInfo { instance.Spec.UserInfo = testUserInfo @@ -4047,7 +4046,7 @@ func TestReconcileServiceInstanceUpdateParameters(t *testing.T) { instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired oldParameters := map[string]interface{}{ @@ -4176,7 +4175,7 @@ func TestReconcileServiceInstanceDeleteParameters(t *testing.T) { instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired oldParameters := map[string]interface{}{ @@ -4351,7 +4350,7 @@ func TestReconcileServiceInstanceUpdatePlan(t *testing.T) { instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired oldParameters := map[string]interface{}{ @@ -4791,7 +4790,7 @@ func TestReconcileServiceInstanceUpdateAsynchronous(t *testing.T) { instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ @@ -5260,7 +5259,7 @@ func TestReconcileServiceInstanceDeleteWithNonExistentPlan(t *testing.T) { instance.Generation = 2 instance.Status.ReconciledGeneration = 1 instance.Status.ObservedGeneration = 1 - instance.Status.Provisioned = true + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: "old-plan-name", ClusterServicePlanExternalID: "old-plan-id", @@ -5326,7 +5325,7 @@ func TestReconcileServiceInstanceUpdateMissingObservedGeneration(t *testing.T) { instance.Status.ReconciledGeneration = 1 // Missing ObservedGeneration and Provisioned after updating Service Catalog instance.Status.ObservedGeneration = 0 - instance.Status.Provisioned = false + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ @@ -5348,7 +5347,7 @@ func TestReconcileServiceInstanceUpdateMissingObservedGeneration(t *testing.T) { if updatedServiceInstance.Status.ObservedGeneration == 0 || updatedServiceInstance.Status.ObservedGeneration != instance.Status.ReconciledGeneration { t.Fatalf("Unexpected ObservedGeneration value: %d", updatedServiceInstance.Status.ObservedGeneration) } - if !updatedServiceInstance.Status.Provisioned { - t.Fatalf("The instance was expected to have Provisioned flag set") + if instance.Status.ProvisionStatus != v1beta1.ServiceInstanceProvisionStatusProvisioned { + t.Fatalf("The instance was expected to be marked as Provisioned") } } diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 75d02b5e0b0..d2e405567ca 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -688,7 +688,7 @@ func getTestServiceInstanceUpdatingPlan() *v1beta1.ServiceInstance { // It's been provisioned successfully. ReconciledGeneration: 1, ObservedGeneration: 1, - Provisioned: true, + ProvisionStatus: v1beta1.ServiceInstanceProvisionStatusProvisioned, DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, } @@ -710,7 +710,7 @@ func getTestServiceInstanceUpdatingParameters() *v1beta1.ServiceInstance { // It's been provisioned successfully. ReconciledGeneration: 1, ObservedGeneration: 1, - Provisioned: true, + ProvisionStatus: v1beta1.ServiceInstanceProvisionStatusProvisioned, DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, } @@ -732,7 +732,7 @@ func getTestServiceInstanceUpdatingParametersOfDeletedPlan() *v1beta1.ServiceIns // It's been provisioned successfully. ReconciledGeneration: 1, ObservedGeneration: 1, - Provisioned: true, + ProvisionStatus: v1beta1.ServiceInstanceProvisionStatusProvisioned, DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, } @@ -778,7 +778,6 @@ func getTestServiceInstanceAsyncUpdating(operation string) *v1beta1.ServiceInsta instance.Status = v1beta1.ServiceInstanceStatus{ ReconciledGeneration: 1, ObservedGeneration: 2, - Provisioned: true, Conditions: []v1beta1.ServiceInstanceCondition{{ Type: v1beta1.ServiceInstanceConditionReady, Status: v1beta1.ConditionFalse, @@ -796,6 +795,7 @@ func getTestServiceInstanceAsyncUpdating(operation string) *v1beta1.ServiceInsta ClusterServicePlanExternalName: "old-plan-name", ClusterServicePlanExternalID: "old-plan-id", }, + ProvisionStatus: v1beta1.ServiceInstanceProvisionStatusProvisioned, DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, } if operation != "" { @@ -822,11 +822,11 @@ func getTestServiceInstanceAsyncDeprovisioning(operation string) *v1beta1.Servic CurrentOperation: v1beta1.ServiceInstanceOperationDeprovision, ReconciledGeneration: 1, ObservedGeneration: 2, - Provisioned: true, ExternalProperties: &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, }, + ProvisionStatus: v1beta1.ServiceInstanceProvisionStatusProvisioned, DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, } if operation != "" { @@ -1949,14 +1949,14 @@ func assertServiceInstanceObservedGeneration(t *testing.T, obj runtime.Object, o } } -func assertServiceInstanceProvisioned(t *testing.T, obj runtime.Object, provisioned bool) { +func assertServiceInstanceProvisioned(t *testing.T, obj runtime.Object, provisionStatus v1beta1.ServiceInstanceProvisionStatus) { instance, ok := obj.(*v1beta1.ServiceInstance) if !ok { fatalf(t, "Couldn't convert object %+v into a *v1beta1.ServiceInstance", obj) } - if e, a := provisioned, instance.Status.Provisioned; e != a { - fatalf(t, "unexpected provisioned flag: expected %v, got %v", e, a) + if e, a := provisionStatus, instance.Status.ProvisionStatus; e != a { + fatalf(t, "unexpected provision status: expected %v, got %v", e, a) } } @@ -2073,7 +2073,7 @@ func assertServiceInstanceErrorBeforeRequest(t *testing.T, obj runtime.Object, r assertServiceInstanceOperationStartTimeSet(t, obj, false) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Status.ObservedGeneration) - assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) assertServiceInstanceInProgressPropertiesNil(t, obj) @@ -2100,7 +2100,7 @@ func assertServiceInstanceOperationInProgressWithParameters(t *testing.T, obj ru assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) - assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) switch operation { @@ -2120,7 +2120,7 @@ func assertServiceInstanceStartingOrphanMitigation(t *testing.T, obj runtime.Obj assertServiceInstanceOperationStartTimeSet(t, obj, false) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) - assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) assertServiceInstanceOrphanMitigationInProgressTrue(t, obj) assertServiceInstanceDeprovisionStatus(t, obj, v1beta1.ServiceInstanceDeprovisionStatusRequired) } @@ -2135,20 +2135,20 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti readyStatus v1beta1.ConditionStatus deprovisionStatus v1beta1.ServiceInstanceDeprovisionStatus ) - var provisioned bool + var provisionStatus v1beta1.ServiceInstanceProvisionStatus switch operation { case v1beta1.ServiceInstanceOperationProvision: - provisioned = true + provisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned reason = successProvisionReason readyStatus = v1beta1.ConditionTrue deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired case v1beta1.ServiceInstanceOperationUpdate: - provisioned = true + provisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned reason = successUpdateInstanceReason readyStatus = v1beta1.ConditionTrue deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired case v1beta1.ServiceInstanceOperationDeprovision: - provisioned = false + provisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned reason = successDeprovisionReason readyStatus = v1beta1.ConditionFalse deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusSucceeded @@ -2158,7 +2158,7 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti assertServiceInstanceOperationStartTimeSet(t, obj, false) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Generation) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) - assertServiceInstanceProvisioned(t, obj, provisioned) + assertServiceInstanceProvisioned(t, obj, provisionStatus) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) if operation == v1beta1.ServiceInstanceOperationDeprovision { @@ -2206,7 +2206,7 @@ func assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t *testing.T, ob assertServiceInstanceCurrentOperationClear(t, obj) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Generation) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) - assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) assertServiceInstanceInProgressPropertiesNil(t, obj) } @@ -2216,7 +2216,7 @@ func assertServiceInstanceRequestFailingErrorStartOrphanMitigation(t *testing.T, assertServiceInstanceCurrentOperation(t, obj, v1beta1.ServiceInstanceOperationProvision) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) - assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) assertServiceInstanceOrphanMitigationInProgressTrue(t, obj) } @@ -2237,7 +2237,7 @@ func assertServiceInstanceRequestRetriableErrorWithParameters(t *testing.T, obj assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) - assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) switch operation { case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) @@ -2265,7 +2265,7 @@ func assertServiceInstanceAsyncStartInProgress(t *testing.T, obj runtime.Object, assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) - assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) switch operation { case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) @@ -2293,7 +2293,7 @@ func assertServiceInstanceAsyncStillInProgress(t *testing.T, obj runtime.Object, assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Status.ObservedGeneration) - assertServiceInstanceProvisioned(t, obj, originalInstance.Status.Provisioned) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) switch operation { case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) diff --git a/pkg/openapi/openapi_generated.go b/pkg/openapi/openapi_generated.go index 1591d95c1ae..9e55138ae33 100644 --- a/pkg/openapi/openapi_generated.go +++ b/pkg/openapi/openapi_generated.go @@ -1507,13 +1507,6 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Format: "int64", }, }, - "provisioned": { - SchemaProps: spec.SchemaProps{ - Description: "Provisioned is the flag that marks whether the instance has been successfully provisioned (i.e. it exists on the broker's side and can be updated or deprovisioned). The flag should be reset after successful deprovisioning.", - Type: []string{"boolean"}, - Format: "", - }, - }, "operationStartTime": { SchemaProps: spec.SchemaProps{ Description: "OperationStartTime is the time at which the current operation began.", @@ -1532,6 +1525,13 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Ref: ref("github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1.ServiceInstancePropertiesState"), }, }, + "provisioned": { + SchemaProps: spec.SchemaProps{ + Description: "ProvisionStatus describes whether the instance is in the provisioned state.", + Type: []string{"string"}, + Format: "", + }, + }, "deprovisionStatus": { SchemaProps: spec.SchemaProps{ Description: "DeprovisionStatus describes what has been done to deprovision the ServiceInstance.", diff --git a/test/integration/clientset_test.go b/test/integration/clientset_test.go index c5d7cba6b4a..347ec39562e 100644 --- a/test/integration/clientset_test.go +++ b/test/integration/clientset_test.go @@ -924,7 +924,7 @@ func testInstanceClient(sType server.StorageType, client servicecatalogclient.In instanceServer.Status = v1beta1.ServiceInstanceStatus{ ReconciledGeneration: instanceServer.Generation, ObservedGeneration: instanceServer.Generation, - Provisioned: true, + ProvisionStatus: v1beta1.ServiceInstanceProvisionStatusProvisioned, Conditions: []v1beta1.ServiceInstanceCondition{readyConditionTrue}, DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired, } From 2e174fe638e8e4889cb3ca6ce2b5202ef380c0ee Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Tue, 27 Feb 2018 16:42:57 +1100 Subject: [PATCH 15/22] Copy ReconciledGeneration from ObservedGeneration and remove hacks for DeletionTimestamp --- pkg/controller/controller_instance.go | 34 +++++++-------------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 912b032bef1..8308f2e5880 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -1128,10 +1128,8 @@ func (c *controller) updateServiceInstanceCondition( // if there was an error // 2 - any error that occurred func (c *controller) recordStartOfServiceInstanceOperation(toUpdate *v1beta1.ServiceInstance, operation v1beta1.ServiceInstanceOperation, inProgressProperties *v1beta1.ServiceInstancePropertiesState) (*v1beta1.ServiceInstance, error) { - currentReconciledGeneration := toUpdate.Status.ReconciledGeneration clearServiceInstanceCurrentOperation(toUpdate) toUpdate.Status.ObservedGeneration = toUpdate.Generation - toUpdate.Status.ReconciledGeneration = currentReconciledGeneration toUpdate.Status.CurrentOperation = operation now := metav1.Now() toUpdate.Status.OperationStartTime = &now @@ -1208,19 +1206,6 @@ func clearServiceInstanceCurrentOperation(toUpdate *v1beta1.ServiceInstance) { toUpdate.Status.OrphanMitigationInProgress = false toUpdate.Status.LastOperation = nil toUpdate.Status.InProgressProperties = nil - // TODO nilebox: update ReconciledGeneration only when operation is finished - toUpdate.Status.ReconciledGeneration = toUpdate.Generation -} - -// rollbackReconciledGenerationOnDeletion resets the ReconciledGeneration if a -// deletion was performed while an async provision or update is running. -// TODO: rework saving off current generation as the start of the async -// operation, see PR 1708/Issue 1587. -func rollbackReconciledGenerationOnDeletion(instance *v1beta1.ServiceInstance, currentReconciledGeneration int64) { - if instance.DeletionTimestamp != nil { - glog.V(4).Infof("Not updating ReconciledGeneration after async operation because there is a deletion pending.") - instance.Status.ReconciledGeneration = currentReconciledGeneration - } } // serviceInstanceHasExistingBindings returns true if there are any existing @@ -1485,13 +1470,12 @@ func (c *controller) processServiceInstanceOperationError(instance *v1beta1.Serv // processProvisionSuccess handles the logging and updating of a // ServiceInstance that has successfully been provisioned at the broker. func (c *controller) processProvisionSuccess(instance *v1beta1.ServiceInstance, dashboardURL *string) error { - instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned setServiceInstanceDashboardURL(instance, dashboardURL) setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, v1beta1.ConditionTrue, successProvisionReason, successProvisionMessage) instance.Status.ExternalProperties = instance.Status.InProgressProperties - currentReconciledGeneration := instance.Status.ReconciledGeneration clearServiceInstanceCurrentOperation(instance) - rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned + instance.Status.ReconciledGeneration = instance.Status.ObservedGeneration if _, err := c.updateServiceInstanceStatus(instance); err != nil { return err @@ -1504,7 +1488,6 @@ func (c *controller) processProvisionSuccess(instance *v1beta1.ServiceInstance, // processProvisionFailure handles the logging and updating of a // ServiceInstance that hit a terminal failure during provision reconciliation. func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition, shouldMitigateOrphan bool) error { - currentReconciledGeneration := instance.Status.ReconciledGeneration if failedCond == nil { return fmt.Errorf("failedCond must not be nil") } @@ -1532,7 +1515,7 @@ func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance, err = fmt.Errorf(failedCond.Message) } else { clearServiceInstanceCurrentOperation(instance) - rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) + instance.Status.ReconciledGeneration = instance.Status.ObservedGeneration } if _, err := c.updateServiceInstanceStatus(instance); err != nil { @@ -1564,9 +1547,8 @@ func (c *controller) processProvisionAsyncResponse(instance *v1beta1.ServiceInst func (c *controller) processUpdateServiceInstanceSuccess(instance *v1beta1.ServiceInstance) error { setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, v1beta1.ConditionTrue, successUpdateInstanceReason, successUpdateInstanceMessage) instance.Status.ExternalProperties = instance.Status.InProgressProperties - currentReconciledGeneration := instance.Status.ReconciledGeneration clearServiceInstanceCurrentOperation(instance) - rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) + instance.Status.ReconciledGeneration = instance.Status.ObservedGeneration if _, err := c.updateServiceInstanceStatus(instance); err != nil { return err @@ -1580,13 +1562,11 @@ func (c *controller) processUpdateServiceInstanceSuccess(instance *v1beta1.Servi // ServiceInstance that hit a terminal failure during update reconciliation. func (c *controller) processUpdateServiceInstanceFailure(instance *v1beta1.ServiceInstance, readyCond, failedCond *v1beta1.ServiceInstanceCondition) error { c.recorder.Event(instance, corev1.EventTypeWarning, readyCond.Reason, readyCond.Message) - currentReconciledGeneration := instance.Status.ReconciledGeneration setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, readyCond.Status, readyCond.Reason, readyCond.Message) setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionFailed, failedCond.Status, failedCond.Reason, failedCond.Message) - clearServiceInstanceCurrentOperation(instance) - rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration) + instance.Status.ReconciledGeneration = instance.Status.ObservedGeneration if _, err := c.updateServiceInstanceStatus(instance); err != nil { return err @@ -1623,11 +1603,12 @@ func (c *controller) processDeprovisionSuccess(instance *v1beta1.ServiceInstance msg = successOrphanMitigationMessage } - instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, v1beta1.ConditionFalse, reason, msg) clearServiceInstanceCurrentOperation(instance) instance.Status.ExternalProperties = nil + instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusSucceeded + instance.Status.ReconciledGeneration = instance.Status.ObservedGeneration if mitigatingOrphan { if _, err := c.updateServiceInstanceStatus(instance); err != nil { @@ -1671,6 +1652,7 @@ func (c *controller) processDeprovisionFailure(instance *v1beta1.ServiceInstance } clearServiceInstanceCurrentOperation(instance) + instance.Status.ReconciledGeneration = instance.Status.ObservedGeneration instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusFailed if _, err := c.updateServiceInstanceStatus(instance); err != nil { From b6e8567dbbb55de1259b8fbc23fe9d6702d1ebb3 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Tue, 27 Feb 2018 17:21:46 +1100 Subject: [PATCH 16/22] Update comment --- pkg/controller/controller_instance.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 8308f2e5880..d48de949372 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -255,7 +255,9 @@ func (c *controller) reconcileServiceInstance(instance *v1beta1.ServiceInstance) } // initObservedGeneration implements ObservedGeneration initialization based on -// ReconciledGeneration for status API migration +// ReconciledGeneration for status API migration. +// Returns true if the status was updated (i.e. the iteration has finished and no +// more processing needed). func (c *controller) initObservedGeneration(instance *v1beta1.ServiceInstance) (bool, error) { if instance.Status.ObservedGeneration == 0 && instance.Status.ReconciledGeneration != 0 { instance.Status.ObservedGeneration = instance.Status.ReconciledGeneration From ef56ba84d307f64852088b1cac11a224a0217bd7 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Tue, 27 Feb 2018 22:00:40 +1100 Subject: [PATCH 17/22] Set ObservedGeneration as soon as we start processing the new spec --- pkg/controller/controller_instance.go | 42 +++++++++++++++++++--- pkg/controller/controller_instance_test.go | 5 ++- pkg/controller/controller_test.go | 2 +- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index d48de949372..43a6bb22164 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -295,6 +295,10 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan } instance = instance.DeepCopy() + // Any status updates from this point should have an updated observed generation + if instance.Status.ObservedGeneration != instance.Generation { + c.prepareServiceInstanceStatus(instance) + } // Update references to ClusterServicePlan / ClusterServiceClass if necessary. // @@ -322,7 +326,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan return c.handleServiceInstanceReconciliationError(instance, err) } - if instance.Status.ObservedGeneration != instance.Generation { + if instance.Status.CurrentOperation == "" { instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationProvision, inProgressProperties) if err != nil { // There has been an update to the instance. Start reconciliation @@ -399,6 +403,10 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns } instance = instance.DeepCopy() + // Any status updates from this point should have an updated observed generation + if instance.Status.ObservedGeneration != instance.Generation { + c.prepareServiceInstanceStatus(instance) + } // Update references to ClusterServicePlan / ClusterServiceClass if necessary. // @@ -426,7 +434,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns return c.handleServiceInstanceReconciliationError(instance, err) } - if instance.Status.ObservedGeneration != instance.Generation { + if instance.Status.CurrentOperation == "" { instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationUpdate, inProgressProperties) if err != nil { // There has been an update to the instance. Start reconciliation @@ -506,6 +514,10 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns glog.V(4).Info(pcb.Message("Processing deleting event")) instance = instance.DeepCopy() + // Any status updates from this point should have an updated observed generation + if instance.Status.ObservedGeneration != instance.Generation { + c.prepareServiceInstanceStatus(instance) + } // If the deprovisioning succeeded or is not needed, then no need to // make a request to the broker. @@ -547,7 +559,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns instance.Status.OperationStartTime = &now } } else { - if instance.Status.ObservedGeneration != instance.Generation { + if instance.Status.CurrentOperation != v1beta1.ServiceInstanceOperationDeprovision { instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationDeprovision, nil) if err != nil { // There has been an update to the instance. Start reconciliation @@ -997,6 +1009,13 @@ func newServiceInstanceFailedCondition(status v1beta1.ConditionStatus, reason, m } } +// resetServiceInstanceCondition sets a single condition on Instnace's status +// to "false" with empty reason and message +func resetServiceInstanceCondition(toUpdate *v1beta1.ServiceInstance, + conditionType v1beta1.ServiceInstanceConditionType) { + setServiceInstanceConditionInternal(toUpdate, conditionType, v1beta1.ConditionFalse, "", "", metav1.Now()) +} + // setServiceInstanceCondition sets a single condition on an Instance's status: if // the condition already exists in the status, it is mutated; if the condition // does not already exist in the status, it is added. Other conditions in the @@ -1118,6 +1137,22 @@ func (c *controller) updateServiceInstanceCondition( return err } +// prepareServiceInstanceStatus sets the instance's observed generation +// and clears the conditions, preparing it for any status updates that can occur +// during the further processing. +// It doesn't send the update request to server. +func (c *controller) prepareServiceInstanceStatus(toUpdate *v1beta1.ServiceInstance) { + toUpdate.Status.ObservedGeneration = toUpdate.Generation + resetServiceInstanceCondition( + toUpdate, + v1beta1.ServiceInstanceConditionReady, + ) + resetServiceInstanceCondition( + toUpdate, + v1beta1.ServiceInstanceConditionFailed, + ) +} + // recordStartOfServiceInstanceOperation updates the instance to indicate that // there is an operation being performed. If the instance was already // performing a different operation, that operation is replaced. The Status of @@ -1131,7 +1166,6 @@ func (c *controller) updateServiceInstanceCondition( // 2 - any error that occurred func (c *controller) recordStartOfServiceInstanceOperation(toUpdate *v1beta1.ServiceInstance, operation v1beta1.ServiceInstanceOperation, inProgressProperties *v1beta1.ServiceInstancePropertiesState) (*v1beta1.ServiceInstance, error) { clearServiceInstanceCurrentOperation(toUpdate) - toUpdate.Status.ObservedGeneration = toUpdate.Generation toUpdate.Status.CurrentOperation = operation now := metav1.Now() toUpdate.Status.OperationStartTime = &now diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index f1456f07892..56c9a821671 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -1986,9 +1986,12 @@ func TestReconcileServiceInstanceDeleteFailedUpdate(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) + + updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index d2e405567ca..d3b24776037 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -2072,7 +2072,7 @@ func assertServiceInstanceErrorBeforeRequest(t *testing.T, obj runtime.Object, r assertServiceInstanceCurrentOperationClear(t, obj) assertServiceInstanceOperationStartTimeSet(t, obj, false) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) - assertServiceInstanceObservedGeneration(t, obj, originalInstance.Status.ObservedGeneration) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) From c1405f8344c0d1e3bd28f6d97d1508b1c100ea30 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 28 Feb 2018 09:31:26 +1100 Subject: [PATCH 18/22] Restore status after invoking UpdateReferences --- pkg/apis/servicecatalog/v1beta1/types.go | 2 +- pkg/controller/controller_instance.go | 14 +++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/pkg/apis/servicecatalog/v1beta1/types.go b/pkg/apis/servicecatalog/v1beta1/types.go index c39927b05a1..8db34083254 100644 --- a/pkg/apis/servicecatalog/v1beta1/types.go +++ b/pkg/apis/servicecatalog/v1beta1/types.go @@ -622,7 +622,7 @@ type ServiceInstanceStatus struct { ExternalProperties *ServiceInstancePropertiesState `json:"externalProperties,omitempty"` // ProvisionStatus describes whether the instance is in the provisioned state. - ProvisionStatus ServiceInstanceProvisionStatus `json:"provisioned"` + ProvisionStatus ServiceInstanceProvisionStatus `json:"provisionStatus"` // DeprovisionStatus describes what has been done to deprovision the // ServiceInstance. diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 43a6bb22164..8602f2605ef 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -297,7 +297,7 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan instance = instance.DeepCopy() // Any status updates from this point should have an updated observed generation if instance.Status.ObservedGeneration != instance.Generation { - c.prepareServiceInstanceStatus(instance) + c.prepareObservedGeneration(instance) } // Update references to ClusterServicePlan / ClusterServiceClass if necessary. @@ -405,7 +405,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns instance = instance.DeepCopy() // Any status updates from this point should have an updated observed generation if instance.Status.ObservedGeneration != instance.Generation { - c.prepareServiceInstanceStatus(instance) + c.prepareObservedGeneration(instance) } // Update references to ClusterServicePlan / ClusterServiceClass if necessary. @@ -516,7 +516,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns instance = instance.DeepCopy() // Any status updates from this point should have an updated observed generation if instance.Status.ObservedGeneration != instance.Generation { - c.prepareServiceInstanceStatus(instance) + c.prepareObservedGeneration(instance) } // If the deprovisioning succeeded or is not needed, then no need to @@ -1093,10 +1093,14 @@ func setServiceInstanceConditionInternal(toUpdate *v1beta1.ServiceInstance, func (c *controller) updateServiceInstanceReferences(toUpdate *v1beta1.ServiceInstance) (*v1beta1.ServiceInstance, error) { pcb := pretty.NewContextBuilder(pretty.ServiceInstance, toUpdate.Namespace, toUpdate.Name) glog.V(4).Info(pcb.Message("Updating references")) + status := toUpdate.Status updatedInstance, err := c.serviceCatalogClient.ServiceInstances(toUpdate.Namespace).UpdateReferences(toUpdate) if err != nil { glog.Errorf(pcb.Messagef("Failed to update references: %v", err)) } + // The UpdateReferences method ignores status changes. + // Restore status that might have changed locally to be able to update it later. + updatedInstance.Status = status return updatedInstance, err } @@ -1137,11 +1141,11 @@ func (c *controller) updateServiceInstanceCondition( return err } -// prepareServiceInstanceStatus sets the instance's observed generation +// prepareObservedGeneration sets the instance's observed generation // and clears the conditions, preparing it for any status updates that can occur // during the further processing. // It doesn't send the update request to server. -func (c *controller) prepareServiceInstanceStatus(toUpdate *v1beta1.ServiceInstance) { +func (c *controller) prepareObservedGeneration(toUpdate *v1beta1.ServiceInstance) { toUpdate.Status.ObservedGeneration = toUpdate.Generation resetServiceInstanceCondition( toUpdate, From afad5851375266484d86042209c46d6b65d5c695 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 28 Feb 2018 17:03:53 +1100 Subject: [PATCH 19/22] Fix review comments --- pkg/controller/controller.go | 18 ++++++++-- pkg/controller/controller_instance.go | 37 ++++++++------------ pkg/controller/controller_test.go | 23 ++---------- pkg/openapi/openapi_generated.go | 4 +-- test/integration/controller_instance_test.go | 6 ++-- test/integration/controller_test.go | 4 +-- test/util/util.go | 31 ++++++++-------- 7 files changed, 56 insertions(+), 67 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 99e4a5e65a5..3af636ea1b7 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -590,11 +590,11 @@ func convertClusterServicePlans(plans []osb.Plan, serviceClassID string) ([]*v1b return servicePlans, nil } -// isServiceInstanceReady returns whether the given instance has a ready condition +// isServiceInstanceConditionTrue returns whether the given instance has a given condition // with status true. -func isServiceInstanceReady(instance *v1beta1.ServiceInstance) bool { +func isServiceInstanceConditionTrue(instance *v1beta1.ServiceInstance, conditionType v1beta1.ServiceInstanceConditionType) bool { for _, cond := range instance.Status.Conditions { - if cond.Type == v1beta1.ServiceInstanceConditionReady { + if cond.Type == conditionType { return cond.Status == v1beta1.ConditionTrue } } @@ -602,6 +602,18 @@ func isServiceInstanceReady(instance *v1beta1.ServiceInstance) bool { return false } +// isServiceInstanceReady returns whether the given instance has a ready condition +// with status true. +func isServiceInstanceReady(instance *v1beta1.ServiceInstance) bool { + return isServiceInstanceConditionTrue(instance, v1beta1.ServiceInstanceConditionReady) +} + +// isServiceInstanceFailed returns whether the instance has a failed condition with +// status true. +func isServiceInstanceFailed(instance *v1beta1.ServiceInstance) bool { + return isServiceInstanceConditionTrue(instance, v1beta1.ServiceInstanceConditionFailed) +} + // NewClientConfigurationForBroker creates a new ClientConfiguration for connecting // to the specified Broker func NewClientConfigurationForBroker(broker *v1beta1.ClusterServiceBroker, authConfig *osb.AuthConfig) *osb.ClientConfiguration { diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 8602f2605ef..c6c959e25d0 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -86,6 +86,8 @@ const ( deprovisioningInFlightMessage string = "Deprovision request for ServiceInstance in-flight to Broker" startingInstanceOrphanMitigationReason string = "StartingInstanceOrphanMitigation" startingInstanceOrphanMitigationMessage string = "The instance provision call failed with an ambiguous error; attempting to deprovision the instance in order to mitigate an orphaned resource" + observedNewGenerationReason string = "ObservedNewGeneration" + observedNewGenerationMessage string = "Observed a new generation of the instance" ) // ServiceInstance handlers and control-loop @@ -515,7 +517,9 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns instance = instance.DeepCopy() // Any status updates from this point should have an updated observed generation - if instance.Status.ObservedGeneration != instance.Generation { + // except for the orphan mitigation (it is considered to be a continuation + // of the previously failed provisioning operation). + if !instance.Status.OrphanMitigationInProgress && instance.Status.ObservedGeneration != instance.Generation { c.prepareObservedGeneration(instance) } @@ -756,18 +760,6 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro } } -// isServiceInstanceFailed returns whether the instance has a failed condition with -// status true. -func isServiceInstanceFailed(instance *v1beta1.ServiceInstance) bool { - for _, condition := range instance.Status.Conditions { - if condition.Type == v1beta1.ServiceInstanceConditionFailed && condition.Status == v1beta1.ConditionTrue { - return true - } - } - - return false -} - // isServiceInstanceProcessedAlready returns true if there is no further processing // needed for the instance based on ObservedGeneration func isServiceInstanceProcessedAlready(instance *v1beta1.ServiceInstance) bool { @@ -1009,13 +1001,6 @@ func newServiceInstanceFailedCondition(status v1beta1.ConditionStatus, reason, m } } -// resetServiceInstanceCondition sets a single condition on Instnace's status -// to "false" with empty reason and message -func resetServiceInstanceCondition(toUpdate *v1beta1.ServiceInstance, - conditionType v1beta1.ServiceInstanceConditionType) { - setServiceInstanceConditionInternal(toUpdate, conditionType, v1beta1.ConditionFalse, "", "", metav1.Now()) -} - // setServiceInstanceCondition sets a single condition on an Instance's status: if // the condition already exists in the status, it is mutated; if the condition // does not already exist in the status, it is added. Other conditions in the @@ -1147,13 +1132,21 @@ func (c *controller) updateServiceInstanceCondition( // It doesn't send the update request to server. func (c *controller) prepareObservedGeneration(toUpdate *v1beta1.ServiceInstance) { toUpdate.Status.ObservedGeneration = toUpdate.Generation - resetServiceInstanceCondition( + reason := observedNewGenerationReason + message := observedNewGenerationMessage + setServiceInstanceCondition( toUpdate, v1beta1.ServiceInstanceConditionReady, + v1beta1.ConditionFalse, + reason, + message, ) - resetServiceInstanceCondition( + setServiceInstanceCondition( toUpdate, v1beta1.ServiceInstanceConditionFailed, + v1beta1.ConditionFalse, + reason, + message, ) } diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index d3b24776037..059cad8a0cb 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -2176,29 +2176,12 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti } func assertServiceInstanceRequestFailingError(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1beta1.ServiceInstance) { - var ( - readyStatus v1beta1.ConditionStatus - deprovisionStatus v1beta1.ServiceInstanceDeprovisionStatus - ) - switch operation { - case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: - readyStatus = v1beta1.ConditionFalse - deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired - case v1beta1.ServiceInstanceOperationDeprovision: - readyStatus = v1beta1.ConditionUnknown - deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusFailed - } - assertServiceInstanceReadyCondition(t, obj, readyStatus, readyReason) - switch operation { - case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationDeprovision: - assertServiceInstanceCondition(t, obj, v1beta1.ServiceInstanceConditionFailed, v1beta1.ConditionTrue, failureReason) - case v1beta1.ServiceInstanceOperationUpdate: - assertServiceInstanceCondition(t, obj, v1beta1.ServiceInstanceConditionFailed, v1beta1.ConditionTrue, failureReason) - } + assertServiceInstanceReadyCondition(t, obj, v1beta1.ConditionFalse, readyReason) + assertServiceInstanceCondition(t, obj, v1beta1.ServiceInstanceConditionFailed, v1beta1.ConditionTrue, failureReason) assertServiceInstanceOperationStartTimeSet(t, obj, false) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceExternalPropertiesUnchanged(t, obj, originalInstance) - assertServiceInstanceDeprovisionStatus(t, obj, deprovisionStatus) + assertServiceInstanceDeprovisionStatus(t, obj, v1beta1.ServiceInstanceDeprovisionStatusRequired) } func assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1beta1.ServiceInstance) { diff --git a/pkg/openapi/openapi_generated.go b/pkg/openapi/openapi_generated.go index 9e55138ae33..305d58f2971 100644 --- a/pkg/openapi/openapi_generated.go +++ b/pkg/openapi/openapi_generated.go @@ -1525,7 +1525,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Ref: ref("github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1.ServiceInstancePropertiesState"), }, }, - "provisioned": { + "provisionStatus": { SchemaProps: spec.SchemaProps{ Description: "ProvisionStatus describes whether the instance is in the provisioned state.", Type: []string{"string"}, @@ -1540,7 +1540,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, }, }, - Required: []string{"conditions", "asyncOpInProgress", "orphanMitigationInProgress", "reconciledGeneration", "observedGeneration", "provisioned", "deprovisionStatus"}, + Required: []string{"conditions", "asyncOpInProgress", "orphanMitigationInProgress", "reconciledGeneration", "observedGeneration", "provisionStatus", "deprovisionStatus"}, }, }, Dependencies: []string{ diff --git a/test/integration/controller_instance_test.go b/test/integration/controller_instance_test.go index 367d6857368..b565b5db7d3 100644 --- a/test/integration/controller_instance_test.go +++ b/test/integration/controller_instance_test.go @@ -568,7 +568,7 @@ func TestUpdateServiceInstanceChangePlans(t *testing.T) { } ct.instance = updatedInstance - if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Generation); err != nil { + if err := util.WaitForInstanceProcessedGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Generation); err != nil { t.Fatalf("error waiting for instance to reconcile: %v", err) } @@ -841,7 +841,7 @@ func TestUpdateServiceInstanceUpdateParameters(t *testing.T) { } ct.instance = updatedInstance - if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Generation); err != nil { + if err := util.WaitForInstanceProcessedGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Generation); err != nil { t.Fatalf("error waiting for instance to reconcile: %v", err) } @@ -1036,7 +1036,7 @@ func TestCreateServiceInstanceWithProvisionFailure(t *testing.T) { } if tc.expectFailCondition { - if err := util.WaitForInstanceReconciledGeneration(ct.client, ct.instance.Namespace, ct.instance.Name, 1); err != nil { + if err := util.WaitForInstanceProcessedGeneration(ct.client, ct.instance.Namespace, ct.instance.Name, 1); err != nil { t.Fatalf("error waiting for instance reconciliation to complete: %v", err) } } diff --git a/test/integration/controller_test.go b/test/integration/controller_test.go index 7fe502283d8..afeba225593 100644 --- a/test/integration/controller_test.go +++ b/test/integration/controller_test.go @@ -142,7 +142,7 @@ func TestBasicFlows(t *testing.T) { t.Fatalf("error updating Instance: %v", err) } - if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Status.ReconciledGeneration+1); err != nil { + if err := util.WaitForInstanceProcessedGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Status.ReconciledGeneration+1); err != nil { t.Fatalf("error waiting for instance to reconcile: %v", err) } @@ -271,7 +271,7 @@ func TestBasicFlowsWithOriginatingIdentity(t *testing.T) { t.Fatalf("error updating Instance: %v", err) } - if err := util.WaitForInstanceReconciledGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Status.ReconciledGeneration+1); err != nil { + if err := util.WaitForInstanceProcessedGeneration(ct.client, testNamespace, testInstanceName, ct.instance.Status.ReconciledGeneration+1); err != nil { t.Fatalf("error waiting for instance to reconcile: %v", err) } diff --git a/test/util/util.go b/test/util/util.go index 4db3d4e13d8..b75531f2f4e 100644 --- a/test/util/util.go +++ b/test/util/util.go @@ -205,19 +205,20 @@ func WaitForInstanceToNotExist(client v1beta1servicecatalog.ServicecatalogV1beta ) } -// WaitForInstanceReconciledGeneration waits for the status of the named instance to +// WaitForInstanceProcessedGeneration waits for the status of the named instance to // have the specified reconciled generation. -func WaitForInstanceReconciledGeneration(client v1beta1servicecatalog.ServicecatalogV1beta1Interface, namespace, name string, reconciledGeneration int64) error { +func WaitForInstanceProcessedGeneration(client v1beta1servicecatalog.ServicecatalogV1beta1Interface, namespace, name string, processedGeneration int64) error { return wait.PollImmediate(500*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { - glog.V(5).Infof("Waiting for instance %v/%v to have reconciled generation of %v", namespace, name, reconciledGeneration) + glog.V(5).Infof("Waiting for instance %v/%v to have processed generation of %v", namespace, name, processedGeneration) instance, err := client.ServiceInstances(namespace).Get(name, metav1.GetOptions{}) if nil != err { return false, fmt.Errorf("error getting Instance %v/%v: %v", namespace, name, err) } - if instance.Status.ObservedGeneration >= reconciledGeneration && - (isServiceInstanceReady(instance) || isServiceInstanceFailed(instance)) { + if instance.Status.ObservedGeneration >= processedGeneration && + (isServiceInstanceReady(instance) || isServiceInstanceFailed(instance)) && + !instance.Status.OrphanMitigationInProgress { return true, nil } @@ -226,11 +227,11 @@ func WaitForInstanceReconciledGeneration(client v1beta1servicecatalog.Servicecat ) } -// isServiceInstanceReady returns whether the given instance has a ready condition +// isServiceInstanceConditionTrue returns whether the given instance has a given condition // with status true. -func isServiceInstanceReady(instance *v1beta1.ServiceInstance) bool { +func isServiceInstanceConditionTrue(instance *v1beta1.ServiceInstance, conditionType v1beta1.ServiceInstanceConditionType) bool { for _, cond := range instance.Status.Conditions { - if cond.Type == v1beta1.ServiceInstanceConditionReady { + if cond.Type == conditionType { return cond.Status == v1beta1.ConditionTrue } } @@ -238,16 +239,16 @@ func isServiceInstanceReady(instance *v1beta1.ServiceInstance) bool { return false } +// isServiceInstanceReady returns whether the given instance has a ready condition +// with status true. +func isServiceInstanceReady(instance *v1beta1.ServiceInstance) bool { + return isServiceInstanceConditionTrue(instance, v1beta1.ServiceInstanceConditionReady) +} + // isServiceInstanceFailed returns whether the instance has a failed condition with // status true. func isServiceInstanceFailed(instance *v1beta1.ServiceInstance) bool { - for _, condition := range instance.Status.Conditions { - if condition.Type == v1beta1.ServiceInstanceConditionFailed && condition.Status == v1beta1.ConditionTrue { - return true - } - } - - return false + return isServiceInstanceConditionTrue(instance, v1beta1.ServiceInstanceConditionFailed) } // WaitForBindingCondition waits for the status of the named binding to contain From a4389d4a123695eb27b22371fd02b6ec0e4f4948 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 28 Feb 2018 17:37:09 +1100 Subject: [PATCH 20/22] Fix tests (ObservedGeneration for orphan mitigation) --- pkg/controller/controller_test.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 059cad8a0cb..3ac58a3dbda 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -2087,19 +2087,27 @@ func assertServiceInstanceOperationInProgress(t *testing.T, obj runtime.Object, func assertServiceInstanceOperationInProgressWithParameters(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, planName, planID string, inProgressParameters map[string]interface{}, inProgressParametersChecksum string, originalInstance *v1beta1.ServiceInstance) { reason := "" + var expectedObservedGeneration int64 switch operation { case v1beta1.ServiceInstanceOperationProvision: reason = provisioningInFlightReason + expectedObservedGeneration = originalInstance.Generation case v1beta1.ServiceInstanceOperationUpdate: reason = instanceUpdatingInFlightReason + expectedObservedGeneration = originalInstance.Generation case v1beta1.ServiceInstanceOperationDeprovision: reason = deprovisioningInFlightReason + if originalInstance.Status.OrphanMitigationInProgress { + expectedObservedGeneration = originalInstance.Status.ObservedGeneration + } else { + expectedObservedGeneration = originalInstance.Generation + } } assertServiceInstanceReadyFalse(t, obj, reason) assertServiceInstanceCurrentOperation(t, obj, operation) assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) - assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceObservedGeneration(t, obj, expectedObservedGeneration) assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) @@ -2136,28 +2144,36 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti deprovisionStatus v1beta1.ServiceInstanceDeprovisionStatus ) var provisionStatus v1beta1.ServiceInstanceProvisionStatus + var observedGeneration int64 switch operation { case v1beta1.ServiceInstanceOperationProvision: provisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned reason = successProvisionReason readyStatus = v1beta1.ConditionTrue deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired + observedGeneration = originalInstance.Generation case v1beta1.ServiceInstanceOperationUpdate: provisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned reason = successUpdateInstanceReason readyStatus = v1beta1.ConditionTrue deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired + observedGeneration = originalInstance.Generation case v1beta1.ServiceInstanceOperationDeprovision: provisionStatus = v1beta1.ServiceInstanceProvisionStatusNotProvisioned reason = successDeprovisionReason readyStatus = v1beta1.ConditionFalse deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusSucceeded + if originalInstance.Status.OrphanMitigationInProgress { + observedGeneration = originalInstance.Status.ObservedGeneration + } else { + observedGeneration = originalInstance.Generation + } } assertServiceInstanceReadyCondition(t, obj, readyStatus, reason) assertServiceInstanceCurrentOperationClear(t, obj) assertServiceInstanceOperationStartTimeSet(t, obj, false) - assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Generation) - assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceReconciledGeneration(t, obj, observedGeneration) + assertServiceInstanceObservedGeneration(t, obj, observedGeneration) assertServiceInstanceProvisioned(t, obj, provisionStatus) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) From 2af0932a7335053adbf144971f98b65beb79405f Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Wed, 28 Feb 2018 22:20:24 +1100 Subject: [PATCH 21/22] Fix test --- pkg/controller/controller_test.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 3ac58a3dbda..afb287fd965 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -2192,12 +2192,25 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti } func assertServiceInstanceRequestFailingError(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1beta1.ServiceInstance) { - assertServiceInstanceReadyCondition(t, obj, v1beta1.ConditionFalse, readyReason) + var ( + readyStatus v1beta1.ConditionStatus + deprovisionStatus v1beta1.ServiceInstanceDeprovisionStatus + ) + switch operation { + case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: + readyStatus = v1beta1.ConditionFalse + deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired + case v1beta1.ServiceInstanceOperationDeprovision: + readyStatus = v1beta1.ConditionUnknown + deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusFailed + } + + assertServiceInstanceReadyCondition(t, obj, readyStatus, readyReason) assertServiceInstanceCondition(t, obj, v1beta1.ServiceInstanceConditionFailed, v1beta1.ConditionTrue, failureReason) assertServiceInstanceOperationStartTimeSet(t, obj, false) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceExternalPropertiesUnchanged(t, obj, originalInstance) - assertServiceInstanceDeprovisionStatus(t, obj, v1beta1.ServiceInstanceDeprovisionStatusRequired) + assertServiceInstanceDeprovisionStatus(t, obj, deprovisionStatus) } func assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1beta1.ServiceInstance) { From bc770d1f66760f1bbde0822eccf1d3f4274c38ff Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Fri, 2 Mar 2018 11:33:23 +1100 Subject: [PATCH 22/22] Fix review comments --- pkg/apis/servicecatalog/types.go | 5 +++-- pkg/apis/servicecatalog/v1beta1/types.go | 5 +++-- pkg/controller/controller_instance.go | 2 ++ 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/apis/servicecatalog/types.go b/pkg/apis/servicecatalog/types.go index ada7e3a6212..55e8b46754a 100644 --- a/pkg/apis/servicecatalog/types.go +++ b/pkg/apis/servicecatalog/types.go @@ -537,12 +537,13 @@ type ServiceInstanceStatus struct { // ReconciledGeneration is the 'Generation' of the serviceInstanceSpec that // was last processed by the controller. The reconciled generation is updated // even if the controller failed to process the spec. - // Deprecated: use ObservedGeneration instead + // Deprecated: use ObservedGeneration with conditions set to true to find + // whether generation was reconciled. ReconciledGeneration int64 // ObservedGeneration is the 'Generation' of the serviceInstanceSpec that // was last processed by the controller. The observed generation is updated - // whenever the status is updated regardless of operation result + // whenever the status is updated regardless of operation result. ObservedGeneration int64 // OperationStartTime is the time at which the current operation began. diff --git a/pkg/apis/servicecatalog/v1beta1/types.go b/pkg/apis/servicecatalog/v1beta1/types.go index 8db34083254..c8ecc038589 100644 --- a/pkg/apis/servicecatalog/v1beta1/types.go +++ b/pkg/apis/servicecatalog/v1beta1/types.go @@ -601,12 +601,13 @@ type ServiceInstanceStatus struct { // ReconciledGeneration is the 'Generation' of the serviceInstanceSpec that // was last processed by the controller. The reconciled generation is updated // even if the controller failed to process the spec. - // Deprecated: use ObservedGeneration instead + // Deprecated: use ObservedGeneration with conditions set to true to find + // whether generation was reconciled. ReconciledGeneration int64 `json:"reconciledGeneration"` // ObservedGeneration is the 'Generation' of the serviceInstanceSpec that // was last processed by the controller. The observed generation is updated - // whenever the status is updated regardless of operation result + // whenever the status is updated regardless of operation result. ObservedGeneration int64 `json:"observedGeneration"` // OperationStartTime is the time at which the current operation began. diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index c6c959e25d0..0d6bfba678a 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -763,6 +763,8 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro // isServiceInstanceProcessedAlready returns true if there is no further processing // needed for the instance based on ObservedGeneration func isServiceInstanceProcessedAlready(instance *v1beta1.ServiceInstance) bool { + // The observed generation is considered to be reconciled if either of the + // conditions is set to true and there is no orphan mitigation in progress return instance.Status.ObservedGeneration >= instance.Generation && (isServiceInstanceReady(instance) || isServiceInstanceFailed(instance)) && !instance.Status.OrphanMitigationInProgress }