Skip to content

Commit

Permalink
Noop repository update should skip verification (#76067)
Browse files Browse the repository at this point in the history
It is common to do an idempotent PUT of a repo, just to ensure it is
there. This commit ensures that if the change is in fact a noop change,
we no longer do any repository verification, since a node having trouble
(for instance running out of heap) could then mean such a no-change
repo update failing.

Closes #76012
  • Loading branch information
henningandersen committed Aug 4, 2021
1 parent ff5017b commit 27edd44
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.client.Client;
import org.elasticsearch.cluster.metadata.RepositoryMetadata;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.env.Environment;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.repositories.fs.FsRepository;
import org.elasticsearch.snapshots.mockstore.MockRepository;
Expand Down Expand Up @@ -81,5 +82,12 @@ public void testUpdateRepository() {

final Repository updatedRepository = repositoriesService.repository(repositoryName);
assertThat(updatedRepository, updated ? not(sameInstance(originalRepository)) : sameInstance(originalRepository));

// check that a noop update does not verify. Since the new data node does not share the same `path.repo`, verification will fail if
// it runs.
internalCluster().startDataOnlyNode(Settings.builder().put(Environment.PATH_REPO_SETTING.getKey(), createTempDir()).build());
assertAcked(
client.admin().cluster().preparePutRepository(repositoryName).setType(updatedRepositoryType).setSettings(repoSettings).get()
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -147,16 +147,16 @@ public void registerRepository(final PutRepositoryRequest request, final ActionL
}

final StepListener<AcknowledgedResponse> acknowledgementStep = new StepListener<>();
final StepListener<Void> publicationStep = new StepListener<>();
final StepListener<Boolean> publicationStep = new StepListener<>(); // Boolean==changed.

if (request.verify()) {

// When publication has completed (and all acks received or timed out) then verify the repository.
// (if acks timed out then acknowledgementStep completes before the master processes this cluster state, hence why we have
// to wait for the publication to be complete too)
final StepListener<List<DiscoveryNode>> verifyStep = new StepListener<>();
publicationStep.whenComplete(ignored -> acknowledgementStep.whenComplete(clusterStateUpdateResponse -> {
if (clusterStateUpdateResponse.isAcknowledged()) {
publicationStep.whenComplete(changed -> acknowledgementStep.whenComplete(clusterStateUpdateResponse -> {
if (clusterStateUpdateResponse.isAcknowledged() && changed) {
// The response was acknowledged - all nodes should know about the new repository, let's verify them
verifyRepository(request.name(), verifyStep);
} else {
Expand Down Expand Up @@ -201,10 +201,6 @@ public ClusterState execute(ClusterState currentState) {
List<RepositoryMetadata> repositoriesMetadata = new ArrayList<>(repositories.repositories().size() + 1);
for (RepositoryMetadata repositoryMetadata : repositories.repositories()) {
if (repositoryMetadata.name().equals(newRepositoryMetadata.name())) {
if (newRepositoryMetadata.equalsIgnoreGenerations(repositoryMetadata)) {
// Previous version is the same as this one no update is needed.
return currentState;
}
Repository existing = RepositoriesService.this.repositories.get(request.name());
if (existing == null) {
existing = RepositoriesService.this.internalRepositories.get(request.name());
Expand All @@ -213,6 +209,10 @@ public ClusterState execute(ClusterState currentState) {
assert existing.getMetadata() == repositoryMetadata;
final RepositoryMetadata updatedMetadata;
if (canUpdateInPlace(newRepositoryMetadata, existing)) {
if (repositoryMetadata.settings().equals(newRepositoryMetadata.settings())) {
// Previous version is the same as this one no update is needed.
return currentState;
}
// we're updating in place so the updated metadata must point at the same uuid and generations
updatedMetadata = repositoryMetadata.withSettings(newRepositoryMetadata.settings());
} else {
Expand Down Expand Up @@ -256,7 +256,7 @@ public void clusterStateProcessed(String source, ClusterState oldState, ClusterS
logger.info("put repository [{}]", request.name());
}
}
publicationStep.onResponse(null);
publicationStep.onResponse(oldState != newState);
}
}
);
Expand Down

0 comments on commit 27edd44

Please sign in to comment.