From 215f51c07f9434a117c37bb7b4601d7406f02c19 Mon Sep 17 00:00:00 2001 From: Harish Bhakuni Date: Thu, 14 Mar 2024 19:36:16 -0700 Subject: [PATCH] Address PR Comments. Signed-off-by: Harish Bhakuni --- .../SegmentReplicationSnapshotIT.java | 20 ++++++++-- .../metadata/MetadataCreateIndexService.java | 2 +- .../opensearch/snapshots/RestoreService.java | 39 ++++++------------- 3 files changed, 29 insertions(+), 32 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SegmentReplicationSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SegmentReplicationSnapshotIT.java index 97b6532cdb1a1..1505d68ebc248 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/SegmentReplicationSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/SegmentReplicationSnapshotIT.java @@ -342,9 +342,20 @@ public void testSnapshotRestoreOnRestrictReplicationSetting() throws Exception { createSnapshot(); - // Delete index + RestoreSnapshotResponse restoreSnapshotResponse = restoreSnapshotWithSettings(restoreIndexSegRepSettings(), RESTORED_INDEX_NAME); + assertEquals(restoreSnapshotResponse.status(), RestStatus.ACCEPTED); + ensureGreen(RESTORED_INDEX_NAME); + GetSettingsResponse settingsResponse = client().admin() + .indices() + .getSettings(new GetSettingsRequest().indices(RESTORED_INDEX_NAME).includeDefaults(true)) + .get(); + assertEquals(settingsResponse.getSetting(RESTORED_INDEX_NAME, SETTING_REPLICATION_TYPE), ReplicationType.SEGMENT.toString()); + + // Delete indices assertAcked(client().admin().indices().delete(new DeleteIndexRequest(INDEX_NAME)).get()); assertFalse("index [" + INDEX_NAME + "] should have been deleted", indexExists(INDEX_NAME)); + assertAcked(client().admin().indices().delete(new DeleteIndexRequest(RESTORED_INDEX_NAME)).get()); + assertFalse("index [" + RESTORED_INDEX_NAME + "] should have been deleted", indexExists(RESTORED_INDEX_NAME)); // Start new set of nodes with cluster level replication type setting and restrict replication type setting. Settings settings = Settings.builder() @@ -366,17 +377,20 @@ public void testSnapshotRestoreOnRestrictReplicationSetting() throws Exception { // Perform snapshot restore logger.info("--> Performing snapshot restore to target index"); - RestoreSnapshotResponse restoreSnapshotResponse = restoreSnapshotWithSettings(null); + restoreSnapshotResponse = restoreSnapshotWithSettings(null); // Assertions assertEquals(restoreSnapshotResponse.status(), RestStatus.ACCEPTED); ensureGreen(RESTORED_INDEX_NAME); - GetSettingsResponse settingsResponse = client().admin() + settingsResponse = client().admin() .indices() .getSettings(new GetSettingsRequest().indices(RESTORED_INDEX_NAME).includeDefaults(true)) .get(); assertEquals(settingsResponse.getSetting(RESTORED_INDEX_NAME, SETTING_REPLICATION_TYPE), ReplicationType.SEGMENT.toString()); + // restore index with cluster default setting + restoreSnapshotWithSettings(restoreIndexSegRepSettings(), RESTORED_INDEX_NAME + "1"); + // Perform Snapshot Restore with different index name IllegalArgumentException exception = expectThrows( IllegalArgumentException.class, diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index 6fc1ef4038767..0e88035c02f18 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -967,7 +967,7 @@ public static void updateReplicationStrategy( final ReplicationType indexReplicationType; if (INDEX_REPLICATION_TYPE_SETTING.exists(requestSettings)) { indexReplicationType = INDEX_REPLICATION_TYPE_SETTING.get(requestSettings); - } else if (INDEX_REPLICATION_TYPE_SETTING.exists(combinedTemplateSettings)) { + } else if (combinedTemplateSettings != null && INDEX_REPLICATION_TYPE_SETTING.exists(combinedTemplateSettings)) { indexReplicationType = INDEX_REPLICATION_TYPE_SETTING.get(combinedTemplateSettings); } else if (CLUSTER_REPLICATION_TYPE_SETTING.exists(clusterSettings)) { indexReplicationType = CLUSTER_REPLICATION_TYPE_SETTING.get(clusterSettings); diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index bc256cbf74d06..3aa7e13c2b9b1 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -177,6 +177,7 @@ public class RestoreService implements ClusterStateApplier { // It's OK to change some settings, but we shouldn't allow simply removing them private static final Set USER_UNREMOVABLE_SETTINGS; + private static final String REMOTE_STORE_INDEX_SETTINGS_REGEX = "index.remote_store.*"; static { Set unremovable = new HashSet<>(USER_UNMODIFIABLE_SETTINGS.size() + 4); @@ -669,18 +670,18 @@ private String[] getIgnoreSettingsInternal() { // related index settings present in the snapshot. String[] indexSettingsToBeIgnored = new String[] {}; if (false == RemoteStoreNodeAttribute.isRemoteStoreAttributePresent(clusterService.getSettings())) { - indexSettingsToBeIgnored = ArrayUtils.concat(indexSettingsToBeIgnored, new String[] { "index.remote_store.*" }); + indexSettingsToBeIgnored = ArrayUtils.concat(indexSettingsToBeIgnored, new String[] { REMOTE_STORE_INDEX_SETTINGS_REGEX }); } return indexSettingsToBeIgnored; } private Settings getOverrideSettingsInternal() { final Settings.Builder settingsBuilder = Settings.builder(); - updateReplicationStrategy( + MetadataCreateIndexService.updateReplicationStrategy( settingsBuilder, request.indexSettings(), clusterService.getSettings(), - clusterService.getClusterSettings() + null ); // remote store settings needs to be overridden if the remote store feature is enabled in the // cluster where snapshot is being restored. @@ -688,24 +689,6 @@ private Settings getOverrideSettingsInternal() { return settingsBuilder.build(); } - private void updateReplicationStrategy( - Settings.Builder settingsBuilder, - Settings requestSettings, - Settings nodeSettings, - ClusterSettings clusterSettings - ) { - // if remote store feature is enabled, override replication strategy to segment. - // if `cluster.index.restrict.replication.type` is enabled and user have not provided - // replication strategy setting with restore request, override replication strategy - // setting to cluster default. - if (isRemoteStoreAttributePresent(nodeSettings)) { - settingsBuilder.put(SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT); - } else if (!INDEX_REPLICATION_TYPE_SETTING.exists(requestSettings) - && clusterSettings.get(IndicesService.CLUSTER_INDEX_RESTRICT_REPLICATION_TYPE_SETTING)) { - settingsBuilder.put(SETTING_REPLICATION_TYPE, clusterSettings.get(CLUSTER_REPLICATION_TYPE_SETTING).name()); - } - } - private void populateIgnoredShards(String index, final Set ignoreShards) { for (SnapshotShardFailure failure : snapshotInfo.shardFailures()) { if (index.equals(failure.index())) { @@ -778,18 +761,18 @@ private void validateExistingIndex( */ private IndexMetadata updateIndexSettings( IndexMetadata indexMetadata, - Settings changeSettings, + Settings overrideSettings, String[] ignoreSettings, - Settings changeSettingsInternal, + Settings overrideSettingsInternal, String[] ignoreSettingsInternal ) { Settings normalizedChangeSettings = Settings.builder() - .put(changeSettings) + .put(overrideSettings) .normalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX) .build(); if (IndexSettings.INDEX_SOFT_DELETES_SETTING.get(indexMetadata.getSettings()) - && IndexSettings.INDEX_SOFT_DELETES_SETTING.exists(changeSettings) - && IndexSettings.INDEX_SOFT_DELETES_SETTING.get(changeSettings) == false) { + && IndexSettings.INDEX_SOFT_DELETES_SETTING.exists(overrideSettings) + && IndexSettings.INDEX_SOFT_DELETES_SETTING.get(overrideSettings) == false) { throw new SnapshotRestoreException( snapshot, "cannot disable setting [" + IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey() + "] on restore" @@ -849,8 +832,8 @@ private IndexMetadata updateIndexSettings( })); // override internal settings - if (changeSettingsInternal != null) { - settingsBuilder.put(changeSettingsInternal).normalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX); + if (overrideSettingsInternal != null) { + settingsBuilder.put(overrideSettingsInternal).normalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX); } settingsBuilder.remove(MetadataIndexStateService.VERIFIED_BEFORE_CLOSE_SETTING.getKey()); return builder.settings(settingsBuilder).build();