Merge cockroachdb#27808

27808: cli: fix alignment in `node status` output r=benesch a=neeral See cockroachdb#27807 `cockroach node status` will hide decommissioning or dead nodes, unless the user requests all nodes to be shown. The data shown is collected from two different RPC calls and these do not line up. For example, a node's liveness will be put together with a different node's decommission status. To view this, decommission the node with NodeID 1 in a cluster. Run `cockroach node status --decommission` and `cockroach node status`. Look at the columns IsLive and UpdatedAt. You'll see they go out of sync. Release note: None Co-authored-by: neeral <[email protected]>
nvanbenschoten · Aug 3, 2018 · 41b1a63 · 41b1a63
2 parents 72b4e71 + d28ee73
commit 41b1a63
Show file tree

Hide file tree

Showing 3 changed files with 141 additions and 17 deletions.
diff --git a/pkg/acceptance/decommission_test.go b/pkg/acceptance/decommission_test.go
@@ -331,7 +331,7 @@ func testDecommissionInner(
 
 		exp := [][]string{
 			decommissionHeader,
-			{strconv.Itoa(int(target)), "true", "0", "true", "true"},
+			{strconv.Itoa(int(target)), "true", "0", "true", "true|false"},
 			decommissionFooter,
 		}
 		if err := matchCSV(o, exp); err != nil {

diff --git a/pkg/cli/node.go b/pkg/cli/node.go
@@ -194,26 +194,44 @@ func runStatusNodeInner(
 	}
 
 	if !showDecommissioned {
-		for _, status := range decommissionStatusResp.Status {
-			if !status.Decommissioning || status.IsLive {
-				// Show this entry.
-				continue
-			}
-			for i := 0; i < len(nodeStatuses); i++ {
-				if nodeStatuses[i].Desc.NodeID == status.NodeID {
-					// Hide this entry (by swapping it out with the last one).
-					last := len(nodeStatuses) - 1
-					nodeStatuses[i] = nodeStatuses[last]
-					nodeStatuses = nodeStatuses[:last]
+		nodeStatuses = hideDecommissioned(nodeStatuses, decommissionStatusResp)
+	}
+	return nodeStatuses, decommissionStatusResp, nil
+}
+
+func hideDecommissioned(
+	nodeStatuses []status.NodeStatus, decommissionStatusResp *serverpb.DecommissionStatusResponse,
+) []status.NodeStatus {
+	var decommissionStatuses []serverpb.DecommissionStatusResponse_Status
+	for _, status := range decommissionStatusResp.Status {
+		if !status.Decommissioning || status.IsLive {
+			// Show this entry.
+			for _, ns := range nodeStatuses {
+				if ns.Desc.NodeID == status.NodeID {
+					decommissionStatuses = append(decommissionStatuses, status)
+					break
 				}
 			}
+			continue
+		}
+		for i := 0; i < len(nodeStatuses); i++ {
+			if nodeStatuses[i].Desc.NodeID == status.NodeID {
+				// Hide this entry (by swapping it out with the last one).
+				last := len(nodeStatuses) - 1
+				nodeStatuses[i] = nodeStatuses[last]
+				nodeStatuses = nodeStatuses[:last]
+			}
 		}
-		// Sort the surviving entries (again) by NodeID.
-		sort.Slice(nodeStatuses, func(i, j int) bool {
-			return nodeStatuses[i].Desc.NodeID < nodeStatuses[j].Desc.NodeID
-		})
 	}
-	return nodeStatuses, decommissionStatusResp, nil
+	// Sort the surviving entries (again) by NodeID.
+	sort.Slice(nodeStatuses, func(i, j int) bool {
+		return nodeStatuses[i].Desc.NodeID < nodeStatuses[j].Desc.NodeID
+	})
+	sort.Slice(decommissionStatuses, func(i, j int) bool {
+		return decommissionStatuses[i].NodeID < decommissionStatuses[j].NodeID
+	})
+	decommissionStatusResp.Status = decommissionStatuses
+	return nodeStatuses
 }
 
 func getStatusNodeHeaders() []string {

diff --git a/pkg/cli/node_test.go b/pkg/cli/node_test.go
@@ -0,0 +1,106 @@
+// Copyright 2018 The Cockroach Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package cli
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
+	"github.com/cockroachdb/cockroach/pkg/server/status"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+)
+
+func makeNodeStatus(nodeID roachpb.NodeID) status.NodeStatus {
+	return status.NodeStatus{
+		Desc: roachpb.NodeDescriptor{NodeID: nodeID},
+	}
+}
+
+func makeDecommissionStatus(
+	nodeID roachpb.NodeID, isLive bool, decommissioning bool,
+) serverpb.DecommissionStatusResponse_Status {
+	return serverpb.DecommissionStatusResponse_Status{
+		NodeID:          nodeID,
+		IsLive:          isLive,
+		Decommissioning: decommissioning,
+	}
+}
+
+func TestHideDecommissioned(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	ns := []status.NodeStatus{
+		makeNodeStatus(1),
+		makeNodeStatus(2),
+		makeNodeStatus(3),
+		makeNodeStatus(4),
+		// Below don't have decommission statuses
+		makeNodeStatus(5),
+		makeNodeStatus(6),
+	}
+	d := serverpb.DecommissionStatusResponse{
+		Status: []serverpb.DecommissionStatusResponse_Status{
+			makeDecommissionStatus(1, true, false),  // live
+			makeDecommissionStatus(2, false, false), // dead but not decommissioning
+			makeDecommissionStatus(3, true, true),   // live, decommissioning
+			makeDecommissionStatus(4, false, true),  // dead and decommissioned --> hide
+			// NodeIDs do not match those in NodeStatuses --> have no effect, ignored
+			makeDecommissionStatus(15, true, false), // live
+			makeDecommissionStatus(16, false, true), // dead and decommissioned
+		},
+	}
+
+	ns = hideDecommissioned(ns, &d)
+
+	expectedNs := []status.NodeStatus{
+		makeNodeStatus(1),
+		makeNodeStatus(2),
+		makeNodeStatus(3),
+		// missing NodeID 4 which is dead and decommissioned
+		makeNodeStatus(5),
+		makeNodeStatus(6),
+	}
+	expectedD := serverpb.DecommissionStatusResponse{
+		Status: []serverpb.DecommissionStatusResponse_Status{
+			makeDecommissionStatus(1, true, false),  // live
+			makeDecommissionStatus(2, false, false), // dead but not decommissioning
+			makeDecommissionStatus(3, true, true),   // live, decommissioning
+		},
+	}
+	if !reflect.DeepEqual(ns, expectedNs) {
+		var expectedIDs []roachpb.NodeID
+		for _, n := range expectedNs {
+			expectedIDs = append(expectedIDs, n.Desc.NodeID)
+		}
+		var IDs []roachpb.NodeID
+		for _, n := range ns {
+			IDs = append(IDs, n.Desc.NodeID)
+		}
+		t.Errorf("unexpected NodeStatuses: want %v, got %v\n", expectedIDs, IDs)
+	}
+
+	if !reflect.DeepEqual(d, expectedD) {
+		var expectedIDs []roachpb.NodeID
+		for _, n := range expectedD.Status {
+			expectedIDs = append(expectedIDs, n.NodeID)
+		}
+		var IDs []roachpb.NodeID
+		for _, n := range d.Status {
+			IDs = append(IDs, n.NodeID)
+		}
+		t.Errorf("unexpected Decommission statuses: want %v, got %v\n", expectedIDs, IDs)
+	}
+}