Make timeout of any request to the broker globally configurable #2666

viviyww · 2019-06-20T06:37:15Z

We find the delay in creating instance binding varies. The current
default is to wait one minute, otherwise the creation is considered
a failure. So I do this patch to solve this problem. And I think this
delay is configured for different usage scenarios or brokers.
For example, when the maximum response time of my service is 1 minute,
we may set it to be 60, While yours is 3 minutes, and then you should
set it to be 180.

This PR is a

Feature Implementation
Bug Fix
Documentation

What this PR does / why we need it:

Which issue(s) this PR fixes

Fixes #

Please leave this checklist in the PR comment so that maintainers can ensure a good PR.

Merge Checklist:

New feature
- Tests
- Documentation
SVCat CLI flag
Server Flag for config
- Chart changes
- removing a flag by marking deprecated and hiding to avoid
  breaking the chart release and existing clients who provide a
  flag that will get an error when they try to update

k8s-ci-robot · 2019-06-20T06:37:28Z

Hi @viviyww. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

viviyww · 2019-06-20T06:41:54Z

/assign @jberkhahn

viviyww · 2019-06-20T06:42:28Z

/assign @mszostok

jberkhahn · 2019-06-20T17:12:29Z

/ok-to-test

mszostok · 2019-06-20T19:02:37Z

pkg/controller/controller_clusterservicebroker_test.go

+	const testOSBTimeOut = time.Second * 80
+	_, _, _, testController, _ := newTestController(t, getTestCatalogConfig())
+	testController.OSBAPITimeOut = testOSBTimeOut
+	if err := reconcileClusterServiceBroker(t, testController, getTestClusterServiceBroker()); err == nil {


u still have wrong expectation :)

if err := reconcileClusterServiceBroker(t, testController, getTestClusterServiceBroker()); err != nil { t.Fatalf("Unexpected reconcileClusterServiceBroker failure: %v", err) }

mszostok · 2019-06-20T19:07:12Z

pkg/controller/controller_clusterservicebroker_test.go

@@ -286,6 +286,19 @@ func TestReconcileClusterServiceBrokerExistingServiceClassAndServicePlan(t *test
 	assertNumberOfActions(t, kubeActions, 0)
 }

+func TestReconcileClusterServiceBrokerSetOSBTimeOut(t *testing.T) {


there is a pattern that each test have a description, e.g. https://github.com/kubernetes-sigs/service-catalog/blob/48e6805363476ffb91faabf6aca6d03c96d123b0/pkg/controller/controller_clusterservicebroker_test.go#L692-L695

so please add sth similar also here

ok, I will recheck it, thanks.

viviyww · 2019-06-21T02:13:06Z

/test pull-service-catalog-unit

viviyww · 2019-06-21T02:15:12Z

/ok-to-test

viviyww · 2019-06-21T03:28:12Z

@mszostok It is ok now, thanks.

viviyww · 2019-06-21T08:06:31Z

/assign @jberkhahn

mszostok · 2019-06-21T08:08:51Z

/lgtm

viviyww · 2019-06-25T11:19:39Z

/assign @MHBauer

viviyww · 2019-07-03T01:46:43Z

/assign @jberkhahn Please help to review it or merge it, thanks.

MHBauer · 2019-07-03T01:59:32Z

pkg/controller/controller_clusterservicebroker_test.go

+		t.Fatalf("This should not fail : %v", err)
+	}
+	createdClient := testController.brokerClientManager.clients[NewClusterServiceBrokerKey(getTestClusterServiceBroker().Name)]
+	if createdClient.clientConfig.TimeoutSeconds != int(testController.OSBAPITimeOut.Seconds()) {


shouldn't this match the constant on 239? both client.config/timeoutseconds and testcontroller.osbapitimeout.seconds?

No, it should not be the same. It is just a test case. Thanks for review it.

you set them to be the same on line 241. why bother having the const then?

MHBauer · 2019-07-03T02:00:38Z

pkg/controller/controller.go

 	clientConfig := osb.DefaultClientConfiguration()
 	clientConfig.Name = meta.Name
 	clientConfig.URL = commonSpec.URL
 	clientConfig.AuthConfig = authConfig
 	clientConfig.EnableAlphaFeatures = true
 	clientConfig.Insecure = commonSpec.InsecureSkipTLSVerify
 	clientConfig.CAData = commonSpec.CABundle
+	clientConfig.TimeoutSeconds = int(osbAPITimeOut.Seconds())


I don't really like that this is a float, but it's what we got.

MHBauer · 2019-07-03T02:03:08Z

I'm confused about where the value is used.

viviyww · 2019-07-03T02:11:52Z

Please see this issue #2657. I think it needs to be configurable. Take one of our usage scenarios, in the broker we developed, the response time exceeds 60s. And right now we can't use it.

We find the delay in creating instance binding varies. The current default is to wait one minute, otherwise the creation is considered a failure. So I do this patch to solve this problem. And I think this delay is configured for different usage scenarios or brokers. For example, when the maximum response time of my service is 1 minute, we may set it to be 60, While yours is 3 minutes, and then you should set it to be 180.

jberkhahn · 2019-07-08T17:51:41Z

/approve

k8s-ci-robot · 2019-07-08T17:51:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jberkhahn, viviyww

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jberkhahn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mszostok · 2019-07-08T18:17:38Z

/lgtm

mszostok · 2019-07-08T18:39:01Z

/test pull-service-catalog-xbuild

mszostok · 2019-07-10T12:43:55Z

/test pull-service-catalog-xbuild

mszostok · 2019-07-10T13:18:18Z

/test pull-service-catalog-xbuild

viviyww · 2019-07-11T01:42:11Z

@mszostok This test case failed for the pkg in the vendor. Maybe it needs to be updated. This problem has little to do with the patch itself. Thanks.

Fix test case pull-service-catalog-xbuild about issue kubernetes-retired#2666

mszostok · 2019-07-11T12:01:34Z

@viviyww but in logs I can see that /usr/sbin/update-ca-certificates: 75: /usr/sbin/update-ca-certificates: rm: Exec format error is the main error. I know that this build is failing from time to time, but here it's really strange that it failed a few times in a row 🤔

jberkhahn · 2019-07-18T02:35:01Z

/retest

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

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 20, 2019

k8s-ci-robot requested review from MHBauer and mszostok June 20, 2019 06:37

viviyww mentioned this pull request Jun 20, 2019

Fix bug in create service binding errors #2622

Closed

10 tasks

k8s-ci-robot assigned jberkhahn Jun 20, 2019

k8s-ci-robot assigned mszostok Jun 20, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 20, 2019

mszostok reviewed Jun 20, 2019

View reviewed changes

mszostok mentioned this pull request Jun 20, 2019

Service Broker timeout is not configurable #2657

Closed

viviyww force-pushed the master6 branch 2 times, most recently from 748a72c to 840e137 Compare June 21, 2019 03:10

viviyww force-pushed the master6 branch from 840e137 to 656af83 Compare June 21, 2019 06:50

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2019

k8s-ci-robot assigned MHBauer Jun 25, 2019

MHBauer reviewed Jul 3, 2019

View reviewed changes

viviyww force-pushed the master6 branch from 656af83 to c49992b Compare July 5, 2019 01:04

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2019

viviyww pushed a commit to viviyww/service-catalog that referenced this pull request Jul 11, 2019

Do dep ensure to keep the dependent libraries up to date

bb6f6b1

Fix test case pull-service-catalog-xbuild about issue kubernetes-retired#2666

k8s-ci-robot merged commit 879696f into kubernetes-retired:master Jul 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make timeout of any request to the broker globally configurable #2666

Make timeout of any request to the broker globally configurable #2666

viviyww commented Jun 20, 2019 •

edited

Loading

k8s-ci-robot commented Jun 20, 2019

viviyww commented Jun 20, 2019

viviyww commented Jun 20, 2019

jberkhahn commented Jun 20, 2019

mszostok Jun 20, 2019 •

edited

Loading

mszostok Jun 20, 2019

viviyww Jun 21, 2019

viviyww commented Jun 21, 2019

viviyww commented Jun 21, 2019

viviyww commented Jun 21, 2019 •

edited

Loading

viviyww commented Jun 21, 2019

mszostok commented Jun 21, 2019

viviyww commented Jun 25, 2019

viviyww commented Jul 3, 2019

MHBauer Jul 3, 2019

viviyww Jul 3, 2019

jberkhahn Jul 3, 2019

MHBauer Jul 3, 2019

MHBauer commented Jul 3, 2019

viviyww commented Jul 3, 2019 •

edited

Loading

jberkhahn commented Jul 8, 2019

k8s-ci-robot commented Jul 8, 2019

mszostok commented Jul 8, 2019

mszostok commented Jul 8, 2019

mszostok commented Jul 10, 2019

mszostok commented Jul 10, 2019

viviyww commented Jul 11, 2019

mszostok commented Jul 11, 2019

jberkhahn commented Jul 18, 2019

Make timeout of any request to the broker globally configurable #2666

Make timeout of any request to the broker globally configurable #2666

Conversation

viviyww commented Jun 20, 2019 • edited Loading

k8s-ci-robot commented Jun 20, 2019

viviyww commented Jun 20, 2019

viviyww commented Jun 20, 2019

jberkhahn commented Jun 20, 2019

mszostok Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

mszostok Jun 20, 2019

Choose a reason for hiding this comment

viviyww Jun 21, 2019

Choose a reason for hiding this comment

viviyww commented Jun 21, 2019

viviyww commented Jun 21, 2019

viviyww commented Jun 21, 2019 • edited Loading

viviyww commented Jun 21, 2019

mszostok commented Jun 21, 2019

viviyww commented Jun 25, 2019

viviyww commented Jul 3, 2019

MHBauer Jul 3, 2019

Choose a reason for hiding this comment

viviyww Jul 3, 2019

Choose a reason for hiding this comment

jberkhahn Jul 3, 2019

Choose a reason for hiding this comment

MHBauer Jul 3, 2019

Choose a reason for hiding this comment

MHBauer commented Jul 3, 2019

viviyww commented Jul 3, 2019 • edited Loading

jberkhahn commented Jul 8, 2019

k8s-ci-robot commented Jul 8, 2019

mszostok commented Jul 8, 2019

mszostok commented Jul 8, 2019

mszostok commented Jul 10, 2019

mszostok commented Jul 10, 2019

viviyww commented Jul 11, 2019

mszostok commented Jul 11, 2019

jberkhahn commented Jul 18, 2019

viviyww commented Jun 20, 2019 •

edited

Loading

mszostok Jun 20, 2019 •

edited

Loading

viviyww commented Jun 21, 2019 •

edited

Loading

viviyww commented Jul 3, 2019 •

edited

Loading