Add asynchronous provisioning test using the fake broker server #923

arschles · 2017-06-08T19:14:13Z

Overview

This patch creates a new test at ./test/integration/controller_instance_test.go, which exercises the controller's ReconcileInstance function against fake clients for everything except for a broker API. This test ensures that, if the broker API returns an HTTP response with a 500 error code, the controller will write the results of that response into the Instance's conditions.

Refactors

The new test borrows a lot of logic from the existing controller unit tests, but it lives alongside the integration tests. I've done quite a bit of refactoring to enable this test to use functions that were previously only available to unit tests. Note that I've done the least amount of refactoring I had to, but there are still quite a few changes here. The result is that the unit test code is slightly more factored and reusable as well.

Follow-ups

While this PR only adds a single additional test, I plan to do several further follow-ups to add several additional tests that would best be written as this type of integration test (such as #984 and #985). The result of those additions will be further (but smaller) refactors and, of course, additional test coverage.

Related Issues

See #914 for details on motivations behind this test.

Partially addresses #914
Requires #928

vaikas · 2017-06-08T22:01:48Z

pkg/controller/controller_instance_test.go

+	sharedInformers.Brokers().Informer().GetStore().Add(getTestBroker())
+	sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass())
+
+	// Specify we want asynchronous provisioning...


This comment seems wrong?

vaikas · 2017-06-08T22:10:43Z

pkg/controller/controller_instance.go

@@ -208,7 +208,8 @@ func (c *controller) reconcileInstance(instance *v1alpha1.Instance) error {
 				return fmt.Errorf("Couldn't create a key for object %+v: %v", instance, err)
 			}
 			c.pollingQueue.Add(key)
-		} else {
+		} else if respCode == http.StatusCreated || respCode == http.StatusOK {


I'm a little perplexed that this is necessary because above L163 any error should be handled and these are the only cases that return a nil error so I'm just trying to understand why this change is necessary.

https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/brokerapi/openservicebroker/open_service_broker_client.go#L165

@vaikas-google good catch. The test code is using the fake broker client instead of the real one, and the fake doesn't check for response codes like the real one does.

I'd like to keep the test intact, so I'm going to see how hard it is to retrofit a fake broker server so we can use the real client here.

derekwaynecarr · 2017-06-09T13:48:35Z

pkg/controller/controller_instance.go

+		} else {
+			// the broker returned a failure response
+			errorProvisionCalledMessage := fmt.Sprintf("provision call failed")
+			c.updateInstanceCondition(


fwiw, i notice a consistent pattern where we ignore any error that can be returned from this.

i think we should either return, or at minimum log the error.

@derekwaynecarr good catch. I'll log and return on these changed portions, and I've created #925 as a follow-up

arschles · 2017-06-09T17:24:26Z

@pmorie @vaikas-google I've put this PR back into in-progress, I may have thought about this wrong. I'll ping you for re-review if I don't end up closing this completely.

This is a continuation of kubernetes-retired#533, and is a pre-requisite for kubernetes-retired#923

arschles · 2017-06-09T19:11:44Z

Update: I've opened #928, which implements a fake broker server and refactors a single test to use the real OSB client against the fake broker server. After that is merged, I'll convert the test herein to use the real client and the fake broker server. Given that I've seen this bug in the wild, I'm hoping the test as-written will show the buggy behavior (although I agree with you @vaikas-google that I don't see how the OSB API client could miss any status codes). If it doesn't, it'll be back to the drawing board to expose this behavior.

* Adding a fake broker server This is a continuation of #533, and is a pre-requisite for #923 * converting a single test to use the broker server * adding boilerplate and docs * naming the pivotal brokerapi package to eliminate possible ambiguity * adding clarifying comment to the catalog requests var * adding better godocs

MHBauer

More detail about why the broker is unsupported.

Non conforming with osbapi how?

MHBauer · 2017-06-20T23:35:07Z

pkg/controller/controller_instance_test.go

@@ -558,6 +559,78 @@ func TestReconcileInstanceAsynchronousNoOperation(t *testing.T) {
 	assertInstanceLastOperation(t, updatedInstance, "")
 }

+// TestReconcileInstanceAsynchronousUnsupportedBrokerError tests to ensure that, on an asynchronous
+// provision, an Instance's conditions get set with a Broker failure that is not OSB API spec


yay, test comment!

vaikas · 2017-06-20T23:38:03Z

pkg/controller/controller_instance_test.go

+
+	// The item should not have been added to the polling queue for later processing
+	if testController.pollingQueue.Len() != 0 {
+		t.Fatalf("Expected the asynchronous instance to end up in the polling queue")


This log message seems wrong?

arschles · 2017-06-20T23:46:07Z

@MHBauer I made the test comment more precise and added detail regarding the OSB spec. I think it will answer your question.

MHBauer · 2017-06-20T23:54:26Z

pkg/controller/controller_instance_test.go

+	sharedInformers.Brokers().Informer().GetStore().Add(getTestBroker())
+	sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass())
+
+	// Specify an error from the instance client


is this the un'expected' error? can you add that word to match the comment above?

@MHBauer just added

MHBauer

@arschles thanks for clarifying.

lgtm

pmorie · 2017-06-23T14:58:46Z

@arschles looks like this needs rebase

arschles · 2017-06-23T19:58:16Z

@pmorie rebase done, and adaptations made to use the new OSB client that the rebase pulled in. PTAL

pmorie

A couple small suggestions

pmorie · 2017-06-26T16:49:03Z

pkg/controller/controller_instance_test.go

+
+	fakeBrokerServerHandler.Catalog = fakebrokerserver.ConvertCatalog(getTestCatalog())
+
+	fakeKubeClient.AddReactor("get", "namespaces", func(action clientgotesting.Action) (bool, runtime.Object, error) {


I put in a litle macro for this

pmorie · 2017-06-26T16:51:40Z

pkg/controller/controller_instance_test.go

+	// verify that 2 kubernetes actions occurred - a GET to the ServiceClass, then a GET to the
+	// Broker
+	kubeActions := fakeKubeClient.Actions()
+	if e, a := 2, len(kubeActions); e != a {


you can use assertNumberOfActions here too

MHBauer · 2017-06-26T22:37:40Z

pkg/apis/servicecatalog/fake/const.go

+	// InstanceName is a name used for test instances
+	InstanceName = "test-instance"
+	// InstanceGUID is the GUID used for test instances
+	InstanceGUID = "IGUID"


I think all of these should have some indication in the name that they are the Test constants.

@MHBauer I argue that they already do when used externally to this package. Recall that the package name will come first, so this specific constant will be referenced as fake.InstanceGUID

Agree that the name of the package carries the meaning. I suggest we call this package 'testfixtures', though, since the code here is not really a 'fake'.

MHBauer · 2017-06-26T22:38:33Z

pkg/apis/servicecatalog/fake/pointers.go

+package fake
+
+func boolPtr(b bool) *bool {
+	return &b


Why do these functions exist?

various fields need to store *bools, and this is a convenience function to create them with a one-liner

Having never actually tried it, I tried it, and apparently you can't take the address of a bool. Bizzare language.

Would it be better to have it once in a toplevel util package than rewriting this in a multiple places.

I have seen truePtr and falsePtr around somewhere.

yes, that would be nice, but out of the scope of this PR

Oh, those are hanging around. We'll probably want to move them here naturally at some point.

pmorie

Very much like where you're headed with this, a couple minor suggestions. TLDR: let's run a whole controller and drive this from informers and waiting on conditions.

pmorie · 2017-06-27T06:27:32Z

pkg/apis/servicecatalog/fake/const.go

+limitations under the License.
+*/
+
+package fake


+1000 to where you're going here, see #514

pmorie · 2017-06-27T06:28:39Z

pkg/apis/servicecatalog/fake/const.go

+	// InstanceName is a name used for test instances
+	InstanceName = "test-instance"
+	// InstanceGUID is the GUID used for test instances
+	InstanceGUID = "IGUID"


Agree that the name of the package carries the meaning. I suggest we call this package 'testfixtures', though, since the code here is not really a 'fake'.

pmorie · 2017-06-27T06:29:04Z

pkg/apis/servicecatalog/fake/pointers.go

+package fake
+
+func boolPtr(b bool) *bool {
+	return &b


Oh, those are hanging around. We'll probably want to move them here naturally at some point.

pmorie · 2017-06-27T06:31:18Z

pkg/controller/status.go

+
+// AssertInstanceReadyTrue ensures that obj is an instance and has a ready condition of True
+// on it
+func AssertInstanceReadyTrue(t *testing.T, obj runtime.Object) {


Do we need to export these? The integration tests should wait for conditions to converge on an expected value, rather than making single assertions.

pmorie · 2017-06-27T06:32:55Z

test/integration/controller_instance_test.go

+	if testController.PollingQueueLen() != 0 {
+		t.Fatalf("Expected polling queue to be empty")
+	}
+	controller.AssertAsyncOpInProgressFalse(t, updatedInstance)


Let's run the controller similar to the other integration tests so that are testing the behavior of a running controller instead of exercising a specific (and very clean) ordering of methods in a test harness

duglin · 2017-07-07T14:10:51Z

rebase needed

pmorie · 2017-07-21T02:50:20Z

@arschles are you planning to rebase this and land it?

n3wscott · 2017-11-13T18:55:16Z

This work looks good but might need to be broken into a few smaller PRs to be able to integrate today.

pmorie · 2018-01-04T22:36:48Z

@arschles can this be closed?

nilebox · 2018-02-19T08:39:37Z

Closing as inactive. @arschles feel free to reopen if needed.

arschles added the in-progress label Jun 8, 2017

arschles added this to the 0.0.10 milestone Jun 8, 2017

arschles requested review from pmorie and derekwaynecarr June 8, 2017 19:14

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

arschles changed the title ~~Making the controller handle unexpected provision results~~ Making the controller handle unexpected provision response codes Jun 8, 2017

arschles removed the in-progress label Jun 8, 2017

vaikas reviewed Jun 8, 2017

View reviewed changes

arschles mentioned this pull request Jun 8, 2017

Instance never provisioned should not invoke broker #891

Merged

derekwaynecarr reviewed Jun 9, 2017

View reviewed changes

arschles added the in-progress label Jun 9, 2017

arschles added a commit to arschles/kubernetes-service-catalog that referenced this pull request Jun 9, 2017

Adding a fake broker server

bdda355

This is a continuation of kubernetes-retired#533, and is a pre-requisite for kubernetes-retired#923

arschles mentioned this pull request Jun 9, 2017

Adding a fake broker server #928

Merged

arschles force-pushed the unexpected-code branch from 5651ee8 to 17a1e1e Compare June 13, 2017 21:30

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

arschles force-pushed the unexpected-code branch 2 times, most recently from e85808e to 336a983 Compare June 20, 2017 20:28

arschles changed the title ~~Making the controller handle unexpected provision response codes~~ Add asynchronous provisioning test using the fake broker server Jun 20, 2017

arschles removed in-progress needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 20, 2017

MHBauer suggested changes Jun 20, 2017

View reviewed changes

vaikas reviewed Jun 20, 2017

View reviewed changes

MHBauer reviewed Jun 20, 2017

View reviewed changes

MHBauer approved these changes Jun 20, 2017

View reviewed changes

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

vaikas added the LGTM2 label Jun 22, 2017

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

arschles force-pushed the unexpected-code branch 2 times, most recently from 47dd389 to f00029e Compare June 23, 2017 19:34

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

arschles force-pushed the unexpected-code branch from 59cfd91 to 6852321 Compare June 26, 2017 16:50

pmorie reviewed Jun 26, 2017

View reviewed changes

arschles removed LGTM1 LGTM2 labels Jun 26, 2017

MHBauer reviewed Jun 26, 2017

View reviewed changes

pmorie reviewed Jun 27, 2017

View reviewed changes

duglin added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2017

arschles force-pushed the unexpected-code branch from 93c6fdf to 8f6dc37 Compare July 10, 2017 23:32

Add asynchronous provisioning test using the fake broker server

ee89698

arschles force-pushed the unexpected-code branch from 8f6dc37 to ee89698 Compare July 10, 2017 23:48

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

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

nilebox closed this Feb 19, 2018


		fakeBrokerServerHandler.Catalog = fakebrokerserver.ConvertCatalog(getTestCatalog())

		fakeKubeClient.AddReactor("get", "namespaces", func(action clientgotesting.Action) (bool, runtime.Object, error) {

Add asynchronous provisioning test using the fake broker server #923

Add asynchronous provisioning test using the fake broker server #923

Conversation

arschles commented Jun 8, 2017 • edited Loading

Overview

Refactors

Follow-ups

Related Issues

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

arschles commented Jun 9, 2017

arschles commented Jun 9, 2017

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

arschles commented Jun 20, 2017

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 Jun 23, 2017

arschles commented Jun 23, 2017

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

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

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

duglin commented Jul 7, 2017

pmorie commented Jul 21, 2017

n3wscott commented Nov 13, 2017

pmorie commented Jan 4, 2018

nilebox commented Feb 19, 2018

arschles commented Jun 8, 2017 •

edited

Loading