handle instance deletion that occurs during async provisioning or async update (#1587) #1708

jboyd01 · 2018-01-31T13:38:50Z

partially fixes #1587 (this only covers deleting an Instance during provisioning or update, it does not deal with async service bindings). I'll do the bindings in a follow up.

Don't bump the ReconciledGeneration after async provisioning or update of an instance if there is a pending deletion.

staebler · 2018-01-31T15:36:52Z

@jboyd01 Don't we need to do the same thing for async updating?

staebler · 2018-01-31T15:41:37Z

test/integration/controller_test.go

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


You cannot change the OSB client reactions while there are reconciliations that could be running. In general, don't change the reactions outside of setup. If the response needs to be dynamic, use a DynamicPollLastOperationReaction.

jboyd01 · 2018-02-01T14:06:44Z

@staebler thanks for the review and slack discussion. I agree with your points, I've got some updates here, but more is needed. Marking as a work in progress

pmorie

Apparently there is no way to delete a review; I'm changing this comment because I reviewed the PR thinking it was supposed to do something else during a sleep-deprived state.

…netes-retired#1587)

…stanceSuccess (), and processProvisionFailure(), corrected and supplemented test cases.

pmorie

LGTM, with the following reservations:

We should consider in the future whether to store the generation an operation is for in the status
We also need this issue to be fixed for async binding
When we fix the issue for async binding, we probably ought to just store the in-progress generation, since async binding is alpha anyway
We should determine if we begin saving the in-progress generation for instances, whether a data migration is required

pmorie · 2018-02-13T21:55:42Z

pkg/controller/controller_instance.go

 	clearServiceInstanceCurrentOperation(instance)

+	if instance.DeletionTimestamp != nil {


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.

Agreed, we should store a correct reconciledGeneration 😀

@jboyd01 leave a TODO then?

kibbles-n-bytes · 2018-02-14T17:40:44Z

test/integration/controller_test.go

+					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 {


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 (shouldFinishPolling?) that we toggle from false to true after issuing the delete request.

I concur with @kibbles-n-bytes on this. So that it's clear, the same race exists in the other test.

staebler

Generally looks good, with the same reservations as @pmorie and @kibbles-n-bytes from their reviews.

staebler · 2018-02-14T18:22:22Z

test/integration/controller_test.go

+					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 {


I concur with @kibbles-n-bytes on this. So that it's clear, the same race exists in the other test.

…tion

jboyd01 · 2018-02-14T19:05:46Z

Thanks for the reviews. Good point and that makes a much better test @kibbles-n-bytes, @staebler, thanks. I've updated both tests.

kibbles-n-bytes · 2018-02-14T21:27:58Z

test/integration/controller_test.go

+							state := osb.StateInProgress
+							select {
+							// nonblocking check for a message to finish async provisioning, return inProgress otherwise
+							case <-done:


There might still be a race here. If a reconciliation starts, and then the deletion timestamp is set, we'd pull true out of done but then fail to update the instance (as our resource is out of date). The next reconciliation loop, we'd then be back to the channel being empty.

I think a better gauge would be if the channel is closed or not.

@kibbles-n-bytes, are you ok with instead moving the declaration and initialization of state := osb.StateInProgress up to the top of the Run() ? This way reading from the channel toggles us to a done state and we stay there for any additional polls.

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.

nilebox · 2018-02-15T00:19:52Z

pkg/controller/controller_instance.go

 	clearServiceInstanceCurrentOperation(instance)

+	if instance.DeletionTimestamp != nil {


Agreed, we should store a correct reconciledGeneration 😀

nilebox · 2018-02-15T00:20:33Z

pkg/controller/controller_instance.go

 	clearServiceInstanceCurrentOperation(instance)

+	if instance.DeletionTimestamp != nil {


@jboyd01 leave a TODO then?

nilebox · 2018-02-15T00:22:20Z

pkg/controller/controller_instance.go

@@ -1474,6 +1483,13 @@ func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance,
 		err = fmt.Errorf(failedCond.Message)
 	} else {
 		clearServiceInstanceCurrentOperation(instance)
+
+		if instance.DeletionTimestamp != nil {


could you extract this code to a separate method instead of copy-pasting? it will make a cleanup in a follow-up PR much easier.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 31, 2018

jboyd01 force-pushed the async-instance-prov-with-delete branch from 06ef7d2 to dcb55e1 Compare January 31, 2018 14:23

staebler reviewed Jan 31, 2018

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 1, 2018

jboyd01 changed the title ~~handle instance deletion that occurs during async provisioning (#1587)~~ (WIP) handle instance deletion that occurs during async provisioning (#1587) Feb 1, 2018

jboyd01 added the do-not-merge label Feb 1, 2018

pmorie reviewed Feb 5, 2018

View reviewed changes

Jay Boyd added 2 commits February 12, 2018 10:12

handle instance deletion that occurs during async provisioning (kuber…

20d3e80

…netes-retired#1587)

updated processUpdateServiceInstanceFailure(), processUpdateServiceIn…

9931835

…stanceSuccess (), and processProvisionFailure(), corrected and supplemented test cases.

jboyd01 force-pushed the async-instance-prov-with-delete branch from 7c92aa7 to c59d259 Compare February 12, 2018 19:54

added/refined tests for delete during async instance operations

e30c78c

jboyd01 force-pushed the async-instance-prov-with-delete branch from c59d259 to e30c78c Compare February 13, 2018 20:26

jboyd01 changed the title ~~(WIP) handle instance deletion that occurs during async provisioning (#1587)~~ handle instance deletion that occurs during async provisioning or async update (#1587) Feb 13, 2018

pmorie reviewed Feb 13, 2018

View reviewed changes

pmorie added LGTM1 and removed do-not-merge labels Feb 13, 2018

kibbles-n-bytes reviewed Feb 14, 2018

View reviewed changes

staebler reviewed Feb 14, 2018

View reviewed changes

reworked termination of async operation to avoid potential race condi…

e4bded0

…tion

fix race condition with a channel

1ec48af

kibbles-n-bytes reviewed Feb 14, 2018

View reviewed changes

use atomic int for signaling end of async operation

1d5a022

nilebox reviewed Feb 15, 2018

View reviewed changes

kibbles-n-bytes added this to the 0.1.8 milestone Feb 15, 2018

add TODO and refactor common code

4754197

nilebox added the LGTM2 label Feb 15, 2018

kibbles-n-bytes merged commit cada49c into kubernetes-retired:master Feb 15, 2018

jboyd01 mentioned this pull request Feb 16, 2018

Deleting a ServiceInstance or ServiceBinding while async operation in progress fails #1587

Closed

kibbles-n-bytes mentioned this pull request Feb 23, 2018

handle binding deletion that occurs during async bind #1760

Merged

jboyd01 mentioned this pull request Mar 5, 2018

Deleting projects is leaving projects on zombie state openshift/origin#18125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle instance deletion that occurs during async provisioning or async update (#1587) #1708

handle instance deletion that occurs during async provisioning or async update (#1587) #1708

jboyd01 commented Jan 31, 2018 •

edited

staebler commented Jan 31, 2018

staebler Jan 31, 2018

jboyd01 commented Feb 1, 2018

pmorie left a comment •

edited

pmorie left a comment

pmorie Feb 13, 2018

nilebox Feb 15, 2018

nilebox Feb 15, 2018

kibbles-n-bytes Feb 14, 2018

staebler Feb 14, 2018

staebler left a comment

staebler Feb 14, 2018

jboyd01 commented Feb 14, 2018

kibbles-n-bytes Feb 14, 2018

jboyd01 Feb 14, 2018

staebler Feb 14, 2018 •

edited

nilebox Feb 15, 2018

nilebox Feb 15, 2018

nilebox Feb 15, 2018

		clearServiceInstanceCurrentOperation(instance)

		if instance.DeletionTimestamp != nil {

handle instance deletion that occurs during async provisioning or async update (#1587) #1708

handle instance deletion that occurs during async provisioning or async update (#1587) #1708

Conversation

jboyd01 commented Jan 31, 2018 • edited

staebler commented Jan 31, 2018

Choose a reason for hiding this comment

jboyd01 commented Feb 1, 2018

pmorie left a comment • edited

Choose a reason for hiding this comment

pmorie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staebler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jboyd01 commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staebler Feb 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jboyd01 commented Jan 31, 2018 •

edited

pmorie left a comment •

edited

staebler Feb 14, 2018 •

edited