e2e explicit plan check #1261

MHBauer · 2017-09-22T01:36:35Z

This PR is not outdated or obsolete. WaitForClusterServicePlanToExist was implemented a second time in the period between now and submission, but it was never used in the e2e tests.

pmorie · 2017-09-22T14:57:42Z

test/e2e/walkthrough.go

 		var (
 			brokerName       = upsbrokername
 			serviceclassName = "user-provided-service"
+			serviceplanName  = coreutil.ConstructPlanName("default", "86064792-7ea2-467b-af93-ac9694d96d52")


Is this a dependency on the content of the ups-broker binary?

Yes, it's hardcoded in the UPS.

pmorie · 2017-10-03T01:23:56Z

I think this is actually obsolete now - correct me if I'm wrong.

MHBauer · 2017-10-19T08:46:26Z

@pmorie This does not look obsolete to me. I see no check for plans in the current walkthrough as it exists. I will rebase.

staebler · 2017-11-21T15:14:44Z

Why do we want the e2e test to wait for the plan to exist prior to creating the instance? From the perspective of service catalog, it is perfectly acceptable to create the instance before the plan is created. Are we adding the wait to the e2e test to make the e2e test more deterministic?

arschles · 2017-11-21T22:57:39Z

It seems like the test wants to make sure plans are actually written down, and not necessarily to make the test more deterministic?

staebler · 2017-11-22T01:35:55Z

Fair enough. Is it worthwhile to check for both classes and all three plans, now that there are multiples of each? Or is it sufficient to verify that it is being done correctly for a single one of each?

staebler · 2017-11-22T01:36:50Z

test/e2e/walkthrough.go

@@ -113,6 +113,10 @@ var _ = framework.ServiceCatalogDescribe("walkthrough", func() {
 		err = util.WaitForClusterServiceClassToExist(f.ServiceCatalogClientSet.ServicecatalogV1beta1(), serviceclassID)
 		Expect(err).NotTo(HaveOccurred(), "failed to wait serviceclass to be ready")

+		By("Waiting for ClusterServicePlan to be ready")
+		err = util.WaitForClusterServicePlanToExist(f.ServiceCatalogClientSet.ServicecatalogV1beta1(), serviceplanID)
+		Expect(err).ShouldNot(HaveOccurred(), "serviceplan never became ready")


The Gomega documentation recommends NotTo instead of ShouldNot when using Expect. That would keep it consistent with the rest of the test as well.

Link to that recommendation? I was choosing the thing that 'read better' when I was looking at it. From what i could tell they were the exact same function.

https://github.com/onsi/gomega/blob/1eecca0ba8e6f5ea5a431ce21d3aee2af0b4c90b/gomega_dsl.go#L288-L289

MHBauer · 2017-11-22T16:11:54Z

The order was not important beyond making sure all objects were fully there before moving on.

I'll have to go back when looking through the rebase. WaitForClusterServicePlanToExist seems to exist now, but I don't see it being used?

I would think we do need the plans to be extant before creating the instances. We could get rejected. If that is true, where does it stop? Why check for classes? or Brokers? Sounds like we could simplify the tests a whole lot if we don't need to check for anything.

I'll look into the multiples of classes and plans thing. The growth of the situation may mean more progress checking needs to be done.

staebler · 2017-11-22T17:15:22Z

I would think we do need the plans to be extant before creating the instances. We could get rejected. If that is true, where does it stop? Why check for classes? or Brokers? Sounds like we could simplify the tests a whole lot if we don't need to check for anything.

@MHBauer What do you mean by "[w]e could get rejected"? Are you suggesting that the API Server could reject the request to create the ServiceInstance? The API Server will only reject a request to create a ServiceInstance due to a missing ClusterServicePlan if the request does not explicitly specify a plan to use and instead requests that the default plan be chosen. That is not the case for this test. There is no need for the ClusterServicePlan, the ClusterServiceClass, or the ClusterServiceBroker to exist prior to making the request to create the ServiceInstance. Obviously, the Controller will not be able to reconcile the ServiceInstance until the ClusterServicePlan, ClusterServiceClass, and ClusterServiceBroker exist, but they can be created out of order.

MHBauer · 2018-03-26T22:52:43Z

@staebler makes sense. It sounds like instead of adding this wait, I should be taking out the existing waits on classes and brokers.

nilebox · 2018-03-26T23:10:42Z

Obviously, the Controller will not be able to reconcile the ServiceInstance until the ClusterServicePlan, ClusterServiceClass, and ClusterServiceBroker exist, but they can be created out of order.

I agree with that of course, but for the sake of ease of debugging failing tests, I would prefer to make sure that class and plan do exist and are healthy before creating an instance. That will help returning a "plan is broken" test failure message instead of "instance provisioning has timed out, something is wrong, go read the logs"

n3wscott · 2018-03-28T23:39:11Z

test/util/util.go

@@ -98,7 +98,7 @@ func WaitForClusterServiceClassToExist(client v1beta1servicecatalog.Servicecatal
 	)
 }

-// WaitForClusterServiceClassToExist waits for the ClusterServiceClass with the given name
+// WaitForClusterServicePlanToExist waits for the ServicePlan with the given name


ClusterServicePlan

updated, thanks

MHBauer · 2018-03-29T03:32:47Z

@staebler in theory, I suppose I could start all the "wait for exists" in separate goroutines, and then wait for them all to finish and join before proceeding. I'd wait and move that to a future change to hit all the tests with the same pattern at the same time.

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

pmorie reviewed Sep 25, 2017

View reviewed changes

MHBauer added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 19, 2017

MHBauer requested review from arschles, krancour, duglin and kibbles-n-bytes November 6, 2017 21:22

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

staebler reviewed Nov 22, 2017

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 26, 2018

MHBauer removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2018

jpeeler added the LGTM1 label Mar 27, 2018

n3wscott reviewed Mar 28, 2018

View reviewed changes

MHBauer added 4 commits March 28, 2018 20:29

e2e explicit plan check

3920304

rebase adjustments

3a91339

suggested dsl style from gomega

e5d24a9

adjust name

b58e2ca

jboyd01 added the LGTM2 label Apr 4, 2018

jboyd01 merged commit 6409003 into kubernetes-retired:master Apr 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e explicit plan check #1261

e2e explicit plan check #1261

MHBauer commented Sep 22, 2017 •

edited

Loading

pmorie Sep 22, 2017

MHBauer Sep 26, 2017

pmorie commented Oct 3, 2017

MHBauer commented Oct 19, 2017

staebler commented Nov 21, 2017

arschles commented Nov 21, 2017

staebler commented Nov 22, 2017

staebler Nov 22, 2017

MHBauer Nov 22, 2017

staebler Nov 22, 2017

MHBauer commented Nov 22, 2017

staebler commented Nov 22, 2017 •

edited

Loading

MHBauer commented Mar 26, 2018

nilebox commented Mar 26, 2018

n3wscott Mar 28, 2018

MHBauer Mar 29, 2018

MHBauer commented Mar 29, 2018

e2e explicit plan check #1261

e2e explicit plan check #1261

Conversation

MHBauer commented Sep 22, 2017 • edited Loading

pmorie Sep 22, 2017

Choose a reason for hiding this comment

MHBauer Sep 26, 2017

Choose a reason for hiding this comment

pmorie commented Oct 3, 2017

MHBauer commented Oct 19, 2017

staebler commented Nov 21, 2017

arschles commented Nov 21, 2017

staebler commented Nov 22, 2017

staebler Nov 22, 2017

Choose a reason for hiding this comment

MHBauer Nov 22, 2017

Choose a reason for hiding this comment

staebler Nov 22, 2017

Choose a reason for hiding this comment

MHBauer commented Nov 22, 2017

staebler commented Nov 22, 2017 • edited Loading

MHBauer commented Mar 26, 2018

nilebox commented Mar 26, 2018

n3wscott Mar 28, 2018

Choose a reason for hiding this comment

MHBauer Mar 29, 2018

Choose a reason for hiding this comment

MHBauer commented Mar 29, 2018

MHBauer commented Sep 22, 2017 •

edited

Loading

staebler commented Nov 22, 2017 •

edited

Loading