Checking for returned errors when updating conditions & finalizers #929

arschles · 2017-06-09T18:57:35Z

This patch adds error checks to all update{Broker,Binding,Instance}Condition calls in the controller, along with tests to ensure that the errors are properly handled.

Fixes #925

arschles · 2017-06-09T19:00:33Z

This is a work in progress. More changes and (likely) some tests to come

arschles · 2017-06-09T23:32:17Z

There are lots more test cases to check for each edge case (every place where update*Condition might return an error) but I'd like to get this change in and do those in a follow-up. I'm taking off the in-progress label to solicit reviews.

cc/ @vaikas-google @pmorie @krancour

MHBauer · 2017-06-09T23:35:10Z

pkg/controller/controller_binding.go

+		); err != nil {
+			glog.Errorf("updating binding condition (%s)", err)
+			return err
+		}
 		c.recorder.Event(binding, api.EventTypeWarning, errorWithOngoingAsyncOperation, s)


do we want to put these events above the binding updates? we'll skip them now.

@MHBauer I left them where they were before. I think it's the right behavior to not emit an event if the update failed, because the state didn't change. Thoughts?

What causes updating the condition in status to fail?

What happens after? Do we try to reconcile again? Eventually it succeeds?

What causes updating the condition in status to fail?

Broken pipe, API server goes down, etc

What happens after? Do we try to reconcile again? Eventually it succeeds?

We periodically re-reconcile everything, yeah. Eventually it succeeds.

MHBauer

I don't understand what's going on enough to lgtm or not.

Overall it seems dangerous somehow. Everything has worked, but we're going to bail at the last second in a bunch of cases because we can't update status. We also don't roll back anything we did. We're not going to end up in an error state on the server, just some weird intermediate one.

I think I'm missing some knowledge.

vaikas · 2017-06-13T10:22:22Z

pkg/controller/controller_binding.go

 			c.recorder.Event(binding, api.EventTypeWarning, errorInjectingBindResultReason, s)
 			return err
 		}
-		c.updateBindingCondition(
+		if err := c.updateBindingCondition(


I'm finding this style here to be harder to read than (IMHO) simpler:
err := c.update...
if err != nil {

But, if others are fine with this style, then so be it :)

vaikas · 2017-06-13T10:23:14Z

pkg/controller/controller_binding.go

 		// Clear the finalizer
 		finalizers.Delete(v1alpha1.FinalizerServiceCatalog)
-		c.updateBindingFinalizers(binding, finalizers.List())
+		if err := c.updateBindingFinalizers(binding, finalizers.List()); err != nil {


So for example here, I think this is more readable using this format because things can fit in a single line :)

vaikas · 2017-06-13T10:25:09Z

pkg/controller/controller_binding.go

 			binding,
 			v1alpha1.BindingConditionReady,
 			v1alpha1.ConditionFalse,
 			errorWithOngoingAsyncOperation,
 			errorWithOngoingAsyncOperationMessage,
-		)
+		); err != nil {
+			glog.Errorf("updating binding condition (%s)", err)


Should we be creating an error event in these failure cases?

I mean error about the update condition failing?

pmorie · 2017-06-14T17:45:18Z

pkg/controller/controller_binding.go

+		); err != nil {
+			glog.Errorf("updating binding condition (%s)", err)
+			return err
+		}
 		c.recorder.Event(binding, api.EventTypeWarning, errorWithOngoingAsyncOperation, s)


What causes updating the condition in status to fail?

Broken pipe, API server goes down, etc

What happens after? Do we try to reconcile again? Eventually it succeeds?

We periodically re-reconcile everything, yeah. Eventually it succeeds.

pmorie · 2017-06-14T17:46:01Z

pkg/controller/controller_binding.go

 					binding,
 					v1alpha1.BindingConditionReady,
 					v1alpha1.ConditionFalse,
 					errorWithParameters,
 					"Error unmarshaling binding parameters. "+s,
-				)
+				); err != nil {
+					glog.Errorf("updating binding condition (%s)", err)


I would prefer to include the text "Error" in the message, even though I am fully aware that the log level says it's an error.

pmorie · 2017-06-14T17:48:57Z

pkg/controller/controller_reconcile_binding_table_test.go

+)
+
+// bindingTestCase represents a single row of a table driven test for reconciling bindings
+type reconcileBindingTestCase struct {


strongly prefer for this to be defined where it is used

Why is this a separate file?

MHBauer · 2017-06-20T22:07:36Z

pkg/controller/controller_binding.go

 				binding,
 				v1alpha1.BindingConditionReady,
 				v1alpha1.ConditionFalse,
 				errorBindCallReason,
-				"Bind call failed. "+s)
+				"Bind call failed. "+s,
+			); err != nil {


note to self, feel free to read and confirm cases : binding creation failed, can't update condition to say binding failed. next time through loop will try to create binding again.

MHBauer · 2017-06-20T22:10:10Z

pkg/controller/controller_binding.go

 			binding,
 			v1alpha1.BindingConditionReady,
 			v1alpha1.ConditionTrue,
 			successInjectedBindResultReason,
 			successInjectedBindResultMessage,
-		)
+		); err != nil {
+			glog.Errorf("updating binding condition (%s)", err)


binding successfully injected, next time through the loop we will try to create and inject a fresh binding?

This seems off to me.

MHBauer · 2017-06-20T22:14:07Z

pkg/controller/controller_binding.go

 				binding,
 				v1alpha1.BindingConditionReady,
 				v1alpha1.ConditionUnknown,
 				errorEjectingBindReason,
 				errorEjectingBindMessage+s,
-			)
+			); err != nil {
+				glog.Errorf("updating binding condition (%s)", err)


if we fail below, do we come back through here and fail to eject the binding? (as it is already ejected?)

How does ConditionUnknown play into this?

MHBauer

Have any of us drawn up a state diagram to show transitions of the conditions between true and false and unknown?

I think some of my concerns are with return statements everywhere which make it difficult to track.

Fixes kubernetes-retired#925

pmorie · 2017-07-21T02:53:25Z

@arschles build failure:

Running gofmt:
*** Please gofmt the following:
pkg/controller/controller_binding_test.go:84:4: expected declaration, found ')'

kibbles-n-bytes · 2017-07-25T21:35:35Z

Also a non-lint error:

[Build & Unit/Integration Tests] pkg/controller/controller_instance.go:549: undefined: toUpdae

n3wscott · 2017-11-13T18:50:25Z

I am going to say this can be closed or needs to be re-written.

staebler · 2017-11-14T03:51:53Z

Agree with @n3wscott. This PR has been obviated by #1149.

pmorie · 2018-01-04T22:37:14Z

Closing since this is obviously stale.

arschles added the in-progress label Jun 9, 2017

arschles added this to the 0.0.10 milestone Jun 9, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2017

arschles changed the title ~~checking for returned errors when updating conditions~~ Checking for returned errors when updating conditions Jun 9, 2017

arschles changed the title ~~Checking for returned errors when updating conditions~~ Checking for returned errors when updating conditions & finalizers Jun 9, 2017

arschles added in-progress and removed in-progress labels Jun 9, 2017

arschles requested review from derekwaynecarr, pmorie and vaikas June 9, 2017 19:21

arschles removed the in-progress label Jun 9, 2017

MHBauer reviewed Jun 9, 2017

View reviewed changes

vaikas reviewed Jun 13, 2017

View reviewed changes

pmorie reviewed Jun 14, 2017

View reviewed changes

pmorie modified the milestones: 0.0.11, 0.0.10 Jun 14, 2017

arschles added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2017

MHBauer reviewed Jun 20, 2017

View reviewed changes

arschles modified the milestones: 0.0.11, 0.0.12 Jun 21, 2017

checking for returned errors when updating conditions

1f2f660

Fixes kubernetes-retired#925

arschles force-pushed the update-errs branch from 1b21ab6 to 1f2f660 Compare July 10, 2017 23:30

arschles removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2017

duglin modified the milestones: 0.0.13, 0.0.14 Jul 13, 2017

duglin modified the milestones: 0.0.14, 0.0.15 Jul 27, 2017

MHBauer added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2017

duglin modified the milestones: 0.0.15, 0.0.16 Aug 10, 2017

duglin modified the milestones: 0.0.16, 0.0.17 Aug 17, 2017

duglin modified the milestones: 0.0.17, 0.0.18 Aug 31, 2017

pmorie closed this Jan 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checking for returned errors when updating conditions & finalizers #929

Checking for returned errors when updating conditions & finalizers #929

arschles commented Jun 9, 2017 •

edited

Loading

arschles commented Jun 9, 2017

arschles commented Jun 9, 2017

MHBauer Jun 9, 2017

arschles Jun 9, 2017

MHBauer Jun 12, 2017

pmorie Jun 14, 2017

MHBauer left a comment

vaikas Jun 13, 2017

vaikas Jun 13, 2017

vaikas Jun 13, 2017

vaikas Jun 13, 2017

pmorie Jun 14, 2017

pmorie Jun 14, 2017

pmorie Jun 14, 2017

MHBauer Jun 20, 2017

MHBauer Jun 20, 2017

MHBauer Jun 20, 2017

MHBauer Jun 20, 2017

MHBauer left a comment

pmorie commented Jul 21, 2017

kibbles-n-bytes commented Jul 25, 2017

n3wscott commented Nov 13, 2017

staebler commented Nov 14, 2017

pmorie commented Jan 4, 2018

Checking for returned errors when updating conditions & finalizers #929

Checking for returned errors when updating conditions & finalizers #929

Conversation

arschles commented Jun 9, 2017 • edited Loading

arschles commented Jun 9, 2017

arschles commented Jun 9, 2017

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

MHBauer 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

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

MHBauer left a comment

Choose a reason for hiding this comment

pmorie commented Jul 21, 2017

kibbles-n-bytes commented Jul 25, 2017

n3wscott commented Nov 13, 2017

staebler commented Nov 14, 2017

pmorie commented Jan 4, 2018

arschles commented Jun 9, 2017 •

edited

Loading