[sled-agent] Separate CockroachDB "start" from CockroachDB "init" #2954

smklein · 2023-04-30T04:35:15Z

This is a necessary part of #727

History

Previously, CockroachDB was initialized via https://www.cockroachlabs.com/docs/stable/cockroach-start-single-node.html
Additionally, the CRDB tables were explicitly initialized via a request to run sql using dbinit.sql

This was historically done on CRDB datasets immediately when they were initialized - see:

omicron/sled-agent/src/storage_manager.rs

Lines 372 to 394 in 9f31e37

    
           // If requested, format the cluster with the initial tables. 
        
           if do_format { 
        
               info!(log, "Formatting CRDB"); 
        
               zone.run_cmd(&[ 
        
                   "/opt/oxide/cockroachdb/bin/cockroach", 
        
                   "sql", 
        
                   "--insecure", 
        
                   "--host", 
        
                   &dataset_info.address.to_string(), 
        
                   "--file", 
        
                   "/opt/oxide/cockroachdb/sql/dbwipe.sql", 
        
               ])?; 
        
               zone.run_cmd(&[ 
        
                   "/opt/oxide/cockroachdb/bin/cockroach", 
        
                   "sql", 
        
                   "--insecure", 
        
                   "--host", 
        
                   &dataset_info.address.to_string(), 
        
                   "--file", 
        
                   "/opt/oxide/cockroachdb/sql/dbinit.sql", 
        
               ])?; 
        
               info!(log, "Formatting CRDB - Completed"); 
        
           }

In [sled-agent] Refactor service management out of StorageManager #2946 this was changed to just "unconditionally format during zone initialization", which is wrong, but was a holdover for...

This PR

Makes CRDB initialization an explicit endpoint. After booting, the CRDB zone only runs https://www.cockroachlabs.com/docs/stable/cockroach-start.html
... but the all CRDB nodes can be started using https://www.cockroachlabs.com/docs/stable/cockroach-init.html , which is invoked by RSS.
To pull this off, RSS invokes a new POST /cockroachdb_init endpoint exposed by the sled agent.

…zones, zone_name -> zone_type, config -> ledger

## Before this PR Running on rack2 and calling `omicron-package uninstall` would involve a fatal termination of the connection, as it would delete the `cxgbe0/ll` and `cxgbe1/ll` IP addresses necessary for contacting the sled. ## After this PR Those addresses are left alone. This is pretty useful for development, as it allows us to run `uninstall` to cleanly wipe a Gimlet, preparing it for future "clean installs".

davepacheco · 2023-06-27T21:14:25Z

I've taken on this PR. The biggest change I've made so far here is that previously, when provisioning a CockroachDB zone, Sled Agent would look up the Cockroach service in DNS, take the addresses it found, and write those into SMF properties. Then the start method would pull those out and use those as the "join" addresses so that Cockroach can find the other nodes in its cluster. The problem with this is that there's nothing to keep those addresses in sync with what's in DNS (which is the source of truth about who's in the cluster). Instead, I've added some code to configure DNS in the CockroachDB zones (using the dns/install SMF service provided by illumos, which I didn't previously know about!) and then use that in the start method to locate the CockroachDB nodes.

It's worth mentioning explicitly that we've designed internal DNS to be able to start without any external runtime dependencies, including CockroachDB (see RFD 248), which means that using DNS here should not cause a problem for cold boot. As I write this, though, I realize that using dig from the start method like I'm currently doing probably isn't going to cut it. We need something that will keep retrying until it succeeds in case it takes a little while for internal-DNS to come up.

There's another bit of this I'm a little worried about, which is exposing an API in sled agent to initialize the CockroachDB cluster. That seems a little dangerous and also overkill since we only ever intend to do this once, and only before the control plane is initialized. I considered changing this to instead have RSS pass configuration to an SMF service which would do this. The problem is that there's not a great way to propagate success/failure information back to RSS so that it can decide whether to proceed (or, I guess, burn down the world and try again). I'm going to defer fixing this for now because we really need to start playing with multi-node CockroachDB.

I originally thought it made sense to put this into "internal-dns" since it's a straightforward CLI that goes with the library exposed by "internal-dns". But it doesn't work due to a few surprising behaviors: - To ship "dnswait", we'd need to build its parent Rust package as an Omicron package. omicron-package requires that the Omicron package name be "internal-dns" if it's going to come from building the "internal-dns" Rust package. - "omicron-package" does some Cargo operation based on packages in Cargo.lock. - Omicron actually has _two_ different "internal-dns" packages in Cargo.lock: one from the local workspace, and one from the copy of Propolis that pulls in the latest Omicron "main". As a result, we cannot package up "dnswait" (or any other Rust binary) if it's in a package in the workspace that's also depended-on by something else external to Omicron (like Propolis) in the workspace.

smklein

I can't approve this because technically it's my PR, but I do approve! This looks great!

smklein · 2023-06-29T00:00:05Z

sled-agent/src/services.rs

+                // Note that when we configure the dns/install service, we're
+                // supplying values for an existing property group on the SMF
+                // *service*.  We're not creating a new property group, nor are
+                // we configuring a property group on the instance.t by default.


Suggested change

// we configuring a property group on the instance.t by default.

// we configuring a property group on the instance by default.

Thanks. Fixed.

smklein · 2023-06-29T00:05:20Z

smf/cockroachdb/method_script.sh

+JOIN_ADDRS="$(/opt/oxide/internal-dns-cli/bin/dnswait cockroach \
+    | head -n 5 \
+    | tr '\n' ,)"


Nice, I like this solution. Thanks for fitting everything into the "self-assembling zone" approach -- it's neat that we can now refresh all our service info by simply restarting the CRDB service.

smklein · 2023-06-29T00:07:42Z

sled-agent/src/http_entrypoints.rs

+/// Initializes a CockroachDB cluster
+#[endpoint {
+    method = POST,
+    path = "/cockroachdb",
+}]
+async fn cockroachdb_init(
+    rqctx: RequestContext<SledAgent>,
+) -> Result<HttpResponseUpdatedNoContent, HttpError> {
+    let sa = rqctx.context();
+    sa.cockroachdb_initialize().await?;
+    Ok(HttpResponseUpdatedNoContent())
+}
+


Agreed with your comment that "this is sketchy, and it would be cool to not have it".

It seems like it could also be possible to have the method script auto-initialize the DB if it's empty, but that seems like it could raise other problems.

All that said, this approach is probably good enough to last a while.

smklein · 2023-06-29T00:11:17Z

sled-agent/src/services.rs

@@ -1074,6 +1085,40 @@ impl ServiceManager {
                let Some(info) = self.inner.sled_info.get() else {
                    return Err(Error::SledAgentNotReady);
                };
+
+                // We want to configure the dns/install SMF service inside the


Should we pull this out into a separate method? I'd be shocked if this was the only service for which we wanted to populate /etc/resolv.conf.

(I mean, arguably, shouldn't we populate basically all our services with /etc/resolv.conf?)

I do think we should do this, but it's more complicated to use it:
#2122 (comment)

so I'd like to defer it for now.

smklein · 2023-06-29T00:21:40Z

internal-dns-cli/src/bin/dnswait.rs

@@ -0,0 +1,98 @@
+// This Source Code Form is subject to the terms of the Mozilla Public


Just to make sure I'm clear - the main reason for using this instead of dig +short <name> is the timeout?

Yeah, I had dig at first, but I realized it will only try a handful of times and only wait a few seconds, and you can't tell from its exit status that it's failed. I needed something that's going to retry an arbitrary number of times, waits until it finds some results, too, and if it's going to fail, fails clearly.

Not for this PR, but maybe it makes sense to move this utility into the omicron-brand zone alongside some of the other network setup that might be more general? Seems a bit like a pain-in-the-butt to manually package it into each zone that could need it

davepacheco · 2023-06-29T00:30:06Z

I've gone ahead and enabled auto-merge: however one accounts this, @smklein both worked on this and both reviewed it.

smklein added 11 commits April 28, 2023 09:54

[sled-agent] Make service_manager responsible for storage services too

7b54128

Merge branch 'main' into storage-manager-cleanup

f410325

CRDB auto-format on boot

0b4b040

better use of 'unique_name' (for storage zones), auto-launch storage …

ef9517c

…zones, zone_name -> zone_type, config -> ledger

Merge branch 'main' into storage-manager-cleanup

f1fd1f5

Merge branch 'main' into storage-manager-cleanup

3a9ad87

[RSS] Explicit set of Bootstrap Agents

5752a83

Refuse to enact a sled plan unless it's on the explicit set

31b52a7

wip

f77f235

RSS explicitly calling to initialize CRDB

2810f93

Read the CRDB address from the running zone

5297950

smklein mentioned this pull request Apr 30, 2023

[sled-agent] Refactor service management out of StorageManager #2946

Merged

Send requests to the right address

c21be48

smklein force-pushed the cockroach-init branch from 305b758 to c21be48 Compare April 30, 2023 06:05

fmt

f11c153

smklein changed the title ~~Separate CockroachDB "start" from CockroachDB "init"~~ [sled-agent] Separate CockroachDB "start" from CockroachDB "init" Apr 30, 2023

smklein added Sled Agent Related to the Per-Sled Configuration and Management storage Related to storage. labels Apr 30, 2023

smklein added 4 commits April 30, 2023 02:24

[RSS] Explicit set of Bootstrap Agents

ec3b1e4

Merge branch 'storage-manager-cleanup' into rss-explicit

c812609

Merge branch 'rss-explicit' into cockroach-init

cbfb8c8

smklein changed the base branch from storage-manager-cleanup to rss-explicit April 30, 2023 06:27

smklein force-pushed the rss-explicit branch from 2b6c7cd to c812609 Compare April 30, 2023 06:57

smklein added 6 commits April 30, 2023 09:19

Fix tests

9d00c93

Merge branch 'storage-manager-cleanup' into rss-explicit

8a08090

Merge branch 'rss-explicit' into cockroach-init

7155257

make serialization happier

cfb7cbc

Merge branch 'rss-explicit' into cockroach-init

6346ca7

Improve parsing for toml, openapi

c37e57e

davepacheco added 3 commits June 26, 2023 15:43

fix property builder for multi-valued properties

c942e53

trust-dns docs sent me down the wrong path

d283479

move the point where we do DNS resolution

ba8820c

davepacheco self-assigned this Jun 27, 2023

davepacheco added this to the FCS milestone Jun 27, 2023

davepacheco added 7 commits June 27, 2023 14:27

fix up comments

05fef51

WIP (untested): use Rust program to resolve DNS

3c54223

clippy nits

4a8fe18

fix path

1406d7f

dnswait should report hostnames

967f03d

should be comma-separated

3c0e157

davepacheco marked this pull request as ready for review June 28, 2023 00:17

Merge branch 'main' into dap/cockroach-init

3546cde

davepacheco mentioned this pull request Jun 28, 2023

Populate /etc for zones #2122

Open

2 tasks

smklein commented Jun 29, 2023

View reviewed changes

review feedback

8bcdbba

davepacheco enabled auto-merge (squash) June 29, 2023 00:29

Merge branch 'main' into dap/cockroach-init

cab937e

davepacheco approved these changes Jun 29, 2023

View reviewed changes

davepacheco disabled auto-merge June 29, 2023 00:32

davepacheco enabled auto-merge (squash) June 29, 2023 00:32

davepacheco added 2 commits June 28, 2023 18:51

fix Cargo.lock

b99b1a5

Merge branch 'main' into dap/cockroach-init

97ab92f

davepacheco merged commit b884c76 into main Jun 29, 2023

davepacheco deleted the cockroach-init branch June 29, 2023 04:37

davepacheco mentioned this pull request Jul 5, 2023

Sled Agent POST /cockroachdb is not idempotent #3499

Open

davepacheco mentioned this pull request Apr 30, 2024

Nexus holds on to specific service backends of crdb and crucible-pantry for too long #3763

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sled-agent] Separate CockroachDB "start" from CockroachDB "init" #2954

[sled-agent] Separate CockroachDB "start" from CockroachDB "init" #2954

smklein commented Apr 30, 2023 •

edited

Loading

davepacheco commented Jun 27, 2023

smklein left a comment

smklein Jun 29, 2023

davepacheco Jun 29, 2023

smklein Jun 29, 2023

smklein Jun 29, 2023

smklein Jun 29, 2023

davepacheco Jun 29, 2023

smklein Jun 29, 2023

davepacheco Jun 29, 2023

smklein Jun 29, 2023

davepacheco commented Jun 29, 2023

	// If requested, format the cluster with the initial tables.
	if do_format {
	info!(log, "Formatting CRDB");
	zone.run_cmd(&[
	"/opt/oxide/cockroachdb/bin/cockroach",
	"sql",
	"--insecure",
	"--host",
	&dataset_info.address.to_string(),
	"--file",
	"/opt/oxide/cockroachdb/sql/dbwipe.sql",
	])?;
	zone.run_cmd(&[
	"/opt/oxide/cockroachdb/bin/cockroach",
	"sql",
	"--insecure",
	"--host",
	&dataset_info.address.to_string(),
	"--file",
	"/opt/oxide/cockroachdb/sql/dbinit.sql",
	])?;
	info!(log, "Formatting CRDB - Completed");
	}

	// we configuring a property group on the instance.t by default.
	// we configuring a property group on the instance by default.

		@@ -0,0 +1,98 @@
		// This Source Code Form is subject to the terms of the Mozilla Public

[sled-agent] Separate CockroachDB "start" from CockroachDB "init" #2954

[sled-agent] Separate CockroachDB "start" from CockroachDB "init" #2954

Conversation

smklein commented Apr 30, 2023 • edited Loading

History

This PR

davepacheco commented Jun 27, 2023

smklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco commented Jun 29, 2023

smklein commented Apr 30, 2023 •

edited

Loading