Use non-zero resync period as last insurance in local volume provisioner #800
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/area local-volume |
cofyc
force-pushed
the
resyncperiod
branch
2 times, most recently
from
f96b60a
to
62e882e
Compare
| // We choose a random resync period between MinResyncPeriod and 2 * | ||
| // MinResyncPeriod, so that local provisioners deployed on multiple nodes | ||
| // at same time don't list the apiserver simultaneously. | ||
| resyncPeriod := time.Duration(float64(config.MinResyncPeriod.Nanoseconds()) * (1 + rand.Float64())) |
There was a problem hiding this comment.
The final value should be multiplied by time.Nanosecond. Also, do we really need Nanosecond granularlity? I think probably Seconds should be sufficient.
There was a problem hiding this comment.
Fixed in e45ee19. Also initialized default source in main.go.
| @@ -53,7 +53,7 @@ func StartLocalController(client *kubernetes.Clientset, ptable deleter.ProcTable | |||
| // We choose a random resync period between MinResyncPeriod and 2 * | |||
| // MinResyncPeriod, so that local provisioners deployed on multiple nodes | |||
| // at same time don't list the apiserver simultaneously. | |||
| resyncPeriod := time.Duration(float64(config.MinResyncPeriod.Nanoseconds()) * (1 + rand.Float64())) | |||
| resyncPeriod := time.Duration(config.MinResyncPeriod.Seconds()*(1+rand.Float64())) * time.Second | |||
There was a problem hiding this comment.
I think if you don't cast it to float64 like before, then you don't need to multiple by time.Second at the end.
There was a problem hiding this comment.
Seconds() method divides it by time.Second to get number of seconds, so we need to multiple by time.Second at the end. time.Duration is int64 in nanoseconds, so we don't need to multiple by time.Nanosecond if we use Nanoseconds().
| @@ -43,6 +44,7 @@ var ( | |||
| ) | |||
|
|
|||
| func main() { | |||
| rand.Seed(time.Now().UTC().UnixNano()) | |||
There was a problem hiding this comment.
This may be causing mac osx build to fail?
There was a problem hiding this comment.
helm install script, fixed in #806.
|
Rebased due to conflicts with #799. |
- Add `MinResyncPeriod` configuration, defaults: 5m0s. - Update JobController to use default resync period. - Test `TestLoadProvisionerConfigs ` with real cases. - Add example for customizing resync period. - Update test/run.sh to fail on errors. - Initialize rand default source on startup.
|
/lgtm |
Fixes #795.
MinResyncPeriodconfiguration, defaults: 5m0s.TestLoadProvisionerConfigswith real cases.