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

Add entry to Deprecation API for CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING #73552

Merged

Conversation

fcofdez
Copy link
Contributor

@fcofdez fcofdez commented May 31, 2021

Relates #47717

@fcofdez fcofdez added :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) Team:Distributed Meta label for distributed team v7.14.0 labels May 31, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

The setting is dynamic and I wonder if we need a cluster check too. While the setting will be archived, behavior will change if the setting is set to false in cluster settings.

Let me follow-up on that outside this issue.


public void testClusterRoutingAllocationIncludeRelocationsSetting() {
Settings settings = Settings.builder()
.put(CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING.getKey(), false).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think both setting to false and true is deprecated, so suggest to randomize:

Suggested change
.put(CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING.getKey(), false).build();
.put(CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING.getKey(), randomBoolean()).build();


static DeprecationIssue checkClusterRoutingAllocationIncludeRelocationsSetting(final Settings settings,
final PluginsAndModules pluginsAndModules) {
if (CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING.exists(settings)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reuse checkRemovedSetting here?

return checkRemovedSetting(settings,
CLUSTER_ROUTING_ALLOCATION_INCLUDE_RELOCATIONS_SETTING,
"https://www.elastic.co/guide/en/elasticsearch/reference/master/migrating-8.0.html#breaking_80_allocation_changes",
DeprecationIssue.Level.WARNING
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's CRITICAL at least in the case where it's in the node settings. If it's set dynamically then that's technically ok to proceed, we'll just archive it, but that's also a case we can fix at runtime so maybe not so bad to call it CRITICAL too? Will the upgrade assistant automatically help users remove this setting if set dynamically or do we need to take action on that front too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to CRITICAL when it's in the node settings. I think we could tackle the case when the setting is set dynamically with the upgrade assistant, we just need to open a ticket in Kibana describing how the assistant can mitigate the problem. I'll open the ticket.

@fcofdez
Copy link
Contributor Author

fcofdez commented Jun 28, 2021

@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM, left an optional comment (can be tackled in a follow-up too if you prefer).

Comment on lines 563 to 567
// The setting is dynamic so it can be defined in the ClusterState settings
return getClusterRoutingAllocationIncludeRelocationsSettingDeprecationIssue(
clusterState.metadata().settings(),
DeprecationIssue.Level.WARNING
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to have this part of the check added separately to DeprecationChecks.CLUSTER_SETTINGS_CHECKS, to avoid having a deprecation issue per node.

@fcofdez
Copy link
Contributor Author

fcofdez commented Jun 29, 2021

@elasticmachine run elasticsearch-ci/part-2

@fcofdez fcofdez merged commit c998213 into elastic:7.x Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>deprecation :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) Team:Distributed Meta label for distributed team v7.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants