NonSlidingUntil for deployment

[jklaw90]

Use NonSlidingUntil over Until in order to account for function duration.
Trying to make the run times on more of an interval in order to make executions times more predictable.

Using NonSlidingWindow will make the deployment behave more like the cronjob timing wise.

[k8s-ci-robot]

Hi @jklaw90. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

[jklaw90]

/assign @seanmalloy

[damemi]

/ok-to-test
Sounds good to me, thanks for the suggestion @jklaw90 !

[ingvagabund]

The intent of the jitter factor is to make sure all clients sending a request to the apiserver do not sent the request at the same time or too close to each other in time. Given the running time of all the strategies is unknown in advance, using the sliding version helps to increase the randomness.

Trying to make the run times on more of an interval in order to make executions times more predictable.

@jklaw90 I wonder what's the motivation for making the execution time more predictable and if there are measurements which might tell us if using the non-sliding version has any benefits for the overall resource utilization (e.g. API requests) of the cluster.

[damemi]

I wonder what's the motivation for making the execution time more predictable

I think the use case could be for example: you expect descheduler to run every 30 minutes, but you have a giant (hypothetical) cluster where each run takes 20 minutes. If the timer doesn't start until the first run completes, that means your 2 runs will actually appear 50 minutes apart

[ingvagabund]

@jklaw90 thank you for opening the topic of the descheduler improvements. Is your motivation to improve the debugability, performance or other aspect of the descheduler?

[jklaw90]

@ingvagabund debugging and testability are why i think we should use the non sliding window.
we have smoke tests that were testing the old cronjob which always ran on an exact interval. The Until method causes the "interval" to not be respected. From our findings when we set the interval to 10m and we were seeing runs ~12 - 15 minutes apart.

Here is an example of the two since the documentation wasn't the clearest for NonSlidingUntil.

func main() {
	wait.NonSlidingUntil(func() {
		log.Println("running")
		time.Sleep(8 * time.Second)
	}, 10*time.Second, nil)
}
// 2022/01/10 08:47:54 running
// 2022/01/10 08:48:04 running
// 2022/01/10 08:48:14 running
// 2022/01/10 08:48:24 running

func main() {
	wait.Until(func() {
		log.Println("running")
		time.Sleep(8 * time.Second)
	}, 10*time.Second, nil)
}
// 2022/01/10 08:48:50 running
// 2022/01/10 08:49:08 running
// 2022/01/10 08:49:26 running
// 2022/01/10 08:49:44 running

[ingvagabund]

@jklaw90 @damemi the use cases you mentioned make clear this change improve the user experience as well. Which is also valuable.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund, jklaw90

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 [ingvagabund]

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

[damemi]

/lgtm