roachtest: assign adminui ports dynamically for virtual clusters#117505
roachtest: assign adminui ports dynamically for virtual clusters#117505craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
|
Ran |
0631a3f to
c5171af
Compare
c5171af to
433ce79
Compare
|
LGTM, but probably worth waiting for #117480 to merge and then unskip those tests in this PR. |
pkg/cmd/roachtest/tests/gossip.go
Outdated
| settings.Env = append(settings.Env, "COCKROACH_SCAN_MAX_IDLE_TIME=5ms") | ||
|
|
||
| c.Start(ctx, t.L(), option.DefaultStartOpts(), settings) | ||
| // TODO(darrylwong): remove SetDefaultSQLPort once #117125 is addressed. |
There was a problem hiding this comment.
Given the below assumption, is the TODO still relevant? i.e., if I understood correctly, this test fails if the invariant adminUIPort = SQLPort + 1 doesn't hold, right?
There was a problem hiding this comment.
Yeah that's true, if we change the port assignment logic then it will fail again.
I changed it to discover the port instead, which seems more correct to me anyway.
This was originally removed in cockroachdb#115599 due to cockroachdb#114097 merging, but adminui was reverted in cockroachdb#117141 and mistakenly did not revert the special case for virtual clusters. Release note: None
433ce79 to
fdea06a
Compare
renatolabs
left a comment
There was a problem hiding this comment.
Thanks for the fix! Left small suggestions.
pkg/cmd/roachtest/cluster.go
Outdated
| return c.adminUIAddr(ctx, l, node, true) | ||
| } | ||
|
|
||
| func (c *clusterImpl) AdminUIPort( |
There was a problem hiding this comment.
Nit: Maybe AdminUIPorts, since this is returning an array of ports?
pkg/cmd/roachtest/cluster.go
Outdated
| } | ||
|
|
||
| func (c *clusterImpl) AdminUIPort( | ||
| ctx context.Context, l *logger.Logger, node option.NodeListOption, |
There was a problem hiding this comment.
nodes, since that argument is a collection?
pkg/roachprod/roachprod.go
Outdated
| } | ||
|
|
||
| // AdminPort finds the AdminUI ports for a cluster. | ||
| func AdminPort( |
pkg/roachprod/roachprod.go
Outdated
| for _, node := range c.Nodes { | ||
| port, err := c.NodeUIPort(ctx, node) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Nit: might be a good idea to include the node in the error message.
pkg/cmd/roachtest/cluster.go
Outdated
| func (c *clusterImpl) AdminUIPort( | ||
| ctx context.Context, l *logger.Logger, node option.NodeListOption, | ||
| ) ([]int, error) { | ||
| return roachprod.AdminPort(ctx, l, c.MakeNodes(node), c.localCertsDir != "") |
…e-one This test relies on assumption `adminUIPort = SQLPort + 1` but adminui is temporarily hardcoded as 26258 while SQLPort will be dynamically assigned without the following override. This changes it to discover the port instead of relying on this assumption. Release note: None
fdea06a to
f6519af
Compare
|
TFTRs bors r=srosenberg, renatolabs |
|
This PR was included in a batch that timed out, it will be automatically retried |
|
Build succeeded: |
|
blathers backport 23.2 |
|
blathers backport 23.1 |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from ca0d2ed to blathers/backport-release-23.1-117505: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
blathers backport 23.1 |
This was originally removed in #115599 due to #114097 merging, but adminui was reverted in #117141 and mistakenly did not revert the special case for virtual clusters. We unskip the multitenant/distsql tests as well.
Release note: None
Fixes: #117150
Fixes: #117149
Epic: None