Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Commit

Permalink
handle instance deletion that occurs during async provisioning or asy…
Browse files Browse the repository at this point in the history 
…nc update (#1587) (#1708)

* handle instance deletion that occurs during async provisioning (#1587)

* updated processUpdateServiceInstanceFailure(), processUpdateServiceInstanceSuccess (), and processProvisionFailure(), corrected and supplemented test cases.

* added/refined tests for delete during async instance operations

* reworked termination of async operation to avoid potential race condition

* fix race condition with a channel

* use atomic int for signaling end of async operation

* add TODO and refactor common code
  • Loading branch information
@kibbles-n-bytes
Jay Boyd authored and kibbles-n-bytes committed Feb 15, 2018
1 parent 3032f01 commit cada49c
Show file tree
Hide file tree
Showing 2 changed files with 189 additions and 0 deletions.
19 changes: 19 additions & 0 deletions pkg/controller/controller_instance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1169,6 +1169,17 @@ func clearServiceInstanceCurrentOperation(toUpdate *v1beta1.ServiceInstance) {
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
// bindings associated with the given ServiceInstance.
func (c *controller) checkServiceInstanceHasExistingBindings(instance *v1beta1.ServiceInstance) error {
Expand Down Expand Up @@ -1434,7 +1445,9 @@ func (c *controller) processProvisionSuccess(instance *v1beta1.ServiceInstance,
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)

if _, err := c.updateServiceInstanceStatus(instance); err != nil {
return err
Expand All @@ -1447,6 +1460,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 {
currentReconciledGeneration := instance.Status.ReconciledGeneration
if failedCond == nil {
return fmt.Errorf("failedCond must not be nil")
}
Expand Down Expand Up @@ -1474,6 +1488,7 @@ func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance,
err = fmt.Errorf(failedCond.Message)
} else {
clearServiceInstanceCurrentOperation(instance)
rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration)
}

if _, err := c.updateServiceInstanceStatus(instance); err != nil {
Expand Down Expand Up @@ -1505,7 +1520,9 @@ 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)

if _, err := c.updateServiceInstanceStatus(instance); err != nil {
return err
Expand All @@ -1519,9 +1536,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 *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)
clearServiceInstanceCurrentOperation(instance)
rollbackReconciledGenerationOnDeletion(instance, currentReconciledGeneration)

if _, err := c.updateServiceInstanceStatus(instance); err != nil {
return err
Expand Down
170 changes: 170 additions & 0 deletions test/integration/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"encoding/json"
"fmt"
"net/http"
"sync/atomic"
"testing"
"time"

Expand Down Expand Up @@ -358,6 +359,175 @@ func TestAsyncProvisionWithMultiplePolls(t *testing.T) {
})
}

// TestServiceInstanceDeleteWithAsyncUpdateInProgress tests that you can delete
// an instance during an async update. That is, if you request a delete during
// an instance update, the instance will be deleted when the update completes
// regardless of success or failure.
func TestServiceInstanceDeleteWithAsyncUpdateInProgress(t *testing.T) {

cases := []struct {
name string
updateSucceeds bool
}{
{
name: "update succeeds",
updateSucceeds: true,
},
{
name: "update fails",
updateSucceeds: false,
},
}

for _, tc := range cases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
var done int32 = 0
ct := controllerTest{
t: t,
broker: getTestBroker(),
instance: getTestInstance(),
skipVerifyingInstanceSuccess: false,
setup: func(ct *controllerTest) {
ct.osbClient.UpdateInstanceReaction.(*fakeosb.UpdateInstanceReaction).Response.Async = true
ct.osbClient.PollLastOperationReaction = fakeosb.DynamicPollLastOperationReaction(
func(_ *osb.LastOperationRequest) (*osb.LastOperationResponse, error) {
state := osb.StateInProgress
d := atomic.LoadInt32(&done)
if d > 0 {
if tc.updateSucceeds {
state = osb.StateSucceeded
} else {
state = osb.StateFailed
}
}
return &osb.LastOperationResponse{State: state}, nil
})
},
}
ct.run(func(ct *controllerTest) {

if err := util.WaitForInstanceCondition(ct.client, ct.instance.Namespace, ct.instance.Name,
v1beta1.ServiceInstanceCondition{
Type: v1beta1.ServiceInstanceConditionReady,
Status: v1beta1.ConditionTrue,
Reason: "ProvisionedSuccessfully",
}); err != nil {
t.Fatalf("error waiting for instance to be ready: %v", err)
}

// add a parameter to the instance for the Update()
ct.instance.Spec.Parameters = convertParametersIntoRawExtension(t,
map[string]interface{}{
"param-key": "new-param-value",
})

_, err := ct.client.ServiceInstances(testNamespace).Update(ct.instance)
if err != nil {
t.Fatalf("error updating instance: %v", err)
}

if err := util.WaitForInstanceCondition(ct.client, ct.instance.Namespace, ct.instance.Name,
v1beta1.ServiceInstanceCondition{
Type: v1beta1.ServiceInstanceConditionReady,
Status: v1beta1.ConditionFalse,
Reason: "UpdatingInstance",
}); err != nil {
t.Fatalf("error waiting for instance to be updating asynchronously: %v", err)
}

if err := ct.client.ServiceInstances(ct.instance.Namespace).Delete(ct.instance.Name, &metav1.DeleteOptions{}); err != nil {
t.Fatalf("failed to delete instance: %v", err)
}

// notify the thread handling DynamicPollLastOperationReaction that it can end the async op
atomic.StoreInt32(&done, 1)

if err := util.WaitForInstanceToNotExist(ct.client, ct.instance.Namespace, ct.instance.Name); err != nil {
t.Fatalf("error waiting for instance to not exist: %v", err)
}

// We deleted the instance above, clear it so test cleanup doesn't fail
ct.instance = nil
})
})
}
}

// TestServiceInstanceDeleteWithAsyncProvisionInProgress tests that you can
// delete an instance during an async provision. Verify the instance is deleted
// when the provisioning completes regardless of success or failure.
func TestServiceInstanceDeleteWithAsyncProvisionInProgress(t *testing.T) {
cases := []struct {
name string
provisionSucceeds bool
}{
{
name: "provision succeeds",
provisionSucceeds: true,
},
{
name: "provision fails",
provisionSucceeds: false,
},
}

for _, tc := range cases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
var done int32 = 0
ct := controllerTest{
t: t,
broker: getTestBroker(),
instance: getTestInstance(),
skipVerifyingInstanceSuccess: true,
setup: func(ct *controllerTest) {
ct.osbClient.ProvisionReaction.(*fakeosb.ProvisionReaction).Response.Async = true
ct.osbClient.PollLastOperationReaction = fakeosb.DynamicPollLastOperationReaction(
func(_ *osb.LastOperationRequest) (*osb.LastOperationResponse, error) {
state := osb.StateInProgress
d := atomic.LoadInt32(&done)
if d > 0 {
if tc.provisionSucceeds {
state = osb.StateSucceeded
} else {
state = osb.StateFailed
}
}
return &osb.LastOperationResponse{State: state}, nil
})
},
}
ct.run(func(ct *controllerTest) {
if err := util.WaitForInstanceCondition(ct.client, ct.instance.Namespace, ct.instance.Name,
v1beta1.ServiceInstanceCondition{
Type: v1beta1.ServiceInstanceConditionReady,
Status: v1beta1.ConditionFalse,
Reason: "Provisioning",
}); err != nil {
t.Fatalf("error waiting for instance to be provisioning asynchronously: %v", err)
}

if err := ct.client.ServiceInstances(ct.instance.Namespace).Delete(ct.instance.Name, &metav1.DeleteOptions{}); err != nil {
t.Fatalf("failed to delete instance: %v", err)
}

// notify the thread handling DynamicPollLastOperationReaction that it can end the async op
atomic.StoreInt32(&done, 1)

if err := util.WaitForInstanceToNotExist(ct.client, ct.instance.Namespace, ct.instance.Name); err != nil {
t.Fatalf("error waiting for instance to not exist: %v", err)
}

// We deleted the instance above, clear it so test cleanup doesn't fail
ct.instance = nil
})
})
}
}

func getUpdateInstanceResponseByPollCountReactions(numOfResponses int, stateProgressions []fakeosb.UpdateInstanceReaction) fakeosb.DynamicUpdateInstanceReaction {
numberOfPolls := 0
numberOfStates := len(stateProgressions)
Expand Down

0 comments on commit cada49c

Please sign in to comment.