Record project deletions in ProjectStateRegistry #130225

pxsalehi · 2025-06-27T15:23:39Z

This will be later used to update the Stateless lease blob, as a project deletion is a notable event that that our stateless cluster consistency check should consider before acknowledging writes.

Relates ES-11207

pxsalehi · 2025-06-27T15:28:50Z

.../src/test/java/org/elasticsearch/cluster/project/ProjectStateRegistrySerializationTests.java

+import java.io.IOException;
+import java.util.stream.IntStream;
+
+public class ProjectStateRegistrySerializationTests extends SimpleDiffableWireSerializationTestCase<ClusterState.Custom> {


There was no serialization test for this, so I added this.

pxsalehi · 2025-06-27T15:50:42Z

server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java


    private final Map<ProjectId, Settings> projectsSettings;
+    // Projects that have been marked for deletion based on their file-based setting
+    private final Set<ProjectId> projectsMarkedForDeletion;


Once the file-based deletion trigger is there, it should update this.

pxsalehi · 2025-06-27T15:52:36Z

server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java

+    // Projects that have been marked for deletion based on their file-based setting
+    private final Set<ProjectId> projectsMarkedForDeletion;
+    // A counter that is incremented each time one or more projects are marked for deletion.
+    private final long projectsMarkedForDeletionGeneration;


This is very similar to DiscoveryNodes#getNodeLeftGeneration but for project deletions.

elasticsearchmachine · 2025-06-27T15:53:34Z

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

ywangd

Looks great. I left some comments.

server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java

ywangd · 2025-06-30T01:42:43Z

server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java

+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o instanceof ProjectStateRegistry == false) return false;
+        ProjectStateRegistry that = (ProjectStateRegistry) o;
+        return projectsMarkedForDeletionGeneration == that.projectsMarkedForDeletionGeneration
+            && Objects.equals(projectsSettings, that.projectsSettings)
+            && Objects.equals(projectsMarkedForDeletion, that.projectsMarkedForDeletion);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(projectsSettings, projectsMarkedForDeletion, projectsMarkedForDeletionGeneration);
+    }


Many of the objects in cluster state compares equality by identity, e.g. DiscoveryNodes, so that it is a bit more performant when there is no change which is often the case. So I'd prefer we use identity equality here if these methods are only used for tests and use a wrapper class in tests.

I wasted a bit of time trying to make it happen before realizing how ugly it can get since this is a bit more than ClusterBlocks and would require more hacking. In fact, SnapshotsInProgress which is a more similar case to this (also a custom) and does have serialization tests, also does what I've done here.

(same with HealthMetadata, RestoreInProgress, to name a few other customs)

OK. There is indeed inconsistency in how we handle equals for cluster state objects. Since this object is linear to number of projects, the upper-bound should not be too high for performing a non-identity equals. We can also revisit it if it proves to be a problem.

If we make this class support the more granular diff than the generic CompleteNameDiff. The identity equality will be much less interesting since I think creating diff is one place equals is called the most often. That is the case for SnapshotsInProgress, but not RestoreInProgress, again inconsistency. In any case, it's not for this PR.

server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java

ywangd · 2025-06-30T01:47:41Z

server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java

+            return new ProjectStateRegistry(
+                projectsSettings.build(),
+                projectsMarkedForDeletion,
+                newProjectMarkedForDeletion ? projectsMarkedForDeletionGeneration + 1 : projectsMarkedForDeletionGeneration


I think we want to reset this generation when cluster term changes similar to nodeLeftGeneration as stated on the original draft PR? But I don't see how we can reset it?

We don't have to. AFAICT, with nodeLeftGeneration this can happen naturally since we build a new DiscoveryNodes when master changes. For project deletions that's not the case. We don't need it either. When comparing leases, term and nodeLeftGeneration are compared first and only if those two are equal we would consider deletions. Point being that the consistency check would have to wait anyway if term is different. Admittedly I think that means whether we rest or not is not important, so resetting is not a bad idea. But since there is no overflow concern here, I'd say we can leave it since as you mentioned doing it would be a bit convoluted probably. We can certainly revisit this, but for now this is how I see it.

OK I also think it's safe to not reset. Overflow does not seem to be a conern either. I do wonder why we reset nodeLeftGeneration. But we can always revisit this if necessary like you suggested.

conceptually it makes sense there and it is possible to do it easier (code-wise) underneath when we build a DiscoveryNodes. If it is not needed, convenient to do, and it makes sense only within a term, why not reset it.

(that's how I see it at least after staring at it for a while!)

…sUnderDeletionToProjectStateRegistry

alexey-ivanov-es · 2025-07-01T14:25:22Z

.../src/test/java/org/elasticsearch/cluster/project/ProjectStateRegistrySerializationTests.java

+    }
+
+    private ProjectStateRegistry mutate(ProjectStateRegistry instance) {
+        if (randomBoolean() && instance.size() > 0) {


Can we have separate tests for different scenarios instead of a single test that changes behavior based on randomBoolean?

Theoretically yes, but this is a very common pattern in serialization tests. unless there is a certain combinations that you think is not covered? Is there something in particular that you have in mind?

ywangd

LGTM

ywangd · 2025-07-02T02:37:33Z

server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java

+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o instanceof ProjectStateRegistry == false) return false;
+        ProjectStateRegistry that = (ProjectStateRegistry) o;
+        return projectsMarkedForDeletionGeneration == that.projectsMarkedForDeletionGeneration
+            && Objects.equals(projectsSettings, that.projectsSettings)
+            && Objects.equals(projectsMarkedForDeletion, that.projectsMarkedForDeletion);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(projectsSettings, projectsMarkedForDeletion, projectsMarkedForDeletionGeneration);
+    }


OK. There is indeed inconsistency in how we handle equals for cluster state objects. Since this object is linear to number of projects, the upper-bound should not be too high for performing a non-identity equals. We can also revisit it if it proves to be a problem.

ywangd · 2025-07-02T02:41:17Z

server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java

-            return new ProjectStateRegistry(projectsSettings.build());
+            final var unknownButUnderDeletion = Sets.difference(projectsMarkedForDeletion, projectsSettings.keys());
+            if (unknownButUnderDeletion.isEmpty() == false) {
+                assert unknownButUnderDeletion.stream().noneMatch(projectsSettings::containsKey);


What I had in mind is something like assert false : "Cannot mark projects ...". On a second thought, an invalid file-based configuration can actually trigger it so that it is not only a programming error. Therefore, I think we don't really want the assertion that I had mind. We also don't need this assertion here since it just asserts the correctness of the above Sets.difference call. Sorry for the back and forth.

ywangd · 2025-07-02T02:55:10Z

server/src/main/java/org/elasticsearch/cluster/project/ProjectStateRegistry.java

+            return new ProjectStateRegistry(
+                projectsSettings.build(),
+                projectsMarkedForDeletion,
+                newProjectMarkedForDeletion ? projectsMarkedForDeletionGeneration + 1 : projectsMarkedForDeletionGeneration


OK I also think it's safe to not reset. Overflow does not seem to be a conern either. I do wonder why we reset nodeLeftGeneration. But we can always revisit this if necessary like you suggested.

…sUnderDeletionToProjectStateRegistry

…ateRegistry

…sUnderDeletionToProjectStateRegistry

Project deletions is used to update the Stateless lease blob, as a project deletion is a notable event that that our stateless cluster consistency check should consider before acknowledging writes. Relates ES-11207

Record project deletions in ProjectStateRegistry

6a296e3

pxsalehi added >non-issue :Distributed Coordination/Distributed A catch all label for anything in the Distributed Coordination area. Please avoid if you can. labels Jun 27, 2025

elasticsearchmachine added the v9.2.0 label Jun 27, 2025

pxsalehi commented Jun 27, 2025

View reviewed changes

pxsalehi requested review from ywangd, mark-vieira and alexey-ivanov-es June 27, 2025 15:53

pxsalehi marked this pull request as ready for review June 27, 2025 15:53

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Jun 27, 2025

ywangd reviewed Jun 30, 2025

View reviewed changes

pxsalehi added 3 commits June 30, 2025 15:52

Merge remote-tracking branch 'upstream/main' into ps250626-addProject…

c33e116

…sUnderDeletionToProjectStateRegistry

address review comments

6329301

Merge remote-tracking branch 'upstream/main' into ps250626-addProject…

fb0bb16

…sUnderDeletionToProjectStateRegistry

pxsalehi requested a review from ywangd July 1, 2025 10:23

alexey-ivanov-es reviewed Jul 1, 2025

View reviewed changes

pxsalehi requested a review from alexey-ivanov-es July 1, 2025 16:12

ywangd approved these changes Jul 2, 2025

View reviewed changes

pxsalehi added 2 commits July 2, 2025 10:36

Merge remote-tracking branch 'upstream/main' into ps250626-addProject…

d5d2c6a

…sUnderDeletionToProjectStateRegistry

remove assertion

c5f2a3b

pxsalehi enabled auto-merge (squash) July 2, 2025 08:41

pxsalehi and others added 4 commits July 2, 2025 12:13

Merge branch 'main' into ps250626-addProjectsUnderDeletionToProjectSt…

de4de95

…ateRegistry

Merge branch 'main' into ps250626-addProjectsUnderDeletionToProjectSt…

2fa3a84

…ateRegistry

minor test simplification

bd57bde

Merge remote-tracking branch 'upstream/main' into ps250626-addProject…

1b724f7

…sUnderDeletionToProjectStateRegistry

pxsalehi merged commit 54c9db9 into elastic:main Jul 2, 2025
32 checks passed

Record project deletions in ProjectStateRegistry #130225

Record project deletions in ProjectStateRegistry #130225

Conversation

pxsalehi commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jun 27, 2025

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pxsalehi Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pxsalehi commented Jun 27, 2025 •

edited

Loading

ywangd Jul 2, 2025 •

edited

Loading

pxsalehi Jul 1, 2025 •

edited

Loading