cli: fix `cockroach quit` by knz · Pull Request #26158 · cockroachdb/cockroach

knz · 2018-05-29T11:13:16Z

Informs/fixes #25870.
Fixes #26144.

This patch fixes the following:

the logic in doShutdown() aims to ignore errors caused by attempts
connect to a server which is closing its gRPC channels, but was
missing one case of such errors: during the initial check whether
the node was running. This patch causes gRPC "closed connection"
errors to become also ignored in that case.
previously if there was a transient gRPC error during a hard
shutdown whereby the shutdown could still succeed, then cockroach quit would fail no matter what. This patch makes cockroach quit
retry a hard shutdown.
the warning messages are now emitted on stderr (via log.Warningf)
instead of stdout.

Release note (bug fix): fix a bug where cockroach quit would
erroneously fail even though the node already successfully shut down.

Release note (cli change): cockroach quit now emits warning message
on its standard error stream, not standard output.

cockroach-teamcity · 2018-05-29T11:13:22Z

This change is

tbg · 2018-05-29T11:21:59Z

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

pkg/cli/start.go, line 1075 at r1 (raw file):

				return nil
			}
			if strings.Contains(err.Error(), "proceeding with hard shutdown") {

Could you extract this string constant so that it's shared with the other use? As is, it will rot too easily.

pkg/cli/start.go, line 1078 at r1 (raw file):

				// Something happened server-side that made the server decide on
				// a hard shutdown of its own. This is not an error, but still
				// inform the user.

Perhaps also mention that we don't fold this into IsClosedConnection because we don't know whether this error can only occur in that circumstance (don't think so).

Comments from Reviewable

This patch fixes the following: - the logic in `doShutdown()` aims to ignore errors caused by attempts connect to a server which is closing its gRPC channels, but was missing one case of such errors: during the initial check whether the node was running. This patch causes gRPC "closed connection" errors to become also ignored in that case. - previously if there was a transient gRPC error during a hard shutdown whereby the shutdown could still succeed, then `cockroach quit` would fail no matter what. This patch makes `cockroach quit` retry a hard shutdown. - the warning messages are now emitted on stderr (via `log.Warningf`) instead of stdout. Release note (bug fix): fix a bug where `cockroach quit` would erroneously fail even though the node already successfully shut down. Release note (cli change): `cockroach quit` now emits warning message on its standard error stream, not standard output.

knz · 2018-05-29T11:32:32Z

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

pkg/cli/start.go, line 1075 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Could you extract this string constant so that it's shared with the other use? As is, it will rot too easily.

I made a mistake -- this string is not generated by the server. Solved the problem in a different way (retry hard shutdown).

pkg/cli/start.go, line 1078 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Perhaps also mention that we don't fold this into IsClosedConnection because we don't know whether this error can only occur in that circumstance (don't think so).

n/a.

Comments from Reviewable

knz · 2018-05-29T11:36:34Z

bors r+

tbg · 2018-05-29T11:43:30Z

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from Reviewable

craig · 2018-05-29T11:55:53Z

Build failed

GitHub CI (Cockroach)

knz · 2018-05-29T11:56:34Z

bors failed on #26098, retrying

bors r+

knz · 2018-05-29T12:09:07Z

bors r-

craig · 2018-05-29T12:09:08Z

Canceled

knz · 2018-05-29T12:09:18Z

(bors was stuck)

bors r+

craig · 2018-05-29T13:11:46Z

Timed out

knz · 2018-05-29T13:13:19Z

bors r+

craig · 2018-05-29T14:14:26Z

Timed out (retrying...)

a-robinson · 2018-05-29T14:44:23Z

pkg/cli/start.go

 	// out, or perhaps drops the connection while waiting). To that end, we first
 	// run a noop DrainRequest. If that fails, we give up.
 	if err := checkNodeRunning(ctx, c); err != nil {
+		if grpcutil.IsClosedConnection(err) {


As @bdarnell asked on #26163, does this swallow the error if you run quit against a server that isn't running?

answered to ben there. if the server does not exist the thing still fails.

knz · 2018-05-29T16:11:00Z

bors r-

knz · 2018-05-29T16:11:08Z

bors r+

couchand · 2018-05-29T16:49:04Z

internal bors weirdness, retrying

bors r=knz

craig · 2018-05-29T18:13:26Z

Build failed (retrying...)

GitHub CI (Cockroach)

26143: opt: Enable additional logic tests for opt configs r=andy-kimball a=andy-kimball Enable more logic tests (orms -> snapshot_unrelated_update). Fix various bugs and issues that were failing tests. 26158: cli: fix `cockroach quit` r=knz a=knz Informs/fixes #25870. Fixes #26144. This patch fixes the following: - the logic in `doShutdown()` aims to ignore errors caused by attempts connect to a server which is closing its gRPC channels, but was missing one case of such errors: during the initial check whether the node was running. This patch causes gRPC "closed connection" errors to become also ignored in that case. - previously if there was a transient gRPC error during a hard shutdown whereby the shutdown could still succeed, then `cockroach quit` would fail no matter what. This patch makes `cockroach quit` retry a hard shutdown. - the warning messages are now emitted on stderr (via `log.Warningf`) instead of stdout. Release note (bug fix): fix a bug where `cockroach quit` would erroneously fail even though the node already successfully shut down. Release note (cli change): `cockroach quit` now emits warning message on its standard error stream, not standard output. 26165: roachtest: enable periodic heap prof output for kv/splits/nodes=3 r=tschottdorf a=petermattis See #26081 Release note: None 26167: encryption: add cli command `cockroach gen encryption-key` r=windchan7 a=windchan7 Added cli command `cockroach gen encryption-key` to generate store keys for encryption at rest. Encryption CLI: #19783. Release note: None Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com> Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com> Co-authored-by: Peter Mattis <petermattis@gmail.com> Co-authored-by: Victor Chen <victor@cockroachlabs.com>

craig · 2018-05-29T18:53:07Z

Build succeeded

GitHub CI (Cockroach)

26163: release-2.0: cli: fix `cockroach quit` r=knz a=knz Backport 1/1 commits from #26158. This `cockroach quit` bug is likely to affect production users, so we need to back-port it. /cc @cockroachdb/release Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>

knz requested a review from tbg May 29, 2018 11:13

knz requested a review from a team as a code owner May 29, 2018 11:13

knz force-pushed the 20180529-quit branch from 92888eb to 23238cb Compare May 29, 2018 11:14

knz mentioned this pull request May 29, 2018

cli: cockroach quit reports transient RPC error during server shutdown, confuses acceptance tests #25870

Closed

knz force-pushed the 20180529-quit branch from 23238cb to 0d7603b Compare May 29, 2018 11:31

knz mentioned this pull request May 29, 2018

release-2.0: cli: fix cockroach quit #26163

Merged

a-robinson reviewed May 29, 2018

View reviewed changes

craig bot merged commit 0d7603b into cockroachdb:master May 29, 2018

tbg mentioned this pull request May 30, 2018

teamcity: failed tests on master: acceptance/TestDockerCLI #26133

Closed

knz deleted the 20180529-quit branch July 2, 2018 09:30

jseldess mentioned this pull request Jul 2, 2018

cli: fix cockroach quit cockroachdb/docs#3340

Closed

knz mentioned this pull request Aug 28, 2018

cli: cockroach quit produces transient errors again #29180

Closed

Conversation

knz commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented May 29, 2018

Uh oh!

tbg commented May 29, 2018

Uh oh!

knz commented May 29, 2018

Uh oh!

knz commented May 29, 2018

Uh oh!

tbg commented May 29, 2018

Uh oh!

craig bot commented May 29, 2018

Build failed

Uh oh!

knz commented May 29, 2018

Uh oh!

knz commented May 29, 2018

Uh oh!

craig bot commented May 29, 2018

Canceled

Uh oh!

knz commented May 29, 2018

Uh oh!

craig bot commented May 29, 2018

Timed out

Uh oh!

knz commented May 29, 2018

Uh oh!

craig bot commented May 29, 2018

Timed out (retrying...)

Uh oh!

a-robinson May 29, 2018

Choose a reason for hiding this comment

Uh oh!

knz May 29, 2018

Choose a reason for hiding this comment

Uh oh!

knz commented May 29, 2018

Uh oh!

knz commented May 29, 2018

Uh oh!

couchand commented May 29, 2018

Uh oh!

craig bot commented May 29, 2018

Build failed (retrying...)

Uh oh!

craig bot commented May 29, 2018

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

knz commented May 29, 2018 •

edited

Loading