diff options
author | Harald Musum <musum@yahooinc.com> | 2023-03-27 09:43:11 +0200 |
---|---|---|
committer | Harald Musum <musum@yahooinc.com> | 2023-03-27 09:43:11 +0200 |
commit | fd9968385a5629be43d9521b3d17a6165a7d2300 (patch) | |
tree | 301f74d082f33f5b2a09e8603d1a52e70c9c0c62 | |
parent | b4deea460ed60e09df90549e29f512f8fed74828 (diff) |
Simplify
-rw-r--r-- | clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java | 171 |
1 files changed, 86 insertions, 85 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 3ed03e94fda..9d8141020c3 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 @@ -8,18 +8,26 @@ import com.yahoo.vdslib.distribution.Group; import com.yahoo.vdslib.state.ClusterState; import com.yahoo.vdslib.state.Node; import com.yahoo.vdslib.state.NodeState; -import com.yahoo.vdslib.state.NodeType; import com.yahoo.vdslib.state.State; import com.yahoo.vespa.clustercontroller.core.hostinfo.HostInfo; import com.yahoo.vespa.clustercontroller.core.hostinfo.Metrics; import com.yahoo.vespa.clustercontroller.core.hostinfo.StorageNode; import com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.SetUnitStateRequest; - import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; +import static com.yahoo.vdslib.state.NodeType.STORAGE; +import static com.yahoo.vdslib.state.State.DOWN; +import static com.yahoo.vdslib.state.State.RETIRED; +import static com.yahoo.vdslib.state.State.UP; +import static com.yahoo.vespa.clustercontroller.core.NodeStateChangeChecker.Result.allowSettingOfWantedState; +import static com.yahoo.vespa.clustercontroller.core.NodeStateChangeChecker.Result.createAlreadySet; +import static com.yahoo.vespa.clustercontroller.core.NodeStateChangeChecker.Result.createDisallowed; +import static com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.SetUnitStateRequest.Condition.FORCE; +import static com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.SetUnitStateRequest.Condition.SAFE; + /** * Checks if a node can be upgraded. * @@ -91,29 +99,28 @@ public class NodeStateChangeChecker { } } - public Result evaluateTransition( - Node node, ClusterState clusterState, SetUnitStateRequest.Condition condition, - NodeState oldWantedState, NodeState newWantedState) { - if (condition == SetUnitStateRequest.Condition.FORCE) { - return Result.allowSettingOfWantedState(); + public Result evaluateTransition(Node node, ClusterState clusterState, SetUnitStateRequest.Condition condition, + NodeState oldWantedState, NodeState newWantedState) { + if (condition == FORCE) { + return allowSettingOfWantedState(); } if (inMoratorium) { - return Result.createDisallowed("Master cluster controller is bootstrapping and in moratorium"); + return createDisallowed("Master cluster controller is bootstrapping and in moratorium"); } - if (condition != SetUnitStateRequest.Condition.SAFE) { - return Result.createDisallowed("Condition not implemented: " + condition.name()); + if (condition != SAFE) { + return createDisallowed("Condition not implemented: " + condition.name()); } - if (node.getType() != NodeType.STORAGE) { - return Result.createDisallowed("Safe-set of node state is only supported for storage nodes! " + + if (node.getType() != STORAGE) { + return createDisallowed("Safe-set of node state is only supported for storage nodes! " + "Requested node type: " + node.getType().toString()); } StorageNodeInfo nodeInfo = clusterInfo.getStorageNodeInfo(node.getIndex()); if (nodeInfo == null) { - return Result.createDisallowed("Unknown node " + node); + return createDisallowed("Unknown node " + node); } // If the new state and description equals the existing, we're done. This is done for 2 cases: @@ -123,41 +130,37 @@ public class NodeStateChangeChecker { // MUST_SET_WANTED_STATE if re-evaluated. This is important for implementing idempotent clients. if (newWantedState.getState().equals(oldWantedState.getState()) && Objects.equals(newWantedState.getDescription(), oldWantedState.getDescription())) { - return Result.createAlreadySet(); + return createAlreadySet(); } - switch (newWantedState.getState()) { - case UP: - return canSetStateUp(nodeInfo, oldWantedState); - case MAINTENANCE: - return canSetStateMaintenanceTemporarily(nodeInfo, clusterState, newWantedState.getDescription()); - case DOWN: - return canSetStateDownPermanently(nodeInfo, clusterState, newWantedState.getDescription()); - default: - return Result.createDisallowed("Destination node state unsupported in safe mode: " + newWantedState); - } + return switch (newWantedState.getState()) { + case UP -> canSetStateUp(nodeInfo, oldWantedState); + case MAINTENANCE -> canSetStateMaintenanceTemporarily(nodeInfo, clusterState, newWantedState.getDescription()); + case DOWN -> canSetStateDownPermanently(nodeInfo, clusterState, newWantedState.getDescription()); + default -> createDisallowed("Destination node state unsupported in safe mode: " + newWantedState); + }; } private Result canSetStateDownPermanently(NodeInfo nodeInfo, ClusterState clusterState, String newDescription) { NodeState oldWantedState = nodeInfo.getUserWantedState(); - if (oldWantedState.getState() != State.UP && !oldWantedState.getDescription().equals(newDescription)) { + if (oldWantedState.getState() != UP && !oldWantedState.getDescription().equals(newDescription)) { // Refuse to override whatever an operator or unknown entity is doing. // // Note: The new state&description is NOT equal to the old state&description: // that would have been short-circuited prior to this. - return Result.createDisallowed("A conflicting wanted state is already set: " + + return createDisallowed("A conflicting wanted state is already set: " + oldWantedState.getState() + ": " + oldWantedState.getDescription()); } State reportedState = nodeInfo.getReportedState().getState(); - if (reportedState != State.UP) { - return Result.createDisallowed("Reported state (" + reportedState + if (reportedState != UP) { + return createDisallowed("Reported state (" + reportedState + ") is not UP, so no bucket data is available"); } State currentState = clusterState.getNodeState(nodeInfo.getNode()).getState(); - if (currentState != State.RETIRED) { - return Result.createDisallowed("Only retired nodes are allowed to be set to DOWN in safe mode - is " + if (currentState != RETIRED) { + return createDisallowed("Only retired nodes are allowed to be set to DOWN in safe mode - is " + currentState); } @@ -165,7 +168,7 @@ public class NodeStateChangeChecker { Integer hostInfoNodeVersion = hostInfo.getClusterStateVersionOrNull(); int clusterControllerVersion = clusterState.getVersion(); if (hostInfoNodeVersion == null || hostInfoNodeVersion != clusterControllerVersion) { - return Result.createDisallowed("Cluster controller at version " + clusterControllerVersion + return createDisallowed("Cluster controller at version " + clusterControllerVersion + " got info for storage node " + nodeInfo.getNodeIndex() + " at a different version " + hostInfoNodeVersion); } @@ -173,43 +176,43 @@ public class NodeStateChangeChecker { Optional<Metrics.Value> bucketsMetric; bucketsMetric = hostInfo.getMetrics().getValueAt(BUCKETS_METRIC_NAME, BUCKETS_METRIC_DIMENSIONS); if (bucketsMetric.isEmpty() || bucketsMetric.get().getLast() == null) { - return Result.createDisallowed("Missing last value of the " + BUCKETS_METRIC_NAME + + return createDisallowed("Missing last value of the " + BUCKETS_METRIC_NAME + " metric for storage node " + nodeInfo.getNodeIndex()); } long lastBuckets = bucketsMetric.get().getLast(); if (lastBuckets > 0) { - return Result.createDisallowed("The storage node manages " + lastBuckets + " buckets"); + return createDisallowed("The storage node manages " + lastBuckets + " buckets"); } - return Result.allowSettingOfWantedState(); + return allowSettingOfWantedState(); } private Result canSetStateUp(NodeInfo nodeInfo, NodeState oldWantedState) { - if (oldWantedState.getState() == State.UP) { + if (oldWantedState.getState() == UP) { // The description is not significant when setting wanting to set the state to UP - return Result.createAlreadySet(); + return createAlreadySet(); } - if (nodeInfo.getReportedState().getState() != State.UP) { - return Result.createDisallowed("Refuse to set wanted state to UP, " + + if (nodeInfo.getReportedState().getState() != UP) { + return createDisallowed("Refuse to set wanted state to UP, " + "since the reported state is not UP (" + nodeInfo.getReportedState().getState() + ")"); } - return Result.allowSettingOfWantedState(); + return allowSettingOfWantedState(); } private Result canSetStateMaintenanceTemporarily(StorageNodeInfo nodeInfo, ClusterState clusterState, String newDescription) { NodeState oldWantedState = nodeInfo.getUserWantedState(); - if (oldWantedState.getState() != State.UP && !oldWantedState.getDescription().equals(newDescription)) { + if (oldWantedState.getState() != UP && !oldWantedState.getDescription().equals(newDescription)) { // Refuse to override whatever an operator or unknown entity is doing. If the description is // identical, we assume it is the same operator. // // Note: The new state&description is NOT equal to the old state&description: // that would have been short-circuited prior to this. - return Result.createDisallowed("A conflicting wanted state is already set: " + + return createDisallowed("A conflicting wanted state is already set: " + oldWantedState.getState() + ": " + oldWantedState.getDescription()); } @@ -218,12 +221,12 @@ public class NodeStateChangeChecker { return otherGroupCheck; } - if (clusterState.getNodeState(nodeInfo.getNode()).getState() == State.DOWN) { - return Result.allowSettingOfWantedState(); + if (clusterState.getNodeState(nodeInfo.getNode()).getState() == DOWN) { + return allowSettingOfWantedState(); } if (anotherNodeInGroupAlreadyAllowed(nodeInfo, newDescription)) { - return Result.allowSettingOfWantedState(); + return allowSettingOfWantedState(); } Result allNodesAreUpCheck = checkAllNodesAreUp(clusterState); @@ -236,7 +239,7 @@ public class NodeStateChangeChecker { return checkDistributorsResult; } - return Result.allowSettingOfWantedState(); + return allowSettingOfWantedState(); } /** @@ -270,52 +273,50 @@ public class NodeStateChangeChecker { /** Returns a disallow-result, if there is a node in the group with wanted state != UP. */ private Result otherNodeInGroupHasWantedState(Group group) { for (var configuredNode : group.getNodes()) { - StorageNodeInfo storageNodeInfo = clusterInfo.getStorageNodeInfo(configuredNode.index()); + int index = configuredNode.index(); + StorageNodeInfo storageNodeInfo = clusterInfo.getStorageNodeInfo(index); if (storageNodeInfo == null) continue; // needed for tests only - State storageNodeWantedState = storageNodeInfo - .getUserWantedState().getState(); - if (storageNodeWantedState != State.UP) { - return Result.createDisallowed( - "At most one group can have wanted state: Other storage node " + configuredNode.index() + + State storageNodeWantedState = storageNodeInfo.getUserWantedState().getState(); + if (storageNodeWantedState != UP) { + return createDisallowed( + "At most one group can have wanted state: Other storage node " + index + " in group " + group.getIndex() + " has wanted state " + storageNodeWantedState); } - State distributorWantedState = clusterInfo.getDistributorNodeInfo(configuredNode.index()) - .getUserWantedState().getState(); - if (distributorWantedState != State.UP) { - return Result.createDisallowed( - "At most one group can have wanted state: Other distributor " + configuredNode.index() + + State distributorWantedState = clusterInfo.getDistributorNodeInfo(index).getUserWantedState().getState(); + if (distributorWantedState != UP) { + return createDisallowed( + "At most one group can have wanted state: Other distributor " + index + " in group " + group.getIndex() + " has wanted state " + distributorWantedState); } } - return Result.allowSettingOfWantedState(); + return allowSettingOfWantedState(); } private Result otherNodeHasWantedState(StorageNodeInfo nodeInfo) { for (var configuredNode : clusterInfo.getConfiguredNodes().values()) { - if (configuredNode.index() == nodeInfo.getNodeIndex()) { + int index = configuredNode.index(); + if (index == nodeInfo.getNodeIndex()) { continue; } - State storageNodeWantedState = clusterInfo.getStorageNodeInfo(configuredNode.index()) - .getUserWantedState().getState(); - if (storageNodeWantedState != State.UP) { - return Result.createDisallowed( + State storageNodeWantedState = clusterInfo.getStorageNodeInfo(index).getUserWantedState().getState(); + if (storageNodeWantedState != UP) { + return createDisallowed( "At most one node can have a wanted state when #groups = 1: Other storage node " + - configuredNode.index() + " has wanted state " + storageNodeWantedState); + index + " has wanted state " + storageNodeWantedState); } - State distributorWantedState = clusterInfo.getDistributorNodeInfo(configuredNode.index()) - .getUserWantedState().getState(); - if (distributorWantedState != State.UP) { - return Result.createDisallowed( + State distributorWantedState = clusterInfo.getDistributorNodeInfo(index).getUserWantedState().getState(); + if (distributorWantedState != UP) { + return createDisallowed( "At most one node can have a wanted state when #groups = 1: Other distributor " + - configuredNode.index() + " has wanted state " + distributorWantedState); + index + " has wanted state " + distributorWantedState); } } - return Result.allowSettingOfWantedState(); + return allowSettingOfWantedState(); } private boolean anotherNodeInGroupAlreadyAllowed(StorageNodeInfo nodeInfo, String newDescription) { @@ -361,33 +362,33 @@ public class NodeStateChangeChecker { for (NodeInfo storageNodeInfo : clusterInfo.getStorageNodeInfos()) { State wantedState = storageNodeInfo.getUserWantedState().getState(); - if (wantedState != State.UP && wantedState != State.RETIRED) { - return Result.createDisallowed("Another storage node wants state " + + if (wantedState != UP && wantedState != RETIRED) { + return createDisallowed("Another storage node wants state " + wantedState.toString().toUpperCase() + ": " + storageNodeInfo.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() + + if (state != UP && state != RETIRED) { + return createDisallowed("Another storage node has state " + state.toString().toUpperCase() + ": " + storageNodeInfo.getNodeIndex()); } } for (NodeInfo distributorNodeInfo : clusterInfo.getDistributorNodeInfos()) { State wantedState = distributorNodeInfo.getUserWantedState().getState(); - if (wantedState != State.UP && wantedState != State.RETIRED) { - return Result.createDisallowed("Another distributor wants state " + wantedState.toString().toUpperCase() + + if (wantedState != UP && wantedState != RETIRED) { + return createDisallowed("Another distributor wants state " + wantedState.toString().toUpperCase() + ": " + distributorNodeInfo.getNodeIndex()); } State state = clusterState.getNodeState(distributorNodeInfo.getNode()).getState(); - if (state != State.UP && state != State.RETIRED) { - return Result.createDisallowed("Another distributor has state " + state.toString().toUpperCase() + + if (state != UP && state != RETIRED) { + return createDisallowed("Another distributor has state " + state.toString().toUpperCase() + ": " + distributorNodeInfo.getNodeIndex()); } } - return Result.allowSettingOfWantedState(); + return allowSettingOfWantedState(); } private Result checkStorageNodesForDistributor( @@ -397,19 +398,19 @@ public class NodeStateChangeChecker { Integer minReplication = storageNode.getMinCurrentReplicationFactorOrNull(); // Why test on != null? Missing min-replication is OK (indicate empty/few buckets on system). if (minReplication != null && minReplication < requiredRedundancy) { - return Result.createDisallowed("Distributor " + return createDisallowed("Distributor " + distributorNodeInfo.getNodeIndex() + " says storage node " + node.getIndex() + " has buckets with redundancy as low as " + storageNode.getMinCurrentReplicationFactorOrNull() + ", but we require at least " + requiredRedundancy); } else { - return Result.allowSettingOfWantedState(); + return allowSettingOfWantedState(); } } } - return Result.allowSettingOfWantedState(); + return allowSettingOfWantedState(); } /** @@ -419,15 +420,15 @@ public class NodeStateChangeChecker { */ private Result checkDistributors(Node node, int clusterStateVersion) { if (clusterInfo.getDistributorNodeInfos().isEmpty()) { - return Result.createDisallowed("Not aware of any distributors, probably not safe to upgrade?"); + return createDisallowed("Not aware of any distributors, probably not safe to upgrade?"); } for (DistributorNodeInfo distributorNodeInfo : clusterInfo.getDistributorNodeInfos()) { Integer distributorClusterStateVersion = distributorNodeInfo.getHostInfo().getClusterStateVersionOrNull(); if (distributorClusterStateVersion == null) { - return Result.createDisallowed("Distributor node " + distributorNodeInfo.getNodeIndex() + return createDisallowed("Distributor node " + distributorNodeInfo.getNodeIndex() + " has not reported any cluster state version yet."); } else if (distributorClusterStateVersion != clusterStateVersion) { - return Result.createDisallowed("Distributor node " + distributorNodeInfo.getNodeIndex() + return createDisallowed("Distributor node " + distributorNodeInfo.getNodeIndex() + " does not report same version (" + distributorNodeInfo.getHostInfo().getClusterStateVersionOrNull() + ") as fleetcontroller (" + clusterStateVersion + ")"); @@ -440,7 +441,7 @@ public class NodeStateChangeChecker { } } - return Result.allowSettingOfWantedState(); + return allowSettingOfWantedState(); } } |