From 25ad020ae894832b0862ad8ad89425569d693b4e Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Mon, 22 Feb 2021 15:42:09 +0100 Subject: [PATCH] Simplify BwC for UUIDs in RepositoryData (#69335) We don't need to separately handle BwC for these two fields since they were both introduced in `7.12`. --- .../s3/S3BlobStoreRepositoryTests.java | 2 +- .../MultiVersionRepositoryAccessIT.java | 4 +- .../cluster/metadata/RepositoryMetadata.java | 6 +- .../repositories/RepositoryData.java | 90 ++++--------------- .../blobstore/BlobStoreRepository.java | 9 +- .../snapshots/SnapshotsService.java | 23 ++--- .../repositories/RepositoryDataTests.java | 8 +- .../AbstractSnapshotIntegTestCase.java | 6 +- 8 files changed, 33 insertions(+), 115 deletions(-) diff --git a/plugins/repository-s3/src/internalClusterTest/java/org/elasticsearch/repositories/s3/S3BlobStoreRepositoryTests.java b/plugins/repository-s3/src/internalClusterTest/java/org/elasticsearch/repositories/s3/S3BlobStoreRepositoryTests.java index 415ee1be2db55..94c96dbc31776 100644 --- a/plugins/repository-s3/src/internalClusterTest/java/org/elasticsearch/repositories/s3/S3BlobStoreRepositoryTests.java +++ b/plugins/repository-s3/src/internalClusterTest/java/org/elasticsearch/repositories/s3/S3BlobStoreRepositoryTests.java @@ -147,7 +147,7 @@ public void testEnforcedCooldownPeriod() throws IOException { final BlobStoreRepository repository = (BlobStoreRepository) repositoriesService.repository(repoName); final RepositoryData repositoryData = getRepositoryData(repository); final RepositoryData modifiedRepositoryData = repositoryData.withVersions(Collections.singletonMap(fakeOldSnapshot, - SnapshotsService.SHARD_GEN_IN_REPO_DATA_VERSION.minimumCompatibilityVersion())).withoutRepositoryUUID().withoutClusterUUID(); + SnapshotsService.SHARD_GEN_IN_REPO_DATA_VERSION.minimumCompatibilityVersion())).withoutUUIDs(); final BytesReference serialized = BytesReference.bytes(modifiedRepositoryData.snapshotsToXContent(XContentFactory.jsonBuilder(), SnapshotsService.OLD_SNAPSHOT_FORMAT)); PlainActionFuture.get(f -> repository.threadPool().generic().execute(ActionRunnable.run(f, () -> diff --git a/qa/repository-multi-version/src/test/java/org/elasticsearch/upgrades/MultiVersionRepositoryAccessIT.java b/qa/repository-multi-version/src/test/java/org/elasticsearch/upgrades/MultiVersionRepositoryAccessIT.java index b7e7119d4fa91..f3d9e70fa5dc0 100644 --- a/qa/repository-multi-version/src/test/java/org/elasticsearch/upgrades/MultiVersionRepositoryAccessIT.java +++ b/qa/repository-multi-version/src/test/java/org/elasticsearch/upgrades/MultiVersionRepositoryAccessIT.java @@ -195,7 +195,7 @@ public void testUpgradeMovesRepoToNewMetaVersion() throws IOException { // 7.12.0+ will try to load RepositoryData during repo creation if verify is true, which is impossible in case of version // incompatibility in the downgrade test step. We verify that it is impossible here and then create the repo using verify=false // to check behavior on other operations below. - final boolean verify = TEST_STEP != TestStep.STEP3_OLD_CLUSTER || SnapshotsService.includesClusterUUID(minNodeVersion) + final boolean verify = TEST_STEP != TestStep.STEP3_OLD_CLUSTER || SnapshotsService.includesUUIDs(minNodeVersion) || minNodeVersion.before(Version.V_7_12_0); if (verify == false) { expectThrowsAnyOf(EXPECTED_BWC_EXCEPTIONS, () -> createRepository(client, repoName, false, true)); @@ -221,7 +221,7 @@ public void testUpgradeMovesRepoToNewMetaVersion() throws IOException { ensureSnapshotRestoreWorks(repoName, "snapshot-2", shards); } } else { - if (SnapshotsService.includesClusterUUID(minNodeVersion) == false) { + if (SnapshotsService.includesUUIDs(minNodeVersion) == false) { assertThat(TEST_STEP, is(TestStep.STEP3_OLD_CLUSTER)); expectThrowsAnyOf(EXPECTED_BWC_EXCEPTIONS, () -> listSnapshots(repoName)); expectThrowsAnyOf(EXPECTED_BWC_EXCEPTIONS, () -> deleteSnapshot(client, repoName, "snapshot-1")); diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/RepositoryMetadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/RepositoryMetadata.java index 833fbce4cb38f..7d72307f2c834 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/RepositoryMetadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/RepositoryMetadata.java @@ -87,7 +87,7 @@ public String type() { /** * Return the repository UUID, if set and known. The repository UUID is stored in the repository and typically populated here when the * repository is registered or when we write to it. It may not be set if the repository is maintaining support for versions before - * {@link SnapshotsService#REPOSITORY_UUID_IN_REPO_DATA_VERSION}. It may not be known if the repository was registered with {@code + * {@link SnapshotsService#UUIDS_IN_REPO_DATA_VERSION}. It may not be known if the repository was registered with {@code * ?verify=false} and has had no subsequent writes. Consumers may, if desired, try and fill in a missing value themselves by retrieving * the {@link RepositoryData} and calling {@link org.elasticsearch.repositories.RepositoriesService#updateRepositoryUuidInMetadata}. * @@ -132,7 +132,7 @@ public long pendingGeneration() { public RepositoryMetadata(StreamInput in) throws IOException { name = in.readString(); - if (in.getVersion().onOrAfter(SnapshotsService.REPOSITORY_UUID_IN_REPO_DATA_VERSION)) { + if (in.getVersion().onOrAfter(SnapshotsService.UUIDS_IN_REPO_DATA_VERSION)) { uuid = in.readString(); } else { uuid = RepositoryData.MISSING_UUID; @@ -156,7 +156,7 @@ public RepositoryMetadata(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(name); - if (out.getVersion().onOrAfter(SnapshotsService.REPOSITORY_UUID_IN_REPO_DATA_VERSION)) { + if (out.getVersion().onOrAfter(SnapshotsService.UUIDS_IN_REPO_DATA_VERSION)) { out.writeString(uuid); } out.writeString(type); diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java b/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java index 8f7fa70e62495..f031655557290 100644 --- a/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java +++ b/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java @@ -164,7 +164,9 @@ private RepositoryData( this.shardGenerations = shardGenerations; this.indexMetaDataGenerations = indexMetaDataGenerations; this.snapshotVersions = snapshotVersions; - this.clusterUUID = clusterUUID; + this.clusterUUID = Objects.requireNonNull(clusterUUID); + assert uuid.equals(MISSING_UUID) == clusterUUID.equals(MISSING_UUID) : "Either repository- and cluster UUID must both be missing" + + " or neither of them must be missing but saw [" + uuid + "][" + clusterUUID + "]"; assert indices.values().containsAll(shardGenerations.indices()) : "ShardGenerations contained indices " + shardGenerations.indices() + " but snapshots only reference indices " + indices.values(); assert indexSnapshots.values().stream().noneMatch(snapshotIdList -> new HashSet<>(snapshotIdList).size() != snapshotIdList.size()) : @@ -215,7 +217,7 @@ public ShardGenerations shardGenerations() { /** * @return The UUID of this repository, or {@link RepositoryData#MISSING_UUID} if this repository has no UUID because it still - * supports access from versions earlier than {@link SnapshotsService#REPOSITORY_UUID_IN_REPO_DATA_VERSION}. + * supports access from versions earlier than {@link SnapshotsService#UUIDS_IN_REPO_DATA_VERSION}. */ public String getUuid() { return uuid; @@ -223,7 +225,7 @@ public String getUuid() { /** * @return the cluster UUID of the cluster that wrote this instance to the repository or {@link RepositoryData#MISSING_UUID} if this - * instance was written by a cluster older than {@link SnapshotsService#CLUSTER_UUID_IN_REPO_DATA_VERSION}. + * instance was written by a cluster older than {@link SnapshotsService#UUIDS_IN_REPO_DATA_VERSION}. */ public String getClusterUUID() { return clusterUUID; @@ -404,28 +406,10 @@ public RepositoryData withGenId(long newGeneration) { } /** - * Make a copy of this instance with the given UUID and all other fields unchanged. - */ - public RepositoryData withUuid(String uuid) { - assert this.uuid.equals(MISSING_UUID) : this.uuid; - assert uuid.equals(MISSING_UUID) == false; - return new RepositoryData( - uuid, - genId, - snapshotIds, - snapshotStates, - snapshotVersions, - indices, - indexSnapshots, - shardGenerations, - indexMetaDataGenerations, - clusterUUID); - } - - /** - * For test purposes, make a copy of this instance with the UUID removed and all other fields unchanged, as if from an older version. + * For test purposes, make a copy of this instance with the cluster- and repository UUIDs removed and all other fields unchanged, + * as if from an older version. */ - public RepositoryData withoutRepositoryUUID() { + public RepositoryData withoutUUIDs() { return new RepositoryData( MISSING_UUID, genId, @@ -436,31 +420,13 @@ public RepositoryData withoutRepositoryUUID() { indexSnapshots, shardGenerations, indexMetaDataGenerations, - clusterUUID); - } - - /** - * For test purposes, make a copy of this instance with the cluster UUID removed and all other fields unchanged, as if from an older - * version. - */ - public RepositoryData withoutClusterUUID() { - return new RepositoryData( - uuid, - genId, - snapshotIds, - snapshotStates, - snapshotVersions, - indices, - indexSnapshots, - shardGenerations, - indexMetaDataGenerations, MISSING_UUID); } public RepositoryData withClusterUuid(String clusterUUID) { assert clusterUUID.equals(MISSING_UUID) == false; return new RepositoryData( - uuid, + uuid.equals(MISSING_UUID) ? UUIDs.randomBase64UUID() : uuid, genId, snapshotIds, snapshotStates, @@ -472,24 +438,6 @@ public RepositoryData withClusterUuid(String clusterUUID) { clusterUUID); } - /** - * For test purposes, make a copy of this instance with the cluster UUID removed and all other fields unchanged, - * as if from an older version. - */ - public RepositoryData withoutClusterUuid() { - return new RepositoryData( - uuid, - genId, - snapshotIds, - snapshotStates, - snapshotVersions, - indices, - indexSnapshots, - shardGenerations, - indexMetaDataGenerations, - MISSING_UUID); - } - /** * Remove snapshots and remove any indices that no longer exist in the repository due to the deletion of the snapshots. * @@ -647,13 +595,11 @@ public XContentBuilder snapshotsToXContent( final Version repoMetaVersion, boolean permitMissingUuid) throws IOException { - final boolean shouldWriteClusterId = SnapshotsService.includesClusterUUID(repoMetaVersion); - final boolean shouldWriteRepoUuid = SnapshotsService.includesRepositoryUuid(repoMetaVersion); + final boolean shouldWriteUUIDS = SnapshotsService.includesUUIDs(repoMetaVersion); final boolean shouldWriteIndexGens = SnapshotsService.useIndexGenerations(repoMetaVersion); final boolean shouldWriteShardGens = SnapshotsService.useShardGenerations(repoMetaVersion); - assert Boolean.compare(shouldWriteClusterId, shouldWriteRepoUuid) <= 0; - assert Boolean.compare(shouldWriteRepoUuid, shouldWriteIndexGens) <= 0; + assert Boolean.compare(shouldWriteUUIDS, shouldWriteIndexGens) <= 0; assert Boolean.compare(shouldWriteIndexGens, shouldWriteShardGens) <= 0; builder.startObject(); @@ -661,10 +607,8 @@ public XContentBuilder snapshotsToXContent( if (shouldWriteShardGens) { // Add min version field to make it impossible for older ES versions to deserialize this object final Version minVersion; - if (shouldWriteClusterId) { - minVersion = SnapshotsService.CLUSTER_UUID_IN_REPO_DATA_VERSION; - } else if (shouldWriteRepoUuid) { - minVersion = SnapshotsService.REPOSITORY_UUID_IN_REPO_DATA_VERSION; + if (shouldWriteUUIDS) { + minVersion = SnapshotsService.UUIDS_IN_REPO_DATA_VERSION; } else if (shouldWriteIndexGens) { minVersion = SnapshotsService.INDEX_GEN_IN_REPO_DATA_VERSION; } else { @@ -673,23 +617,19 @@ public XContentBuilder snapshotsToXContent( builder.field(MIN_VERSION, minVersion.toString()); } - if (shouldWriteRepoUuid) { + if (shouldWriteUUIDS) { if (uuid.equals(MISSING_UUID)) { assert permitMissingUuid : "missing uuid"; } else { builder.field(UUID, uuid); } - } else { - assert uuid.equals(MISSING_UUID) : "lost uuid " + uuid; - } - - if (shouldWriteClusterId) { if (clusterUUID.equals(MISSING_UUID)) { assert permitMissingUuid : "missing clusterUUID"; } else { builder.field(CLUSTER_UUID, clusterUUID); } } else { + assert uuid.equals(MISSING_UUID) : "lost uuid " + uuid; assert clusterUUID.equals(MISSING_UUID) : "lost clusterUUID " + clusterUUID; } diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java index 7b31e33c91f66..ce87c8f429fb7 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java @@ -139,7 +139,6 @@ import java.util.stream.Stream; import static org.elasticsearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshot.FileInfo.canonicalName; -import static org.elasticsearch.repositories.RepositoryData.MISSING_UUID; /** * BlobStore - based implementation of Snapshot Repository @@ -1923,17 +1922,13 @@ public void clusterStateProcessed(String source, ClusterState oldState, ClusterS } private RepositoryData updateRepositoryData(RepositoryData repositoryData, Version repositoryMetaversion, long newGen) { - if (SnapshotsService.includesClusterUUID(repositoryMetaversion)) { + if (SnapshotsService.includesUUIDs(repositoryMetaversion)) { final String clusterUUID = clusterService.state().metadata().clusterUUID(); if (repositoryData.getClusterUUID().equals(clusterUUID) == false) { repositoryData = repositoryData.withClusterUuid(clusterUUID); } } - if (SnapshotsService.includesRepositoryUuid(repositoryMetaversion) && repositoryData.getUuid().equals(MISSING_UUID)) { - return repositoryData.withGenId(newGen).withUuid(UUIDs.randomBase64UUID()); - } else { - return repositoryData.withGenId(newGen); - } + return repositoryData.withGenId(newGen); } /** diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java index bd7a85fc24bff..13c6e8c99cf99 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java @@ -131,10 +131,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus public static final Version INDEX_GEN_IN_REPO_DATA_VERSION = Version.V_7_9_0; - public static final Version REPOSITORY_UUID_IN_REPO_DATA_VERSION = Version.V_7_12_0; - - // TODO: fold this into #REPOSITORY_UUID_IN_REPO_DATA_VERSION and remove separate handling of uuid and clusterUUID BwC where possible - public static final Version CLUSTER_UUID_IN_REPO_DATA_VERSION = Version.V_7_12_0; + public static final Version UUIDS_IN_REPO_DATA_VERSION = Version.V_7_12_0; public static final Version OLD_SNAPSHOT_FORMAT = Version.V_7_5_0; @@ -2443,23 +2440,13 @@ public static boolean useIndexGenerations(Version repositoryMetaVersion) { } /** - * Checks whether the metadata version supports writing a repository uuid to the repository. - * - * @param repositoryMetaVersion version to check - * @return true if version supports writing repository uuid to the repository - */ - public static boolean includesRepositoryUuid(Version repositoryMetaVersion) { - return repositoryMetaVersion.onOrAfter(REPOSITORY_UUID_IN_REPO_DATA_VERSION); - } - - /** - * Checks whether the metadata version supports writing the cluster uuid to the repository. + * Checks whether the metadata version supports writing the cluster- and repository-uuid to the repository. * * @param repositoryMetaVersion version to check - * @return true if version supports {@link ShardGenerations} + * @return true if version supports writing cluster- and repository-uuid to the repository */ - public static boolean includesClusterUUID(Version repositoryMetaVersion) { - return repositoryMetaVersion.onOrAfter(CLUSTER_UUID_IN_REPO_DATA_VERSION); + public static boolean includesUUIDs(Version repositoryMetaVersion) { + return repositoryMetaVersion.onOrAfter(UUIDS_IN_REPO_DATA_VERSION); } /** Deletes snapshot from repository diff --git a/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java b/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java index a5bff7c0f4ec9..227cbf16666bf 100644 --- a/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java @@ -65,8 +65,7 @@ public void testIndicesToUpdateAfterRemovingSnapshot() { } public void testXContent() throws IOException { - RepositoryData repositoryData = - generateRandomRepoData().withUuid(UUIDs.randomBase64UUID(random())).withClusterUuid(UUIDs.randomBase64UUID(random())); + RepositoryData repositoryData = generateRandomRepoData().withClusterUuid(UUIDs.randomBase64UUID(random())); XContentBuilder builder = JsonXContent.contentBuilder(); repositoryData.snapshotsToXContent(builder, Version.CURRENT); try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(builder))) { @@ -144,7 +143,7 @@ public void testInitIndices() { // test that initializing indices works Map> indices = randomIndices(snapshotIds); RepositoryData newRepoData = new RepositoryData( - repositoryData.getUuid(), + UUIDs.randomBase64UUID(random()), repositoryData.getGenId(), snapshotIds, snapshotStates, @@ -198,8 +197,7 @@ public void testGetSnapshotState() { public void testIndexThatReferencesAnUnknownSnapshot() throws IOException { final XContent xContent = randomFrom(XContentType.values()).xContent(); - final RepositoryData repositoryData = - generateRandomRepoData().withUuid(UUIDs.randomBase64UUID()).withClusterUuid(UUIDs.randomBase64UUID(random())); + final RepositoryData repositoryData = generateRandomRepoData().withClusterUuid(UUIDs.randomBase64UUID(random())); XContentBuilder builder = XContentBuilder.builder(xContent); repositoryData.snapshotsToXContent(builder, Version.CURRENT); diff --git a/test/framework/src/main/java/org/elasticsearch/snapshots/AbstractSnapshotIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/snapshots/AbstractSnapshotIntegTestCase.java index e30b795634776..69a9283d36964 100644 --- a/test/framework/src/main/java/org/elasticsearch/snapshots/AbstractSnapshotIntegTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/snapshots/AbstractSnapshotIntegTestCase.java @@ -148,10 +148,8 @@ protected void disableRepoConsistencyCheck(String reason) { protected RepositoryData getRepositoryData(String repoName, Version version) { final RepositoryData repositoryData = getRepositoryData(repoName); - if (SnapshotsService.includesRepositoryUuid(version) == false) { - return repositoryData.withoutRepositoryUUID().withoutClusterUUID(); - } else if (SnapshotsService.includesClusterUUID(version) == false) { - return repositoryData.withoutClusterUuid(); + if (SnapshotsService.includesUUIDs(version) == false) { + return repositoryData.withoutUUIDs(); } else { return repositoryData; }