@saad-ali : (trying to learn). So the empty subname "" means that all the UnmountDevice operations will run one after another right?

Correct.

@saad-ali @smarterclayton The run interface was a nice and simple interface. We are burdending the compelxity of the "sub" logic into callers who don't care. We should add a new routine for the subLogic that allows for running on two dimensions as opposed to one. That also means callers don't have to use a "" second argument that is just confusing.

How about

Run(target, operationFunc func() error) error
RunSubTargets(target string,  targetSub string, operationFunc func() error) error

Minor nit here - we usually call function variables fooFn or fn and function type aliases FooFunc.

Agree on splitting. Alternatively, you could reorder the function like:

Run(fn func() error, targets ...string)

and have identical targets serialize (you might have to join the targets to do that or create a special map key type, so it's not hugely important to me). Either way making clear that target has to be unique.

My brain twisted a bit at this - just to verify, we're going to take and hold lock, but cond will be signaled inside the lock? So lock dominates cond here as well as in wait?

@smarterclayton @saad-ali SO defer order according to https://blog.golang.org/defer-panic-and-recover is Last-In is First-Out. Which means that what we are doing here as I under stand it is

1. lock the structure
2. Do all the work in the datastructure required.
3. unlock structure
4. Wakeup anything potentially waiting before this routine takes the lock, or while the lock is taken here. There is a window between the defers where we release the lock and then do the signal but in that case the Wait routine should perform the correct action as the length of the slice should be updated.

Saad is that correct? I recommend commenting the desired order of the defers and the reasoning behind them.

My brain twisted a bit at this - just to verify, we're going to take and hold lock, but cond will be signaled inside the lock? So lock dominates cond here as well as in wait?

Cond will be signaled outside the lock here. But the wait is protected by the lock.

So pretty much what @matchstick said, the last operation to occur is signaling any waiting routines.

The wait code (introduced in #25399 and #28153) is all existing code, so I didn't give it much throught in this PR. Looking at it again, I think it's ok as is. Will add comments to explain the order of execution.

I think there might be performance issue with the current exponential backoff design. Consider a scenario to creat a rc consisting of N pods which all reference the same disk . After desired world populator adds the volume and pod information, reconciler starts issuing waitforattach/mount requests periodically. In the first round, reconciler scans all the pod volumes and issues total N requests almost at the same time. With gorountinemap, only one of them can succeed and some others will have ExponentialBackoffError which cause them to wait the same amount of time (double the current loop duration) to retry. After passing the durationBeforeRetry reconciler will issue N or N-1 (if the first finishes) requests again at around the same time, causing them to double the same wait time again. The same step repeat in the following rounds, each time only one request can be granted, and the waiting time to retry keeps doubling until reaching the maxDurationBeforeRetry. From my initial experiments, looks like mount operation finishes very fast which reduce the above problem. But wait for attach operation seems takes longer time. One possible solution is to introduce randomness into the algorithm.

With gorountinemap, only one of them can succeed

With this change all of them will be able to run mount in parallel.

some others will have ExponentialBackoffError

ExponentialBackoff does not apply when an operation is rejected because there is an existing operation (only when there is an execution error).

Here I am more focusing on the operations for attachable volumes. As you explained, if ExponentialBackoff only apply for execution error, it may not cause serious performance issue. But still, it may end up spending more time than necessary. For example, if the volume becomes attachable in 1 min, depending on the timing, it is possible for all N pods waiting for more than 3 mins before starting mount operations. Also it may have higher chances of operation is rejected because there is an existing operation. What will happen if there is a hanging operation in goroutine?

For volumes that are not attachable, since they can run in parallel without problem. Why need to run them through goroutine?

This feels like a 1.4 conversation if the exponential behaviour has not
changed. I am not saying JinXu's idea don't have merit, but we do need the
fix in 1.3 and if this is how it worked in 1.2 let's decouple this
conversation.

mrubin

On Wed, Jul 20, 2016 at 10:16 AM, Jing Xu notifications@github.com wrote:

In pkg/util/goroutinemap/exponentialbackoff/exponential_backoff.go
#28939 (comment)
:

+*/
+
+// Package exponentialbackoff contains logic for implementing exponential
+// backoff for GoRoutineMap and NestedPendingOperations.
+package exponentialbackoff
+
+import (

• "fmt"
• "time"
  +)

+const (

• // initialDurationBeforeRetry is the amount of time after an error occurs
• // that GoroutineMap will refuse to allow another operation to start with
• // the same target (if exponentialBackOffOnError is enabled). Each
• // successive error results in a wait 2x times the previous.

Here I am more focusing on the operations for attachable volumes. As you
explained, if ExponentialBackoff only apply for execution error, it may not
cause serious performance issue. But still, it may end up spending more
time than necessary. For example, if the volume becomes attachable in 1
min, depending on the timing, it is possible for all N pods waiting for
more than 3 mins before starting mount operations. Also it may have higher
chances of operation is rejected because there is an existing operation.
What will happen if there is a hanging operation in goroutine?

For volumes that are not attachable, since they can run in parallel
without problem. Why need to run them through goroutine?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/28939/files/88d495026dd3f741b1398531cbf6c484c0c03773#r71567301,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ASzXe9_0zvzED3kQRFsrfcDHldrKqTdnks5qXlf0gaJpZM4JMGj8
.

if the volume becomes attachable in 1 min, depending on the timing, it is possible for all N pods waiting for more than 3 mins before starting mount operations

That's not correct. If multiple pods on a node are referencing the same attachable volume, the attach and mount device steps are serialized and only happen once for all the pods. Once a device is attached and globally mounted, subsequent pods only need to do the bind mount (which, with this change is parallelized).

Also it may have higher chances of operation is rejected because there is an existing operation.

That makes little difference in execution speed. The reconciler retries very quickly.

What will happen if there is a hanging operation in goroutine

If an attachable volume fails to attach or complete global mount, regardless of if the operations fail or hang, they will block bind mounting. That is necessary and by design, an attachable volume can not be bind mounted until these operations succeed.

For volumes that are not attachable, since they can run in parallel without problem. Why need to run them through goroutine?

They must be run in a goroutine so that the caller is not blocked. They are run using nestedpendingoperations to prevent the multiple operations from being triggered on the same pod/volume.