roachprod: force wait=true for stop --signal=9

[tbg]

Touches #77334.

Release note: None

[cockroach-teamcity]

This change is 

[srosenberg]

Thanks for PR! Minor comments, otherwise it's good to go.

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @tbg)

---

pkg/roachprod/roachprod.go, line 680 at r1 (raw file):

	// If Wait is set, roachprod waits until the PID disappears (i.e. the
	// process has terminated).
	Wait bool // forced to true when Sig == 9

Might be worthwhile to also change roachprod's DefaultStopOpts.Wait default to true.

---

pkg/roachprod/install/cluster_synced.go, line 228 at r1 (raw file):

func (c *SyncedCluster) Stop(ctx context.Context, l *logger.Logger, sig int, wait bool) error {
	if sig == 9 {
		// `kill -9` without wait is never what a caller wants. See #77334.

While I can't think of a specific use-case which would require wait=false, it would improve debugging experience to log in the case that the wait value is overridden below.

[tbg]

TFTR! No changes, but would like you to take a look at my responses before I merge.

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

---

pkg/roachprod/roachprod.go, line 680 at r1 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

Might be worthwhile to also change roachprod's DefaultStopOpts.Wait default to true.

I intentionally didn't do that, or things like this will rot:

	stopOpts := option.DefaultStopOpts()
	stopOpts.RoachprodOpts.Sig = 2

DefaultStopOpts() has 50 usages. I would like to minimize potential disruption during the stability period (and would like to backport this without sweating). Generally I still like the plan to move from Wait -> to NoWait but I think that should be done separately from fixing the missing wait in kill -9 that exists pervasively across the test suite and is ever desirable.

---

pkg/roachprod/install/cluster_synced.go, line 228 at r1 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

While I can't think of a specific use-case which would require wait=false, it would improve debugging experience to log in the case that the wait value is overridden below.

It won't be useful to log this. Tests call c.Stop all the time and since I am averse to changing the default for the Wait flag at this moment (see other comment) it would just get spammed all over the place (into already spammy logs :-) )

[srosenberg]

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @otan and @tbg)

---

pkg/roachprod/roachprod.go, line 680 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I intentionally didn't do that, or things like this will rot:

	stopOpts := option.DefaultStopOpts()
	stopOpts.RoachprodOpts.Sig = 2

DefaultStopOpts() has 50 usages. I would like to minimize potential disruption during the stability period (and would like to backport this without sweating). Generally I still like the plan to move from Wait -> to NoWait but I think that should be done separately from fixing the missing wait in kill -9 that exists pervasively across the test suite and is ever desirable.

Sounds good; would you mind leaving a TODO for a follow-up refactoring.

---

pkg/roachprod/install/cluster_synced.go, line 228 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

It won't be useful to log this. Tests call c.Stop all the time and since I am averse to changing the default for the Wait flag at this moment (see other comment) it would just get spammed all over the place (into already spammy logs :-) )

Got it; logs are way too spammy as is :(

[tbg]

bors r=srosenberg

Not going to put a TODO in the code but instead leaving #77334 open.

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)