Add an option to limit the number of concurrent mkfs calls #115379
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Welcome @artemvmin! |
k8s-ci-robot
added
needs-ok-to-test
|
Hi @artemvmin. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
artemvmin
force-pushed
the
serial-mkfs
branch
from
99c5571 to
e4ca834
Compare
k8s-ci-robot
added
release-note
|
/assign @mattcary |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -562,6 +563,17 @@ func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target | |||
| return nil | |||
| } | |||
|
|
|||
| func (mounter *SafeFormatAndMount) format(fstype string, args []string) ([]byte, error) { | |||
| if mounter.formatSem != nil { | |||
| mounter.formatSem <- 1 | |||
There was a problem hiding this comment.
Is it possible to add a unit test using FakeExec?
There was a problem hiding this comment.
Added tests for SetMax() and format(). Let me know what you think.
artemvmin
force-pushed
the
serial-mkfs
branch
from
e4ca834 to
0d6701d
Compare
k8s-ci-robot
added
size/L
artemvmin
force-pushed
the
serial-mkfs
branch
2 times, most recently
from
d47bc18 to
ae900cc
Compare
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm Will need high-level approver for all the test changes, maybe @liggitt is right for that? |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: b9d5fa035138c86d572c8a42cb4e2debf49d0638
|
|
|
||
| // SetMaxConcurrentFormat sets the maximum number of concurrent format | ||
| // operations executed by the mounter. The timeout controls the maximum | ||
| // duration of a format operation before it is treated as complete. A max |
There was a problem hiding this comment.
is a timed out format operation considered complete and successful or complete and failed? is a timed out format operation interrupted or allowed to continue in the background? if it is interrupted, does that have the potential to corrupt anything? is there a way to set concurrent operations without setting a timeout?
There was a problem hiding this comment.
Good questions. PTAL and let me know if it's clarified.
Is there a way to set concurrent operations without setting a timeout?
I implemented it without a timeout originally, but requiring a timeout seems like the safest strategy to avoid gridlock (e.g. from a stuck format operation). Do you think we should make it optional?
| mounter.acquireFormatSem() | ||
|
|
||
| out := make(chan *formatResult, 1) | ||
| go mounter.execFormat(fstype, args, out) |
There was a problem hiding this comment.
what happens if execFormat panics? if the goal is to release the semaphore after timeout and let the execFormat continue to block returning, it seems safer to put the semaphore release and timeout inside a goroutine, and leave the calls to mounter.Exec.Command completely alone and in the main goroutine:
done := make(chan struct{})
defer close(done)
// get the semaphore
mounter.acquireFormatSem()
go func(){
// release the semaphore when done or timeout is hit
defer mounter.releaseFormatSem()
// set up a timer that cleans up on exit
timeout := time.NewTimer(mounter.formatTimeout)
defer timeout.Stop()
select {
case <-done:
case <-timeout.C:
}
}()
return mounter.Exec.Command("mkfs."+fstype, args...).CombinedOutput()There was a problem hiding this comment.
I'm also not sure the acquireFormatSem / releaseFormatSem indirection is helpful... would making the entire obtain/release flow conditional be clearer:
if m.formatSem != nil {
// track when mount completes
done := make(chan struct{})
defer close(done)
// block until a free slot opens up
m.formatSem <- struct{}{}
go func() {
// release our slot
defer func() { <-m.formatSem }()
// set up a timer that cleans up on exit
timeout := time.NewTimer(m.formatTimeout)
defer timeout.Stop()
// block until done or timed out
select {
case <-done:
case <-timeout.C:
}
}()
}
return m.Exec.Command("mkfs."+fstype, args...).CombinedOutput()There was a problem hiding this comment.
I was wondering how to use defers, but was struggling since my sem release logic was stuck in the center of the function. The done channel is brilliant.
The time.After() timer not cleaning up unless fired is very subtle (https://pkg.go.dev/time#After). Thanks for pointing that out.
Also, I haven't found a definitive decision on which channel type to use for a basic signal. After some thought, it seems like struct{} really is the only unary type.
artemvmin
force-pushed
the
serial-mkfs
branch
from
0857442 to
95188ab
Compare
k8s-ci-robot
removed
the
lgtm
artemvmin
force-pushed
the
serial-mkfs
branch
from
95188ab to
d2dd415
Compare
|
/approve will leave final lgtm to storage reviewer |
k8s-ci-robot
added
the
approved
|
/retest |
|
/retest pull-kubernetes-e2e-gce |
|
@artemvmin: The
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
|
/test pull-kubernetes-e2e-gce |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 75bd4f234c66618e8cfb6b320ba62a032f2be11f
|
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
What type of PR is this?
/kind feature
/sig storage
What this PR does / why we need it:
We have observed the PDCSI driver OOMing when attempting to provision > 100 volumes concurrently, exceeding 4
mkfscalls per second. This CL adds an optional semaphore to limit the number ofmkfscalls, which can reduce memory spikes and make garbage collection more reliable.Reviewer notes:
This change pulls in the new k8s.io/utils version, which modifies the error messages for CIDR validation. Tests relying on the CIDR validation error string had to be updated to the new format.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?