cli: remove `cockroach quit` #82988

knz · 2022-06-16T13:09:40Z

Release note (backward-incompatible change): The cockroach quit
command was removed. It had been deprecated since v20.1. To shut down
a node gracefully, send a SIGTERM signal to it.

cc @taroface

cockroach-teamcity · 2022-06-16T13:09:54Z

This change is

erikgrinaker · 2022-06-16T14:16:55Z

Unsure if this needs to be documented - is roachprod public-facing?

No, roachprod isn't public-facing, so we can skip the release note.

srosenberg · 2022-06-16T14:20:41Z

Unsure if this needs to be documented - is roachprod public-facing?

No, roachprod isn't public-facing, so we can skip the release note.

Technically it is; e.g., cloud report playbook uses it.

erikgrinaker

LGTM at a high level.

erikgrinaker · 2022-06-16T14:23:46Z

build/archive/contents/README

@@ -19,7 +19,7 @@ To interact with a one-node SQL cluster:

     $ cockroach start --background --insecure
     $ cockroach sql --insecure
-     $ cockroach quit --insecure
+     $ killall -TERM cockroach # to shut down the nodes


One thing to note here is that on some Unices (looking at you Solaris), killall will kill all processes on the system. Maybe that's obscure enough that we don't have to worry about it, given that we only officially support Linux and macOS does something similar here. But worth keeping in mind if we're going to sprinkle this all over our docs etc.

Then again, if some people do run those Unices, I guess they're already aware.

Good point, Erik! Personally, I never run killall. In this case, I'd probably use pgrep cockroach | xargs kill -TERM, but I am also fine with killall being in README.

erikgrinaker · 2022-06-16T14:34:53Z

Unsure if this needs to be documented - is roachprod public-facing?

No, roachprod isn't public-facing, so we can skip the release note.

Technically it is; e.g., cloud report playbook uses it.

Doesn't it make certain assumptions about our cloud setup though, e.g. project names and VM names and stuff? Seems like more trouble than it's worth to officially support it.

It's called out in the README too:

roachprod is an internal tool for creating and testing CockroachDB clusters. Use at your own risk!

knz · 2022-06-16T14:37:44Z

I'll remove the release note. (After the other changes that CI will tell me to make.)

srosenberg · 2022-06-16T15:48:05Z

build/verify-archive.sh

@@ -9,7 +9,7 @@
 set -euo pipefail

 apt-get update
-apt-get install -y autoconf bison cmake libncurses-dev
+apt-get install -y autoconf bison cmake libncurses-dev psmisc


I assume that's for killall? Maybe we could do away by using pgrep cockroach | kill -TERM instead?

srosenberg · 2022-06-16T15:49:58Z

pkg/cli/flags_test.go

@@ -962,32 +962,33 @@ func TestClientConnSettings(t *testing.T) {

 	// For some reason, when run under stress all these test cases fail due to the
 	// `--host` flag being unknown to quitCmd. Just skip this under stress.
+	// TODO(knz): Check if this skip still applies.


Would be nice to lift the skip if this no longer holds.

Release note: None

As of v20.1, the recommended way to shut down a node is to send SIGTERM, then wait for the process to gracefully terminate, then *only if it doesn't terminate within a reasonable delay* follow up with SIGKILL. The command `cockroach quit` was deprecated. In order to enable graceful shutdown tests, we thus need a new mechanism in `roachprod` to "send a signal then wait a reasonable amount of time until the process terminates". This patch introduces this via the new command-line flag `--max-wait`. Release note: None

Release note: None

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @srosenberg, and @tbg)

build/verify-archive.sh line 12 at r7 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

I assume that's for killall? Maybe we could do away by using pgrep cockroach | kill -TERM instead?

pgrep is part of procps which needs to be installed to. Changed to use that instead.

build/archive/contents/README line 22 at r6 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

Good point, Erik! Personally, I never run killall. In this case, I'd probably use pgrep cockroach | xargs kill -TERM, but I am also fine with killall being in README.

Changed to pkill.

pkg/cli/flags_test.go line 965 at r7 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

Would be nice to lift the skip if this no longer holds.

I agree. But I don't think this should hold this PR.

knz · 2022-06-16T17:03:40Z

bors r=erikgrinaker,srosenberg

craig · 2022-06-16T18:31:30Z

Build failed (retrying...):

GitHub CI (Cockroach)

knz · 2022-06-16T21:15:26Z

@tbg points out this might be flaky. Will investigate further.

bors r-

craig · 2022-06-16T21:15:28Z

Canceled.

Release note: None

Release note (backward-incompatible change): The `cockroach quit` command was removed. It had been deprecated since v20.1. To shut down a node gracefully, send a SIGTERM signal to it.

knz · 2022-06-16T21:53:50Z

bors r=erikgrinaker,srosenberg

craig · 2022-06-17T01:46:30Z

Build succeeded:

GitHub CI (Cockroach)

89526: bincheck: work around deprecated `cockroach quit` r=matthewtodd a=matthewtodd The `cockroach quit` subcommand has been [removed][1]. The recommended way to issue a graceful shutdown is sending a `SIGTERM` signal. [1]: #82988 Release note: None Co-authored-by: Matthew Todd <todd@cockroachlabs.com>

The `cockroach quit` subcommand has been removed[1]. The recommended way to issue a graceful shutdown is sending a `SIGTERM` signal. [1]: #82988 Release note: None

knz requested review from erikgrinaker and tbg June 16, 2022 13:09

knz requested review from a team as code owners June 16, 2022 13:09

knz force-pushed the 20220616-quit branch 2 times, most recently from 7c804b1 to fe0792a Compare June 16, 2022 13:28

erikgrinaker approved these changes Jun 16, 2022

View reviewed changes

srosenberg reviewed Jun 16, 2022

View reviewed changes

knz added 5 commits June 16, 2022 18:00

cli: move the RPC client code to a separate file

831a3cd

Release note: None

cli: move the node shutdown code to its own file

4066038

Release note: None

cli: rename quitCtx to drainCtx

540e83d

Release note: None

roachtest: avoid cockroach quit to shut down nodes

1835a35

Release note: None

knz force-pushed the 20220616-quit branch from fe0792a to 7d00a01 Compare June 16, 2022 16:08

knz commented Jun 16, 2022

View reviewed changes

knz force-pushed the 20220616-quit branch from 7d00a01 to 6a0434b Compare June 16, 2022 17:03

knz requested a review from a team June 16, 2022 17:03

knz added 2 commits June 16, 2022 21:53

*: remove mentions of cockroach quit

ae543f7

Release note: None

cli: remove cockroach quit

903437a

Release note (backward-incompatible change): The `cockroach quit` command was removed. It had been deprecated since v20.1. To shut down a node gracefully, send a SIGTERM signal to it.

knz force-pushed the 20220616-quit branch from 6a0434b to 903437a Compare June 16, 2022 21:53

craig bot merged commit 8f23486 into cockroachdb:master Jun 17, 2022

cockroach-teamcity mentioned this pull request Jun 17, 2022

cli: remove cockroach quit cockroachdb/docs#14314

Closed

knz deleted the 20220616-quit branch June 17, 2022 07:51

aayushshah15 mentioned this pull request Jun 21, 2022

roachtest: decommission/nodes=4/duration=1h0m0s failed #83060

Closed

tbg mentioned this pull request Jun 23, 2022

*: perf regression in tpccbench/nodes=3/cpu=16 (and others) around Jun 18th #83273

Closed

matthewtodd mentioned this pull request Oct 6, 2022

bincheck: work around deprecated cockroach quit #89526

Merged

This was referenced Oct 11, 2022

release-22.2.0: bincheck: work around deprecated cockroach quit #89712

Merged

release-22.2: bincheck: work around deprecated cockroach quit #89713

Merged

rafiss mentioned this pull request Apr 15, 2024

server: expose the ability to shutdown after a drain #120520

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: remove `cockroach quit` #82988

cli: remove `cockroach quit` #82988

knz commented Jun 16, 2022 •

edited

Loading

cockroach-teamcity commented Jun 16, 2022

erikgrinaker commented Jun 16, 2022

srosenberg commented Jun 16, 2022

erikgrinaker left a comment

erikgrinaker Jun 16, 2022 •

edited

Loading

srosenberg Jun 16, 2022

erikgrinaker commented Jun 16, 2022 •

edited

Loading

knz commented Jun 16, 2022

srosenberg Jun 16, 2022

srosenberg Jun 16, 2022

knz left a comment

knz commented Jun 16, 2022

craig bot commented Jun 16, 2022

knz commented Jun 16, 2022

craig bot commented Jun 16, 2022

knz commented Jun 16, 2022

craig bot commented Jun 17, 2022

cli: remove cockroach quit #82988

cli: remove cockroach quit #82988

Conversation

knz commented Jun 16, 2022 • edited Loading

cockroach-teamcity commented Jun 16, 2022

erikgrinaker commented Jun 16, 2022

srosenberg commented Jun 16, 2022

erikgrinaker left a comment

Choose a reason for hiding this comment

erikgrinaker Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

srosenberg Jun 16, 2022

Choose a reason for hiding this comment

erikgrinaker commented Jun 16, 2022 • edited Loading

knz commented Jun 16, 2022

srosenberg Jun 16, 2022

Choose a reason for hiding this comment

srosenberg Jun 16, 2022

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz commented Jun 16, 2022

craig bot commented Jun 16, 2022

knz commented Jun 16, 2022

craig bot commented Jun 16, 2022

knz commented Jun 16, 2022

craig bot commented Jun 17, 2022

cli: remove `cockroach quit` #82988

cli: remove `cockroach quit` #82988

knz commented Jun 16, 2022 •

edited

Loading

erikgrinaker Jun 16, 2022 •

edited

Loading

erikgrinaker commented Jun 16, 2022 •

edited

Loading