roachtest: add node-kill operation#122899
Conversation
|
This change is |
herkolategan
left a comment
There was a problem hiding this comment.
Left a few small comments, mostly roachtest convenience APIs related. One general question, It seems that clean-up is responsible for the timing regarding when the node is started back up again. Would there be value in having nodes be down for a specified (random) amount of time, rather than a fixed amount of time?
| if err != nil { | ||
| o.Fatal(err) | ||
| } | ||
| err = c.RunE(ctx, option.WithNodes(c.Node(nid)), |
There was a problem hiding this comment.
@renatolabs may know how to structure this better, but we could instead of doing an ExternalPGUrl use the new command util to optionally add insecure or certs to drain:
Something like this (untested):
cmd := roachtestutil.NewCommand(fmt.Sprintf("%s node drain", test.DefaultCockroachPath)).
Flag("logtostderr", "INFO").
Flag("port", fmt.Sprintf("{pgport:%d}", nid)).
MaybeOption(!c.IsSecure(),"insecure").
MaybeFlag(c.IsSecure(),"certs", install.CockroachNodeCertsDir).
Option("self")
err = c.RunE(ctx, option.WithNodes(c.Node(nid)), cmd.String())
Also not sure if secure will be correct here, it's normally passed in (stateless)
01461fe to
c042dd0
Compare
|
Thanks for the review! Addressed the points brought up.
This is a good point, and something I'd want |
9fae50a to
c0acd40
Compare
|
TFTR! bors r=renatolabs |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build failed (retrying...): |
|
The linter is not happy. bors r- |
|
Canceled. |
|
Apologies, didn't realize the linter isn't under |
This change adds a node-kill operation with 4 variants:
one that drains and one that doesn't x {SIGKILL, SIGTERM}.
Epic: none
Release note: None
|
Required checks are green now. bors r=renatolabs |
This change adds a node-kill operation with 4 variants: one that drains and one that doesn't x {SIGKILL, SIGTERM}.
Epic: none
Release note: None