cli: add timeouts to node status and node ls

[tbg]

GRPC in its recent versions has become quite noisy and in particular does
not play well with context cancellation.

Mute it in all client commands except when actually starting a server.

Refactor the cli package to avoid the previously common mixing of stoppers
and contexts. Nobody really needs the stopper usually (and if so, they
could make their own); it is simpler to just have a context and cancel it
when done.

Touches #16489 (by making it more straightforward to use timeouts).

Fixes #20308.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Very nice cleanup getting rid of the stopper mess!

---

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 7 of 7 files at r3, 6 of 6 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

pkg/cli/error.go, line 67 at r4 (raw file):

		case *net.OpError:
			return connDropped()
		default:

nit: no need for the default cases here and below. I'd remove them from all three switches and move the return err out.

---

pkg/cli/init.go, line 60 at r3 (raw file):

}

func getInitClient() (serverpb.InitClient, *stop.Stopper, error) {

why delete this one but leave getStatusClient?

---

pkg/cli/start.go, line 949 at r3 (raw file):

	}
	stopper.AddCloser(stop.CloserFn(func() {
		_ = conn.Close()

How was this handled before? Did we just never close the connection?

---

pkg/cli/start.go, line 969 at r3 (raw file):

}

func stopperContext(stopper *stop.Stopper) context.Context {

Do we still need this function?

---

pkg/util/grpcutil/log.go, line 33 at r2 (raw file):

)

var discardLogger = grpclog.NewLoggerV2(ioutil.Discard, ioutil.Discard, ioutil.Discard)

I'd expect that we would at least want to print errors to stderr outside of starting a cluster. When you say that gRPC has become noisy, do you mean that it's printing spurious errors or warnings?

---

Comments from Reviewable

[tbg]

Review status: 1 of 17 files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.

---

pkg/cli/error.go, line 67 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: no need for the default cases here and below. I'd remove them from all three switches and move the return err out.

Done.

---

pkg/cli/init.go, line 60 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

why delete this one but leave getStatusClient?

It had two callers, but just inlined it there. Removed.

---

pkg/cli/start.go, line 949 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

How was this handled before? Did we just never close the connection?

That's what it seems like to me.

---

pkg/cli/start.go, line 969 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we still need this function?

Removed the remaining callers and put this method to sleep.

---

pkg/util/grpcutil/log.go, line 33 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'd expect that we would at least want to print errors to stderr outside of starting a cluster. When you say that gRPC has become noisy, do you mean that it's printing spurious errors or warnings?

Yeah, it prints a bunch of warnings. Some of this is because using context cancellation tears down everything eagerly, but I don't think it's just that. How do you like the latest iteration?

---

Comments from Reviewable

[nvanbenschoten]

---

Reviewed 1 of 2 files at r6, 6 of 7 files at r7, 6 of 6 files at r8, 2 of 2 files at r9, 5 of 5 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/util/grpcutil/log.go, line 57 at r10 (raw file):

type logger log.Severity

func (severity *logger) Info(args ...interface{}) {

Do these receivers need to be pointers?

---

Comments from Reviewable

[tbg]

Had to unfortunately reintroduce waiting for the stopper, see latest commit.

---

Review status: 1 of 18 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/util/grpcutil/log.go, line 57 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do these receivers need to be pointers?

Yeah, see the comment above (synthetic stack frames).

---

Comments from Reviewable

[tbg]

TFTR!