Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-23.1: roachtest: assign adminui ports dynamically for virtual clusters #117737

Closed

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Jan 12, 2024

Backport 2/2 commits from #117505 on behalf of @DarrylWong.

/cc @cockroachdb/release


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


Release justification: Test only change

@blathers-crl blathers-crl bot requested a review from a team as a code owner January 12, 2024 16:08
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-117505 branch from 745273d to 902351c Compare January 12, 2024 16:08
@blathers-crl blathers-crl bot requested review from herkolategan and DarrylWong and removed request for a team January 12, 2024 16:08
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Jan 12, 2024
Copy link
Author

blathers-crl bot commented Jan 12, 2024

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Backports should only be created for serious
    issues
    or test-only changes.
  • Backports should not break backwards-compatibility.
  • Backports should change as little code as possible.
  • Backports should not change on-disk formats or node communication protocols.
  • Backports should not add new functionality (except as defined
    here).
  • Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
  • All backports must be reviewed by the owning areas TL and one additional
    TL. For more information as to how that review should be conducted, please consult the backport
    policy
    .
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
  • Your backport must be accompanied by a post to the appropriate Slack
    channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Jan 12, 2024
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

Release note: None
…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
@DarrylWong
Copy link
Contributor

DarrylWong commented Jan 12, 2024

This PR works as expected, setting the port dynamically, but the multitenant/distsql tests still fail.

It looks like --format json isn't supported as a table display format in 23.1, causing upsertVirtualClusterMetadata to error out. Even though this function was added a few months ago, it looks like it was never used until we switched to the new API.

I'm not too familiar with what the table display format is used for, so I'm not sure which format should be used instead of JSON. Actually I see now that we just parse it for the virtual cluster IDs 🤔

@renatolabs
Copy link
Contributor

Ah, that's unfortunate. We could patch something up here, but I'm wondering if we should have the same logic across branches (and use something other than JSON to parse this data). Either way, this will introduce a bit of delay to this backport.

@renatolabs
Copy link
Contributor

We can now bundle backports of #115599, #117505, and #117745 to 23.1 in a single PR and things should work.

Copy link

github-actions bot commented Feb 6, 2024

Reminder: it has been 3 weeks please merge or close your backport!

@DarrylWong
Copy link
Contributor

I tried backporting all three in #117837 but running into more issues 😭

Trying to run the test locally gives me connection refused errors. I see that in 23.1 there is a local port offset to avoid local port collisions that isn't there on 23.2 and above.

It seems to me that we need to also backport the fix for local port collisions as well, but I haven't dug around to find what exactly the fix is yet.

Running it on gce gets past that point, but it fails trying to collect stats with:

pq: rpc error: code = Unavailable desc = error reading from server: read tcp 10.142.0.5:44378->10.142.0.5:29004: read: connection reset by peer

I haven't had the chance to dig into what this error is either, but just commenting in case it rings a bell to anyone and to mention that this backport isn't forgotten, just running into some issues 😢

Copy link

Reminder: it has been 3 weeks please merge or close your backport!

@DarrylWong DarrylWong closed this Apr 24, 2024
@mgartner mgartner deleted the blathers/backport-release-23.1-117505 branch May 2, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches blathers-backport This is a backport that Blathers created automatically. no-backport-pr-activity O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants