-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Integrate project global blocks into existing checks for cluster blocks #129467
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
Integrate project global blocks into existing checks for cluster blocks #129467
Conversation
…rojectGlobalBlocks
| } | ||
| } | ||
|
|
||
| public void globalBlockedRaiseException(ProjectId projectId, ClusterBlockLevel level) throws ClusterBlockException { |
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.
globalBlockedRaiseException(ClusterBlockLevel level) (the non-project-aware flavour of this) has some 200 callers mostly Transport actions. These seem to be the bulk of what it takes to integrate the project global cluster blocks into existing checks for blocks. I've been through half of them and have update some based on the following.
- If a TransportAction's corresponding RestAction has a
@ServerlessScope(Scope.PUBLIC)annotation, then I'd make its block check use this project aware version. In most cases, the action is already project aware or at least has a ProjectResolver, in some cases, I've added that as well. - Some of the
Scope.INTERNALones, probably still need to consider project block, e.g.TransportDeleteDataStreamLifecycleAction,TransportForceMergeAction. I've updated some as well. - There are also some TransportActions that don't necessarily have a RestAction, but to me they seem to need to be project-aware w.r.t. checking blocks e.g. some of the persistent task related stuff such as
CompletionPersistentTaskAction. I haven't dived into those.
I think properly going through all of these, is a rather big effort since in many cases, the relavent code is not project-aware and it is not clear to me if it will be. So my plan is to go through the second half of these as well and do the obvious ones. And open a ticket to come back to this. I assume we'd need to communicate this as part of "how to make things project-aware" when teams are doing their chuck of MP-compatibility.
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.
globalBlockedRaiseException(ClusterBlockLevel level) (the non-project-aware flavour of this) has some 200 callers mostly Transport actions.
I think you mean globalBlockedException(ClusterBlockLevel).
CompletionPersistentTaskAction
This one is tricky since persistent task can be both cluster and project scoped. I think we might need something like the follows
if (projectResolver.supportsMultipleProjects() == false) {
return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE);
} else {
final var projectId = projectResolver.getProjectId();
if (projectId == null || ProjectId.DEFAULT.equals(projectId)) {
return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE);
} else {
return state.blocks().globalBlockedException(projectId, ClusterBlockLevel.METADATA_WRITE);
}
}We can iterate on this with a separate PR.
And open a ticket to come back to this
I agree. We don't own every action that uses the blocks. It makes sense to raise the awareness for other teams.
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
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.
LGTM
Mostly minor comments except the one suggests leaving UpdatePersistentTaskStatusAction out of this PR.
| } | ||
| } | ||
|
|
||
| public void globalBlockedRaiseException(ProjectId projectId, ClusterBlockLevel level) throws ClusterBlockException { |
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.
globalBlockedRaiseException(ClusterBlockLevel level) (the non-project-aware flavour of this) has some 200 callers mostly Transport actions.
I think you mean globalBlockedException(ClusterBlockLevel).
CompletionPersistentTaskAction
This one is tricky since persistent task can be both cluster and project scoped. I think we might need something like the follows
if (projectResolver.supportsMultipleProjects() == false) {
return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE);
} else {
final var projectId = projectResolver.getProjectId();
if (projectId == null || ProjectId.DEFAULT.equals(projectId)) {
return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE);
} else {
return state.blocks().globalBlockedException(projectId, ClusterBlockLevel.METADATA_WRITE);
}
}We can iterate on this with a separate PR.
And open a ticket to come back to this
I agree. We don't own every action that uses the blocks. It makes sense to raise the awareness for other teams.
...org/elasticsearch/action/admin/indices/template/get/TransportGetComponentTemplateAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/ingest/GetPipelineTransportAction.java
Outdated
Show resolved
Hide resolved
| clusterState.blocks().globalBlockedRaiseException(projectResolver.getProjectId(), ClusterBlockLevel.READ); | ||
|
|
||
| ProjectState projectState = projectResolver.getProjectState(clusterState); |
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.
Nit:
| clusterState.blocks().globalBlockedRaiseException(projectResolver.getProjectId(), ClusterBlockLevel.READ); | |
| ProjectState projectState = projectResolver.getProjectState(clusterState); | |
| ProjectState projectState = projectResolver.getProjectState(clusterState); | |
| clusterState.blocks().globalBlockedRaiseException(projectState.projectId(), ClusterBlockLevel.READ); | |
| protected ClusterBlockException checkBlock(Request request, ClusterState state) { | ||
| // Cluster is not affected but we look up repositories in metadata | ||
| return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE); | ||
| return state.blocks().globalBlockedException(projectResolver.getProjectId(), ClusterBlockLevel.METADATA_WRITE); |
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.
This is similar to CompletionPersistentTaskAction in that persistent tasks can be either project or cluster scoped. I'd leave this to a different PR that focuses on persistent tasks. What do you think?
|
Thanks, Yang. I'll merge this.
|
…ks (Part 2) (elastic#129570) Relates elastic#129467 Resolves ES-11209
This PR updates some of the existing checks for cluster blocks to also consider project global blocks. It adds a few project-aware flavour of existing methods in
ClusterBlocks.globalBlockedRaiseExceptionis the mostly used one. I've updated only some of the obvious ones here.Follow up to #127978
Relates ES-11209