Skip to content

Commit

Permalink
Fixing tests and updating code
Browse files Browse the repository at this point in the history 
Signed-off-by: Dharmesh 💤 <[email protected]>
  • Loading branch information
@psychbot
psychbot committed Sep 11, 2023
1 parent aaf5adb commit 14f8f3a
Show file tree
Hide file tree
Showing 10 changed files with 66 additions and 60 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
## [Unreleased 2.x]
### Added
- [Remote Store] Introducing concept of RestrictedSystemRepositorySettings for system repository ([#9839](https://github.com/opensearch-project/OpenSearch/pull/9839))
- [Remote Store] Adding support to restrict creation & deletion if system repository and mutation of immutable settings of system repository ([#9839](https://github.com/opensearch-project/OpenSearch/pull/9839))

### Dependencies
- Bump `peter-evans/create-or-update-comment` from 2 to 3 ([#9575](https://github.com/opensearch-project/OpenSearch/pull/9575))
Expand Down
3 changes: 1 addition & 2 deletions server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteIndexRecoveryIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import java.nio.file.Path;

import static org.opensearch.remotestore.RemoteStoreBaseIntegTestCase.remoteStoreClusterSettings;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;

@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0)
public class RemoteIndexRecoveryIT extends IndexRecoveryIT {
Expand Down Expand Up @@ -58,7 +57,7 @@ public Settings indexSettings() {
@After
public void teardown() {
internalCluster().wipeIndices("_all");
assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME));
clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get();
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public void setup() {
@After
public void teardown() {
internalCluster().wipeIndices("_all");
assertAcked(clusterAdmin().prepareDeleteRepository(BASE_REMOTE_REPO));
clusterAdmin().prepareCleanupRepository(BASE_REMOTE_REPO).get();
}

@Override
Expand Down
5 changes: 2 additions & 3 deletions ...src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT;
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY;
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;

public class RemoteStoreBaseIntegTestCase extends OpenSearchIntegTestCase {
protected static final String REPOSITORY_NAME = "test-remote-store-repo";
Expand Down Expand Up @@ -274,8 +273,8 @@ public void teardown() {
assertRemoteStoreRepositoryOnAllNodes(REPOSITORY_NAME);
assertRemoteStoreRepositoryOnAllNodes(REPOSITORY_2_NAME);
internalCluster().wipeIndices("_all");
assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME));
assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_2_NAME));
clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get();
clusterAdmin().prepareCleanupRepository(REPOSITORY_2_NAME).get();
}

public RepositoryMetadata buildRepositoryMetadata(DiscoveryNode node, String name) {
Expand Down
3 changes: 1 addition & 2 deletions ...rnalClusterTest/java/org/opensearch/remotestore/SegmentReplicationUsingRemoteStoreIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import java.nio.file.Path;

import static org.opensearch.remotestore.RemoteStoreBaseIntegTestCase.remoteStoreClusterSettings;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;

/**
* This class runs Segment Replication Integ test suite with remote store enabled.
Expand Down Expand Up @@ -51,6 +50,6 @@ public void setup() {
@After
public void teardown() {
internalCluster().wipeIndices("_all");
assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME));
clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get();
}
}
3 changes: 1 addition & 2 deletions ...sterTest/java/org/opensearch/remotestore/SegmentReplicationWithRemoteStorePressureIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import java.nio.file.Path;

import static org.opensearch.remotestore.RemoteStoreBaseIntegTestCase.remoteStoreClusterSettings;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;

/**
* This class executes the SegmentReplicationPressureIT suite with remote store integration enabled.
Expand Down Expand Up @@ -50,6 +49,6 @@ public void setup() {
@After
public void teardown() {
internalCluster().wipeIndices("_all");
assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME));
clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get();
}
}
4 changes: 2 additions & 2 deletions server/src/main/java/org/opensearch/node/remotestore/RemoteStoreNodeService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ public RepositoriesMetadata updateRepositoriesMetadata(DiscoveryNode joiningNode
for (RepositoryMetadata existingRepositoryMetadata : existingRepositories.repositories()) {
if (newRepositoryMetadata.name().equals(existingRepositoryMetadata.name())) {
try {
newRepositoryMetadata = repositoriesService.get()
.ensureValidSystemRepositoryUpdate(newRepositoryMetadata, existingRepositoryMetadata);
repositoriesService.get().ensureValidSystemRepositoryUpdate(newRepositoryMetadata, existingRepositoryMetadata);
newRepositoryMetadata = existingRepositoryMetadata;
repositoryAlreadyPresent = true;
break;
} catch (RepositoryException e) {
Expand Down
88 changes: 41 additions & 47 deletions server/src/main/java/org/opensearch/repositories/RepositoriesService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
import org.opensearch.cluster.SnapshotsInProgress;
import org.opensearch.cluster.ack.ClusterStateUpdateResponse;
import org.opensearch.cluster.metadata.CryptoMetadata;
import org.opensearch.cluster.metadata.IndexMetadata;
import org.opensearch.cluster.metadata.Metadata;
import org.opensearch.cluster.metadata.RepositoriesMetadata;
import org.opensearch.cluster.metadata.RepositoryMetadata;
Expand Down Expand Up @@ -238,16 +237,34 @@ 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)) {
RepositoryMetadata updatedRepositoryMetadata = newRepositoryMetadata;
if (isSystemRepositorySettingPresent(repositoryMetadata.settings())) {
Settings updatedSettings = Settings.builder()
.put(newRepositoryMetadata.settings())
.put(SYSTEM_REPOSITORY_SETTING.getKey(), true)
.build();
updatedRepositoryMetadata = new RepositoryMetadata(
newRepositoryMetadata.name(),
newRepositoryMetadata.type(),
updatedSettings,
newRepositoryMetadata.cryptoMetadata()
);
}
if (repositoryMetadata.name().equals(updatedRepositoryMetadata.name())) {
if (updatedRepositoryMetadata.equalsIgnoreGenerations(repositoryMetadata)) {
// Previous version is the same as this one no update is needed.
return currentState;
}
ensureCryptoSettingsAreSame(repositoryMetadata, request);
found = true;
repositoriesMetadata.add(ensureValidSystemRepositoryUpdate(newRepositoryMetadata, repositoryMetadata));
if (isSystemRepositorySettingPresent(repositoryMetadata.settings())) {
ensureValidSystemRepositoryUpdate(updatedRepositoryMetadata, repositoryMetadata);
repositoriesMetadata.add(updatedRepositoryMetadata);
} else {
repositoriesMetadata.add(newRepositoryMetadata);
}
} else {
repositoriesMetadata.add(repositoryMetadata);
repositoriesMetadata.add(updatedRepositoryMetadata);
}
}
if (!found) {
Expand Down Expand Up @@ -317,9 +334,7 @@ public ClusterState execute(ClusterState currentState) {
for (RepositoryMetadata repositoryMetadata : repositories.repositories()) {
if (Regex.simpleMatch(request.name(), repositoryMetadata.name())) {
ensureRepositoryNotInUse(currentState, repositoryMetadata.name());
if (isSystemRepositorySettingPresent(repositoryMetadata.settings())) {
ensureSystemRepositoryNotInUse(currentState, repositoryMetadata);
}
ensureNotSystemRepository(repositoryMetadata);
logger.info("delete repository [{}]", repositoryMetadata.name());
changed = true;
} else {
Expand Down Expand Up @@ -770,40 +785,28 @@ public void updateRepositoriesMap(Map<String, Repository> repos) {
}
}

private static void ensureSystemRepositoryNotInUse(ClusterState clusterState, RepositoryMetadata repositoryMetadata) {
if (isRepositoryInUse(clusterState, repositoryMetadata)) {
throw new IllegalStateException("trying to modify or unregister repository that is currently used");
}
}

private static boolean isRepositoryInUse(ClusterState clusterState, RepositoryMetadata repositoryMetadata) {
// We can add more validations where we want to ensure that the system repository is not deleted if in use.
final Metadata idxMetadata = clusterState.getMetadata();
final String repository = repositoryMetadata.name();
for (IndexMetadata metadata : idxMetadata) {
if (IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING.get(metadata.getSettings())) {
String segmentRepositoryInIndex = IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING.get(metadata.getSettings());
String translogRepositoryInIndex = IndexMetadata.INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING.get(metadata.getSettings());

if (segmentRepositoryInIndex.equals(repository) || translogRepositoryInIndex.equals(repository)) {
return true;
}
}
private static void ensureNotSystemRepository(RepositoryMetadata repositoryMetadata) {
if (isSystemRepositorySettingPresent(repositoryMetadata.settings())) {
throw new RepositoryException(repositoryMetadata.name(), "cannot delete a system repository");
}
return false;
}

private static boolean isSystemRepositorySettingPresent(Settings repositoryMetadataSettings) {
return SYSTEM_REPOSITORY_SETTING.get(repositoryMetadataSettings);
}

private static boolean isValueEqual(String newValue, String currentValue) {
private static boolean isValueEqual(String key, String newValue, String currentValue) {
if (newValue == null && currentValue == null) {
return true;
}
if (newValue == null) {
throw new IllegalArgumentException("new value cannot be empty, " + "current value [" + currentValue + "]");
throw new IllegalArgumentException("[" + key + "] cannot be empty, " + "current value [" + currentValue + "]");
}
if (newValue.equals(currentValue) == false) {
throw new IllegalArgumentException(
"trying to modify an unmodifiable attribute of system repository from "
"trying to modify an unmodifiable attribute "
+ key
+ " of system repository from "
+ "current value ["
+ currentValue
+ "] to new value ["
Expand All @@ -814,14 +817,10 @@ private static boolean isValueEqual(String newValue, String currentValue) {
return true;
}

public RepositoryMetadata ensureValidSystemRepositoryUpdate(
RepositoryMetadata newRepositoryMetadata,
RepositoryMetadata currentRepositoryMetadata
) {
public void ensureValidSystemRepositoryUpdate(RepositoryMetadata newRepositoryMetadata, RepositoryMetadata currentRepositoryMetadata) {
if (isSystemRepositorySettingPresent(currentRepositoryMetadata.settings())) {
Settings.Builder updatedSettings = Settings.builder().put(newRepositoryMetadata.settings());
try {
isValueEqual(newRepositoryMetadata.type(), currentRepositoryMetadata.type());
isValueEqual("type", newRepositoryMetadata.type(), currentRepositoryMetadata.type());

Repository repository = repositories.get(currentRepositoryMetadata.name());
Settings newRepositoryMetadataSettings = newRepositoryMetadata.settings();
Expand All @@ -833,21 +832,16 @@ public RepositoryMetadata ensureValidSystemRepositoryUpdate(
.collect(Collectors.toList());

for (String restrictedSettingKey : restrictedSettings) {
String newSettingValue = newRepositoryMetadataSettings.get(restrictedSettingKey);
String currentSettingValue = currentRepositoryMetadataSettings.get(restrictedSettingKey);

isValueEqual(newSettingValue, currentSettingValue);

if (currentSettingValue != null) {
updatedSettings.put(restrictedSettingKey, currentSettingValue);
}
isValueEqual(
restrictedSettingKey,
newRepositoryMetadataSettings.get(restrictedSettingKey),
currentRepositoryMetadataSettings.get(restrictedSettingKey)
);
}
} catch (IllegalArgumentException e) {
throw new RepositoryException(currentRepositoryMetadata.name(), e.getMessage());
}
return new RepositoryMetadata(currentRepositoryMetadata.name(), currentRepositoryMetadata.type(), updatedSettings.build());
}
return newRepositoryMetadata;
}

@Override
Expand Down
8 changes: 8 additions & 0 deletions server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
import java.util.function.Consumer;
import java.util.function.Function;

import static org.opensearch.repositories.blobstore.BlobStoreRepository.SYSTEM_REPOSITORY_SETTING;
import static org.hamcrest.Matchers.equalTo;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doAnswer;
Expand Down Expand Up @@ -201,6 +202,13 @@ public void testRegisterRejectsInvalidRepositoryNames() {
}
}

public void testUpdateOrRegisterRejectsForSystemRepository() {
String repoName = "name";
PutRepositoryRequest request = new PutRepositoryRequest(repoName);
request.settings(Settings.builder().put(SYSTEM_REPOSITORY_SETTING.getKey(), true).build());
expectThrows(RepositoryException.class, () -> repositoriesService.registerOrUpdateRepository(request, null));
}

public void testRepositoriesStatsCanHaveTheSameNameAndDifferentTypeOverTime() {
String repoName = "name";
expectThrows(RepositoryMissingException.class, () -> repositoriesService.repository(repoName));
Expand Down
9 changes: 8 additions & 1 deletion test/framework/src/main/java/org/opensearch/test/TestCluster.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.opensearch.index.IndexNotFoundException;
import org.opensearch.indices.IndexTemplateMissingException;
import org.opensearch.repositories.RepositoryMissingException;
import org.opensearch.repositories.blobstore.BlobStoreRepository;
import org.opensearch.test.hamcrest.OpenSearchAssertions;

import java.io.Closeable;
Expand Down Expand Up @@ -253,7 +254,13 @@ public void wipeRepositories(String... repositories) {
}
for (String repository : repositories) {
try {
client().admin().cluster().prepareDeleteRepository(repository).execute().actionGet();
if (BlobStoreRepository.SYSTEM_REPOSITORY_SETTING.get(
client().admin().cluster().prepareGetRepositories(repository).execute().actionGet().repositories().get(0).settings()
) == false) {
client().admin().cluster().prepareDeleteRepository(repository).execute().actionGet();
} else {
client().admin().cluster().prepareCleanupRepository(repository).execute().actionGet();
}
} catch (RepositoryMissingException ex) {
// ignore
}
Expand Down

0 comments on commit 14f8f3a

Please sign in to comment.