Bug 1577083

Summary: Should not permit "ms" value(such as "2ms") for relistDuration in clusterservicebroker
Product: OpenShift Container Platform Reporter: Zhang Cheng <chezhang>
Component: Service CatalogAssignee: Marko Luksa <mluksa>
Status: CLOSED NOTABUG QA Contact: Zhang Cheng <chezhang>
Severity: medium Docs Contact:
Priority: medium    
Version: 3.10.0CC: aos-bugs, chezhang, jaboyd, jiazha, jmatthew, mluksa, zhsun, zitang
Target Milestone: ---   
Target Release: 3.10.0   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2018-05-17 13:30:33 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

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:
Always


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: 
None

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
John,

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>

DESCRIPTION:
     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
Marko

I'm OKay for your explanation. Thanks.