Add a keepalive timeout on the locket client #12
Conversation
There was a problem hiding this comment.
Looks good to me.
Correlates to the following PR: cloudfoundry/diego-release#675
|
Just making sure I understand this fully + why there's no default in locket itself: If config values are omitted entirely from a locket client, the behavior will be unchanged from current (10s default interval applied by grpc, and infinite timeout). If clients add this config, the behaviors will be changed to what was desired in diego-release/#627. Correct? Are there any downsides associated with modifying other (non-diego) locket clients outside of diego to add these defaults? |
|
@geofffranks Your assumption is partially right. If someone does not configure the client, the interval will be 10s and the timeout will be 22s. The value for the default timeout was proposed by @ameowlia and seems reasonable to me as well. With this approach, we leave the door open for users of the locket client to configure it how they want. As to any downsides, from what I know, there should be none. |
But this is only because of the defaults set in diego-release's jobs' spec files, correct? If something like |
1 similar comment
But this is only because of the defaults set in diego-release's jobs' spec files, correct? If something like |
|
Yes, I think this would happen, but to be honest I have not tested this. Will do it tomorrow |
|
So I tested it with the tps_watcher job. Both the timeout and keepalive time were 0 when set in the gRPC client. But I am not sure it matters. As seen here https://github.com/grpc/grpc-go/blob/v1.54.0/dialoptions.go#L471, if the time is less than 10s, it still set's it to 10. As to the timeout, I think 0 just means infinity again. So yes, even though both params are 0, It will most probably behave the way it used to before. |
[DIEGO-RELEASE] Add locket client keepalive time and timeout to jobs: cloudfoundry/diego-release#722 [DIEGO-RELEASE] Make max_containers prop configurable: cloudfoundry/diego-release#876 [BBS] Add request metrics for BBS - still open: cloudfoundry/bbs#80 [BBS] Use scheduling info instead of the whole desiredLRP: cloudfoundry/bbs#79 [BBS] Add routing info endpoint: cloudfoundry/bbs#66 [BBS] Remove cpu_weight limit: cloudfoundry/bbs#81 [EXECUTOR] Improve error handling on process start - still open: cloudfoundry/executor#91 [LOCKET] Add a keepalive timeout on the locket client: cloudfoundry/locket#12 [GROOTFS] UsedVolumesSize and UsedStoreInBytes metrics: cloudfoundry/grootfs#155 [ROUTE-EMITTER] Use routing info bbs endpoint when syncing: cloudfoundry/route-emitter#23 [ROUTE-EMITTER] Use routing_info for desired_lrp's when there are missing actual_lrp's: cloudfoundry/route-emitter#26 [SILK-RELEASE] Deduplicate Iptables Rules with Dynamic ASG's: cloudfoundry/silk-release#101 [SILK-RELEASE] Make container_metadata_file_check_timeout on silk-shutdown configurable: cloudfoundry/silk-release#111
[DIEGO-RELEASE] Add locket client keepalive time and timeout to jobs: cloudfoundry/diego-release#722 [DIEGO-RELEASE] Make max_containers prop configurable: cloudfoundry/diego-release#876 [BBS] Add request metrics for BBS - still open: cloudfoundry/bbs#80 [BBS] Use scheduling info instead of the whole desiredLRP: cloudfoundry/bbs#79 [BBS] Add routing info endpoint: cloudfoundry/bbs#66 [BBS] Remove cpu_weight limit: cloudfoundry/bbs#81 [EXECUTOR] Improve error handling on process start - still open: cloudfoundry/executor#91 [LOCKET] Add a keepalive timeout on the locket client: cloudfoundry/locket#12 [GROOTFS] UsedVolumesSize and UsedStoreInBytes metrics: cloudfoundry/grootfs#155 [ROUTE-EMITTER] Use routing info bbs endpoint when syncing: cloudfoundry/route-emitter#23 [ROUTE-EMITTER] Use routing_info for desired_lrp's when there are missing actual_lrp's: cloudfoundry/route-emitter#26 [SILK-RELEASE] Deduplicate Iptables Rules with Dynamic ASG's: cloudfoundry/silk-release#101 [SILK-RELEASE] Make container_metadata_file_check_timeout on silk-shutdown configurable: cloudfoundry/silk-release#111 wip
[DIEGO-RELEASE] Add locket client keepalive time and timeout to jobs: cloudfoundry/diego-release#722 [DIEGO-RELEASE] Make max_containers prop configurable: cloudfoundry/diego-release#876 [BBS] Add request metrics for BBS - still open: cloudfoundry/bbs#80 [BBS] Use scheduling info instead of the whole desiredLRP: cloudfoundry/bbs#79 [BBS] Add routing info endpoint: cloudfoundry/bbs#66 [BBS] Remove cpu_weight limit: cloudfoundry/bbs#81 [EXECUTOR] Improve error handling on process start - still open: cloudfoundry/executor#91 [LOCKET] Add a keepalive timeout on the locket client: cloudfoundry/locket#12 [GROOTFS] UsedVolumesSize and UsedStoreInBytes metrics: cloudfoundry/grootfs#155 [ROUTE-EMITTER] Use routing info bbs endpoint when syncing: cloudfoundry/route-emitter#23 [ROUTE-EMITTER] Use routing_info for desired_lrp's when there are missing actual_lrp's: cloudfoundry/route-emitter#26 [SILK-RELEASE] Deduplicate Iptables Rules with Dynamic ASG's: cloudfoundry/silk-release#101 [SILK-RELEASE] Make container_metadata_file_check_timeout on silk-shutdown configurable: cloudfoundry/silk-release#111 wip
With this change we add a keepalive timeout to the locket gRPC client. We also make the keepalive "Time" property part of the locket client config, so both of them can be changed. This change brings great improvements to the locket client's ability to reconnect to another server faster under network delays.
More info in the Diego issue: cloudfoundry/diego-release#627
PR in the Diego repo for the changes in the jobs config: cloudfoundry/diego-release#675