- 
                Notifications
    You must be signed in to change notification settings 
- Fork 25.6k
Projects reserved state is moved to ProjectStateRegistry #132332
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
Conversation
| Pinging @elastic/es-core-infra (Team:Core/Infra) | 
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.
I have some initial comments.
Also, ProjectSecrets and other entities that can be created via file-based settings need to be initially created and stored in ProjectStateRegistry as well. I think it's reasonable to have them as separate PRs. If that is the case, do we have a JIRA issue for it? Thanks!
        
          
                ...er/src/main/java/org/elasticsearch/reservedstate/service/ReservedProjectStateUpdateTask.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                server/src/main/java/org/elasticsearch/cluster/metadata/ProjectMetadata.java
          
            Show resolved
            Hide resolved
        
              
          
                server/src/main/java/org/elasticsearch/cluster/ClusterState.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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.
Still need to read through the test changes. Meanwhile I have some minor comments for the production code.
I like the changes! It would be great if someone from the core-infra team can review this as well. In addition, do you mind sending a message to the MP channel to announce this change before merging so that CP is aware any potential implication by us dropping the old-style project reserved metadata in wire communication? Thanks!
        
          
                server/src/main/java/org/elasticsearch/cluster/ClusterState.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                server/src/main/java/org/elasticsearch/cluster/metadata/ProjectMetadata.java
          
            Show resolved
            Hide resolved
        
              
          
                server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                server/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | public void handleSnapshotRestore( | ||
| ClusterState clusterState, | ||
| ClusterState.Builder builder, | ||
| Metadata.Builder mdBuilder, | ||
| ProjectId projectId | ||
| ) { | 
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.
The parameters are getting a bit out of hand. They are all here to just support resetting the version of file settings. We can potentially extract an interface to represent them and pass different implementation based on whether the setup is multi-project. It does not need to be this PR. Could you please add a @FixForMultiProject annotation to it and we can work on that later.
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.
Could you please address this comment? Thanks!
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.
ES-12796
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.
I have a question about whether we want to keep a consistent reserved_state output in cluster state for single project.
| public void testSerialization() throws IOException { | ||
| ClusterState clusterState = buildClusterState(); | ||
| BytesStreamOutput out = new BytesStreamOutput(); | ||
| clusterState.writeTo(out); | ||
|  | ||
| // check it deserializes ok | ||
| NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(ClusterModule.getNamedWriteables()); | ||
| ClusterState deserialisedClusterState = ClusterState.readFrom( | ||
| new NamedWriteableAwareStreamInput(out.bytes().streamInput(), namedWriteableRegistry), | ||
| null | ||
| ); | ||
|  | ||
| // check it matches the original object | ||
| Metadata deserializedMetadata = deserialisedClusterState.metadata(); | ||
| assertThat(deserializedMetadata.projects(), aMapWithSize(1)); | ||
| assertThat(deserializedMetadata.projects(), hasKey(ProjectId.DEFAULT)); | ||
|  | ||
| assertThat(deserializedMetadata.getProject(ProjectId.DEFAULT).templates(), hasKey("template")); | ||
| assertThat(deserializedMetadata.getProject(ProjectId.DEFAULT).indices(), hasKey("index")); | ||
| } | 
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.
I am ok with having the new test. But I am curious on why it is needed here? Does not seem relate to the production code change?
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.
It would be great if you could address this comment as well. Thanks!
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.
But I am curious on why it is needed here? Does not seem relate to the production code change?
In an earlier version I was making changes to cluster state serialization and added this test (along with another serialization test). I reverted those changes and the other test, but decided to keep this one since I think it's good to have a unit test for basic cluster state serialization/deserialization
| // need to combine reserved state together into a single block so we don't get duplicate keys | ||
| // and not include it in the project xcontent output (through the lack of multi-project params) | ||
| // use a tree map so the order is deterministic | ||
| final Map<String, ReservedStateMetadata> clusterReservedState = new TreeMap<>(reservedStateMetadata); | ||
| clusterReservedState.putAll(project.reservedStateMetadata()); | 
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.
The intention of this code is keep the output from the GetClusterState API identical in non-MP and MP (when the API is invoked via a project URL, i.e. single project context). Removing it without any replace means the responses will now differ, i.e.
// non-MP
{
  "metadata": {
    "reserved_state": { ... }
  }
}
vs
// MP when a single project is requested
{
  "metadata": { ... },
  "projects_registry": {
    "projects": [
      { 
        "reserved_state": { ... }
      }
    ]
  }
}
Maybe it's OK for them to be different. But we need to make an explicit decision here.
Also, there is an existing issue that we don't filter ProjectStateRegistry for the requested project in TransportClusterStateAction so that it currently shows all projects when only a single project is requested. (EDIT: This is not an unique issue for ProjectStateRegistry. For example, blocks also have the same issue. So no need to fix it in this PR. The other point about different output formats between stateful and MP still stands).
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.
On a second thought, the difference in ClusterState API output format between non-MP and MP setups can also be addressed in future PRs. There should not be any BWC concern to change the MP output in future. So nothing is needed in this PR other than maybe raise a JIRA issue so that we don't forget about it.
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.
Thank you for highlighting this. I'll address the project filtering issue in a separate PR since it's straightforward, and I don't want to leave it for later.
Regarding the reserved state looking the same as before for a single project: we still have Metadata-level reserved state for cluster-level reserved state, and for all existing clusters it will still contain all the reserved state
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.
I'll address the project filtering issue in a separate PR
👍
Regarding the reserved state looking the same as before for a single project: we still have Metadata-level reserved state for cluster-level reserved state, and for all existing clusters it will still contain all the reserved state
Yes it is not an issue for existing clusters (non-MP). My question is about MP cluster. We have a general principle that the API output for a single project should be the same regardless whether the cluster is non-MP or MP. For the GetClusterState API, non-MP already sets the convention that reserved state is printed under Metadata. So my question is whether a MP cluster should always do the same as oppose to print them under cluster level customs. Note we already intentionally hide the whole concept of ProjectMetadata in the output from a MP cluster with the intention to keep it consistent with non-MP output.
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.
PR for the filtering issue: #133401
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.
Yes it is not an issue for existing clusters (non-MP). My question is about MP cluster. We have a general principle that the API output for a single project should be the same regardless whether the cluster is non-MP or MP. For the GetClusterState API, non-MP already sets the convention that reserved state is printed under Metadata. So my question is whether a MP cluster should always do the same as oppose to print them under cluster level customs. Note we already intentionally hide the whole concept of ProjectMetadata in the output from a MP cluster with the intention to keep it consistent with non-MP output.
I see now, thanks for the clarification. I'm a bit concerned we're going to make a mistake in one of these many branches eventually, but I'll support the convention. However, I'd prefer to handle it at the transport action level. I think that's cleaner, and I believe we don't want to store project's reserved state metadata to disk
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.
I'll do it in a separate PR, because turned out that I need changes from both this PR and #133401 😕
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.
I'd prefer to handle it at the transport action level
That might be an option as well. I don't have strong opinion.
I'll do it in a separate PR
Yeah that's what I meant. No need for this PR. Since it affects only MP cluster, there is no real BWC either.
| 
 ProjectSecrets - yes, I don't fully understand the workflow behind why we need to move other fields. However, in this case I would prefer to have just the  | 
| 
 Sorry I don't quite follow this comment. Could you please elaborate a bit?  | 
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 pending green CI
Thanks a lot for the work. I'd appreciate if you could address the other comments that I had. I am also putting down a couple suggestions again here to keep them in one place for easy consumption.
- We need a ticket to address ProjectSecrets
- It would be great to have someone from core-infra to take a look as well
- It would be great to send a message in the MP channel before merging since it may destabilze existing MP clusters because new nodes will drop reserved states from old nodes. I suspect it should self-heal but a heads-up message in the channel is helpful in any case.
| } | ||
| } else if (token == XContentParser.Token.START_OBJECT) { | ||
| switch (currentFieldName) { | ||
| // Remove this | 
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: It would be better to raise a JIRA issue and link the number here to provide more context.
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.
ES-12795
| projectBuilder.put(IndexTemplateMetadata.Builder.fromXContent(parser, parser.currentName())); | ||
| } | ||
| } | ||
| // Remove this | 
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.
Similar nit here.
        
          
                server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | public void handleSnapshotRestore( | ||
| ClusterState clusterState, | ||
| ClusterState.Builder builder, | ||
| Metadata.Builder mdBuilder, | ||
| ProjectId projectId | ||
| ) { | 
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.
Could you please address this comment? Thanks!
| public void testSerialization() throws IOException { | ||
| ClusterState clusterState = buildClusterState(); | ||
| BytesStreamOutput out = new BytesStreamOutput(); | ||
| clusterState.writeTo(out); | ||
|  | ||
| // check it deserializes ok | ||
| NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(ClusterModule.getNamedWriteables()); | ||
| ClusterState deserialisedClusterState = ClusterState.readFrom( | ||
| new NamedWriteableAwareStreamInput(out.bytes().streamInput(), namedWriteableRegistry), | ||
| null | ||
| ); | ||
|  | ||
| // check it matches the original object | ||
| Metadata deserializedMetadata = deserialisedClusterState.metadata(); | ||
| assertThat(deserializedMetadata.projects(), aMapWithSize(1)); | ||
| assertThat(deserializedMetadata.projects(), hasKey(ProjectId.DEFAULT)); | ||
|  | ||
| assertThat(deserializedMetadata.getProject(ProjectId.DEFAULT).templates(), hasKey("template")); | ||
| assertThat(deserializedMetadata.getProject(ProjectId.DEFAULT).indices(), hasKey("index")); | ||
| } | 
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.
It would be great if you could address this comment as well. Thanks!
|  | ||
| validateForReservedState( | ||
| projectResolver.getProjectMetadata(state).reservedStateMetadata().values(), | ||
| ProjectStateRegistry.get(state).reservedStateMetadata(projectResolver.getProjectId()).values(), | 
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.
Should we add a convenience method to get reservedStateMetadata to ProjectState as we've done for settings so this could just be projectResolver.getProjectState(state).reservedStateMetadata()?
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.
I am not strongly oppose it, but I don't think reserved state metadata is needed in a project state - for example, Settings can be used by many components, but there are not so many things that are interested in the reserved state
| Ticket to move ProjectSecrets: ES-12793 | 
) This commit refactors how project reserved state is stored and accessed in Elasticsearch. Instead of storing reserved state metadata directly within each ProjectMetadata object, the commit moves it to a centralized ProjectStateRegistry. Note: In mixed-version multiproject clusters, this may cause existing reserved state metadata for projects to temporarily disappear until all nodes have been upgraded and restarted. It also maintains the logic for resetting reserved state metadata during snapshot restore, ensuring that the reserved state will be reread and re-applied to the cluster state.
…3936) As it was noted in #132332 we have a general principle that API output for a single project should be the same regardless of whether the cluster is non-MP or MP. (However, the reserved metadata merge logic was never properly implemented to support this) As a result of changes in that PR, response formats diverged. This PR makes the single project GetClusterState API response for MP clusters match the non-MP format.
This commit refactors how project reserved state is stored and accessed in Elasticsearch. Instead of storing reserved state metadata directly within each ProjectMetadata object, the commit moves it to a centralized ProjectStateRegistry.
Note: In mixed-version multiproject clusters, this may cause existing reserved state metadata for projects to temporarily disappear until all nodes have been upgraded and restarted.
It also maintains the logic for resetting reserved state metadata during snapshot restore, ensuring that the reserved state will be reread and re-applied to the cluster state.