From d68318224dd9adb5d5aca7cf16cac2dbc19e4349 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 29 Mar 2023 12:48:10 +0200 Subject: Create real distribution config for all tests --- .../core/NodeStateChangeCheckerTest.java | 102 ++++++++++----------- configdefinitions/src/vespa/stor-distribution.def | 2 +- 2 files changed, 51 insertions(+), 53 deletions(-) diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java index 60d4866a33e..61f87d1a88d 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java @@ -3,7 +3,6 @@ package com.yahoo.vespa.clustercontroller.core; import com.yahoo.vdslib.distribution.ConfiguredNode; import com.yahoo.vdslib.distribution.Distribution; -import com.yahoo.vdslib.distribution.Group; import com.yahoo.vdslib.distribution.GroupVisitor; import com.yahoo.vdslib.state.ClusterState; import com.yahoo.vdslib.state.Node; @@ -22,16 +21,13 @@ import static com.yahoo.vdslib.state.NodeType.STORAGE; import static com.yahoo.vdslib.state.State.DOWN; import static com.yahoo.vdslib.state.State.INITIALIZING; import static com.yahoo.vdslib.state.State.UP; +import static com.yahoo.vespa.clustercontroller.core.NodeStateChangeChecker.Result; import static com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.SetUnitStateRequest.Condition.FORCE; import static com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.SetUnitStateRequest.Condition.SAFE; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -import static com.yahoo.vespa.clustercontroller.core.NodeStateChangeChecker.Result; public class NodeStateChangeCheckerTest { @@ -72,10 +68,12 @@ public class NodeStateChangeCheckerTest { } private ContentCluster createCluster(int nodeCount) { + return createCluster(nodeCount, 1); + } + + private ContentCluster createCluster(int nodeCount, int groupCount) { Collection nodes = createNodes(nodeCount); - Distribution distribution = mock(Distribution.class); - Group group = new Group(2, "two"); - when(distribution.getRootGroup()).thenReturn(group); + Distribution distribution = new Distribution(createDistributionConfig(nodeCount, groupCount)); return new ContentCluster("Clustername", nodes, distribution); } @@ -193,10 +191,11 @@ public class NodeStateChangeCheckerTest { void testSafeMaintenanceDisallowedWhenDistributorInGroupIsDown() { // Nodes 0-3, distributor 0 being in maintenance with "Orchestrator" description. // 2 groups: nodes 0-1 is group 0, 2-3 is group 1. - ContentCluster cluster = createCluster(4); + ContentCluster cluster = createCluster(4, 2); cluster.clusterInfo().getDistributorNodeInfo(0) .setWantedState(new NodeState(STORAGE, DOWN).setDescription("Orchestrator")); - HierarchicalGroupVisiting visiting = makeHierarchicalGroupVisitingWith2Groups(4); + HierarchicalGroupVisiting visiting = new HierarchicalGroupVisitingAdapter(cluster.getDistribution()); + var nodeStateChangeChecker = new NodeStateChangeChecker( requiredRedundancy, visiting, cluster.clusterInfo(), false, cluster.maxNumberOfGroupsAllowedToBeDown()); ClusterState clusterStateWith0InMaintenance = clusterState(String.format( @@ -228,9 +227,9 @@ public class NodeStateChangeCheckerTest { void testSafeMaintenanceWhenOtherStorageNodeInGroupIsSuspended() { // Nodes 0-3, storage node 0 being in maintenance with "Orchestrator" description. // 2 groups: nodes 0-1 is group 0, 2-3 is group 1. - ContentCluster cluster = createCluster(4); + ContentCluster cluster = createCluster(4, 2); cluster.clusterInfo().getStorageNodeInfo(0).setWantedState(new NodeState(STORAGE, State.MAINTENANCE).setDescription("Orchestrator")); - HierarchicalGroupVisiting visiting = makeHierarchicalGroupVisitingWith2Groups(4); + HierarchicalGroupVisiting visiting = new HierarchicalGroupVisitingAdapter(cluster.getDistribution()); var nodeStateChangeChecker = new NodeStateChangeChecker( requiredRedundancy, visiting, cluster.clusterInfo(), false, cluster.maxNumberOfGroupsAllowedToBeDown()); ClusterState clusterStateWith0InMaintenance = clusterState(String.format( @@ -259,46 +258,6 @@ public class NodeStateChangeCheckerTest { } } - /** - * Make a HierarchicalGroupVisiting with the given number of nodes, with 2 groups: - * Group "0" is nodes 0-1, group "1" is 2-3. - */ - private HierarchicalGroupVisiting makeHierarchicalGroupVisitingWith2Groups(int nodes) { - int groups = 2; - if (nodes % groups != 0) { - throw new IllegalArgumentException("Cannot have 2 groups with an odd number of nodes: " + nodes); - } - int nodesPerGroup = nodes / groups; - - var configBuilder = new StorDistributionConfig.Builder() - .active_per_leaf_group(true) - .ready_copies(2) - .redundancy(2) - .initial_redundancy(2); - - configBuilder.group(new StorDistributionConfig.Group.Builder() - .index("invalid") - .name("invalid") - .capacity(nodes) - .partitions("1|*")); - - int nodeIndex = 0; - for (int i = 0; i < groups; ++i) { - var groupBuilder = new StorDistributionConfig.Group.Builder() - .index(String.valueOf(i)) - .name(String.valueOf(i)) - .capacity(nodesPerGroup) - .partitions(""); - for (int j = 0; j < nodesPerGroup; ++j, ++nodeIndex) { - groupBuilder.nodes(new StorDistributionConfig.Group.Nodes.Builder() - .index(nodeIndex)); - } - configBuilder.group(groupBuilder); - } - - return new HierarchicalGroupVisitingAdapter(new Distribution(configBuilder.build())); - } - @Test void testSafeSetStateDistributors() { NodeStateChangeChecker nodeStateChangeChecker = createChangeChecker(createCluster(1)); @@ -753,4 +712,43 @@ public class NodeStateChangeCheckerTest { nodes.add(new ConfiguredNode(i, false)); return nodes; } + + private StorDistributionConfig createDistributionConfig(int nodes) { + return createDistributionConfig(nodes, 1); + } + + private StorDistributionConfig createDistributionConfig(int nodes, int groups) { + if (nodes % groups != 0) { + throw new IllegalArgumentException("Cannot have " + groups + " groups with an odd number of nodes: " + nodes); + } + int nodesPerGroup = nodes / groups; + + var configBuilder = new StorDistributionConfig.Builder() + .active_per_leaf_group(true) + .ready_copies(2) + .redundancy(2) + .initial_redundancy(2); + + configBuilder.group(new StorDistributionConfig.Group.Builder() + .index("invalid") + .name("invalid") + .capacity(nodes) + .partitions("1|*")); + + int nodeIndex = 0; + for (int i = 0; i < groups; ++i) { + var groupBuilder = new StorDistributionConfig.Group.Builder() + .index(String.valueOf(i)) + .name(String.valueOf(i)) + .capacity(nodesPerGroup) + .partitions(""); + for (int j = 0; j < nodesPerGroup; ++j, ++nodeIndex) { + groupBuilder.nodes(new StorDistributionConfig.Group.Nodes.Builder() + .index(nodeIndex)); + } + configBuilder.group(groupBuilder); + } + return configBuilder.build(); + } + } diff --git a/configdefinitions/src/vespa/stor-distribution.def b/configdefinitions/src/vespa/stor-distribution.def index f35a941b6ae..c5599a8589b 100644 --- a/configdefinitions/src/vespa/stor-distribution.def +++ b/configdefinitions/src/vespa/stor-distribution.def @@ -23,7 +23,7 @@ ready_copies int default=0 ## If this option is set true, the distributor will try to enforce one active copy ## of buckets per leaf hierarchical group. This is a simple implementation for ## search to be able to setup top level dispatcher to only send search to all -## nodes in one group as they have a static cost per node used. It used, +## nodes in one group as they have a static cost per node used. If used, ## hierarchical grouping can not be used for other purposes. Using this option ## implies that: ## - ready_copies == redundancy -- cgit v1.2.3 From b8ff57771da71564dc40ba3e3cc0e05d27c062a5 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 29 Mar 2023 13:47:53 +0200 Subject: Make sure to make correct config for flat clusters (1 group) --- .../core/NodeStateChangeCheckerTest.java | 26 +++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java index 61f87d1a88d..f3c83dba316 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java @@ -714,13 +714,33 @@ public class NodeStateChangeCheckerTest { } private StorDistributionConfig createDistributionConfig(int nodes) { - return createDistributionConfig(nodes, 1); + var configBuilder = new StorDistributionConfig.Builder() + .active_per_leaf_group(true) + .ready_copies(2) + .redundancy(2) + .initial_redundancy(2); + + var groupBuilder = new StorDistributionConfig.Group.Builder() + .index("invalid") + .name("invalid") + .capacity(nodes); + int nodeIndex = 0; + for (int j = 0; j < nodes; ++j, ++nodeIndex) { + groupBuilder.nodes(new StorDistributionConfig.Group.Nodes.Builder() + .index(nodeIndex)); + } + configBuilder.group(groupBuilder); + + return configBuilder.build(); } + // When more than 1 group private StorDistributionConfig createDistributionConfig(int nodes, int groups) { - if (nodes % groups != 0) { + if (groups == 1) return createDistributionConfig(nodes); + + if (nodes % groups != 0) throw new IllegalArgumentException("Cannot have " + groups + " groups with an odd number of nodes: " + nodes); - } + int nodesPerGroup = nodes / groups; var configBuilder = new StorDistributionConfig.Builder() -- cgit v1.2.3 From 450e7cb4f76c1313a7728c96d9ae128ee33f9e93 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 29 Mar 2023 14:00:19 +0200 Subject: Use correct redundancy and don't set active_per_leaf_group --- .../vespa/clustercontroller/core/NodeStateChangeCheckerTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java index f3c83dba316..86386f7e5c3 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java @@ -715,10 +715,9 @@ public class NodeStateChangeCheckerTest { private StorDistributionConfig createDistributionConfig(int nodes) { var configBuilder = new StorDistributionConfig.Builder() - .active_per_leaf_group(true) - .ready_copies(2) - .redundancy(2) - .initial_redundancy(2); + .ready_copies(requiredRedundancy) + .redundancy(requiredRedundancy) + .initial_redundancy(requiredRedundancy); var groupBuilder = new StorDistributionConfig.Group.Builder() .index("invalid") -- cgit v1.2.3 From d3732a7476d2504580c474f4792619cff01fddd3 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 29 Mar 2023 14:02:24 +0200 Subject: Simplify now that we can create HierarchicalGroupVisiting in constructor --- .../vespa/clustercontroller/core/ContentCluster.java | 9 ++------- .../core/NodeStateChangeChecker.java | 11 +++++------ .../core/NodeStateChangeCheckerTest.java | 19 +++++-------------- 3 files changed, 12 insertions(+), 27 deletions(-) diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java index 9347fadc0e0..57a4f66ef17 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java @@ -138,13 +138,8 @@ public class ContentCluster { Node node, ClusterState clusterState, SetUnitStateRequest.Condition condition, NodeState oldState, NodeState newState, boolean inMoratorium, int maxNumberOfGroupsAllowedToBeDown) { - NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker( - distribution.getRedundancy(), - new HierarchicalGroupVisitingAdapter(distribution), - clusterInfo, - inMoratorium, - maxNumberOfGroupsAllowedToBeDown - ); + NodeStateChangeChecker nodeStateChangeChecker = + new NodeStateChangeChecker(distribution, clusterInfo, inMoratorium, maxNumberOfGroupsAllowedToBeDown); return nodeStateChangeChecker.evaluateTransition(node, clusterState, condition, oldState, newState); } diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java index 7a77bb2b571..bf9ce2b4568 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.clustercontroller.core; import com.yahoo.lang.MutableBoolean; import com.yahoo.lang.SettableOptional; import com.yahoo.vdslib.distribution.ConfiguredNode; +import com.yahoo.vdslib.distribution.Distribution; import com.yahoo.vdslib.distribution.Group; import com.yahoo.vdslib.state.ClusterState; import com.yahoo.vdslib.state.Node; @@ -44,13 +45,12 @@ public class NodeStateChangeChecker { private final boolean inMoratorium; public NodeStateChangeChecker( - int requiredRedundancy, - HierarchicalGroupVisiting groupVisiting, + Distribution distribution, ClusterInfo clusterInfo, boolean inMoratorium, int maxNumberOfGroupsAllowedToBeDown) { - this.requiredRedundancy = requiredRedundancy; - this.groupVisiting = groupVisiting; + this.requiredRedundancy = distribution.getRedundancy(); + this.groupVisiting = new HierarchicalGroupVisitingAdapter(distribution); this.clusterInfo = clusterInfo; this.inMoratorium = inMoratorium; } @@ -392,8 +392,7 @@ public class NodeStateChangeChecker { return allowSettingOfWantedState(); } - private Result checkStorageNodesForDistributor( - DistributorNodeInfo distributorNodeInfo, List storageNodes, Node node) { + private Result checkStorageNodesForDistributor(DistributorNodeInfo distributorNodeInfo, List storageNodes, Node node) { for (StorageNode storageNode : storageNodes) { if (storageNode.getIndex() == node.getIndex()) { Integer minReplication = storageNode.getMinCurrentReplicationFactorOrNull(); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java index 86386f7e5c3..f480e68cf3d 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java @@ -3,7 +3,6 @@ package com.yahoo.vespa.clustercontroller.core; import com.yahoo.vdslib.distribution.ConfiguredNode; import com.yahoo.vdslib.distribution.Distribution; -import com.yahoo.vdslib.distribution.GroupVisitor; import com.yahoo.vdslib.state.ClusterState; import com.yahoo.vdslib.state.Node; import com.yahoo.vdslib.state.NodeState; @@ -41,11 +40,6 @@ public class NodeStateChangeCheckerTest { private static final NodeState MAINTENANCE_NODE_STATE = createNodeState(State.MAINTENANCE, "Orchestrator"); private static final NodeState DOWN_NODE_STATE = createNodeState(DOWN, "RetireEarlyExpirer"); - private static final HierarchicalGroupVisiting noopVisiting = new HierarchicalGroupVisiting() { - @Override public boolean isHierarchical() { return false; } - @Override public void visit(GroupVisitor visitor) { } - }; - private static NodeState createNodeState(State state, String description) { return new NodeState(STORAGE, state).setDescription(description); } @@ -63,7 +57,7 @@ public class NodeStateChangeCheckerTest { } private NodeStateChangeChecker createChangeChecker(ContentCluster cluster) { - return new NodeStateChangeChecker(requiredRedundancy, noopVisiting, cluster.clusterInfo(), + return new NodeStateChangeChecker(cluster.getDistribution(), cluster.clusterInfo(), false, cluster.maxNumberOfGroupsAllowedToBeDown()); } @@ -127,7 +121,7 @@ public class NodeStateChangeCheckerTest { void testDeniedInMoratorium() { ContentCluster cluster = createCluster(4); var nodeStateChangeChecker = new NodeStateChangeChecker( - requiredRedundancy, noopVisiting, cluster.clusterInfo(), true, cluster.maxNumberOfGroupsAllowedToBeDown()); + cluster.getDistribution(), cluster.clusterInfo(), true, cluster.maxNumberOfGroupsAllowedToBeDown()); Result result = nodeStateChangeChecker.evaluateTransition( new Node(STORAGE, 10), defaultAllUpClusterState(), SAFE, UP_NODE_STATE, MAINTENANCE_NODE_STATE); @@ -169,7 +163,7 @@ public class NodeStateChangeCheckerTest { @Test void testSafeMaintenanceDisallowedWhenOtherDistributorInFlatClusterIsSuspended() { - // Nodes 0-3, storage node 0 being in maintenance with "Orchestrator" description. + // Nodes 0-3, distributor 0 being in maintenance with "Orchestrator" description. ContentCluster cluster = createCluster(4); cluster.clusterInfo().getDistributorNodeInfo(0) .setWantedState(new NodeState(DISTRIBUTOR, DOWN).setDescription("Orchestrator")); @@ -194,10 +188,8 @@ public class NodeStateChangeCheckerTest { ContentCluster cluster = createCluster(4, 2); cluster.clusterInfo().getDistributorNodeInfo(0) .setWantedState(new NodeState(STORAGE, DOWN).setDescription("Orchestrator")); - HierarchicalGroupVisiting visiting = new HierarchicalGroupVisitingAdapter(cluster.getDistribution()); - var nodeStateChangeChecker = new NodeStateChangeChecker( - requiredRedundancy, visiting, cluster.clusterInfo(), false, cluster.maxNumberOfGroupsAllowedToBeDown()); + cluster.getDistribution(), cluster.clusterInfo(), false, cluster.maxNumberOfGroupsAllowedToBeDown()); ClusterState clusterStateWith0InMaintenance = clusterState(String.format( "version:%d distributor:4 .0.s:d storage:4", currentClusterStateVersion)); @@ -229,9 +221,8 @@ public class NodeStateChangeCheckerTest { // 2 groups: nodes 0-1 is group 0, 2-3 is group 1. ContentCluster cluster = createCluster(4, 2); cluster.clusterInfo().getStorageNodeInfo(0).setWantedState(new NodeState(STORAGE, State.MAINTENANCE).setDescription("Orchestrator")); - HierarchicalGroupVisiting visiting = new HierarchicalGroupVisitingAdapter(cluster.getDistribution()); var nodeStateChangeChecker = new NodeStateChangeChecker( - requiredRedundancy, visiting, cluster.clusterInfo(), false, cluster.maxNumberOfGroupsAllowedToBeDown()); + cluster.getDistribution(), cluster.clusterInfo(), false, cluster.maxNumberOfGroupsAllowedToBeDown()); ClusterState clusterStateWith0InMaintenance = clusterState(String.format( "version:%d distributor:4 storage:4 .0.s:m", currentClusterStateVersion)); -- cgit v1.2.3 From d1d827fdcaf636f927a1e79637f3dca2d89f1dcb Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 29 Mar 2023 14:14:49 +0200 Subject: Simplify NodeStateChangeChecker constructor --- .../vespa/clustercontroller/core/ContentCluster.java | 5 ++--- .../clustercontroller/core/NodeStateChangeChecker.java | 15 ++++++--------- .../core/restapiv2/requests/SetNodeStateRequest.java | 4 ++-- .../core/NodeStateChangeCheckerTest.java | 12 ++++-------- .../core/restapiv2/requests/SetNodeStateRequestTest.java | 10 +++++----- 5 files changed, 19 insertions(+), 27 deletions(-) diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java index 57a4f66ef17..9538167c6de 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java @@ -136,10 +136,9 @@ public class ContentCluster { */ public NodeStateChangeChecker.Result calculateEffectOfNewState( Node node, ClusterState clusterState, SetUnitStateRequest.Condition condition, - NodeState oldState, NodeState newState, boolean inMoratorium, int maxNumberOfGroupsAllowedToBeDown) { + NodeState oldState, NodeState newState, boolean inMoratorium) { - NodeStateChangeChecker nodeStateChangeChecker = - new NodeStateChangeChecker(distribution, clusterInfo, inMoratorium, maxNumberOfGroupsAllowedToBeDown); + NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker(this, inMoratorium); return nodeStateChangeChecker.evaluateTransition(node, clusterState, condition, oldState, newState); } diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java index bf9ce2b4568..2025dfef562 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java @@ -4,7 +4,6 @@ package com.yahoo.vespa.clustercontroller.core; import com.yahoo.lang.MutableBoolean; import com.yahoo.lang.SettableOptional; import com.yahoo.vdslib.distribution.ConfiguredNode; -import com.yahoo.vdslib.distribution.Distribution; import com.yahoo.vdslib.distribution.Group; import com.yahoo.vdslib.state.ClusterState; import com.yahoo.vdslib.state.Node; @@ -43,16 +42,14 @@ public class NodeStateChangeChecker { private final HierarchicalGroupVisiting groupVisiting; private final ClusterInfo clusterInfo; private final boolean inMoratorium; + private final int maxNumberOfGroupsAllowedToBeDown; - public NodeStateChangeChecker( - Distribution distribution, - ClusterInfo clusterInfo, - boolean inMoratorium, - int maxNumberOfGroupsAllowedToBeDown) { - this.requiredRedundancy = distribution.getRedundancy(); - this.groupVisiting = new HierarchicalGroupVisitingAdapter(distribution); - this.clusterInfo = clusterInfo; + public NodeStateChangeChecker(ContentCluster cluster, boolean inMoratorium) { + this.requiredRedundancy = cluster.getDistribution().getRedundancy(); + this.groupVisiting = new HierarchicalGroupVisitingAdapter(cluster.getDistribution()); + this.clusterInfo = cluster.clusterInfo(); this.inMoratorium = inMoratorium; + this.maxNumberOfGroupsAllowedToBeDown = cluster.maxNumberOfGroupsAllowedToBeDown(); } public static class Result { diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java index a2e77b4e3dd..1c72594377a 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java @@ -126,8 +126,8 @@ public class SetNodeStateRequest extends Request { NodeState wantedState = nodeInfo.getUserWantedState(); NodeState newWantedState = getRequestedNodeState(newStates, node); - Result result = cluster.calculateEffectOfNewState(node, currentClusterState, condition, wantedState, newWantedState, - inMasterMoratorium, cluster.maxNumberOfGroupsAllowedToBeDown()); + Result result = cluster.calculateEffectOfNewState(node, currentClusterState, condition, wantedState, + newWantedState, inMasterMoratorium); log.log(Level.FINE, () -> "node=" + node + " current-cluster-state=" + currentClusterState + // Includes version in output format diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java index f480e68cf3d..cb5f397b890 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java @@ -57,8 +57,7 @@ public class NodeStateChangeCheckerTest { } private NodeStateChangeChecker createChangeChecker(ContentCluster cluster) { - return new NodeStateChangeChecker(cluster.getDistribution(), cluster.clusterInfo(), - false, cluster.maxNumberOfGroupsAllowedToBeDown()); + return new NodeStateChangeChecker(cluster, false); } private ContentCluster createCluster(int nodeCount) { @@ -120,8 +119,7 @@ public class NodeStateChangeCheckerTest { @Test void testDeniedInMoratorium() { ContentCluster cluster = createCluster(4); - var nodeStateChangeChecker = new NodeStateChangeChecker( - cluster.getDistribution(), cluster.clusterInfo(), true, cluster.maxNumberOfGroupsAllowedToBeDown()); + var nodeStateChangeChecker = new NodeStateChangeChecker(cluster, true); Result result = nodeStateChangeChecker.evaluateTransition( new Node(STORAGE, 10), defaultAllUpClusterState(), SAFE, UP_NODE_STATE, MAINTENANCE_NODE_STATE); @@ -188,8 +186,7 @@ public class NodeStateChangeCheckerTest { ContentCluster cluster = createCluster(4, 2); cluster.clusterInfo().getDistributorNodeInfo(0) .setWantedState(new NodeState(STORAGE, DOWN).setDescription("Orchestrator")); - var nodeStateChangeChecker = new NodeStateChangeChecker( - cluster.getDistribution(), cluster.clusterInfo(), false, cluster.maxNumberOfGroupsAllowedToBeDown()); + var nodeStateChangeChecker = new NodeStateChangeChecker(cluster, false); ClusterState clusterStateWith0InMaintenance = clusterState(String.format( "version:%d distributor:4 .0.s:d storage:4", currentClusterStateVersion)); @@ -221,8 +218,7 @@ public class NodeStateChangeCheckerTest { // 2 groups: nodes 0-1 is group 0, 2-3 is group 1. ContentCluster cluster = createCluster(4, 2); cluster.clusterInfo().getStorageNodeInfo(0).setWantedState(new NodeState(STORAGE, State.MAINTENANCE).setDescription("Orchestrator")); - var nodeStateChangeChecker = new NodeStateChangeChecker( - cluster.getDistribution(), cluster.clusterInfo(), false, cluster.maxNumberOfGroupsAllowedToBeDown()); + var nodeStateChangeChecker = new NodeStateChangeChecker(cluster, false); ClusterState clusterStateWith0InMaintenance = clusterState(String.format( "version:%d distributor:4 storage:4 .0.s:m", currentClusterStateVersion)); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java index b208ff7fb27..6d93eadfe2a 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java @@ -1,5 +1,4 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - package com.yahoo.vespa.clustercontroller.core.restapiv2.requests; import com.yahoo.vdslib.state.ClusterState; @@ -17,14 +16,12 @@ import com.yahoo.vespa.clustercontroller.utils.staterestapi.response.SetResponse import com.yahoo.vespa.clustercontroller.utils.staterestapi.response.UnitState; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; - import java.util.HashMap; import java.util.Map; import java.util.Optional; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; -import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -32,7 +29,10 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class SetNodeStateRequestTest { + private static final String REASON = "operator"; + private static final boolean inMasterMoratorium = false; + private final ContentCluster cluster = mock(ContentCluster.class); private final SetUnitStateRequest.Condition condition = SetUnitStateRequest.Condition.SAFE; private final Map newStates = new HashMap<>(); @@ -41,7 +41,7 @@ public class SetNodeStateRequestTest { private final Node storageNode = new Node(NodeType.STORAGE, NODE_INDEX); private final NodeListener stateListener = mock(NodeListener.class); private final ClusterState currentClusterState = mock(ClusterState.class); - private boolean inMasterMoratorium = false; + private boolean probe = false; @BeforeEach @@ -130,7 +130,7 @@ public class SetNodeStateRequestTest { when(unitState.getId()).thenReturn(wantedStateString); when(unitState.getReason()).thenReturn(REASON); - when(cluster.calculateEffectOfNewState(any(), any(), any(), any(), any(), anyBoolean(), anyInt())).thenReturn(result); + when(cluster.calculateEffectOfNewState(any(), any(), any(), any(), any(), anyBoolean())).thenReturn(result); when(storageNodeInfo.isStorage()).thenReturn(storageNode.getType() == NodeType.STORAGE); when(storageNodeInfo.getNodeIndex()).thenReturn(storageNode.getIndex()); -- cgit v1.2.3 From 0dae4ed807b19bf558b508bac057ae479b71c492 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 29 Mar 2023 15:17:43 +0200 Subject: MInor changes after code review --- .../clustercontroller/core/NodeStateChangeCheckerTest.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java index cb5f397b890..e789a3cc6a6 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java @@ -161,7 +161,7 @@ public class NodeStateChangeCheckerTest { @Test void testSafeMaintenanceDisallowedWhenOtherDistributorInFlatClusterIsSuspended() { - // Nodes 0-3, distributor 0 being in maintenance with "Orchestrator" description. + // Nodes 0-3, distributor 0 being down with "Orchestrator" description. ContentCluster cluster = createCluster(4); cluster.clusterInfo().getDistributorNodeInfo(0) .setWantedState(new NodeState(DISTRIBUTOR, DOWN).setDescription("Orchestrator")); @@ -731,9 +731,9 @@ public class NodeStateChangeCheckerTest { var configBuilder = new StorDistributionConfig.Builder() .active_per_leaf_group(true) - .ready_copies(2) - .redundancy(2) - .initial_redundancy(2); + .ready_copies(groups) + .redundancy(groups) + .initial_redundancy(groups); configBuilder.group(new StorDistributionConfig.Group.Builder() .index("invalid") @@ -741,14 +741,13 @@ public class NodeStateChangeCheckerTest { .capacity(nodes) .partitions("1|*")); - int nodeIndex = 0; for (int i = 0; i < groups; ++i) { var groupBuilder = new StorDistributionConfig.Group.Builder() .index(String.valueOf(i)) .name(String.valueOf(i)) .capacity(nodesPerGroup) .partitions(""); - for (int j = 0; j < nodesPerGroup; ++j, ++nodeIndex) { + for (int nodeIndex = 0; nodeIndex < nodesPerGroup; ++nodeIndex) { groupBuilder.nodes(new StorDistributionConfig.Group.Nodes.Builder() .index(nodeIndex)); } -- cgit v1.2.3