diff options
3 files changed, 48 insertions, 27 deletions
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 60671c96474..54240330de3 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 @@ -92,9 +92,9 @@ public class NodeStateChangeChecker { return new Result(Action.ALREADY_SET, "Basic preconditions fulfilled and new state is already effective"); } - public boolean settingWantedStateIsAllowed() { - return action == Action.MUST_SET_WANTED_STATE; - } + public boolean settingWantedStateIsAllowed() { return action == Action.MUST_SET_WANTED_STATE; } + + public boolean settingWantedStateIsNotAllowed() { return ! settingWantedStateIsAllowed(); } public boolean wantedStateAlreadySet() { return action == Action.ALREADY_SET; @@ -200,7 +200,7 @@ public class NodeStateChangeChecker { private Result canSetStateUp(NodeInfo nodeInfo, NodeState oldWantedState) { if (oldWantedState.getState() == UP) { - // The description is not significant when setting wanting to set the state to UP + // The description is not significant when wanting to set the state to UP return createAlreadySet(); } @@ -228,7 +228,7 @@ public class NodeStateChangeChecker { if (maxNumberOfGroupsAllowedToBeDown == -1) { var otherGroupCheck = anotherNodeInAnotherGroupHasWantedState(nodeInfo); - if (!otherGroupCheck.settingWantedStateIsAllowed()) { + if (otherGroupCheck.settingWantedStateIsNotAllowed()) { return otherGroupCheck; } if (anotherNodeInGroupAlreadyAllowed(nodeInfo, newDescription)) { @@ -246,13 +246,13 @@ public class NodeStateChangeChecker { } Result allNodesAreUpCheck = checkAllNodesAreUp(clusterState); - if (!allNodesAreUpCheck.settingWantedStateIsAllowed()) { + if (allNodesAreUpCheck.settingWantedStateIsNotAllowed()) { log.log(FINE, "allNodesAreUpCheck: " + allNodesAreUpCheck); return allNodesAreUpCheck; } Result checkDistributorsResult = checkDistributors(nodeInfo.getNode(), clusterState.getVersion()); - if (!checkDistributorsResult.settingWantedStateIsAllowed()) { + if (checkDistributorsResult.settingWantedStateIsNotAllowed()) { log.log(FINE, "checkDistributors: "+ checkDistributorsResult); return checkDistributorsResult; } @@ -271,7 +271,7 @@ public class NodeStateChangeChecker { groupVisiting.visit(group -> { if (!groupContainsNode(group, nodeInfo.getNode())) { Result result = otherNodeInGroupHasWantedState(group); - if (!result.settingWantedStateIsAllowed()) { + if (result.settingWantedStateIsNotAllowed()) { anotherNodeHasWantedState.set(result); // Have found a node that is suspended, halt the visiting return false; @@ -283,7 +283,7 @@ public class NodeStateChangeChecker { return anotherNodeHasWantedState.asOptional().orElseGet(Result::allowSettingOfWantedState); } else { - // Return a disallow-result if there is another node with a wanted state + // Returns a disallow-result if there is another node with a wanted state return otherNodeHasWantedState(nodeInfo); } } @@ -335,7 +335,7 @@ public class NodeStateChangeChecker { } else { // Return a disallow-result if there is another node with a wanted state var otherNodeHasWantedState = otherNodeHasWantedState(nodeInfo); - if ( ! otherNodeHasWantedState.settingWantedStateIsAllowed()) + if (otherNodeHasWantedState.settingWantedStateIsNotAllowed()) return Optional.of(otherNodeHasWantedState); } return Optional.empty(); @@ -484,7 +484,8 @@ public class NodeStateChangeChecker { return allowSettingOfWantedState(); } - private Result checkStorageNodesForDistributor(DistributorNodeInfo distributorNodeInfo, List<StorageNode> storageNodes, Node node) { + private Result checkStorageNodesForDistributor(DistributorNodeInfo distributorNodeInfo, Node node) { + List<StorageNode> storageNodes = distributorNodeInfo.getHostInfo().getDistributor().getStorageNodes(); for (StorageNode storageNode : storageNodes) { if (storageNode.getIndex() == node.getIndex()) { Integer minReplication = storageNode.getMinCurrentReplicationFactorOrNull(); @@ -526,9 +527,8 @@ public class NodeStateChangeChecker { + ") as fleetcontroller (" + clusterStateVersion + ")"); } - List<StorageNode> storageNodes = distributorNodeInfo.getHostInfo().getDistributor().getStorageNodes(); - Result storageNodesResult = checkStorageNodesForDistributor(distributorNodeInfo, storageNodes, node); - if (!storageNodesResult.settingWantedStateIsAllowed()) { + Result storageNodesResult = checkStorageNodesForDistributor(distributorNodeInfo, node); + if (storageNodesResult.settingWantedStateIsNotAllowed()) { return storageNodesResult; } } 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 c4fd7cb69b9..43687d51937 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 @@ -13,7 +13,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import java.util.ArrayList; -import java.util.Collection; import java.util.List; import static com.yahoo.vdslib.state.NodeType.DISTRIBUTOR; @@ -168,12 +167,9 @@ public class NodeStateChangeCheckerTest { @Test void testMaintenanceAllowedFor2Of4Groups() { - // 4 groups with 1 node in each group - Collection<ConfiguredNode> nodes = createNodes(4); - StorDistributionConfig config = createDistributionConfig(4, 4); - int maxNumberOfGroupsAllowedToBeDown = 2; - var cluster = new ContentCluster("Clustername", nodes, new Distribution(config), maxNumberOfGroupsAllowedToBeDown); + // 4 groups with 1 node in each group + var cluster = createCluster(4, 4, maxNumberOfGroupsAllowedToBeDown); setAllNodesUp(cluster, HostInfo.createHostInfo(createDistributorHostInfo(4, 5, 6))); var nodeStateChangeChecker = createChangeChecker(cluster); @@ -219,12 +215,9 @@ public class NodeStateChangeCheckerTest { @Test void testMaintenanceAllowedFor2Of4Groups8Nodes() { - // 4 groups with 2 nodes in each group - Collection<ConfiguredNode> nodes = createNodes(8); - StorDistributionConfig config = createDistributionConfig(8, 4); - int maxNumberOfGroupsAllowedToBeDown = 2; - var cluster = new ContentCluster("Clustername", nodes, new Distribution(config), maxNumberOfGroupsAllowedToBeDown); + // 4 groups with 2 nodes in each group + var cluster = createCluster(8, 4, maxNumberOfGroupsAllowedToBeDown); setAllNodesUp(cluster, HostInfo.createHostInfo(createDistributorHostInfo(4, 5, 6))); var nodeStateChangeChecker = createChangeChecker(cluster); @@ -271,6 +264,34 @@ public class NodeStateChangeCheckerTest { setStorageNodeWantedStateToMaintenance(cluster, nodeIndex); } + // 2 nodes in group 0 and 2 nodes in group 1 in maintenance, try to set storage node 4 in group 2 to maintenance, should fail + { + ClusterState clusterState = clusterState(String.format("version:%d distributor:8 storage:8 .0.s:m .1.s:m .2.s:m .3.s:m", currentClusterStateVersion)); + int nodeIndex = 4; + Node node = new Node(STORAGE, nodeIndex); + Result result = nodeStateChangeChecker.evaluateTransition(node, clusterState, SAFE, UP_NODE_STATE, MAINTENANCE_NODE_STATE); + assertFalse(result.settingWantedStateIsAllowed(), result.toString()); + assertFalse(result.wantedStateAlreadySet()); + assertEquals("At most 2 groups can have wanted state: [0, 1]", result.getReason()); + } + + // 2 nodes in group 0 up again but buckets not in sync and 2 nodes in group 1 in maintenance, + // try to set storage node 4 in group 2 to maintenance + /* WIP + { + ClusterState clusterState = clusterState(String.format("version:%d distributor:8 storage:8 .2.s:m .3.s:m", currentClusterStateVersion)); + setStorageNodeWantedState(cluster, 0, UP, ""); + setStorageNodeWantedState(cluster, 1, UP, ""); + int nodeIndex = 4; + Node node = new Node(STORAGE, nodeIndex); + Result result = nodeStateChangeChecker.evaluateTransition(node, clusterState, SAFE, UP_NODE_STATE, MAINTENANCE_NODE_STATE); + assertFalse(result.settingWantedStateIsAllowed(), result.toString()); + assertFalse(result.wantedStateAlreadySet()); + assertEquals("At most 2 groups can have wanted state: [0, 1]", result.getReason()); + } + + */ + // 2 nodes in group 0 in maintenance, storage node 3 in group 1 is in maintenance with another description // (set in maintenance by operator), try to set storage node 3 in group 1 to maintenance, should bew allowed { diff --git a/vdslib/src/main/java/com/yahoo/vdslib/state/NodeState.java b/vdslib/src/main/java/com/yahoo/vdslib/state/NodeState.java index 4bb27e8599e..c69b4f79aa0 100644 --- a/vdslib/src/main/java/com/yahoo/vdslib/state/NodeState.java +++ b/vdslib/src/main/java/com/yahoo/vdslib/state/NodeState.java @@ -41,8 +41,8 @@ public class NodeState implements Cloneable { } /** - * A state can not be forced to be in a state above it's reported state. - * For instance, a down being down, cannot be forced up, but a node being down can be forced in maintenance. + * A state can not be forced to be in a state above its reported state. + * For instance, a node being down cannot be forced to up, but a node being down can be forced to maintenance. */ public boolean above(NodeState other) { return (state.ordinal() > other.state.ordinal()); |