[WIP] [reconfigurator] start removing expunged zones by sunshowers · Pull Request #5211 · oxidecomputer/omicron

sunshowers · 2024-03-07T00:40:31Z

Putting this up for early review. This implements internal bookkeeping for the blueprint builder, as well as database storage for expunged Nexus zones.

This does not implement any of the other things we discussed in Tuesday's meeting -- let's discuss this tomorrow.

Created using spr 1.3.6-beta.1

jgallagher · 2024-03-07T16:43:18Z

            sled_ip_allocators: BTreeMap::new(),
            zones: BlueprintZones::new(parent_blueprint),
            zones_in_service: parent_blueprint.zones_in_service.clone(),
+            expunged_nexus_zones: parent_blueprint.expunged_nexus_zones.clone(),


Happy to discuss this during the meeting later today, but just jotting down a couple of thoughts here:

Once a zone gets added to expunged_nexus_zones, how does the planner know when it's okay to remove it from a subsequent child blueprint?

Does this need to be Nexus zones specifically? Or could it be zones_expunged like zones_in_service? (I haven't thought about this much, but am assuming we're eventually going to want to track other kinds of zones that need execution work to be expunged.)

Happy to discuss this during the meeting later today, but just jotting down a couple of thoughts here:

Once a zone gets added to expunged_nexus_zones, how does the planner know when it's okay to remove it from a subsequent child blueprint?

I was wondering the same thing. Should there also be a 'decommissioned' column for when the zone has been completely removed?

Does this need to be Nexus zones specifically? Or could it be zones_expunged like zones_in_service? (I haven't thought about this much, but am assuming we're eventually going to want to track other kinds of zones that need execution work to be expunged.)

I was also thinking the same thing. It seems somewhat silly to have a separate table per zone type unless each of those tables would have different rows because they store other "checkpoint" information while decommissioning a zone. I don't think we were planning to do this inside the zone table, but I'm not sure.

The latter we've taken into account by switching to a disposition enum, which we'll store inside bp_omicron_zone.

davepacheco · 2024-03-07T17:53:08Z

            .sleds
            .keys()
            .map(|sled_id| {
+                // XXX: do we need to handle, or check for, initially expunged


I would say no. The only thing we could do here is mark zones expunged if we find them on sleds that are expunged. But that's more of the planner's job. The idea behind the initial blueprint is that if you executed it, it wouldn't do anything. (That's why this is in BlueprintBuilder, which is not supposed to be that smart, and not Planner.)

davepacheco

Nice. (I know this is still a WIP.)

davepacheco · 2024-03-07T17:55:14Z

+        let sled_info = self.sled_resources(sled_id)?;
+
+        match sled_info.state {
+            SledState::Active => {}
+            SledState::Decommissioned => {
+                return Err(Error::InvalidEnsure {
+                    zone_tag: ZoneTag::InternalNtp,
+                    sled_id,
+                    reason: InvalidEnsureReason::Decommissioned,
+                });
+            }
+        }
+
+        match sled_info.policy {
+            SledPolicy::InService { .. } => {
+                // Non-provisionable sleds should still get NTP zones.
+            }
+            SledPolicy::Expunged => {
+                return Err(Error::InvalidEnsure {
+                    zone_tag: ZoneTag::InternalNtp,
+                    sled_id,
+                    reason: InvalidEnsureReason::Expunged,
+                });
+            }
+        }
+


Seems like we could use a helper for this.

davepacheco · 2024-03-07T17:55:46Z

+                return Err(Error::InvalidEnsure {
+                    zone_tag: ZoneTag::InternalNtp,
+                    sled_id,
+                    reason: InvalidEnsureReason::Decommissioned,
+                });


I wonder if this and the other InvalidEnsure should be PlannerErrors.

davepacheco · 2024-03-07T17:58:03Z

+        // Check that all the zone IDs exist. It is an internal invariant
+        // violation for a zone to be in the list of current sled zones, but
+        // not in zones_in_service.


Why is that? Eventually we want to be able to have zones that exist but not in service, and it should be possible to expunge them I think.

Yes -- I was thinking about this last night and what I did here is wrong. Thanks for catching this.

davepacheco · 2024-03-07T18:03:30Z

+
+        // Also add the list of expunged Nexus zones to self.expunged_nexus_zones.
+        self.expunged_nexus_zones.extend(expunge.iter().filter_map(|z| {
+            (z.zone_type.tag() == ZoneTag::Nexus).then_some(z.id)


FYI, I think there's already a is_nexus(). (No reason to change this though.)

davepacheco · 2024-03-07T18:04:16Z

+    // Precondition: we must have checked that it is valid for the zone to be
+    // added to the sled. (TODO: actually check this here, or use the type
+    // system to statically verify this.)


Can you say more about this? What does "valid" mean? Does this panic or return an error or do something bad if this precondition is violated?

davepacheco · 2024-03-07T18:04:57Z

+    /// error if the sled ID wasn't found.
+    ///
+    /// Return true in `expunge_fn` to remove a zone, or false to retain it.
+    pub fn sled_expunge_zones(


Out of curiosity, why not just a sled_expunge_zone(sled_id, zone_id)?

davepacheco · 2024-03-07T18:06:30Z

+            // We could in principle have a decommissioned sled which has
+            // resources allocated to it -- if we do, then we should log this
+            // as an error because that is not a legal state, but remove those
+            // zones anyway.
+            match sled_info.state {
+                SledState::Active => {}
+                SledState::Decommissioned => {
+                    error!(
+                        &log,
+                        "sled has state Decommissioned, yet has resources allocated to it; \
+                         removing them here (sled policy is \"{}\")", sled_info.policy;
+                    );
+
+                    return Some("decommissioned");
+                }
+            }
+
+            match sled_info.policy {
+                SledPolicy::InService { .. } => None,
+                SledPolicy::Expunged => Some("expunged"),
+            }
+        }


Is it possible to have a sled in state "decommissioned" that's not got policy "expunged"? (If not, I think a lot of this could be simplified -- there'd only be one possible expunge reason and you'd only need to check one field to identify it.)

davepacheco · 2024-03-07T18:08:13Z

+            // Decommissioned and expunged sleds don't get any services. (These
+            // are explicit matches so that when more states are added, this
+            // fails to compile.)


Similarly I wonder if this could just look at "expunged" now. It feels like only the policy should determine how we plan resources for the sled.

…on enum next to each zone (#5238) This greatly simplifies the handling of the expunged disposition in #5211. It also is more understandable, and less prone to making mistakes. This PR only changes the in-memory representation of what is currently `zones_in_service`. I've skipped doing the DB migration in this PR because it'll be easier to just do it wholesale in #5211. There are some small differences in diff output that I think make sense. But I took a bigger swing at it in #5270, which depends on this.

Created using spr 1.3.6-beta.1

…#5413) Before we added the `display()` method, the way to get nicely formatted blueprints was to use the Debug impl. While working on #5211, I noticed that these callers were still using the old way to do things. Fix this up. I tested this by making the `Debug` impl for `Blueprint` panic, then saw which tests failed. I also realized that we didn't have tests for omdb's blueprints. I believe at the time it wasn't possible to write a test for this because we didn't have an initial blueprint -- but now we do, so include a test. (I also wanted to ensure that the actual blueprint ID was what we expected, so I expanded the scope of `redact_variable` a bit. Introduce `ExtraRedactions` so we can handle both fixed- and variable-length redactions, and use the same logic for UUIDs.)

sunshowers · 2024-04-05T02:11:47Z

Turns out it's too hard to make this rebase cleanly, so I'm going to start from scratch and copy over bits as they make sense.

sunshowers added 4 commits March 6, 2024 16:40

[𝘀𝗽𝗿] initial version

624f2ac

Created using spr 1.3.6-beta.1

Mostly done with first cut

72c64d7

Created using spr 1.3.6-beta.1

Database updates

63224bb

Created using spr 1.3.6-beta.1

Fix compile failures

867c8e7

Created using spr 1.3.6-beta.1

davepacheco assigned sunshowers Mar 7, 2024

jgallagher reviewed Mar 7, 2024

View reviewed changes

davepacheco reviewed Mar 7, 2024

View reviewed changes

This was referenced Mar 9, 2024

[reconfigurator] replace Blueprint::zones_in_service with a disposition enum next to each zone #5238

Merged

Add internal API and omdb command to expunge a sled #5234

Merged

sunshowers mentioned this pull request Mar 29, 2024

Add BlueprintZoneFilter to all_omicron_zones #5348

Merged

3 tasks

smklein mentioned this pull request Apr 2, 2024

[reconfigurator] Only provision to zpools with in-service physical disks #5375

Closed

sunshowers mentioned this pull request Apr 4, 2024

[nexus] fix up some uses of blueprints that were using the Debug impl #5413

Merged

rebase, doesn't actually work yet

f377e2c

Created using spr 1.3.6-beta.1

sunshowers changed the base branch from main to sunshowers/spr/main.wip-reconfigurator-start-removing-expunged-zones April 4, 2024 23:35

try again? oh github

915e18f

Created using spr 1.3.6-beta.1

sunshowers closed this Apr 5, 2024

Uh oh!

Conversation

sunshowers commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunshowers Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunshowers commented Apr 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sunshowers commented Mar 7, 2024 •

edited

Loading

sunshowers Apr 2, 2024 •

edited

Loading