roachtest: rationalize cluster destruction

[andreimatei]

Cluster destruction can be initiated from two places: tests finishing
and a signal being received. Some synchronization is required for
handling races on this destruction. This patch makes this
synchronization nicer. Before this patch, the case where a signal was
received and then a test finishes was handled OK, but the reverse I
think was not.

This patch also kills a global containing a list of all the clusters and
encapsulates it in a nicer clusterRegistry.

Release note: None

[cockroach-teamcity]

This change is 

[andreimatei]

friendly ping

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @petermattis)

[andreimatei]

Bob do you want to be my pal here?

[bobvawter]

Reviewed 2 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @petermattis)

---

pkg/cmd/roachtest/cluster.go, line 255 at r1 (raw file):

	select {
	case <-done:
	case <-time.After(5 * time.Minute):

Should this be done by calling context.WithTimeout on the Context passed into destroyAllClusters?

---

pkg/cmd/roachtest/cluster.go, line 680 at r1 (raw file):

	// destroyed is used to coordinate between different goroutines that want to
	// destroy a cluster.
	destroyed chan struct{}

Maybe change the type here to <-chan struct{} since the only useful thing to do here is to select or close()?

---

pkg/cmd/roachtest/cluster.go, line 695 at r1 (raw file):

		return false
	}
	c.destroyState.mu.destroying = true

It seems odd that a should function would mutate the state of the cluster; the name sounds idempotent. Maybe rename it to beginDestroy or something and document that this function is a one-shot.

---

pkg/cmd/roachtest/cluster.go, line 1072 at r1 (raw file):

// destroyInner implements the logic for Destroy but without closing c.l.
func (c *cluster) destroyInner(ctx context.Context) {

The name of this method is confusing w.r.t. doDestroy. Can the main Destroy method defer to a single doDestroy() that has a parameter that controls whether or not to close the associated logger?

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter and @petermattis)

---

pkg/cmd/roachtest/cluster.go, line 255 at r1 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

Should this be done by calling context.WithTimeout on the Context passed into destroyAllClusters?

done

---

pkg/cmd/roachtest/cluster.go, line 680 at r1 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

Maybe change the type here to <-chan struct{} since the only useful thing to do here is to select or close()?

well you can't close a receive-only channel. Morally this reference (which is the only reference to this chan) is used both for receiving and for sending (i.e. closing).

---

pkg/cmd/roachtest/cluster.go, line 695 at r1 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

It seems odd that a should function would mutate the state of the cluster; the name sounds idempotent. Maybe rename it to beginDestroy or something and document that this function is a one-shot.

see now

---

pkg/cmd/roachtest/cluster.go, line 1072 at r1 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

The name of this method is confusing w.r.t. doDestroy. Can the main Destroy method defer to a single doDestroy() that has a parameter that controls whether or not to close the associated logger?

I don't know what I was drinking when I wrote this. See now.

[bobvawter]

Reviewed 2 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @petermattis)

---

pkg/cmd/roachtest/cluster.go, line 1166 at r3 (raw file):

	c.destroyState.mu.Lock()
	close(c.destroyState.mu.destroyed)
	ch := c.destroyState.mu.destroyed

Move assignment of ch one line up and call close(ch)?

[andreimatei]

TFTR!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @bobvawter and @petermattis)

---

pkg/cmd/roachtest/cluster.go, line 1166 at r3 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

Move assignment of ch one line up and call close(ch)?

done

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)