Bug 1577083 - Should not permit "ms" value(such as "2ms") for relistDuration in clusterservicebroker
Summary: Should not permit "ms" value(such as "2ms") for relistDuration in clusterserv...
Alias: None
Product: OpenShift Container Platform
Classification: Red Hat
Component: Service Catalog
Version: 3.10.0
Hardware: Unspecified
OS: Unspecified
Target Milestone: ---
: 3.10.0
Assignee: Marko Luksa
QA Contact: Zhang Cheng
Depends On:
TreeView+ depends on / blocked
Reported: 2018-05-11 07:44 UTC by Zhang Cheng
Modified: 2018-05-17 13:30 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Last Closed: 2018-05-17 13:30:33 UTC
Target Upstream Version:

Attachments (Terms of Use)

Description Zhang Cheng 2018-05-11 07:44:35 UTC
Description of problem: 
Should not permit negative value(such as "2ms") for relistDuration in clusterservicebroker, will sending relistRequests each 2 second while setting relistDuration=2ms in clusterservicebroker.

service-catalog & asb image using images from brew registry:
service-catalog: v3.10.0-0.38.0;Upstream:v0.1.16
asb: 1.2.10

How reproducible:

Steps to Reproduce:
1. Edit clusterservicebroker ansible-service-broker, set relistDuration=2ms and save
# oc edit clusterservicebroker ansible-service-broker
2. Check if value of relistDuration have been changed.
# oc get clusterservicebroker ansible-service-broker -o yaml | grep relistDuration
  relistDuration: 2ms
3. Check logs of asb to check duration of relist request.
# oc logs asb-1-btsks | grep "AnsibleBroker::Catalog"

Actual results:  
1. Edit and save succeed.
2. relistDuration have been changed to "2ms"
3. Sending relistRequests each 2 second while setting relistDuration=2ms
# oc logs asb-1-btsks | grep "AnsibleBroker::Catalog"
[2018-05-11T07:30:23.378Z] [INFO] - AnsibleBroker::Catalog
[2018-05-11T07:30:26.176Z] [INFO] - AnsibleBroker::Catalog
[2018-05-11T07:30:28.994Z] [INFO] - AnsibleBroker::Catalog
[2018-05-11T07:30:31.776Z] [INFO] - AnsibleBroker::Catalog
[2018-05-11T07:30:34.579Z] [INFO] - AnsibleBroker::Catalog
[2018-05-11T07:30:37.375Z] [INFO] - AnsibleBroker::Catalog

Expected results: 
1. The changes should not be permitted.

Addition info: 

Comment 1 John Matthews 2018-05-11 15:23:13 UTC
Please help clarify the issue.

I see you are setting the relistInterval to 2ms.
So this is a small value.

I don't believe we should call this small value a negative value. A negative value would be something like "-2" which you are not doing.

Perhaps this BZ is asking for a validation to not allow values that are too small to be practically applied?

Example don't allow values below 1 or 2 seconds?

As this BZ looks to be aligned to Service Catalog I am reassigning

Comment 2 Zhang Cheng 2018-05-14 02:23:09 UTC

My expected result is we should not permit user to set "ms" for relistInterval, that is too small and will be confusion for user.

Comment 3 Marko Luksa 2018-05-16 12:35:15 UTC
I'm not sure we should do this. Even if a user sets relistDuration to less than a second, the controller will not perform a relist that often. In reality, the relist occurs every two seconds at most; usually, the time between two relists is much longer (30+ seconds):

I0516 12:27:10.684630       1 controller.go:57] Catalog()
I0516 12:27:12.594053       1 controller.go:57] Catalog()
I0516 12:27:14.595778       1 controller.go:57] Catalog()
I0516 12:27:40.697343       1 controller.go:57] Catalog()
I0516 12:27:42.701069       1 controller.go:57] Catalog()
I0516 12:27:44.701125       1 controller.go:57] Catalog()
I0516 12:27:47.168746       1 controller.go:57] Catalog()

The relistDuration documentation already explains why this is so:

$ k explain clusterservicebroker.spec.relistDuration
KIND:     ClusterServiceBroker
VERSION:  servicecatalog.k8s.io/v1beta1

RESOURCE: relistDuration <Object>

     RelistDuration is the frequency by which a controller will relist the
     broker when the RelistBehavior is set to
     ServiceBrokerRelistBehaviorDuration. Users are cautioned against
     configuring low values for the RelistDuration, as this can easily overload
     the controller manager in an environment with many brokers. The actual
     interval is intrinsically governed by the configured resync interval of the
     controller, which acts as a minimum bound. For example, with a resync
     interval of 5m and a RelistDuration of 2m, relists will occur at the resync
     interval of 5m.

I don't think we should care about this. There's no way for the user to break anything. And if they want to have the controller perform the relist operation as often as possible, they should be allowed to set the value to e.g. 1ms. I don't think we should set an arbitrary lower bound.

Comment 4 Zhang Cheng 2018-05-16 14:11:23 UTC

I'm OKay for your explanation. Thanks.

Note You need to log in before you can comment on or make changes to this bug.