cli: fix alignment in `node status` output #27808

neeral · 2018-07-20T16:54:00Z

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

cockroach-teamcity · 2018-07-20T16:54:05Z

This change is

neeral

See issue for evidence of testing

Reviewable status: complete! 0 of 0 LGTMs obtained

neeral

There are no tests for pkg/cli/node.go so I performed manual testing.

Reviewable status: complete! 0 of 0 LGTMs obtained

tbg

I'm not confident the code is now correct. Concretely, I think you also have to add to decommissionStatuses when the lop in line 204 decides not to hide the entry.

Could you pull out line 196-220 into a helper that you can then unit test?

neeral

I've added UT for hideDecommission function. The loop on line 204 didn't seem to affect it. One thing I have noticed is that node statuses that don't have a decommission status is valid (before this PR). Should I exclude such entries (node IDs 5 and 6 in the test)?

Reviewable status: complete! 0 of 0 LGTMs obtained

tbg

I think it's fine to print the nodes which don't have a decommission status. (That's better than removing them or erroring out).

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

knz · 2018-07-26T16:58:22Z

@neeral can you please rebase your patch on the latest master branch. Currently it uses a submodule ref that doesn't exist any more.

neeral · 2018-07-26T19:32:52Z

I've rebased onto the latest release-2.0 branch.

tbg · 2018-07-26T19:38:19Z

@knz this PR only merges against release-2.0. The code on master already doesn't have this bug any more.

knz · 2018-07-26T21:25:11Z

Yes sorry I meant release-2.0.
The point was that it was using a submodule sha that didn't exist any more. This is now fixed.

knz · 2018-07-26T21:25:37Z

@tschottdorf I'm not reviewing this further. If you're happy, feel free to merge.

tbg · 2018-07-28T18:44:14Z

@neeral there's a flake in the decommission test:

decommission_test.go:338: csv input:
    id,is_live,gossiped_replicas,is_decommissioning,is_draining
    1,true,2,true,false
    id,is_live,gossiped_replicas,is_decommissioning,is_draining
    1,true,0,true,false
    
    All target nodes report that they hold no more data. Please verify cluster health before removing the nodes.
    
    expected:
    [][]string{
        {"id", "is_live", "gossiped_replicas", "is_decommissioning", "is_draining"},
        {"1", "true", "0", "true", "true"},
        {"All target nodes report that they hold no more data. Please verify cluster health before removing the nodes."},
    }
    errors:<nil>
    row #2, col #5: found "false" which does not match "true"

You just have to change the test so that it accepts true|false for the is_draining field.

tbg · 2018-07-31T16:43:08Z

bors r+

craig · 2018-07-31T17:11:46Z

Build failed

GitHub CI (Cockroach)

`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

knz · 2018-08-02T16:26:22Z

thanks for the fix

bors r+

craig · 2018-08-02T16:54:15Z

Build failed

GitHub CI (Cockroach)

neeral

@knz @tschottdorf Bors keeps failing but when I run TC build it passes. Is there known flakiness here?
Most recently it failed on acceptance_test:TestJSONBUpgrade which is unrelated to changes I have made.
Could someone re-trigger bors?

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

benesch · 2018-08-03T18:03:18Z

bors r+

craig · 2018-08-03T19:27:39Z

Timed out

benesch · 2018-08-03T20:22:19Z

bors r+

craig · 2018-08-03T21:26:30Z

Build failed

GitHub CI (Cockroach)

benesch · 2018-08-03T21:42:06Z

bors r+

craig · 2018-08-03T22:39:42Z

Build failed

GitHub CI (Cockroach)

benesch · 2018-08-03T22:47:27Z

bors r+

27808: cli: fix alignment in `node status` output r=benesch a=neeral See #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]>

craig · 2018-08-03T23:15:48Z

Build succeeded

GitHub CI (Cockroach)

neeral requested a review from a team as a code owner July 20, 2018 16:54

neeral commented Jul 20, 2018

View reviewed changes

knz changed the title ~~Fix alignment in node status output~~ cli: fix alignment in node status output Jul 21, 2018

knz requested a review from tbg July 21, 2018 15:13

tbg reviewed Jul 24, 2018

View reviewed changes

neeral force-pushed the decommission-status-slic branch from b836fc8 to 9a651aa Compare July 26, 2018 01:14

neeral commented Jul 26, 2018

View reviewed changes

tbg approved these changes Jul 26, 2018

View reviewed changes

neeral force-pushed the decommission-status-slic branch from 9a651aa to 75305b1 Compare July 26, 2018 19:32

neeral force-pushed the decommission-status-slic branch from 75305b1 to 3c2e308 Compare July 26, 2018 20:37

neeral force-pushed the decommission-status-slic branch 2 times, most recently from e2f9dfb to 1d407ed Compare July 27, 2018 23:07

neeral force-pushed the decommission-status-slic branch from 1d407ed to 62f363a Compare July 30, 2018 02:58

neeral force-pushed the decommission-status-slic branch from 62f363a to d28ee73 Compare August 1, 2018 17:08

neeral commented Aug 3, 2018

View reviewed changes

craig bot merged commit d28ee73 into cockroachdb:release-2.0 Aug 3, 2018

tbg mentioned this pull request Aug 7, 2018

teamcity: failed tests on release-2.0: acceptance/TestDecommission #28224

Closed

knz mentioned this pull request Aug 7, 2018

cli: node status output misaligns decommission status #27807

Closed

tbg mentioned this pull request Aug 15, 2018

cli: node status --decommission mixes up output #28620

Closed

mrtracy mentioned this pull request Aug 27, 2018

teamcity: failed tests on release-2.0: acceptance/TestDecommission #27742

Closed

tbg mentioned this pull request Aug 30, 2018

cockroach node status shows incorrect nodes as decommissioned #29308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: fix alignment in `node status` output #27808

cli: fix alignment in `node status` output #27808

neeral commented Jul 20, 2018

cockroach-teamcity commented Jul 20, 2018

neeral left a comment

neeral left a comment

tbg left a comment

neeral left a comment

tbg left a comment

knz commented Jul 26, 2018

neeral commented Jul 26, 2018

tbg commented Jul 26, 2018

knz commented Jul 26, 2018

knz commented Jul 26, 2018

tbg commented Jul 28, 2018

tbg commented Jul 31, 2018

craig bot commented Jul 31, 2018

knz commented Aug 2, 2018

craig bot commented Aug 2, 2018

neeral left a comment

benesch commented Aug 3, 2018

craig bot commented Aug 3, 2018

benesch commented Aug 3, 2018

craig bot commented Aug 3, 2018

benesch commented Aug 3, 2018

craig bot commented Aug 3, 2018

benesch commented Aug 3, 2018

craig bot commented Aug 3, 2018

cli: fix alignment in node status output #27808

cli: fix alignment in node status output #27808

Conversation

neeral commented Jul 20, 2018

cockroach-teamcity commented Jul 20, 2018

neeral left a comment

Choose a reason for hiding this comment

neeral left a comment

Choose a reason for hiding this comment

tbg left a comment

Choose a reason for hiding this comment

neeral left a comment

Choose a reason for hiding this comment

tbg left a comment

Choose a reason for hiding this comment

knz commented Jul 26, 2018

neeral commented Jul 26, 2018

tbg commented Jul 26, 2018

knz commented Jul 26, 2018

knz commented Jul 26, 2018

tbg commented Jul 28, 2018

tbg commented Jul 31, 2018

craig bot commented Jul 31, 2018

Build failed

knz commented Aug 2, 2018

craig bot commented Aug 2, 2018

Build failed

neeral left a comment

Choose a reason for hiding this comment

benesch commented Aug 3, 2018

craig bot commented Aug 3, 2018

Timed out

benesch commented Aug 3, 2018

craig bot commented Aug 3, 2018

Build failed

benesch commented Aug 3, 2018

craig bot commented Aug 3, 2018

Build failed

benesch commented Aug 3, 2018

craig bot commented Aug 3, 2018

Build succeeded

cli: fix alignment in `node status` output #27808

cli: fix alignment in `node status` output #27808