-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
verifyInitStatus(i, i < 2) | ||
if i == 1 { | ||
cluster.UnsealCore(t, core) | ||
time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well.
There was a problem hiding this 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!
Actually this seems to be incomplete. Brian's comments in the jira:
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.
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.
Fixes #9618. |
vault.TestWaitActive(t, leaderCore.Core) | ||
} | ||
|
||
joinFunc := func(client *api.Client, addClientCerts bool) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call.
Also make half-joined raft peers consider storage to be initialized, whether or not they're sealed.
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.