Stop timer and correctly drain it #101475

ash2k · 2021-04-26T05:13:50Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Correctly drains timers acquired from backoff manager. This API is easy to misuse, I wonder if a better one is possible. Something like a function that takes a function and, after it returns, cleans things up properly. That would probably cater for only a subset of use cases though.

Does this PR introduce a user-facing change?

NONE

/sig api-machinery

ash2k · 2021-04-26T05:15:09Z

pkg/volume/csi/csi_attacher.go

@@ -475,14 +475,14 @@ func (c *csiAttacher) waitForVolumeAttachDetachStatusWithLister(volumeHandle, at
 		clock         = &clock.RealClock{}
 	)
 	backoffMgr := wait.NewExponentialBackoffManager(initBackoff, maxBackoff, resetDuration, backoffFactor, jitter, clock)
-	defer backoffMgr.Backoff().Stop()


This is incorrect as this may get another timer before draining the existing one. This can happen if ctx signals done. See docs in

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/wait/wait.go

Lines 291 to 295 in 5b42681

// BackoffManager manages backoff with a particular scheme based on its underlying implementation. It provides

// an interface to return a timer for backoff, and caller shall backoff until Timer.C() drains. If the second Backoff()

// is called before the timer from the first Backoff() call finishes, the first timer will NOT be drained and result in

// undetermined behavior.

// The BackoffManager is supposed to be called in a single-threaded environment.

Good catch. Yes, after reviewing the API this defer clearly doesn't work.

ash2k · 2021-04-26T05:15:49Z

pkg/volume/csi/csi_attacher.go

@@ -491,6 +491,7 @@ func (c *csiAttacher) waitForVolumeAttachDetachStatusWithLister(volumeHandle, at
 				return nil
 			}
 		case <-ctx.Done():
+			t.Stop()


We don't have to drain the timer's channel because the backoff manager is private to this function.

the <-t.C() case can return, do we need to call t.Stop() if it does?

No, it's a Timer (not a Ticker), it fires once and there is no need to stop it after that.

ash2k · 2021-04-26T05:16:49Z

staging/src/k8s.io/apimachinery/pkg/util/wait/wait.go

@@ -166,6 +166,9 @@ func BackoffUntil(f func(), backoff BackoffManager, sliding bool, stopCh <-chan
 		// of every loop to prevent extra executions of f().
 		select {
 		case <-stopCh:
+			if !t.Stop() {
+				<-t.C()
+			}


Backoff manager was provided to this function so it must stop and drain the timer's channel here, otherwise the owner of the manager cannot reuse it. Even if the owner didn't care, stopping the timer earlier releases some resources so is beneficial.

ash2k · 2021-04-26T07:20:14Z

/retest

jpbetz · 2021-04-27T20:09:46Z

/assign @jpbetz
/triage accepted

k8s-triage-robot · 2021-07-27T08:24:14Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

ash2k · 2021-07-27T10:18:24Z

/remove-lifecycle stale
/retest

ping @jpbetz

jpbetz

One question then LGTM.

jpbetz · 2021-07-27T17:52:14Z

pkg/volume/csi/csi_attacher.go

@@ -475,14 +475,14 @@ func (c *csiAttacher) waitForVolumeAttachDetachStatusWithLister(volumeHandle, at
 		clock         = &clock.RealClock{}
 	)
 	backoffMgr := wait.NewExponentialBackoffManager(initBackoff, maxBackoff, resetDuration, backoffFactor, jitter, clock)
-	defer backoffMgr.Backoff().Stop()


Good catch. Yes, after reviewing the API this defer clearly doesn't work.

jpbetz · 2021-07-27T17:55:12Z

pkg/volume/csi/csi_attacher.go

@@ -491,6 +491,7 @@ func (c *csiAttacher) waitForVolumeAttachDetachStatusWithLister(volumeHandle, at
 				return nil
 			}
 		case <-ctx.Done():
+			t.Stop()


the <-t.C() case can return, do we need to call t.Stop() if it does?

jpbetz

/lgtm

jpbetz · 2021-07-27T20:50:07Z

/priority important-longterm

jpbetz · 2021-07-27T20:50:50Z

@caesarxuchao, @saad-ali would either of you be willing to approve?

caesarxuchao · 2021-07-28T01:12:27Z

/lgtm
/approve

ash2k · 2021-07-28T02:37:58Z

/assign davidz627

@davidz627 mind taking a look

ash2k · 2021-08-08T13:22:00Z

/unassign davidz627
/assign jsafrane

@jsafrane mind taking a look?

jsafrane · 2021-08-09T13:51:14Z

/approve
should be fine for CSI

k8s-ci-robot · 2021-08-09T13:51:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, caesarxuchao, jpbetz, jsafrane

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:

~~pkg/volume/csi/OWNERS~~ [jsafrane]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [caesarxuchao]

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

smarterclayton · 2023-01-13T19:01:31Z

This API is easy to misuse, I wonder if a better one is possible. Something like a function that takes a function and, after it returns, cleans things up properly. That would probably cater for only a subset of use cases though.

I am in the process of unifying and aligning some of the methods here, and I concur that clock.Timer is very dangerous and has no clear semantics for a consumer to use, because it differs significantly enough from Go's time.Timer that usage can be complicated. At a minimum, the contract of C() would have to be clarified to describe whether users can cache C(), or whether Stop() must be called only after Reset is called (as users would naively assume). The presence of BackoffManager complicates this even more - since we expose Stop() on Timer any consumer of a backoff manager can "break" the entire backoff manager.

From a design perspective the right option is probably to have the backoff manager return the duration to wait (i.e., be just a variant of backoff), and let the loop manage the timer reuse itself. I may try that along with #107826 and consider the input of #114531.

smarterclayton · 2023-01-13T20:33:35Z

I took some of the comments in this and the related issues and applied them in #115064 to reduce the potential for accidental misuse.

Correctly drain timer

5b42681

k8s-ci-robot requested review from lavalamp and verult April 26, 2021 05:14

ash2k commented Apr 26, 2021

View reviewed changes

k8s-ci-robot assigned jpbetz Apr 27, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 27, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 27, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 27, 2021

jpbetz reviewed Jul 27, 2021

View reviewed changes

jpbetz approved these changes Jul 27, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2021

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 27, 2021

k8s-ci-robot assigned caesarxuchao Jul 28, 2021

k8s-ci-robot assigned davidz627 Jul 28, 2021

k8s-ci-robot assigned jsafrane and unassigned davidz627 Aug 8, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2021

k8s-ci-robot merged commit a7af9f6 into kubernetes:master Aug 9, 2021

k8s-ci-robot added this to the v1.23 milestone Aug 9, 2021

ash2k deleted the ash2k/stop-timer branch August 9, 2021 23:19

zqzten mentioned this pull request Dec 16, 2022

apimachinery/wait: tweak #114531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop timer and correctly drain it #101475

Stop timer and correctly drain it #101475

ash2k commented Apr 26, 2021 •

edited

ash2k Apr 26, 2021

jpbetz Jul 27, 2021

ash2k Apr 26, 2021

jpbetz Jul 27, 2021

ash2k Jul 27, 2021

ash2k Apr 26, 2021

ash2k commented Apr 26, 2021

jpbetz commented Apr 27, 2021

k8s-triage-robot commented Jul 27, 2021

ash2k commented Jul 27, 2021

jpbetz left a comment

jpbetz Jul 27, 2021

jpbetz Jul 27, 2021

jpbetz left a comment

jpbetz commented Jul 27, 2021

jpbetz commented Jul 27, 2021

caesarxuchao commented Jul 28, 2021

ash2k commented Jul 28, 2021

ash2k commented Aug 8, 2021

jsafrane commented Aug 9, 2021

k8s-ci-robot commented Aug 9, 2021

smarterclayton commented Jan 13, 2023

smarterclayton commented Jan 13, 2023

	// BackoffManager manages backoff with a particular scheme based on its underlying implementation. It provides
	// an interface to return a timer for backoff, and caller shall backoff until Timer.C() drains. If the second Backoff()
	// is called before the timer from the first Backoff() call finishes, the first timer will NOT be drained and result in
	// undetermined behavior.
	// The BackoffManager is supposed to be called in a single-threaded environment.

Stop timer and correctly drain it #101475

Stop timer and correctly drain it #101475

Conversation

ash2k commented Apr 26, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash2k commented Apr 26, 2021

jpbetz commented Apr 27, 2021

k8s-triage-robot commented Jul 27, 2021

ash2k commented Jul 27, 2021

jpbetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbetz left a comment

Choose a reason for hiding this comment

jpbetz commented Jul 27, 2021

jpbetz commented Jul 27, 2021

caesarxuchao commented Jul 28, 2021

ash2k commented Jul 28, 2021

ash2k commented Aug 8, 2021

jsafrane commented Aug 9, 2021

k8s-ci-robot commented Aug 9, 2021

smarterclayton commented Jan 13, 2023

smarterclayton commented Jan 13, 2023

ash2k commented Apr 26, 2021 •

edited