-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce System Repository #9088
Changes from all commits
07e9142
47bb5fa
a5d0efc
5cf828b
12d070f
c098601
c92eddd
84d7030
338cb55
7cdc767
23a82f7
137d05d
8ec8b5c
f0125b8
840055c
58b5dca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,6 +82,7 @@ | |
import java.util.stream.Stream; | ||
|
||
import static org.opensearch.repositories.blobstore.BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY; | ||
import static org.opensearch.repositories.blobstore.BlobStoreRepository.SYSTEM_REPOSITORY_SETTING; | ||
|
||
/** | ||
* Service responsible for maintaining and providing access to snapshot repositories on nodes. | ||
|
@@ -159,7 +160,7 @@ | |
* @param request register repository request | ||
* @param listener register repository listener | ||
*/ | ||
public void registerRepository(final PutRepositoryRequest request, final ActionListener<ClusterStateUpdateResponse> listener) { | ||
public void registerOrUpdateRepository(final PutRepositoryRequest request, final ActionListener<ClusterStateUpdateResponse> listener) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we try to register a new system repository, do we call this method? or that flow would be separate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That should be a separate flow |
||
assert lifecycle.started() : "Trying to register new repository but service is in state [" + lifecycle.state() + "]"; | ||
|
||
final RepositoryMetadata newRepositoryMetadata = new RepositoryMetadata(request.name(), request.type(), request.settings()); | ||
|
@@ -210,30 +211,41 @@ | |
RepositoriesMetadata repositories = metadata.custom(RepositoriesMetadata.TYPE); | ||
if (repositories == null) { | ||
logger.info("put repository [{}]", request.name()); | ||
ensureNonSystemRepository(request.settings(), request.name()); | ||
repositories = new RepositoriesMetadata( | ||
Collections.singletonList(new RepositoryMetadata(request.name(), request.type(), request.settings())) | ||
); | ||
} else { | ||
boolean found = false; | ||
List<RepositoryMetadata> repositoriesMetadata = new ArrayList<>(repositories.repositories().size() + 1); | ||
|
||
RepositoryMetadata currentRepositoryMetadata = null; | ||
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; | ||
} else if (SYSTEM_REPOSITORY_SETTING.get(repositoryMetadata.settings()) != SYSTEM_REPOSITORY_SETTING.get( | ||
newRepositoryMetadata.settings() | ||
)) { | ||
throw new RepositoryException( | ||
repositoryMetadata.name(), | ||
"trying to modify system repository attribute for a repository" | ||
); | ||
} | ||
found = true; | ||
currentRepositoryMetadata = repositoryMetadata; | ||
repositoriesMetadata.add(newRepositoryMetadata); | ||
} else { | ||
repositoriesMetadata.add(repositoryMetadata); | ||
} | ||
} | ||
if (!found) { | ||
logger.info("put repository [{}]", request.name()); | ||
ensureNonSystemRepository(newRepositoryMetadata.settings(), newRepositoryMetadata.name()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
repositoriesMetadata.add(new RepositoryMetadata(request.name(), request.type(), request.settings())); | ||
} else { | ||
logger.info("update repository [{}]", request.name()); | ||
validateRestrictedSystemRepositorySettingsAndType(currentRepositoryMetadata, newRepositoryMetadata); | ||
} | ||
repositories = new RepositoriesMetadata(repositoriesMetadata); | ||
} | ||
|
@@ -261,6 +273,29 @@ | |
); | ||
} | ||
|
||
private void validateRestrictedSystemRepositorySettingsAndType( | ||
RepositoryMetadata currentRepositoryMetadata, | ||
RepositoryMetadata newRepositoryMetadata | ||
) { | ||
if (SYSTEM_REPOSITORY_SETTING.get(currentRepositoryMetadata.settings())) { | ||
if (newRepositoryMetadata.type() != currentRepositoryMetadata.type()) { | ||
throw new RepositoryException( | ||
currentRepositoryMetadata.name(), | ||
"trying to modify system repository type " + currentRepositoryMetadata.type() | ||
); | ||
} | ||
Repository repository = repositories.get(currentRepositoryMetadata.name()); | ||
for (Setting<?> setting : repository.restrictedSystemRepositorySettings()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if (newRepositoryMetadata.settings().get(setting.getKey()) != null) { | ||
throw new RepositoryException( | ||
repository.getMetadata().name(), | ||
"trying to modify restricted system repository settings " + setting.getKey() | ||
); | ||
} | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Unregisters repository in the cluster | ||
* <p> | ||
|
@@ -288,6 +323,7 @@ | |
boolean changed = false; | ||
for (RepositoryMetadata repositoryMetadata : repositories.repositories()) { | ||
if (Regex.simpleMatch(request.name(), repositoryMetadata.name())) { | ||
ensureNonSystemRepository(repositoryMetadata.settings(), request.name()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error message would be confusing here : |
||
ensureRepositoryNotInUse(currentState, repositoryMetadata.name()); | ||
logger.info("delete repository [{}]", repositoryMetadata.name()); | ||
changed = true; | ||
|
@@ -499,6 +535,21 @@ | |
throw new RepositoryMissingException(repositoryName); | ||
} | ||
|
||
/** | ||
* Returns registered system repository | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need a separate API? Shouldn't the name still be unique and retrievable via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is to make sure we always use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This design is inconsistent, the returned |
||
* <p>* | ||
* @param repositoryName repository name | ||
* @return registered system repository | ||
* @throws RepositoryMissingException if repository with such name isn't registered | ||
*/ | ||
public Repository getSystemRepository(String repositoryName) { | ||
Repository repository = repositories.get(repositoryName); | ||
if (repository != null && SYSTEM_REPOSITORY_SETTING.get(repository.getMetadata().settings())) { | ||
return repository; | ||
} | ||
throw new RepositoryMissingException(repositoryName); | ||
} | ||
|
||
public List<RepositoryStatsSnapshot> repositoriesStats() { | ||
List<RepositoryStatsSnapshot> archivedRepoStats = repositoriesStatsArchive.getArchivedStats(); | ||
List<RepositoryStatsSnapshot> activeRepoStats = getRepositoryStatsForActiveRepositories(); | ||
|
@@ -610,6 +661,12 @@ | |
} | ||
} | ||
|
||
public static void ensureNonSystemRepository(Settings repoSettings, String repository) { | ||
if (SYSTEM_REPOSITORY_SETTING.get(repoSettings)) { | ||
throw new RepositoryException(repository, "cannot register a system repository externally"); | ||
} | ||
} | ||
|
||
public static void validateRepositoryMetadataSettings( | ||
ClusterService clusterService, | ||
final String repositoryName, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ | |
import org.opensearch.cluster.node.DiscoveryNode; | ||
import org.opensearch.common.Nullable; | ||
import org.opensearch.common.lifecycle.LifecycleComponent; | ||
import org.opensearch.common.settings.Setting; | ||
import org.opensearch.index.mapper.MapperService; | ||
import org.opensearch.core.index.shard.ShardId; | ||
import org.opensearch.index.snapshots.IndexShardSnapshotStatus; | ||
|
@@ -55,6 +56,8 @@ | |
|
||
import java.io.IOException; | ||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.function.Consumer; | ||
import java.util.function.Function; | ||
|
@@ -237,6 +240,13 @@ | |
*/ | ||
boolean isReadOnly(); | ||
|
||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would a callers use this field? It seems like it makes handling the |
||
* Returns true if the repository is managed by the system directly and doesn't allow managing the lifetime of the | ||
* repository through external APIs | ||
* @return true if the repository is system managed | ||
*/ | ||
boolean isSystemRepository(); | ||
|
||
/** | ||
* Creates a snapshot of the shard based on the index commit point. | ||
* <p> | ||
|
@@ -341,6 +351,14 @@ | |
throw new UnsupportedOperationException(); | ||
} | ||
|
||
/** | ||
* Returns the list of restricted system repository settings that cannot be mutated once repository is created* | ||
* @return the list of settings | ||
*/ | ||
default List<Setting<?>> restrictedSystemRepositorySettings() { | ||
return Collections.emptyList(); | ||
} | ||
|
||
/** | ||
* Retrieve shard snapshot status for the stored snapshot | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the name? No parameters were modified nor was the comment altered.