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

Omit master-ineligible nodes from initial_master_nodes #93131

Conversation

DaveCTurner
Copy link
Contributor

Today the test-clusters framework sets cluster.initial_master_nodes to the names of all the nodes in the cluster, but this only makes sense if they're all master-eligible. This commit filters this setting down to just the master-eligible node names.

Today the `test-clusters` framework sets `cluster.initial_master_nodes`
to the names of all the nodes in the cluster, but this only makes sense
if they're all master-eligible. This commit filters this setting down to
just the master-eligible node names.
@DaveCTurner DaveCTurner added >bug :Delivery/Build Build or test infrastructure v8.7.0 labels Jan 23, 2023
@DaveCTurner
Copy link
Contributor Author

NB not sure this is the best way to identify the master-eligible nodes.

@elasticsearchmachine elasticsearchmachine added the Team:Delivery Meta label for Delivery team label Jan 23, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

This probably won't work as-is. DefaultSetrtingsProvider is exactly that, a "default". It won't see settings added later by the user or other settings providers. So if a user configures roles on a cluster this code will actually run before that happens. We'll have to defer to setup of initial_master_nodes until after any other settings have been configured. I can take a stab at this.

This is another item that's always historically been the case. Is this problematic now, or just non-optimal?

@DaveCTurner
Copy link
Contributor Author

I stumbled across it while wanting to run a stateless cluster with a dedicated master, a dedicated indexing node and a dedicated search node so I could attach the right debuggers to the right nodes. I can live with them all being master-eligible for now.

@mark-vieira
Copy link
Contributor

I stumbled across it while wanting to run a stateless cluster with a dedicated master, a dedicated indexing node and a dedicated search node so I could attach the right debuggers to the right nodes. I can live with them all being master-eligible for now.

Will the cluster you describe fail to start as implemented today? Do all nodes have to be configured master-eligible?

@DaveCTurner
Copy link
Contributor Author

Will the cluster you describe fail to start as implemented today?

Yes.

Do all nodes have to be configured master-eligible?

Technically it works if >½ of them are master-eligible.

@mark-vieira
Copy link
Contributor

Ok, we should definitely resolve this then. I'll take a look at the best way to pass this setting.

@mark-vieira
Copy link
Contributor

Closing in favor of #93212

@DaveCTurner DaveCTurner deleted the 2023-01-23-test-clusters-initial_master_nodes branch January 24, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants