roachtest/bank: don't start chaos until nodes are serving SQL

[nvanbenschoten]

Informs #30064.

Release note: None

[nvanbenschoten]

Please hold off on this. I thought it would fix the second class of failures we saw in #30064, but I just had a repro so it doesn't seem like it did.

[tbg]

I'm surprised why you didn't change the break on l156 into a return.

[nvanbenschoten]

I still have this cleanup on my plate, including the l156 fix, but I'd like to actually get this test working without flakes first, which is turning out to be surprisingly tricky.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Ok, this has been updated based on the discussion in #30064. PTAL.

Things changed a bit in #31011, so the first commit was replaced with a bit of cleanup around that change.

[petermattis]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/cmd/roachtest/bank.go, line 210 at r1 (raw file):

	consistentIdx int,
) {
	s.waitGroup.Add(1)

Is this kosher? You're calling Add from a goroutine and then calling Wait from the main routine. I think this would hit an internal assertion in WaitGroup, except that the Wait doesn't happen right away.

---

pkg/cmd/roachtest/bank.go, line 215 at r3 (raw file):

	// Don't begin the chaos monkey until all nodes are serving SQL connections.
	// This ensures that we don't test cluster initialization under chaos.
	for i := 0; i < c.nodes; i++ {

Nit: for i := 0; i <= c.nodes; i++. Then in the body you can use i instead of i+1.

[nvanbenschoten]

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/cmd/roachtest/bank.go, line 210 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Is this kosher? You're calling Add from a goroutine and then calling Wait from the main routine. I think this would hit an internal assertion in WaitGroup, except that the Wait doesn't happen right away.

Yeah, you're absolutely right.

---

pkg/cmd/roachtest/bank.go, line 215 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: for i := 0; i <= c.nodes; i++. Then in the body you can use i instead of i+1.

Make it for i := 1; i <= c.nodes; i++ and you've got a deal.

[tbg]

mod Peter's comments which it looks you're fixing up right now.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale)

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale)

---

pkg/cmd/roachtest/bank.go, line 210 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Yeah, you're absolutely right.

Done.

---

pkg/cmd/roachtest/bank.go, line 215 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Make it for i := 1; i <= c.nodes; i++ and you've got a deal.

Done.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale)

[a-robinson]

Having just read through the recent comments on #30064 I agree this is the right change to make. We should also reduce the migration lease duration, though. There's no need for it to be so long given that we renew it in the background while running migrations.

[nvanbenschoten]

TFTRs.

We should also reduce the migration lease duration, though.

I was hoping for a quick fix here, but it looks like this is tied to the sql.LeaseManager, which uses the same duration for all leases. This will have to wait on other improvements to the leasing system, like #24041.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)