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

Add cockroach-admin dropshot server #5822

Merged
merged 11 commits into from
May 29, 2024
Merged

Conversation

jgallagher
Copy link
Contributor

The goal here is to use this server to run cockroach node decommission for expunged cockroach zones. For this initial PR, the only endpoint provided wraps cockroach node status; wrapping decommission is not as trivial so I figured it should go into a separate PR.

There are currently no callers of this service, but stood up an a4x2 and confirmed I could talk to it from the switch zone via curl:

root@oxz_switch:~# curl http://[fd00:1122:3344:103::3]:32222/node/status
{"all_nodes":[{"node_id":"1","address":"[fd00:1122:3344:103::3]:32221","sql_address":"[fd00:1122:3344:103::3]:32221","build":"v22.1.9-dirty","started_at":"2024-05-24T16:47:33.137256Z","updated_at":"2024-05-24T19:01:11.345263Z","locality":"","is_available":true,"is_live":true},{"node_id":"2","address":"[fd00:1122:3344:102::3]:32221","sql_address":"[fd00:1122:3344:102::3]:32221","build":"v22.1.9-dirty","started_at":"2024-05-24T16:41:23.877326Z","updated_at":"2024-05-24T19:01:10.946872Z","locality":"","is_available":true,"is_live":true},{"node_id":"3","address":"[fd00:1122:3344:102::4]:32221","sql_address":"[fd00:1122:3344:102::4]:32221","build":"v22.1.9-dirty","started_at":"2024-05-24T16:41:24.020025Z","updated_at":"2024-05-24T19:01:11.112721Z","locality":"","is_available":true,"is_live":true},{"node_id":"4","address":"[fd00:1122:3344:101::4]:32221","sql_address":"[fd00:1122:3344:101::4]:32221","build":"v22.1.9-dirty","started_at":"2024-05-24T16:41:42.706769Z","updated_at":"2024-05-24T19:01:10.944673Z","locality":"","is_available":true,"is_live":true},{"node_id":"5","address":"[fd00:1122:3344:101::3]:32221","sql_address":"[fd00:1122:3344:101::3]:32221","build":"v22.1.9-dirty","started_at":"2024-05-24T16:41:43.079549Z","updated_at":"2024-05-24T19:01:11.326557Z","locality":"","is_available":true,"is_live":true}]}

Comment on lines 1718 to 1719
.add_property("listen_addr", "astring", crdb_listen_addr)
.add_property("listen_port", "astring", crdb_listen_port)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, feel free to tell me to go do this myself, but if we're passing the socketaddr directly to the cockroach-admin service, should we do the same for the cockroachdb service?

(Just feels weird to be asymmetrical with "ip + port" vs "socketaddr" here)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair. I skimmed over this and thought maybe cockroach wanted them as separate args, but it looks like our method script just squishes them back into a socket address. I'll change this to match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7e3ca84 (after doing a bit of rework in 312e11d to clarify where we want an IP vs a socket address)

cockroach-admin/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +20 to +21
# See omicron-rpaths for more about the "pq-sys" dependency.
pq-sys = "*"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little surprised we need this + the build.rs script. I thought we would have needed this to get access to a copy of postgres - this is normally necessary for Diesel, but is it also necessary to invoke the CLI?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's required by the unit tests that want to spin up cockroach. I could maybe change this to a dev-dependency, but then I have to tweak the build script to also only set rpaths when building for tests? Seems fine-ish, although we have that

// NOTE: This file MUST be kept in sync with the other build.rs files in this
// repository.

bit in the copy-paste build.rs that wouldn't be valid anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhhh, gotcha, because the unit test scaffolding is making calls with diesel? no objection to this as-is, just was curious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so - I didn't really look into it when I saw the rpath failure in CI.

cockroach-admin/src/bin/cockroach-admin.rs Outdated Show resolved Hide resolved
cockroach-admin/src/cockroach_cli.rs Outdated Show resolved Hide resolved
Comment on lines +377 to +380
// Ensure that if `cockroach node status` changes in a future CRDB version
// bump, we have a test that will fail to force us to check whether our
// current parsing is still valid.
#[tokio::test]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hell yeah, was looking for this test, thrilled it's here

cockroach-admin/src/cockroach_cli.rs Outdated Show resolved Hide resolved
@jgallagher jgallagher merged commit 0e3e613 into main May 29, 2024
21 checks passed
@jgallagher jgallagher deleted the john/cockroach-admin-server branch May 29, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants