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

Be consistent with how we report init status. #10498

Merged
merged 7 commits into from
Dec 8, 2020

Conversation

ncabatoff
Copy link
Contributor

@ncabatoff ncabatoff commented Dec 4, 2020

This is the same fix as #9652, together with changing the seal-status api to behave the same as the health and init apis, i.e. use Core.Initialized to govern what we return as our initialized status.

@ncabatoff ncabatoff marked this pull request as ready for review December 4, 2020 19:15
verifyInitStatus(i, i < 2)
if i == 1 {
cluster.UnsealCore(t, core)
time.Sleep(5 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

I am sure you might have thought of not using sleep here. Would it be possible to use RetryUntil here?

}

for i := range cluster.Cores {
verifyInitStatus(i, i < 1)
Copy link
Member

Choose a reason for hiding this comment

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

The < logic is neat!

verifyInitStatus(i, true)
if i == 2 {
cluster.UnsealCore(t, core)
time.Sleep(5 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

Here as well.

Copy link
Member

@vishalnayak vishalnayak left a comment

Choose a reason for hiding this comment

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

Left, optional test comments. Otherwise LGTM!

@ncabatoff
Copy link
Contributor Author

Actually this seems to be incomplete. Brian's comments in the jira:

I think it logically makes sense that a node that has joined a raft cluster but has yet to be unsealed is, for all intents and purposes, initialized

Prior to the unseal/sleep the init status appears as false, which doesn't fit with the above.

…hed bootstrapping consider storage to be initialized.
@ncabatoff
Copy link
Contributor Author

Prior to the unseal/sleep the init status appears as false, which doesn't fit with the above.

Correction: it appears as true prior to unseal after the join, as well as after join+unseal, but in between it could report init=false. This is when we've bootstrapped enough to set Core.raftInfo = nil, but haven't yet written the master key and other things Core.Initialized checks. Fixed by checking for whether the raft backend is initialized in that case, which would happen before we set raftInfo=nil.

…alized in some places. Also did the same for a couple of other places where the new behaviour is questionable.
@ncabatoff
Copy link
Contributor Author

Fixes #9618.

vault.TestWaitActive(t, leaderCore.Core)
}

joinFunc := func(client *api.Client, addClientCerts bool) {
Copy link
Member

Choose a reason for hiding this comment

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

May be the join function can do away with the addClientCerts logic as we are only setting it to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call.

vault/external_tests/raft/raft_test.go Show resolved Hide resolved
@ncabatoff ncabatoff added this to the 1.6.1 milestone Dec 8, 2020
@ncabatoff ncabatoff merged commit 9459932 into master Dec 8, 2020
@ncabatoff ncabatoff deleted the consistent-init-status-apis branch December 8, 2020 18:55
@ncabatoff ncabatoff self-assigned this Dec 14, 2020
ncabatoff added a commit that referenced this pull request Dec 14, 2020
Also make half-joined raft peers consider storage to be initialized, whether or not they're sealed.
ncabatoff added a commit that referenced this pull request Dec 14, 2020
Also make half-joined raft peers consider storage to be initialized, whether or not they're sealed.
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.

3 participants