From d28ee73e246622b70b51fe9978a469f7e20a3ccb Mon Sep 17 00:00:00 2001 From: neeral Date: Fri, 20 Jul 2018 09:11:34 -0700 Subject: [PATCH] cli: Fix alignment in `node status` output `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 --- pkg/acceptance/decommission_test.go | 2 +- pkg/cli/node.go | 50 ++++++++----- pkg/cli/node_test.go | 106 ++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+), 17 deletions(-) create mode 100644 pkg/cli/node_test.go diff --git a/pkg/acceptance/decommission_test.go b/pkg/acceptance/decommission_test.go index 05f02ae50919..26c9531f5b37 100644 --- 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 index 27ff41652cbd..b10d6353faef 100644 --- 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 new file mode 100644 index 000000000000..5bd9279937e7 --- /dev/null +++ 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) + } +}