This repository has been archived by the owner on May 6, 2022. It is now read-only.
handle instance deletion that occurs during async provisioning or async update (#1587) #1708
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -358,6 +358,181 @@ 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() | ||
| done := make(chan bool) | ||
|
|
||
| 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 | ||
| select { | ||
| // nonblocking check for a message to finish async update, return inProgress otherwise | ||
| case <-done: | ||
| if tc.updateSucceeds { | ||
| state = osb.StateSucceeded | ||
| } else { | ||
| state = osb.StateFailed | ||
| } | ||
| default: | ||
| } | ||
| 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 { | ||
|
There was a problem hiding this comment. I believe there's a race here between the update completing and the deletion timestamp getting set. Instead of basing the response of the last operation on the number of polls that occurred, I think we should use a boolean ( There was a problem hiding this comment. I concur with @kibbles-n-bytes on this. So that it's clear, the same race exists in the other test. |
||
| t.Fatalf("failed to delete instance: %v", err) | ||
| } | ||
|
|
||
| // notify the thread handling DynamicPollLastOperationReaction that it can end the async op | ||
| done <- true | ||
|
|
||
| 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() | ||
| done := make(chan bool) | ||
|
|
||
| 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 | ||
| select { | ||
| // nonblocking check for a message to finish async provisioning, return inProgress otherwise | ||
| case <-done: | ||
|
There was a problem hiding this comment. There might still be a race here. If a reconciliation starts, and then the deletion timestamp is set, we'd pull I think a better gauge would be if the channel is closed or not. There was a problem hiding this comment. @kibbles-n-bytes, are you ok with instead moving the declaration and initialization of There was a problem hiding this comment. What about using an atomic int instead of a channel at all? I think that makes it clearer what is going on from a readability standpoint. The reaction function reads the atomic int. If the value is 0, then the state is in-progress. Otherwise, the state is succeeded or failed. The run function sets the atomic int to 1 after making the delete. |
||
| if tc.provisionSucceeds { | ||
| state = osb.StateSucceeded | ||
| } else { | ||
| state = osb.StateFailed | ||
| } | ||
| default: | ||
| } | ||
| 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 | ||
| done <- true | ||
|
|
||
| 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) | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a fine workaround for now but I do think that we should consider, as a follow-up to this, storing the generation that the in-progress operation is for, so that when we complete an operation we can set the reconciled generation correctly to the generation that started the operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we should store a correct
reconciledGeneration😀There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jboyd01 leave a TODO then?