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

Don't bootstrap when bootstrap_expect is zero #9672

Merged
merged 4 commits into from
Jan 4, 2021

Conversation

notnoop
Copy link
Contributor

@notnoop notnoop commented Dec 18, 2020

When bootstrap_expect is 0, the Nomad server shouldn't attempt to bootstrap and elect itself as the leader. This is the behavior of Consul, the previous Nomad behavior, as well as intended behaviod.

The behavior was accidentally changed in https://github.com/hashicorp/nomad/pull/7252/files#diff-adc7208ec155586c7694004df7060da8916517d28041e0660d171d73bfdfee47L86-R86 . Prior to that, Nomad only attempted to bootstrap if BootstrapExpect is non-zero, while the change made it such that bootstrapping is always attempted regardless of bootstrap expect value.

Added a failing test in https://app.circleci.com/pipelines/github/hashicorp/nomad/13931/workflows/4f71cf9d-5617-4ce1-b92c-749cfddf05f2/jobs/127419 .

@notnoop notnoop requested a review from schmichael December 18, 2020 13:19
@notnoop notnoop self-assigned this Dec 18, 2020
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

Great find and the test fills an important gap. Don't forget the changelog entry.

Comment on lines 476 to 482
time.Sleep(500 * time.Millisecond)

bootstrapped := atomic.LoadInt32(&s1.config.Bootstrapped)
require.Zero(t, bootstrapped, "expecting non-bootstrapped servers")

p, _ := s1.numPeers()
require.Zero(t, p, "number of peers in Raft")
Copy link
Member

Choose a reason for hiding this comment

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

Can this be put in a WaitForResult instead of using a sleep? If not please comment on the sleep so it's clear why sleeping is the preferred approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's tricky, as we are testing for a negative in a concurrent environment: the server starts in non-bootstrap mode with 0 peers, and we want to test that the server remains so even it registers itself into serf. Without time delay, the test may pass just because the server hasn't setup raft yet. The sleep doesn't completely rule out that possibility, but makes it less likely.

I'll add a note.

@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui January 4, 2021 14:10 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad January 4, 2021 14:10 Inactive
@notnoop notnoop merged commit 2c2c4e0 into master Jan 4, 2021
@notnoop notnoop deleted the b-bootstrapping-unexpectedly branch January 4, 2021 14:26
notnoop pushed a commit that referenced this pull request Mar 18, 2021
Don't bootstrap when `bootstrap_expect` is zero
@github-actions
Copy link

github-actions bot commented Dec 5, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants