diff options
author | Håkon Hallingstad <hakon@verizonmedia.com> | 2021-02-19 02:17:58 +0100 |
---|---|---|
committer | Håkon Hallingstad <hakon@verizonmedia.com> | 2021-02-19 02:17:58 +0100 |
commit | 6775196e500e6685e35222e45b8fb04a3fedb85f (patch) | |
tree | 4deaf4f776101c308a387f95829faea572d7913e /clustercontroller-core | |
parent | 0be286e9026e96f8a1b032a2f2a08e943cf771ec (diff) |
Fail safe maintenance if other nodes are not up
Diffstat (limited to 'clustercontroller-core')
4 files changed, 52 insertions, 88 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 01ec8d65c28..1b78833480c 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 @@ -203,8 +203,6 @@ public class ContentCluster { NodeState oldState, NodeState newState) { NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker( - minStorageNodesUp, - minRatioOfStorageNodesUp, distribution.getRedundancy(), new HierarchicalGroupVisitingAdapter(distribution), clusterInfo 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 acddfa478b1..f3e7941fcd2 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 @@ -29,20 +29,14 @@ public class NodeStateChangeChecker { public static final String BUCKETS_METRIC_NAME = "vds.datastored.bucket_space.buckets_total"; public static final Map<String, String> BUCKETS_METRIC_DIMENSIONS = Map.of("bucketSpace", "default"); - private final int minStorageNodesUp; - private final double minRatioOfStorageNodesUp; private final int requiredRedundancy; private final HierarchicalGroupVisiting groupVisiting; private final ClusterInfo clusterInfo; public NodeStateChangeChecker( - int minStorageNodesUp, - double minRatioOfStorageNodesUp, int requiredRedundancy, HierarchicalGroupVisiting groupVisiting, ClusterInfo clusterInfo) { - this.minStorageNodesUp = minStorageNodesUp; - this.minRatioOfStorageNodesUp = minRatioOfStorageNodesUp; this.requiredRedundancy = requiredRedundancy; this.groupVisiting = groupVisiting; this.clusterInfo = clusterInfo; @@ -159,9 +153,9 @@ public class NodeStateChangeChecker { + currentState); } - Result thresholdCheckResult = checkUpThresholds(clusterState); - if (!thresholdCheckResult.settingWantedStateIsAllowed()) { - return thresholdCheckResult; + Result allNodesAreUpCheck = checkAllNodesAreUp(clusterState); + if (!allNodesAreUpCheck.settingWantedStateIsAllowed()) { + return allNodesAreUpCheck; } HostInfo hostInfo = nodeInfo.getHostInfo(); @@ -226,9 +220,9 @@ public class NodeStateChangeChecker { return Result.allowSettingOfWantedState(); } - Result ongoingChanges = anyNodeSetToMaintenance(clusterState); - if (!ongoingChanges.settingWantedStateIsAllowed()) { - return ongoingChanges; + Result allNodesAreUpCheck = checkAllNodesAreUp(clusterState); + if (!allNodesAreUpCheck.settingWantedStateIsAllowed()) { + return allNodesAreUpCheck; } Result checkDistributorsResult = checkDistributors(nodeInfo.getNode(), clusterState.getVersion()); @@ -236,11 +230,6 @@ public class NodeStateChangeChecker { return checkDistributorsResult; } - Result thresholdCheckResult = checkUpThresholds(clusterState); - if (!thresholdCheckResult.settingWantedStateIsAllowed()) { - return thresholdCheckResult; - } - return Result.allowSettingOfWantedState(); } @@ -281,48 +270,38 @@ public class NodeStateChangeChecker { return false; } - private Result anyNodeSetToMaintenance(ClusterState clusterState) { - for (NodeInfo nodeInfo : clusterInfo.getAllNodeInfo()) { - if (clusterState.getNodeState(nodeInfo.getNode()).getState() == State.MAINTENANCE) { - return Result.createDisallowed("Another node is already in maintenance:" + nodeInfo.getNodeIndex()); + private Result checkAllNodesAreUp(ClusterState clusterState) { + // This method verifies both storage nodes and distributors are up (or retired). + // The complicated part is making a summary error message. + + for (NodeInfo storageNodeInfo : clusterInfo.getStorageNodeInfo()) { + State wantedState = storageNodeInfo.getUserWantedState().getState(); + if (wantedState != State.UP && wantedState != State.RETIRED) { + return Result.createDisallowed("Another storage node wants state " + + wantedState.toString().toUpperCase() + ": " + storageNodeInfo.getNodeIndex()); } - if (nodeInfo.getWantedState().getState() == State.MAINTENANCE) { - return Result.createDisallowed("Another node wants maintenance:" + nodeInfo.getNodeIndex()); + + State state = clusterState.getNodeState(storageNodeInfo.getNode()).getState(); + if (state != State.UP && state != State.RETIRED) { + return Result.createDisallowed("Another storage node has state " + state.toString().toUpperCase() + + ": " + storageNodeInfo.getNodeIndex()); } } - return Result.allowSettingOfWantedState(); - } - private int contentNodesWithAvailableNodeState(ClusterState clusterState) { - final int nodeCount = clusterState.getNodeCount(NodeType.STORAGE); - int upNodesCount = 0; - for (int i = 0; i < nodeCount; ++i) { - final Node node = new Node(NodeType.STORAGE, i); - final State state = clusterState.getNodeState(node).getState(); - if (state == State.UP || state == State.RETIRED || state == State.INITIALIZING) { - upNodesCount++; + for (NodeInfo distributorNodeInfo : clusterInfo.getDistributorNodeInfo()) { + State wantedState = distributorNodeInfo.getUserWantedState().getState(); + if (wantedState != State.UP && wantedState != State.RETIRED) { + return Result.createDisallowed("Another distributor wants state " + wantedState.toString().toUpperCase() + + ": " + distributorNodeInfo.getNodeIndex()); } - } - return upNodesCount; - } - private Result checkUpThresholds(ClusterState clusterState) { - if (clusterInfo.getStorageNodeInfo().size() < minStorageNodesUp) { - return Result.createDisallowed("There are only " + clusterInfo.getStorageNodeInfo().size() + - " storage nodes up, while config requires at least " + minStorageNodesUp); + State state = clusterState.getNodeState(distributorNodeInfo.getNode()).getState(); + if (state != State.UP && state != State.RETIRED) { + return Result.createDisallowed("Another distributor has state " + state.toString().toUpperCase() + + ": " + distributorNodeInfo.getNodeIndex()); + } } - final int nodesCount = clusterInfo.getStorageNodeInfo().size(); - final int upNodesCount = contentNodesWithAvailableNodeState(clusterState); - - if (nodesCount == 0) { - return Result.createDisallowed("No storage nodes in cluster state"); - } - if (((double)upNodesCount) / nodesCount < minRatioOfStorageNodesUp) { - return Result.createDisallowed("Not enough storage nodes running: " + upNodesCount - + " of " + nodesCount + " storage nodes are up which is less that the required fraction of " - + minRatioOfStorageNodesUp); - } return Result.allowSettingOfWantedState(); } 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 7ed4b4b3226..34e52eb82c4 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 @@ -16,9 +16,7 @@ import org.junit.Test; import java.text.ParseException; import java.util.ArrayList; import java.util.Collection; -import java.util.HashSet; import java.util.List; -import java.util.Set; import static org.hamcrest.core.Is.is; import static org.hamcrest.core.StringContains.containsString; @@ -32,10 +30,8 @@ import static org.mockito.Mockito.when; public class NodeStateChangeCheckerTest { - private static final int minStorageNodesUp = 3; private static final int requiredRedundancy = 4; private static final int currentClusterStateVersion = 2; - private static final double minRatioOfStorageNodesUp = 0.9; private static final Node nodeDistributor = new Node(NodeType.DISTRIBUTOR, 1); private static final Node nodeStorage = new Node(NodeType.STORAGE, 1); @@ -61,30 +57,14 @@ public class NodeStateChangeCheckerTest { } private NodeStateChangeChecker createChangeChecker(ContentCluster cluster) { - return new NodeStateChangeChecker(minStorageNodesUp, minRatioOfStorageNodesUp, requiredRedundancy, - visitor -> {}, cluster.clusterInfo()); + return new NodeStateChangeChecker(requiredRedundancy, visitor -> {}, cluster.clusterInfo()); } private ContentCluster createCluster(Collection<ConfiguredNode> nodes) { Distribution distribution = mock(Distribution.class); Group group = new Group(2, "to"); when(distribution.getRootGroup()).thenReturn(group); - return new ContentCluster("Clustername", nodes, distribution, minStorageNodesUp, 0.0); - } - - private StorageNodeInfo createStorageNodeInfo(int index, State state) { - Distribution distribution = mock(Distribution.class); - Group group = new Group(2, "to"); - when(distribution.getRootGroup()).thenReturn(group); - - String clusterName = "Clustername"; - Set<ConfiguredNode> configuredNodeIndexes = new HashSet<>(); - ContentCluster cluster = new ContentCluster(clusterName, configuredNodeIndexes, distribution, minStorageNodesUp, 0.0); - - String rpcAddress = ""; - StorageNodeInfo storageNodeInfo = new StorageNodeInfo(cluster, index, false, rpcAddress, distribution); - storageNodeInfo.setReportedState(new NodeState(NodeType.STORAGE, state), 3 /* time */); - return storageNodeInfo; + return new ContentCluster("Clustername", nodes, distribution, 3, 0.0); } private String createDistributorHostInfo(int replicationfactor1, int replicationfactor2, int replicationfactor3) { @@ -137,8 +117,7 @@ public class NodeStateChangeCheckerTest { public void testUnknownStorageNode() { ContentCluster cluster = createCluster(createNodes(4)); NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker( - 5 /* min storage nodes */, minRatioOfStorageNodesUp, requiredRedundancy, - visitor -> {}, cluster.clusterInfo()); + requiredRedundancy, visitor -> {}, cluster.clusterInfo()); NodeStateChangeChecker.Result result = nodeStateChangeChecker.evaluateTransition( new Node(NodeType.STORAGE, 10), defaultAllUpClusterState(), SetUnitStateRequest.Condition.SAFE, UP_NODE_STATE, MAINTENANCE_NODE_STATE); @@ -160,17 +139,23 @@ public class NodeStateChangeCheckerTest { @Test public void testCanUpgradeSafeMissingStorage() { + // Create a content cluster with 4 nodes, and storage node with index 3 down. ContentCluster cluster = createCluster(createNodes(4)); setAllNodesUp(cluster, HostInfo.createHostInfo(createDistributorHostInfo(4, 5, 6))); + cluster.clusterInfo().getStorageNodeInfo(3).setReportedState(new NodeState(NodeType.STORAGE, State.DOWN), 0); + ClusterState clusterStateWith3Down = clusterState(String.format( + "version:%d distributor:4 storage:4 .3.s:d", + currentClusterStateVersion)); + + // We should then be denied setting storage node 1 safely to maintenance. NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker( - 5 /* min storage nodes */, minRatioOfStorageNodesUp, requiredRedundancy, visitor -> {}, - cluster.clusterInfo()); + requiredRedundancy, visitor -> {}, cluster.clusterInfo()); NodeStateChangeChecker.Result result = nodeStateChangeChecker.evaluateTransition( - nodeStorage, defaultAllUpClusterState(), SetUnitStateRequest.Condition.SAFE, + nodeStorage, clusterStateWith3Down, SetUnitStateRequest.Condition.SAFE, UP_NODE_STATE, MAINTENANCE_NODE_STATE); assertFalse(result.settingWantedStateIsAllowed()); assertFalse(result.wantedStateAlreadySet()); - assertThat(result.getReason(), is("There are only 4 storage nodes up, while config requires at least 5")); + assertThat(result.getReason(), is("Another storage node has state DOWN: 3")); } @Test @@ -402,7 +387,7 @@ public class NodeStateChangeCheckerTest { NodeStateChangeChecker.Result result = transitionToMaintenanceWithOneStorageNodeDown(otherIndex); assertFalse(result.settingWantedStateIsAllowed()); assertFalse(result.wantedStateAlreadySet()); - assertThat(result.getReason(), containsString("Not enough storage nodes running")); + assertThat(result.getReason(), containsString("Another storage node has state DOWN: 2")); } @Test diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java index 8a9cfb4be98..5bb7897b8e7 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java @@ -206,7 +206,7 @@ public class SetNodeStateTest extends StateRestApiTest { @Test public void testShouldModifyStorageSafeOk() throws Exception { setUp(false); - SetResponse setResponse = restAPI.setUnitState(new SetUnitStateRequestImpl("music/storage/1") + SetResponse setResponse = restAPI.setUnitState(new SetUnitStateRequestImpl("music/storage/2") .setNewState("user", "maintenance", "whatever reason.") .setCondition(SetUnitStateRequest.Condition.SAFE)); assertThat(setResponse.getReason(), is("ok")); @@ -265,20 +265,22 @@ public class SetNodeStateTest extends StateRestApiTest { assertSetUnitState(1, State.MAINTENANCE, null); // sanity-check // Because 2 is in a different group maintenance should be denied - assertSetUnitStateCausesAlreadyInMaintenance(2, State.MAINTENANCE); + assertSetUnitStateCausesAlreadyInWantedMaintenance(2, State.MAINTENANCE); // Because 3 and 5 are in the same group as 1, these should be OK assertSetUnitState(3, State.MAINTENANCE, null); assertUnitState(1, "user", State.MAINTENANCE, "whatever reason."); // sanity-check assertUnitState(3, "user", State.MAINTENANCE, "whatever reason."); // sanity-check assertSetUnitState(5, State.MAINTENANCE, null); - assertSetUnitStateCausesAlreadyInMaintenance(2, State.MAINTENANCE); // sanity-check + assertSetUnitStateCausesAlreadyInWantedMaintenance(2, State.MAINTENANCE); // sanity-check // Set all to up assertSetUnitState(1, State.UP, null); assertSetUnitState(1, State.UP, null); // sanity-check assertSetUnitState(3, State.UP, null); - assertSetUnitStateCausesAlreadyInMaintenance(2, State.MAINTENANCE); // sanity-check + // Because 1 is in maintenance, even though user wanted state is UP, trying to set 2 to + // maintenance will fail. + assertSetUnitStateCausesAlreadyInMaintenance(2, State.MAINTENANCE); assertSetUnitState(5, State.UP, null); } @@ -306,11 +308,11 @@ public class SetNodeStateTest extends StateRestApiTest { } private void assertSetUnitStateCausesAlreadyInWantedMaintenance(int index, State state) throws StateRestApiException { - assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another node wants maintenance:([0-9]+)$"); + assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another storage node wants state MAINTENANCE: ([0-9]+)$"); } private void assertSetUnitStateCausesAlreadyInMaintenance(int index, State state) throws StateRestApiException { - assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another node is already in maintenance:([0-9]+)$"); + assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another storage node has state MAINTENANCE: ([0-9]+)$"); } private void assertSetUnitStateCausesAlreadyInMaintenance(int index, State state, String reasonRegex) |