From 2adcb2d510ac8667f771842572c4b2c8bcaf2992 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 5 May 2025 16:52:05 +1000 Subject: [PATCH 1/6] Add projectsDelta method to ClusterChangedEvent Similar to nodesDelta, the projectsDelta provides a centrailzed method to tell the added and removed projects so that cluster listeners/appliers do not need to compute it individually. Note the delta currently does not have updated. This needs to wait till we stop eagerly creating ProjectMetadata.Builder in Metadata.Builder. --- .../cluster/ClusterChangedEvent.java | 34 +++++++++++ .../cluster/ClusterChangedEventTests.java | 61 +++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java b/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java index f1f3b7354055c..9deca57237059 100644 --- a/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java +++ b/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java @@ -13,9 +13,11 @@ import org.elasticsearch.cluster.metadata.IndexGraveyard.IndexGraveyardDiff; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.metadata.Metadata; +import org.elasticsearch.cluster.metadata.ProjectId; import org.elasticsearch.cluster.metadata.ProjectMetadata; import org.elasticsearch.cluster.node.DiscoveryNodes; import org.elasticsearch.cluster.routing.IndexRoutingTable; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.gateway.GatewayService; import org.elasticsearch.index.Index; @@ -41,6 +43,8 @@ public class ClusterChangedEvent { private final DiscoveryNodes.Delta nodesDelta; + private final ProjectsDelta projectsDelta; + public ClusterChangedEvent(String source, ClusterState state, ClusterState previousState) { Objects.requireNonNull(source, "source must not be null"); Objects.requireNonNull(state, "state must not be null"); @@ -49,6 +53,7 @@ public ClusterChangedEvent(String source, ClusterState state, ClusterState previ this.state = state; this.previousState = previousState; this.nodesDelta = state.nodes().delta(previousState.nodes()); + this.projectsDelta = calculateProjectDelta(previousState.metadata(), state.metadata()); } /** @@ -237,6 +242,13 @@ public boolean nodesChanged() { return nodesRemoved() || nodesAdded(); } + /** + * Returns the {@link ProjectsDelta} between the previous cluster state and the new cluster state. + */ + public ProjectsDelta projectDelta() { + return projectsDelta; + } + /** * Determines whether or not the current cluster state represents an entirely * new cluster, either when a node joins a cluster for the first time or when @@ -336,4 +348,26 @@ private List indicesDeletedFromTombstones() { .toList(); } + private static ProjectsDelta calculateProjectDelta(Metadata previousMetadata, Metadata currentMetadata) { + if (previousMetadata.projects().size() == 1 + && previousMetadata.hasProject(ProjectId.DEFAULT) + && currentMetadata.projects().size() == 1 + && currentMetadata.hasProject(ProjectId.DEFAULT)) { + return EMPTY_PROJECT_DELTA; + } + + return new ProjectsDelta( + Collections.unmodifiableSet(Sets.difference(currentMetadata.projects().keySet(), previousMetadata.projects().keySet())), + Collections.unmodifiableSet(Sets.difference(previousMetadata.projects().keySet(), currentMetadata.projects().keySet())) + ); + + } + + private static final ProjectsDelta EMPTY_PROJECT_DELTA = new ProjectsDelta(Set.of(), Set.of()); + + public record ProjectsDelta(Set added, Set removed) { + public boolean isEmpty() { + return added.isEmpty() && removed.isEmpty(); + } + } } diff --git a/server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java b/server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java index c08c82e27fa8b..8c04ddd6163ff 100644 --- a/server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java @@ -14,7 +14,9 @@ import org.elasticsearch.cluster.metadata.IndexGraveyard; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.metadata.Metadata; +import org.elasticsearch.cluster.metadata.ProjectId; import org.elasticsearch.cluster.metadata.ProjectMetadata; +import org.elasticsearch.cluster.metadata.ReservedStateMetadata; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodeRole; import org.elasticsearch.cluster.node.DiscoveryNodeUtils; @@ -520,6 +522,65 @@ public void testChangedCustomMetadataSetMultiProject() { assertEquals(Set.of(IndexGraveyard.TYPE, project2Custom.getWriteableName()), event.changedCustomProjectMetadataSet()); } + public void testProjectsDelta() { + final var state0 = ClusterState.builder(TEST_CLUSTER_NAME).build(); + + // No project changes + final var state1 = ClusterState.builder(state0) + .metadata(Metadata.builder(state0.metadata()).put(ReservedStateMetadata.builder("test").build())) + .build(); + ClusterChangedEvent event = new ClusterChangedEvent("test", state1, state0); + assertTrue(event.projectDelta().isEmpty()); + + // Add projects + final List projectIds = randomList(1, 5, ESTestCase::randomUniqueProjectId); + Metadata.Builder metadataBuilder = Metadata.builder(state1.metadata()); + for (ProjectId projectId : projectIds) { + metadataBuilder.put(ProjectMetadata.builder(projectId)); + } + final var state2 = ClusterState.builder(state1).metadata(metadataBuilder.build()).build(); + event = new ClusterChangedEvent("test", state2, state1); + assertThat(event.projectDelta().added(), containsInAnyOrder(projectIds.toArray())); + assertThat(event.projectDelta().removed(), empty()); + + // Add more projects and delete one + final var removedProjectIds = randomNonEmptySubsetOf(projectIds); + final List moreProjectIds = randomList(1, 3, ESTestCase::randomUniqueProjectId); + metadataBuilder = Metadata.builder(state2.metadata()); + GlobalRoutingTable.Builder routingTableBuilder = GlobalRoutingTable.builder(state2.globalRoutingTable()); + for (ProjectId projectId : removedProjectIds) { + metadataBuilder.removeProject(projectId); + routingTableBuilder.removeProject(projectId); + } + for (ProjectId projectId : moreProjectIds) { + metadataBuilder.put(ProjectMetadata.builder(projectId)); + } + + final var state3 = ClusterState.builder(state2).metadata(metadataBuilder.build()).routingTable(routingTableBuilder.build()).build(); + + event = new ClusterChangedEvent("test", state3, state2); + assertThat(event.projectDelta().added(), containsInAnyOrder(moreProjectIds.toArray())); + assertThat(event.projectDelta().removed(), containsInAnyOrder(removedProjectIds.toArray())); + + // Remove all projects + final List remainingProjects = state3.metadata() + .projects() + .keySet() + .stream() + .filter(projectId -> ProjectId.DEFAULT.equals(projectId) == false) + .toList(); + metadataBuilder = Metadata.builder(state3.metadata()); + routingTableBuilder = GlobalRoutingTable.builder(state3.globalRoutingTable()); + for (ProjectId projectId : remainingProjects) { + metadataBuilder.removeProject(projectId); + routingTableBuilder.removeProject(projectId); + } + final var state4 = ClusterState.builder(state3).metadata(metadataBuilder.build()).routingTable(routingTableBuilder.build()).build(); + event = new ClusterChangedEvent("test", state4, state3); + assertThat(event.projectDelta().added(), empty()); + assertThat(event.projectDelta().removed(), containsInAnyOrder(remainingProjects.toArray())); + } + private static class CustomClusterMetadata2 extends TestClusterCustomMetadata { protected CustomClusterMetadata2(String data) { super(data); From fbe4b8188ae5a33de20e868d7c249454477f38d7 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 6 May 2025 15:39:06 +1000 Subject: [PATCH 2/6] address feedback --- .../cluster/ClusterChangedEvent.java | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java b/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java index 9deca57237059..68cef72d03de6 100644 --- a/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java +++ b/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java @@ -17,7 +17,6 @@ import org.elasticsearch.cluster.metadata.ProjectMetadata; import org.elasticsearch.cluster.node.DiscoveryNodes; import org.elasticsearch.cluster.routing.IndexRoutingTable; -import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.gateway.GatewayService; import org.elasticsearch.index.Index; @@ -353,19 +352,25 @@ private static ProjectsDelta calculateProjectDelta(Metadata previousMetadata, Me && previousMetadata.hasProject(ProjectId.DEFAULT) && currentMetadata.projects().size() == 1 && currentMetadata.hasProject(ProjectId.DEFAULT)) { - return EMPTY_PROJECT_DELTA; + return ProjectsDelta.EMPTY; } - return new ProjectsDelta( - Collections.unmodifiableSet(Sets.difference(currentMetadata.projects().keySet(), previousMetadata.projects().keySet())), - Collections.unmodifiableSet(Sets.difference(previousMetadata.projects().keySet(), currentMetadata.projects().keySet())) - ); + final Set added = new HashSet<>(); + final Set removed = new HashSet<>(previousMetadata.projects().keySet()); + for (var currentProject : currentMetadata.projects().keySet()) { + if (removed.remove(currentProject) == false) { + added.add(currentProject); + } + } + assert added.contains(ProjectId.DEFAULT) == false; + assert removed.contains(ProjectId.DEFAULT) == false; + return new ProjectsDelta(Collections.unmodifiableSet(added), Collections.unmodifiableSet(removed)); } - private static final ProjectsDelta EMPTY_PROJECT_DELTA = new ProjectsDelta(Set.of(), Set.of()); - public record ProjectsDelta(Set added, Set removed) { + private static final ProjectsDelta EMPTY = new ProjectsDelta(Set.of(), Set.of()); + public boolean isEmpty() { return added.isEmpty() && removed.isEmpty(); } From e02d5d57f801243dd029b76cc0348dcc97d16109 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 6 May 2025 16:31:24 +1000 Subject: [PATCH 3/6] fix tests --- .../AsyncTaskMaintenanceServiceTests.java | 2 +- ...ueryableBuiltInRolesSynchronizerTests.java | 4 +- .../support/SecurityIndexManagerTests.java | 49 ++++++++++++------- 3 files changed, 34 insertions(+), 21 deletions(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/async/AsyncTaskMaintenanceServiceTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/async/AsyncTaskMaintenanceServiceTests.java index 27444c5efb5c8..0b800ffb5269b 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/async/AsyncTaskMaintenanceServiceTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/async/AsyncTaskMaintenanceServiceTests.java @@ -67,7 +67,7 @@ public void testStartStopDuringClusterChanges() { final String localNodeId = randomIdentifier(); final String alternateNodeId = randomIdentifier(); - final Metadata.Builder metadataBuilder = Metadata.builder(); + final Metadata.Builder metadataBuilder = Metadata.builder(Metadata.EMPTY_METADATA); final GlobalRoutingTable.Builder grtBuilder = GlobalRoutingTable.builder(); final ProjectId p1 = ProjectId.fromId("p1"); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java index eca0e875ffbf1..5d3430cbef640 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java @@ -330,8 +330,8 @@ public void testSecurityIndexDeleted() { synchronizer.clusterChanged(event(currentClusterState, previousClusterState)); - verify(previousClusterState, times(1)).metadata(); - verify(currentClusterState, times(1)).metadata(); + verify(previousClusterState, times(2)).metadata(); + verify(currentClusterState, times(2)).metadata(); verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerTests.java index 159f99a881a1d..d371e6e587a79 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerTests.java @@ -95,6 +95,7 @@ import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.oneOf; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -315,10 +316,13 @@ public void testIndexHealthChangeListeners() { final AtomicReference previousState = new AtomicReference<>(); final AtomicReference currentState = new AtomicReference<>(); final TriConsumer listener = (projId, prevState, state) -> { - projectIdRef.set(projId); - previousState.set(prevState); - currentState.set(state); - listenerCalled.set(true); + assertThat(projId, oneOf(ProjectId.DEFAULT, projectId)); + if (projId.equals(projectId)) { + projectIdRef.set(projId); + previousState.set(prevState); + currentState.set(state); + listenerCalled.set(true); + } }; manager.addStateListener(listener); @@ -573,9 +577,11 @@ public void testIndexOutOfDateListeners() { manager.clusterChanged(new ClusterChangedEvent("test-event", clusterState, clusterState)); AtomicBoolean upToDateChanged = new AtomicBoolean(); manager.addStateListener((projId, prev, current) -> { - assertThat(projId, equalTo(projectId)); - listenerCalled.set(true); - upToDateChanged.set(prev.isIndexUpToDate != current.isIndexUpToDate); + assertThat(projId, oneOf(ProjectId.DEFAULT, projectId)); + if (projId.equals(projectId)) { + listenerCalled.set(true); + upToDateChanged.set(prev.isIndexUpToDate != current.isIndexUpToDate); + } }); assertThat(manager.getProject(projectId).isIndexUpToDate(), is(true)); @@ -779,7 +785,7 @@ public void testGetRoleMappingsCleanupMigrationStatus() { metadataBuilder.put(builder.build()); // No role mappings in cluster state yet - metadataBuilder.putCustom(RoleMappingMetadata.TYPE, new RoleMappingMetadata(Set.of())); + metadataBuilder.getProject(projectId).putCustom(RoleMappingMetadata.TYPE, new RoleMappingMetadata(Set.of())); assertThat( SecurityIndexManager.getRoleMappingsCleanupMigrationStatus( @@ -803,10 +809,13 @@ public void testGetRoleMappingsCleanupMigrationStatus() { metadataBuilder.put(builder.build()); // Role mappings in cluster state with fallback name - metadataBuilder.putCustom( - RoleMappingMetadata.TYPE, - new RoleMappingMetadata(Set.of(new ExpressionRoleMapping(RoleMappingMetadata.FALLBACK_NAME, null, null, null, null, true))) - ); + metadataBuilder.getProject(projectId) + .putCustom( + RoleMappingMetadata.TYPE, + new RoleMappingMetadata( + Set.of(new ExpressionRoleMapping(RoleMappingMetadata.FALLBACK_NAME, null, null, null, null, true)) + ) + ); assertThat( SecurityIndexManager.getRoleMappingsCleanupMigrationStatus( @@ -830,10 +839,11 @@ public void testGetRoleMappingsCleanupMigrationStatus() { metadataBuilder.put(builder.build()); // Role mappings in cluster state - metadataBuilder.putCustom( - RoleMappingMetadata.TYPE, - new RoleMappingMetadata(Set.of(new ExpressionRoleMapping("role_mapping_1", null, null, null, null, true))) - ); + metadataBuilder.getProject(projectId) + .putCustom( + RoleMappingMetadata.TYPE, + new RoleMappingMetadata(Set.of(new ExpressionRoleMapping("role_mapping_1", null, null, null, null, true))) + ); assertThat( SecurityIndexManager.getRoleMappingsCleanupMigrationStatus( @@ -1090,7 +1100,7 @@ private ClusterState.Builder createClusterState( String mappings, Map compatibilityVersions ) { - final Metadata metadata = Metadata.builder() + final Metadata metadata = Metadata.builder(Metadata.EMPTY_METADATA) .put(createProjectMetadata(projectId, indexName, aliasName, format, state, mappings)) .build(); final GlobalRoutingTable routingTable = GlobalRoutingTableTestHelper.buildRoutingTable(metadata, RoutingTable.Builder::addAsNew); @@ -1141,7 +1151,10 @@ public static ClusterState state(ProjectId projectId) { .masterNodeId("1") .localNodeId("1") .build(); - final Metadata metadata = Metadata.builder().put(ProjectMetadata.builder(projectId)).generateClusterUuidIfNeeded().build(); + final Metadata metadata = Metadata.builder(Metadata.EMPTY_METADATA) + .put(ProjectMetadata.builder(projectId)) + .generateClusterUuidIfNeeded() + .build(); return ClusterState.builder(CLUSTER_NAME).nodes(nodes).metadata(metadata).build(); } From 869a9e0c5b11d3b2bd2e4d2bb9e7d0bf34dc116f Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 9 May 2025 12:45:29 +1000 Subject: [PATCH 4/6] revert to use sets.difference --- .../cluster/ClusterChangedEvent.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java b/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java index 68cef72d03de6..b4004ab6ede7f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java +++ b/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java @@ -17,6 +17,7 @@ import org.elasticsearch.cluster.metadata.ProjectMetadata; import org.elasticsearch.cluster.node.DiscoveryNodes; import org.elasticsearch.cluster.routing.IndexRoutingTable; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.gateway.GatewayService; import org.elasticsearch.index.Index; @@ -348,24 +349,23 @@ private List indicesDeletedFromTombstones() { } private static ProjectsDelta calculateProjectDelta(Metadata previousMetadata, Metadata currentMetadata) { - if (previousMetadata.projects().size() == 1 - && previousMetadata.hasProject(ProjectId.DEFAULT) - && currentMetadata.projects().size() == 1 - && currentMetadata.hasProject(ProjectId.DEFAULT)) { + if (previousMetadata == currentMetadata + || (previousMetadata.projects().size() == 1 + && previousMetadata.hasProject(ProjectId.DEFAULT) + && currentMetadata.projects().size() == 1 + && currentMetadata.hasProject(ProjectId.DEFAULT))) { return ProjectsDelta.EMPTY; } - final Set added = new HashSet<>(); - final Set removed = new HashSet<>(previousMetadata.projects().keySet()); - for (var currentProject : currentMetadata.projects().keySet()) { - if (removed.remove(currentProject) == false) { - added.add(currentProject); - } - } + final Set added = Collections.unmodifiableSet( + Sets.difference(currentMetadata.projects().keySet(), previousMetadata.projects().keySet()) + ); + final Set removed = Collections.unmodifiableSet( + Sets.difference(previousMetadata.projects().keySet(), currentMetadata.projects().keySet()) + ); assert added.contains(ProjectId.DEFAULT) == false; assert removed.contains(ProjectId.DEFAULT) == false; - - return new ProjectsDelta(Collections.unmodifiableSet(added), Collections.unmodifiableSet(removed)); + return new ProjectsDelta(added, removed); } public record ProjectsDelta(Set added, Set removed) { From 7dc1765784db24360f7a828c961d156cce7ae503 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 9 May 2025 15:32:17 +1000 Subject: [PATCH 5/6] disable assertions since too many tests add/remove default project --- .../cluster/ClusterChangedEvent.java | 5 +- .../AsyncTaskMaintenanceServiceTests.java | 2 +- ...ueryableBuiltInRolesSynchronizerTests.java | 4 +- .../support/SecurityIndexManagerTests.java | 49 +++++++------------ 4 files changed, 24 insertions(+), 36 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java b/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java index b4004ab6ede7f..8285900f0b00f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java +++ b/server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java @@ -363,8 +363,9 @@ private static ProjectsDelta calculateProjectDelta(Metadata previousMetadata, Me final Set removed = Collections.unmodifiableSet( Sets.difference(previousMetadata.projects().keySet(), currentMetadata.projects().keySet()) ); - assert added.contains(ProjectId.DEFAULT) == false; - assert removed.contains(ProjectId.DEFAULT) == false; + // TODO: Enable the following assertions once tests no longer add or remove default projects + // assert added.contains(ProjectId.DEFAULT) == false; + // assert removed.contains(ProjectId.DEFAULT) == false; return new ProjectsDelta(added, removed); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/async/AsyncTaskMaintenanceServiceTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/async/AsyncTaskMaintenanceServiceTests.java index 0b800ffb5269b..27444c5efb5c8 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/async/AsyncTaskMaintenanceServiceTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/async/AsyncTaskMaintenanceServiceTests.java @@ -67,7 +67,7 @@ public void testStartStopDuringClusterChanges() { final String localNodeId = randomIdentifier(); final String alternateNodeId = randomIdentifier(); - final Metadata.Builder metadataBuilder = Metadata.builder(Metadata.EMPTY_METADATA); + final Metadata.Builder metadataBuilder = Metadata.builder(); final GlobalRoutingTable.Builder grtBuilder = GlobalRoutingTable.builder(); final ProjectId p1 = ProjectId.fromId("p1"); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java index 5d3430cbef640..eca0e875ffbf1 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java @@ -330,8 +330,8 @@ public void testSecurityIndexDeleted() { synchronizer.clusterChanged(event(currentClusterState, previousClusterState)); - verify(previousClusterState, times(2)).metadata(); - verify(currentClusterState, times(2)).metadata(); + verify(previousClusterState, times(1)).metadata(); + verify(currentClusterState, times(1)).metadata(); verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerTests.java index d371e6e587a79..159f99a881a1d 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerTests.java @@ -95,7 +95,6 @@ import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; -import static org.hamcrest.Matchers.oneOf; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -316,13 +315,10 @@ public void testIndexHealthChangeListeners() { final AtomicReference previousState = new AtomicReference<>(); final AtomicReference currentState = new AtomicReference<>(); final TriConsumer listener = (projId, prevState, state) -> { - assertThat(projId, oneOf(ProjectId.DEFAULT, projectId)); - if (projId.equals(projectId)) { - projectIdRef.set(projId); - previousState.set(prevState); - currentState.set(state); - listenerCalled.set(true); - } + projectIdRef.set(projId); + previousState.set(prevState); + currentState.set(state); + listenerCalled.set(true); }; manager.addStateListener(listener); @@ -577,11 +573,9 @@ public void testIndexOutOfDateListeners() { manager.clusterChanged(new ClusterChangedEvent("test-event", clusterState, clusterState)); AtomicBoolean upToDateChanged = new AtomicBoolean(); manager.addStateListener((projId, prev, current) -> { - assertThat(projId, oneOf(ProjectId.DEFAULT, projectId)); - if (projId.equals(projectId)) { - listenerCalled.set(true); - upToDateChanged.set(prev.isIndexUpToDate != current.isIndexUpToDate); - } + assertThat(projId, equalTo(projectId)); + listenerCalled.set(true); + upToDateChanged.set(prev.isIndexUpToDate != current.isIndexUpToDate); }); assertThat(manager.getProject(projectId).isIndexUpToDate(), is(true)); @@ -785,7 +779,7 @@ public void testGetRoleMappingsCleanupMigrationStatus() { metadataBuilder.put(builder.build()); // No role mappings in cluster state yet - metadataBuilder.getProject(projectId).putCustom(RoleMappingMetadata.TYPE, new RoleMappingMetadata(Set.of())); + metadataBuilder.putCustom(RoleMappingMetadata.TYPE, new RoleMappingMetadata(Set.of())); assertThat( SecurityIndexManager.getRoleMappingsCleanupMigrationStatus( @@ -809,13 +803,10 @@ public void testGetRoleMappingsCleanupMigrationStatus() { metadataBuilder.put(builder.build()); // Role mappings in cluster state with fallback name - metadataBuilder.getProject(projectId) - .putCustom( - RoleMappingMetadata.TYPE, - new RoleMappingMetadata( - Set.of(new ExpressionRoleMapping(RoleMappingMetadata.FALLBACK_NAME, null, null, null, null, true)) - ) - ); + metadataBuilder.putCustom( + RoleMappingMetadata.TYPE, + new RoleMappingMetadata(Set.of(new ExpressionRoleMapping(RoleMappingMetadata.FALLBACK_NAME, null, null, null, null, true))) + ); assertThat( SecurityIndexManager.getRoleMappingsCleanupMigrationStatus( @@ -839,11 +830,10 @@ public void testGetRoleMappingsCleanupMigrationStatus() { metadataBuilder.put(builder.build()); // Role mappings in cluster state - metadataBuilder.getProject(projectId) - .putCustom( - RoleMappingMetadata.TYPE, - new RoleMappingMetadata(Set.of(new ExpressionRoleMapping("role_mapping_1", null, null, null, null, true))) - ); + metadataBuilder.putCustom( + RoleMappingMetadata.TYPE, + new RoleMappingMetadata(Set.of(new ExpressionRoleMapping("role_mapping_1", null, null, null, null, true))) + ); assertThat( SecurityIndexManager.getRoleMappingsCleanupMigrationStatus( @@ -1100,7 +1090,7 @@ private ClusterState.Builder createClusterState( String mappings, Map compatibilityVersions ) { - final Metadata metadata = Metadata.builder(Metadata.EMPTY_METADATA) + final Metadata metadata = Metadata.builder() .put(createProjectMetadata(projectId, indexName, aliasName, format, state, mappings)) .build(); final GlobalRoutingTable routingTable = GlobalRoutingTableTestHelper.buildRoutingTable(metadata, RoutingTable.Builder::addAsNew); @@ -1151,10 +1141,7 @@ public static ClusterState state(ProjectId projectId) { .masterNodeId("1") .localNodeId("1") .build(); - final Metadata metadata = Metadata.builder(Metadata.EMPTY_METADATA) - .put(ProjectMetadata.builder(projectId)) - .generateClusterUuidIfNeeded() - .build(); + final Metadata metadata = Metadata.builder().put(ProjectMetadata.builder(projectId)).generateClusterUuidIfNeeded().build(); return ClusterState.builder(CLUSTER_NAME).nodes(nodes).metadata(metadata).build(); } From 77f84f628634cc03f0f18e3e5a8de288bc0665c3 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 9 May 2025 16:29:29 +1000 Subject: [PATCH 6/6] fix test --- .../persistence/TrainedModelCacheMetadataServiceTests.java | 1 + .../support/QueryableBuiltInRolesSynchronizerTests.java | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/inference/persistence/TrainedModelCacheMetadataServiceTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/inference/persistence/TrainedModelCacheMetadataServiceTests.java index b333b3596aba5..387c6f36af1ee 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/inference/persistence/TrainedModelCacheMetadataServiceTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/inference/persistence/TrainedModelCacheMetadataServiceTests.java @@ -74,6 +74,7 @@ public void testRefreshCacheVersionOnMasterNode() { ClusterState clusterState = mock(ClusterState.class); when(clusterState.clusterRecovered()).thenReturn(true); when(clusterState.nodes()).thenReturn(clusterNodes); + when(clusterState.metadata()).thenReturn(Metadata.EMPTY_METADATA); modelCacheMetadataService.clusterChanged(new ClusterChangedEvent("test", clusterState, ClusterState.EMPTY_STATE)); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java index eca0e875ffbf1..5d3430cbef640 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java @@ -330,8 +330,8 @@ public void testSecurityIndexDeleted() { synchronizer.clusterChanged(event(currentClusterState, previousClusterState)); - verify(previousClusterState, times(1)).metadata(); - verify(currentClusterState, times(1)).metadata(); + verify(previousClusterState, times(2)).metadata(); + verify(currentClusterState, times(2)).metadata(); verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); }