acceptance: fix spurious docker test failure unexpected extra event &{0 die} (after [])

[knz]

Needed for #86049.

The root cause of the issue is that the docker container for the DB nodes were crashing, because the test framework was incorrectly deleting the data/log directory before shutting down the nodes.

However, this root cause was mostly hidden because of another mistake: the assertion that checks the health of the nodes was not doing its job properly in most cases. So it believed the nodes were healthy when in fact they were not.

This PR fixes both problems.

Informs #58955.
Informs #58951.
Informs #58218.

Release justification: non-production code changes

[cockroach-teamcity]

This change is 

[knz]

As far as I can see, this problem has existed for a very long time. Throughout most of its existence, it was lucky that 15ms was nearly never enough in Assert to retrieve the docker failure event from the node container. I guess the docker controller has become faster in recent versions.

[knz]

NB CI failing due to #86838

[rickystewart]

Awesome, it's great to finally see this fixed.

[renatolabs]

Thanks for fixing this! 🎉 Left a few minor comments.

Reviewed 1 of 1 files at r1, 3 of 3 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 2 of 2 files at r11, 1 of 1 files at r12, 2 of 2 files at r13, 1 of 1 files at r14.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

---

pkg/acceptance/cluster/dockercluster.go line 591 at r10 (raw file):

func (l *DockerCluster) monitor(ctx context.Context, monitorDone chan struct{}) {
	defer func() { close(monitorDone) }()

Nit: why not defer close(monitorDone)?

---

pkg/acceptance/cluster/dockercluster.go line 650 at r10 (raw file):

	l.monitorCtx, l.monitorCtxCancelFunc = context.WithCancel(context.Background())
	l.monitorDone = make(chan struct{})
	go l.monitor(ctx, l.monitorDone)

Nit: it would make sense if monitorDone were always associated with the monitor() call. In other words, not passing a channel to the monitor function and closing monitorDone there instead.

---

pkg/acceptance/cluster/dockercluster.go line 684 at r14 (raw file):

		// The expected event channel is buffered and should contain
		// all expected events already.
		const almostZero = 15 * time.Millisecond

Curious: if we expect the channel to contain all expected events at this point, why do we wait 15ms?

---

pkg/acceptance/cluster/dockercluster.go line 689 at r14 (raw file):

			break
		}
		t.Logf("expecting event: %v", exp)

Could we change these log messages to use %#v instead, for a more readable output?

[knz]

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

---

pkg/acceptance/cluster/dockercluster.go line 591 at r10 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Nit: why not defer close(monitorDone)?

sure why not

---

pkg/acceptance/cluster/dockercluster.go line 684 at r14 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Curious: if we expect the channel to contain all expected events at this point, why do we wait 15ms?

If we specify zero, the timeout branch will be taken and the channel will not be read, resulting in incorrect behavior.

---

pkg/acceptance/cluster/dockercluster.go line 689 at r14 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Could we change these log messages to use %#v instead, for a more readable output?

In general I'd be friendly to this idea, but given how simple the struct is, it's not worth it here.

[knz]

bors r=rickystewart,renatolabs

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)