Skip to content

Make RepositoriesService project-aware #129821

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

Merged

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Jun 23, 2025

This PR makes RepositoriesService project aware so that the basic Put, Get, Delete and Verify repository actions are now project scoped.

It intentionally leaves the following aspects out of scope for the current changes:

  • Repository stats reporting
  • Repository clean-up, analysis and integrity verification
  • Repository usages for searchable snapshots and CCR

They will be worked on separately. One main reason for leaving them out is that they are not needed by OBS which is currently blocked by repository/snapshot changes. They may also have their own complexities, e.g. stats reporting.

Resolves: ES-10478

Comment on lines 677 to 686
/**
* Apply changes for one project. The project can be either newly added, removed or an existing one.
*
* @param version The cluster state version of the change.
* @param state The current project state, or {@code null} if the project was removed.
* @param previousState the previous project state, or {@code null} if the project was newly added.
*/
private void applyProjectState(long version, @Nullable ProjectState state, @Nullable ProjectState previousState) {
assert state != null || previousState != null : "state and previousState cannot both be null";
assert state == null || assertReadonlyRepositoriesNotInUseForWrites(state);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changeset of this method can be a bit deceptively large due to method extraction and indentation difference. It is largely the same as the exisitng applyClusterState method with some additional logic for new and removed projects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Any chance we could break up this method so not both states can be nullable so it is a bit more readable? I find it rather odd but I understand that's because you want to keep this logic the same with just some null checks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially started with breaking up the method for added, removed and existing projects. But found out that processing existing projects requires all logic (because repo can still be added/removed for existing projects) which led to duplication and felt non-ideal. That said, I tried it again by separating "removed projects" out in its own method and it seems to be a better middle-ground (compared to 1 or 3 methods). Please let me know whether it looks better to you. f62e3fe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most changes are cascading effects from adding projectId to this class and its methods.

Comment on lines +50 to +53
'^cat.repositories/*/*',
'^cat.snapshots/*/*',
'^cluster.desired_balance/10_basic/*',
'^cluster.stats/10_basic/snapshot stats reported in get cluster stats',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-mute these repository related YAML tests since they require either create, get or delete snapshots to work in MP setup. Previously these tests passed but it was in fact incorrect because they always target the default project.

Comment on lines -64 to +66
'^snapshot.get_repository/20_repository_uuid/*',
'^snapshot.get_repository/*/*',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same mute here. They will get unmute again once repositoriy and snapshot are truly MP ready.

@ywangd ywangd added >non-issue :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Jun 23, 2025
@ywangd ywangd marked this pull request as ready for review June 23, 2025 06:44
@ywangd ywangd requested review from DaveCTurner and pxsalehi June 23, 2025 06:44
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Jun 23, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Jun 23, 2025
@pxsalehi
Copy link
Member

pxsalehi commented Jun 23, 2025

Related to making snapshotting code project aware, I've opened ES-12121 recently. Not saying it needs to be done here, but since you've gone through the code for making things project aware, it might be worth taking a look into where the block checks could be also added, and add some note to that ticket if you thing it helps.

I see you've done some of that here. If you think that's it for ES-12121, feel free to close that one too. Thanks.

@@ -60,7 +62,9 @@ public Collection<PutRepositoryRequest> prepare(Object input) {
for (var repositoryRequest : repositories) {
validate(repositoryRequest);
RepositoriesService.validateRepositoryName(repositoryRequest.name());
repositoriesService.validateRepositoryCanBeCreated(repositoryRequest);
@FixForMultiProject
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even if it is very obvious to you, I'd recommend adding a comment/description to these annotations so it is immediately clear what the action item is, or any other detail/suggestion you might have to resolve it. I've left the other annotations that have no comment/description uncommented but please do that for them too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure see 75f2b75

Comment on lines 254 to 255
public Set<ProjectId> commonProjects() {
return projectsDelta.common(state);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this fit with the existing pattern of deltas? could you just use a set operation where ever this is needed (currentProjects - added)? Seems a bit odd to have this here. Maybe as a static method on two given metadata?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was not super happy with the changes either. I was trying to achieve (1) compute the common set once and (2) not storing the common set separately if it is the same as all projects. I reverted the changes and let the caller compute the common set of projects ca0ea7a

repositoriesService.registerInternalRepository(request.getName(), request.getType());
@FixForMultiProject
final var projectId = ProjectId.DEFAULT;
repositoriesService.registerInternalRepository(projectId, request.getName(), request.getType());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's there to fix? i imagine this will only be default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe if CCR is never expected to work in multi-project. With the initial namespacing effort, we generally agreed that it is better if all elasticsearch is project-aware because it is easier to reason with instead of having to remember which part is and which part is not. For example, we made ILM project aware even though it is not used in serverless. There are certainly pratical limits on how much we can push this kinda of effort. But leaving an annotation here seems to be the right thing to do until we can firmly decide what to do with CCR. I also added an description and linked the issue for the annotation.

@@ -602,7 +602,9 @@ static void refreshRepositoryUuids(
return;
}

for (Repository repository : repositoriesService.getRepositories().values()) {
@FixForMultiProject
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so making restore project-aware is also deferred to a follow up?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes restore is tracked by ES-10228. It's also not needed by OBS.

Comment on lines 677 to 686
/**
* Apply changes for one project. The project can be either newly added, removed or an existing one.
*
* @param version The cluster state version of the change.
* @param state The current project state, or {@code null} if the project was removed.
* @param previousState the previous project state, or {@code null} if the project was newly added.
*/
private void applyProjectState(long version, @Nullable ProjectState state, @Nullable ProjectState previousState) {
assert state != null || previousState != null : "state and previousState cannot both be null";
assert state == null || assertReadonlyRepositoriesNotInUseForWrites(state);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Any chance we could break up this method so not both states can be nullable so it is a bit more readable? I find it rather odd but I understand that's because you want to keep this logic the same with just some null checks.

@ywangd
Copy link
Member Author

ywangd commented Jun 24, 2025

I didn't know the existence of ES-12121. Thanks for the reminder. I did intend to address the issue along the way while working on related APIs. It's not completed yet since we still need the same treatmeant for other snapshot and retore APIs. I added it as a linked item to ES-10166, ES-10227, ES-12158 and ES-10478. I will close ES-12121 once all the linked items are completed.

@ywangd ywangd requested a review from pxsalehi June 24, 2025 01:44
@ywangd ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jun 24, 2025
@elasticsearchmachine elasticsearchmachine merged commit e1c930f into elastic:main Jun 25, 2025
32 checks passed
@ywangd ywangd deleted the repositories-service-project-aware branch June 25, 2025 00:34
mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Jun 25, 2025
This PR makes RepositoriesService project aware so that the basic Put,
Get, Delete and Verify repository actions are now project scoped. 

It intentionally leaves the following aspects out of scope for the
current changes: * Repository stats reporting * Repository clean-up,
analysis and integrity verification * Repository usages for searchable
snapshots and CCR

They will be worked on separately. One main reason for leaving them out
is that they are not needed by OBS which is currently blocked by
repository/snapshot changes. They may also have their own complexities,
e.g. stats reporting.

Resolves: ES-10478
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >non-issue serverless-linked Added by automation, don't add manually Team:Distributed Coordination Meta label for Distributed Coordination team v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants