Skip to content

Conversation

@fcofdez
Copy link
Contributor

@fcofdez fcofdez commented Sep 1, 2021

Move the shard to a replica in an older version when the primary
is located in the upgraded node during the first rolling restart
round.

Closes #76595

Move the shard to a replica in an older version when the primary
is located in the upgraded node during the first rolling restart
round.

Closes elastic#76595
@fcofdez fcofdez added >test Issues or PRs that are addressing/adding tests :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. v8.0.0 Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.16.0 labels Sep 1, 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.

I left a couple of comments.

String currentPrimaryNodeId = getPrimaryNodeIdOfShard(indexName, 0);
assertThat(getNodeVersion(currentPrimaryNodeId), is(equalTo(UPGRADE_FROM_VERSION)));

updateIndexSettings(
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 this risks balancing back the shard to the upgraded node. You can disable balancing and reenable it after reestablishing replica. Or we could just keep the exclusion until the next round. I think I prefer the latter.

// Drop replicas
updateIndexSettings(indexName, Settings.builder().put(IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.getKey(), 0));
if (FIRST_MIXED_ROUND) {
String primaryNodeId = getPrimaryNodeIdOfShard(indexName, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should not instead just find the node on the new version and put the exclusion in always? I think there is a risk of the shard moving between the check here and adding replicas below.

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.


// Drop replicas
updateIndexSettings(indexName, Settings.builder().put(IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.getKey(), 0));
ensureGreen(indexName);
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think this ensureGreen does anything?

}
}

private void updateIndexSettings(String indexName, Settings settings) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks nearly identical to the static updateIndexSettings inherited from ESRestTestCase, do we need this?

@fcofdez
Copy link
Contributor Author

fcofdez commented Sep 28, 2021

@elasticmachine update branch

@fcofdez
Copy link
Contributor Author

fcofdez commented Sep 29, 2021

@elasticmachine update branch

@fcofdez fcofdez merged commit 2fdb5a8 into elastic:master Sep 29, 2021
@fcofdez fcofdez added the auto-backport Automatically create backport pull requests when merged label Sep 29, 2021
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
7.x

fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request Sep 29, 2021
Move the shard to a replica in an older version when the primary
is located in the upgraded node during the first rolling restart
round.

Closes elastic#76595
fcofdez added a commit that referenced this pull request Sep 29, 2021
Move the shard to a replica in an older version when the primary
is located in the upgraded node during the first rolling restart
round.

Closes #76595
Backport of #77134
@jakelandis jakelandis removed the v8.0.0 label Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. >test Issues or PRs that are addressing/adding tests v7.16.0 v8.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] SnapshotBasedRecoveryIT testSnapshotBasedRecovery failing

5 participants