Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a small pool of workers to run postUnsealFuncs in parallel #18244

Merged
merged 11 commits into from
Dec 12, 2022

Conversation

sgmiller
Copy link
Collaborator

@sgmiller sgmiller commented Dec 6, 2022

Initially size 32, configurable via a VAULT_POSTUNSEAL_FUNC_CONCURRENCY
variable in case of trouble.

@sgmiller sgmiller requested a review from a team December 6, 2022 17:20
@vercel
Copy link

vercel bot commented Dec 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
vault ✅ Ready (Inspect) Visit Preview Dec 6, 2022 at 5:47PM (UTC)

}
for i, v := range c.postUnsealFuncs {
wg.Add(1)
workerChans[i%postUnsealFuncConcurrency] <- v
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know a terrible amount about c.postUnsealFuncs but if this variable contains truly independent functions that we can run in parallel, I wonder if it makes more sense to just loop over them and run each one in its own goroutine, vs having a worker pool with a static size.

My main concern with this particular piece is index wraparound when len(c.postUnsealFuncs) > postUnsealFuncConcurrency. At that point, you'll be overwriting the contents of the workerChans slice, no? Given the default size of 32, maybe the probability of this is low (I don't know what len(c.postUnsealFuncs) typically is), but since the size is tunable, it could be anything.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue concretely is that we have some customers with 5k+ PKI mounts that need to be migrated when upgrading from <=1.10.x to 1.11+.

If we run 4k+ PKI mount migration in full parallel, we absolutely slam backend storage and also the Vault node's CPU, in addition to potentially having too many goroutines, thus starving Raft's heatbeats (if its in use).

Setting a relatively safe upper limit here is good I think. I'd like to maybe see 2x CPU count by default rather than a hard-coded default (just to prevent starvation on nodes with a single CPU core).

Each mount's migration/initialize func yes, is strictly independent, but the combination of work of all of them might be enough to bring down Vault if run in parallel.

Today, we run them strictly serially, which does avoid this problem, but means we strictly stall for IO, then do compute, then stall for more IO, &c.. --- and if each migration only takes 100ms, on 5k+ mounts, we're sitting north of 8 minutes to even unseal, which blows past the default context deadline.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd thought about runtime.NumCPUS(), which I'm back to liking again.

Copy link
Contributor

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

vault/core.go Outdated Show resolved Hide resolved
sgmiller and others added 2 commits December 12, 2022 14:26
Co-authored-by: Nick Cabatoff <ncabatoff@hashicorp.com>
vault/core.go Outdated
for v := range workerChans[i] {
v()
func() {
defer wg.Done()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We Add every time we send to a channel, so I agree we need to Done every time we read from it. Why the extra func/defer though? Couldn't we just have a loop body of { v(); wg.Done() }?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although alternatively, we could instead Add once per postUnsealFuncConcurrency and Done when the worker exits.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I suspect so. The defer would still let us Done in a panic, but we'd have bigger problems then.

@sgmiller sgmiller merged commit 3c842fb into main Dec 12, 2022
@sgmiller sgmiller deleted the parallize-initfuncs branch December 12, 2022 23:07
AnPucel pushed a commit that referenced this pull request Jan 14, 2023
* Initial worker pool

* Run postUnsealFuncs in parallel

* Use the old logic for P=1

* changelog

* Use a CPU count relative worker pool

* Update vault/core.go

Co-authored-by: Nick Cabatoff <ncabatoff@hashicorp.com>

* Done must be called once per postUnsealFunc

* Defer is overkill

Co-authored-by: Nick Cabatoff <ncabatoff@hashicorp.com>
cipherboy added a commit that referenced this pull request Jan 27, 2023
In further review with new understanding after #18244, loading
configuration and CRLs within the backend's initialize function is the
ideal approach: Factory construction is strictly serial, resulting in
backend initialization blocking until config and CRLs are loaded.
By using an InitializeFunc(...), we delay loading until after all
backends are constructed (either right on startup in 1.12+, else during
the initial PeriodicFunc(...) invocation on 1.11 and earlier).

We also invoke initialize automatically on test Factory construction.

Resolves: #17847

Co-authored-by: valli_0x <personallune@mail.ru>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
cipherboy added a commit that referenced this pull request Jan 27, 2023
* Move cert auth backend setup into initialize

In further review with new understanding after #18244, loading
configuration and CRLs within the backend's initialize function is the
ideal approach: Factory construction is strictly serial, resulting in
backend initialization blocking until config and CRLs are loaded.
By using an InitializeFunc(...), we delay loading until after all
backends are constructed (either right on startup in 1.12+, else during
the initial PeriodicFunc(...) invocation on 1.11 and earlier).

We also invoke initialize automatically on test Factory construction.

Resolves: #17847

Co-authored-by: valli_0x <personallune@mail.ru>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add changelog entry

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

---------

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Co-authored-by: valli_0x <personallune@mail.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants