Adding automatic bundle on zone death #3829

bnaecker · 2023-08-07T23:38:52Z

Moves zone bundle code to free functions in its own module, out of the ServiceManager itself.
Adds handling of Propolis zones, by reworking the locking around the instance manager.
Adds sled-agent endpoint for listing all zone bundles, even those not corresponding to an existing zone.
Adds a "cause" to the zone bundle metadata, indicating why it was created.
Some QoL improvements to zone-bundle, allowing listing bundles from zones matching a filter (or all), along with parseable output.
Improves robustness of extracting GATEWAY_MAC from the ARP entries for the provided GATEWAY_IP, and adds warning if the proxy-arp entries are not provided.
Extracts log files which may have been archived to a U.2 as well as the M.2-local log files
Adds basic mechanism for running zone-specific commands. Not used yet.

- Moves zone bundle code to free functions in its own module, out of the `ServiceManager` itself. - Adds handling of Propolis zones, by reworking the locking around the instance manager. - Adds sled-agent endpoint for listing all zone bundles, even those not corresponding to an existing zone. - Adds a "cause" to the zone bundle metadata, indicating why it was created. - Some QoL improvements to `zone-bundle`, allowing listing bundles from zones matching a filter (or all), along with parseable output. - Improves robustness of extracting `GATEWAY_MAC` from the ARP entries for the provided `GATEWAY_IP`, and adds warning if the proxy-arp entries are not provided. - Extracts log files which may have been archived to a U.2 as well as the M.2-local log files - Adds basic mechanism for running zone-specific commands. Not used yet.

bnaecker · 2023-08-07T23:46:35Z

This is part of #1598. It adds support for zone bundles to Propolis instances, and also automatically creates bundles for zones before they are destroyed. This took much longer than I anticipated, mostly due to the very awkward interface between the sled-agent and the underlying instances. The main sticking point is that the lock around the full BTreeMap of instances was a std::sync::Mutex. while the lock around the instances themselves is tokio::sync::Mutex. (The zone bundling code is also async.) It's nearly impossible to hold a sync lock across an await point, so this required moving the instance map lock to an async one, which had a bunch of other fallout.

The other issue I hit was part of #3713. That moves rotated log files out off the M.2s and to some arbitrary U.2. I wasn't expecting that, and it's an example of the unintended coupling I was worried about in the writeup in #3388. Now the bundling logic is aware of that archival, and needs to go search the possible archive locations.

Anyway, that's a lot of implementation detail, but here's some high-level tests of the whole thing running now:

bnaecker@shale : ~/omicron $ ./target/release/zone-bundle --host fd00:1122:3344:101::1 ls
Zone                                                             Bundle ID                            Created
oxz_nexus_6a7b95ab-4195-44f3-a2a5-60064ce1587f                   650d86e9-e485-4582-b6be-66245b716765 2023-08-07 23:26:42.040415517 UTC
bnaecker@shale : ~/omicron $ ./target/release/zone-bundle --host fd00:1122:3344:101::1 list-zones
oxz_switch
oxz_internal_dns_d639d7a2-b290-499a-b09d-ba5c56ece97a
oxz_internal_dns_f97dc6be-e5c9-4f2b-a34d-cbf821c6e268
oxz_internal_dns_59171a80-b655-4201-a18a-2ae832b4f0a3
oxz_ntp_ef57a98d-159d-4fe2-a51b-9500c4bf8f29
oxz_cockroachdb_469567dc-8da6-44cd-8cc8-80a457691cca
oxz_cockroachdb_c3b6e935-124c-48a0-9576-9f13e76eaa61
oxz_cockroachdb_77134fbe-0e91-48a8-b54f-16a21cbefca7
oxz_cockroachdb_e8b944ee-39da-4e36-adfc-bf78cc2c21ff
oxz_cockroachdb_9a30f4d0-e7b6-4c24-a5c8-776e5ac451f1
oxz_crucible_936ff2c8-3648-428a-9d23-fea637916015
oxz_crucible_91e6cb07-887a-4257-8fff-8ddc5cf981aa
oxz_oximeter_5b36dff8-842d-4613-8e5e-3d677b370bc6
oxz_crucible_8b87ae1c-a99f-482c-bad8-48c98476cd04
oxz_crucible_e9e0fbae-fdbe-45f2-925e-adf217d46bcf
oxz_nexus_104852c6-06fb-4c1d-b78e-5742145acad1
oxz_crucible_pantry_d507ac9d-53b2-495a-a296-64da73107d80
oxz_crucible_pantry_a4ee219f-e4ad-45ca-9544-a0bd6b0cf1fe
oxz_crucible_pantry_4a2e059b-4826-48f8-ba34-e74f78772494
oxz_clickhouse_1fde5647-1a1e-42b1-8f2a-5b3191cb8284
oxz_external_dns_6193ec54-556f-4f31-940a-5d045337299c
oxz_external_dns_5317b71f-f649-48a0-960c-edce64f38de4
oxz_crucible_727f70ec-fd40-413c-b552-afe2b72ed22c
oxz_nexus_30570377-8dfd-4ae2-8c83-4bfd2dc32ba4
oxz_nexus_6a7b95ab-4195-44f3-a2a5-60064ce1587f
oxz_propolis-server_84dafabe-4ef3-49c9-8fce-4820d3dec68a
bnaecker@shale : ~/omicron $ ./target/release/zone-bundle --host fd00:1122:3344:101::1 create oxz_propolis-server_84dafabe-4ef3-49c9-8fce-4820d3dec68a
Created zone bundle: oxz_propolis-server_84dafabe-4ef3-49c9-8fce-4820d3dec68a/421b401b-cd8c-42eb-b73b-88d6bb4c573c
bnaecker@shale : ~/omicron $ ./target/release/zone-bundle --host fd00:1122:3344:101::1 ls
Zone                                                             Bundle ID                            Created
oxz_nexus_6a7b95ab-4195-44f3-a2a5-60064ce1587f                   650d86e9-e485-4582-b6be-66245b716765 2023-08-07 23:26:42.040415517 UTC
oxz_propolis-server_84dafabe-4ef3-49c9-8fce-4820d3dec68a         421b401b-cd8c-42eb-b73b-88d6bb4c573c 2023-08-07 23:29:09.291215570 UTC
bnaecker@shale : ~/omicron $ ../oxide.rs/target/release/oxide instance stop --project foo --instance inst0
success
Instance {
    description: "test instance",
    hostname: "inst0",
    id: 22090927-bcbd-43c2-b06f-0d77c183f9d7,
    memory: ByteCount(
        1073741824,
    ),
    name: Name(
        "inst0",
    ),
    ncpus: InstanceCpuCount(
        2,
    ),
    project_id: 3f5ac354-91d0-4bed-a52a-e265cab4a850,
    run_state: Stopping,
    time_created: 2023-08-07T23:28:34.220437Z,
    time_modified: 2023-08-07T23:28:34.220437Z,
    time_run_state_updated: 2023-08-07T23:29:23.561350Z,
}
bnaecker@shale : ~/omicron $ ./target/release/zone-bundle --host fd00:1122:3344:101::1 ls -o zone-name,bundle-id,cause
Zone                                                             Bundle ID                            Cause
oxz_nexus_6a7b95ab-4195-44f3-a2a5-60064ce1587f                   650d86e9-e485-4582-b6be-66245b716765 ExplicitRequest
oxz_propolis-server_84dafabe-4ef3-49c9-8fce-4820d3dec68a         421b401b-cd8c-42eb-b73b-88d6bb4c573c ExplicitRequest
oxz_propolis-server_84dafabe-4ef3-49c9-8fce-4820d3dec68a         b7e02d88-ecc2-4a10-a173-baa95d615945 TerminatedInstance

This shows that:

There was one zone bundle originally, for a Nexus zone.
I created one manually for the existing Propolis zone managing my instance.
A new bundle was created automatically on zone death.
It's also good to note that the Propolis zone bundles are still listed and gettable, even though the zone itself is gone.

One side note: there's only one other time we actually destroy zones, which is when the sled-agent starts up and finds zones it didn't expect. I'm not really in a position to test that, though we do take bundles for those zones and mark them with a specific ZoneBundleCause::UnexpectedZone.

sled-agent/src/services.rs

sled-agent/src/instance.rs

smklein · 2023-08-08T00:02:57Z

sled-agent/src/instance.rs

+        if let Err(e) = zone_bundle::create(
+            &self.log,
+            &running_state.running_zone,
+            &context,
+        )
+        .await
+        {
+            error!(
+                self.log,
+                "Failed to take zone bundle for terminated instance";
+                "zone_name" => &zname,
+                "reason" => ?e,
+            );


So, on the one hand, doing this is great, because we'll capture a lot more info. On the other hand, it seems like it'll massively inflate the amount of traffic moving onto the M.2s (and U.2s, if I understand #3713 correctly).

Do we ever trim down the set of zone bundles? E.g., removing the oldest ones when we fill up too much space?

(honestly all other comments on this PR are nits, this is the one that IMO matters most)

Other than calls to zone-bundle delete, we do not ever remove bundles. That's an in-progress PR I'll be putting up next. I agree it's critical -- I debated putting it in this PR, but it was already borderline in terms of size for a review.

Also, an argument could be made for not taking automatic instance zone bundles here until the cleanup stuff is in place. I think enough folks have lost the race against the zone-cleanup machinery when an instance dies that it would be valuable. But it's certainly not a clear-cut decision.

Yup, we can land them separately, but I wouldn't want to ship this PR to customers without that follow-up, since that'll make us hit our resource limits with no reprieve.

However, if that's already on your radar, I'm okay punting it -- as long as we're on the same page about urgency.

Sounds good, it's next up on my queue. I'll get it out in the next day or two, hopefully.

smklein · 2023-08-08T00:52:25Z

tools/create_virtual_hardware.sh

@@ -59,7 +64,26 @@ function ensure_softnpu_zone {
    success "softnpu zone exists"
 }

+function warn_if_physical_link_and_no_proxy_arp {


These warning seem great, but should they be a separate PR?

Sure, they can be. I definitely ran into them while testing this, but I can split them out as well, no problem.

smklein · 2023-08-08T00:52:44Z

tools/scrimlet/softnpu-init.sh

@@ -27,7 +27,24 @@ fi

 #   $ arp -a | grep 192.168.21.1
 #   e1000g1 192.168.21.1         255.255.255.255          90:ec:77:2e:70:27
-GATEWAY_MAC=${GATEWAY_MAC:=$(arp -a | grep "$GATEWAY_IP" | awk -F ' ' '{print $NF}')}
+#
+# Add an extrac space at the end of the search pattern passed to `grep`, so that


Suggested change

# Add an extrac space at the end of the search pattern passed to `grep`, so that

# Add an extra space at the end of the search pattern passed to `grep`, so that

But see comment above -- should this be a separate PR?

gjcolombo

I still need to look at zone_bundle.rs in more detail but had one comment about the instance manager locking that I wanted to get down before paging things out for the day.

gjcolombo · 2023-08-08T03:44:19Z

sled-agent/src/instance_manager.rs

+        name: &str,
+    ) -> Result<ZoneBundleMetadata, BundleError> {
+        for (_propolis_id, instance) in
+            self.inner.instances.lock().await.values()


Since the Propolis ID is part of the value in the map, I wonder if we could instead do something like this:

if let Some(instance) = self.inner .instances .lock() .unwrap() .values() .find(|(pid, _)| name == propolis_zone_name(pid)) .cloned() { match instance.request_zone_bundle(name).await { // we already know the name matches InstanceZoneBundleResult::NotMe => unreachable!(), // other cases as before } }

Cloning the Instance is OK because it's just a wrapper around an Arc that contains the actual InstanceInner. The other instance manager routines do basically this to get around the sync/async mismatch; the difference is that they're looking up instances directly by key instead of trying to look for matching values, so they can call get whereas this code will have to find.

I think this allows you not to hold the instance manager lock while taking the bundle, which (if I'm understanding everything else correctly) should be enough to avoid having to turn that lock into a Tokio mutex. (It also saves us from holding a big lock over a possibly-long-running file copy operation.)

That's a great suggestion, thanks Greg. All the contortions were definitely predicated on not cloning, and so needing to hold both locks. That's not great, and thanks to this idea, not needed!

I think this should also get rid of the InstanceZoneBundleResult, since the NotMe variant will be obviated by the call to find(), and we can report Unavailable as a BundleError directly.

Thanks!

So this exact construction doesn't quite work, because the lock on the instance map is still in scope inside the if-let. However! We can use let-else instead:

let Some((_propolis_id, instance)) = self .inner .instances .lock() .unwrap() .values() .find(|(propolis_id, _instance)| name == propolis_zone_name(propolis_id)) .cloned() else { return Err(BundleError::NoSuchZone { name: name.to_string() }); }; instance.request_zone_bundle().await

I really do love that syntax addition.

Rather than making the instance-map lock async, and holding it for the duration of the zone bundle operation, we keep it sync. Instead, clone the instance itself, which is pretty cheap since it's in an `Arc`. This keeps the code simpler and should improve overall throughput. Also makes a small change to the zone bundle filename for zone-specific commands, to ensure they don't collide.

sled-agent/src/instance.rs

bnaecker requested review from smklein and gjcolombo August 7, 2023 23:38

smklein reviewed Aug 8, 2023

View reviewed changes

gjcolombo reviewed Aug 8, 2023

View reviewed changes

bnaecker force-pushed the auto-bundle branch from fb7cc7a to fd9cedf Compare August 8, 2023 17:54

bnaecker requested review from smklein and gjcolombo August 8, 2023 17:54

smklein approved these changes Aug 8, 2023

View reviewed changes

gjcolombo approved these changes Aug 8, 2023

View reviewed changes

sled-agent/src/instance.rs Outdated Show resolved Hide resolved

Typo fix

6e31651

bnaecker enabled auto-merge (squash) August 8, 2023 21:43

bnaecker merged commit 9b1867b into main Aug 9, 2023

bnaecker deleted the auto-bundle branch August 9, 2023 02:11

smklein mentioned this pull request Aug 14, 2023

Adds background task which automatically removes zone bundles #3868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding automatic bundle on zone death #3829

Adding automatic bundle on zone death #3829

bnaecker commented Aug 7, 2023

bnaecker commented Aug 7, 2023 •

edited

Loading

smklein Aug 8, 2023

bnaecker Aug 8, 2023

bnaecker Aug 8, 2023

smklein Aug 8, 2023

bnaecker Aug 8, 2023

smklein Aug 8, 2023

bnaecker Aug 8, 2023

smklein Aug 8, 2023

gjcolombo left a comment •

edited

Loading

gjcolombo Aug 8, 2023

bnaecker Aug 8, 2023

bnaecker Aug 8, 2023

	# Add an extrac space at the end of the search pattern passed to `grep`, so that
	# Add an extra space at the end of the search pattern passed to `grep`, so that

Adding automatic bundle on zone death #3829

Adding automatic bundle on zone death #3829

Conversation

bnaecker commented Aug 7, 2023

bnaecker commented Aug 7, 2023 • edited Loading

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

gjcolombo left a comment • edited Loading

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

bnaecker commented Aug 7, 2023 •

edited

Loading

gjcolombo left a comment •

edited

Loading