Deleting things that FM creates during execution

[smklein]

The Task

Currently, our FM system creates sitreps, and later in an execution phase, does a bunch of work in a background rendezvous task.

This task is responsible for creating and updating database state ("Go create an alert", "Go create a support bundle"), and will also be responsible for updating SP and sled state ("go turn on these LEDs", "Go tell a host OS that we can drop a fault from fmdump").

It should also be the responsibility of the FM system and rendezvous task to delete objects which it creates.

The Problem

However, because distributed systems, this isn't as simple as "create the object if it's in the sitrep, delete if it isn't in the sitrep".

Sitrep execution happens within each Nexus, and each Nexus can be executing an arbitrarily-old sitrep.

Suppose we have the following scenario:

• Nexus A, executing sitrep 100: "Create alert A"
• Nexus B, executing sitrep 101: "Delete alert A"

If these executed sequentially: great, everything works. However, because these are different processes, running on different machines, we could have the ordering of:

• Nexus A executes sitrep 100 (creates A)
• Nexus B executes sitrep 101 (deletes A)
• Nexus A executes sitrep 100 again - A is re-created as a zombie alert. This would be bad!

The Fix

There are several, with various pros/cons

Just use a transaction

Basically: Only do the object creation if the "sitrep we're executing is still the target sitrep"
Pros: Simple
Cons: High contention. Can block new sitrep insertion behind execution.

Tombstones

If we delete an object from the DB, leave behind a tombstone indicating it was removed, to prevent it from being re-created.
Pros: Simple, low-contention, straightforward
Cons: When can we remove the tombstones?

Generation numbers

When updating a database row - like "fm_alert" - create an auxiliary table which is called something like "fm_alert_metadata". This table should contain the generation number of the sitrep (or an equivalent "only increasing" generation for the set of alerts emitted by that sitrep).

When creating alerts, we only proceed "if the on-disk generation is less than or equal to that of our currently-executing sitrep". Otherwise, we know we're trying to do old work, and bail out immediately. The "check generation + create" operations should happen transactionally. Once we've finished with the "creation" steps, we could update the on-disk generation number.

Pros: Low contention.
Cons: Adds boilerplate, a little tricky to get right with pagination

[hawkw]

Per our discussion earlier, I felt like the metadata table felt like the nicest solution: we couldn't really nicely come up with a situation under which tombstones should be deleted, and the tables we are dealing with here are (unlike the sitrep tables) shared with other subsystems and it seems a shame to have to lock them when other parts of the system are also going to be trying to touch them unrelatedly.

[hawkw]

This task is responsible for creating and updating database state ("Go create an alert", "Go create a support bundle"), and will also be responsible for updating SP and sled state ("go turn on these LEDs", "Go tell a host OS that we can drop a fault from fmdump").

As a teensy, insignificant nit, I was sort of imagining that the non-database-y execution bits would have its own background task (which is why I named the one we have "fm_rendezvous" rather than "fm_execution" or similar). In hindsight, I'm not actually sure if there's really any big reason for these to be separate tasks, but... 🤷‍♀️

[mergeconflict]

I think there's something tacitly implied in this issue: just as we have request_alert and request_support_bundle, which add an AlertRequest and a SupportBundleRequest to a case, it seems like we maybe also need some sort of explicit delete_alert and delete_support_bundle. I'm not sure when a DE would decide it wants to do this though...

Do we just want the deletion to be implicitly triggered when the active sitrep no longer contains the requested alert / support bundle? Do we want it to happen later, when the case is closed (or GC'ed)?

[hawkw]

I think there's something tacitly implied in this issue: just as we have request_alert and request_support_bundle, which add an AlertRequest and a SupportBundleRequest to a case, it seems like we maybe also need some sort of explicit delete_alert and delete_support_bundle. I'm not sure when a DE would decide it wants to do this though...

Do we just want the deletion to be implicitly triggered when the active sitrep no longer contains the requested alert / support bundle? Do we want it to happen later, when the case is closed (or GC'ed)?

In both of these cases, it's not the DEs that are deleting these objects, so I don't think that we need a mechanism for it to request that they be deleted. Instead, we expect that:

• support bundles can be explicitly deleted by an operator
• support bundles may be automatically GCed
• alerts which have been delivered successfully (or to which no receivers were subscribed) will be automatically GCed

The goal here is not to give the diagnosis engine a way to delete these objects, but to ensure that when they are deleted by other mechanisms, the FM rendezvous task does not re-create them.

[mergeconflict]

The goal here is not to give the diagnosis engine a way to delete these objects, but to ensure that when they are deleted by other mechanisms, the FM rendezvous task does not re-create them.

This makes sense! But @smklein wrote in the issue description:

It should also be the responsibility of the FM system and rendezvous task to delete objects which it creates.

... so I think you might be picturing different things?

[hawkw]

@mergeconflict I don't think these are necessarily at odds. My point is that the diagnosis engines are probably not going to be requesting that objects be deleted. It's plausible that the rendezvous task will be responsible for driving at least some deletions, especially for objects that aren't deleted by an external mechanism. But, I think that we should be starting with the concrete use case of "FM creates a support bundle for some case, and then later, an operator, who has downloaded the support bundle, manually deletes it via the API. How do we ensure that FM rendezvous doesn't immediately go and create a new support bundle?"

[mergeconflict]

I am struggling with this!

Sean describes this sequence:

• Nexus A executes sitrep 100 (creates A)
• Nexus B executes sitrep 101 (deletes A)
• Nexus A executes sitrep 100 again - A is re-created as a zombie alert. This would be bad!

But I think Eliza is describing a different situation, in which alerts or support bundles (I'll just call these "resources") are deleted out-of-band by a user/operator or by their own GC process. So there's a different sequence we also need to be worried about, where the sitrep doesn't even have to be stale:

• Nexus A executes sitrep 100, creates resource
• Operator hard-deletes the resource
• Nexus B also executes sitrep 100, creates the resource again

I believe there are a couple cases where this would happen. The first is when fm_rendezvous is triggered by a new sitrep:

omicron/nexus/src/app/background/init.rs

Lines 1180 to 1181 in 979bc71

	watchers: vec![Box::new(sitrep_watcher.clone())],
	activator: task_fm_rendezvous,

All Nexus instances race to execute the sitrep; this is meant to be idempotent — that is, we don't create duplicate resources — but it's possible (albeit unlikely) that one instance will be slow enough that the above sequence could occur. The second, and probably more likely case where it could happen is on periodic execution:

omicron/nexus/src/app/background/init.rs

Line 1171 in 979bc71

period: config.fm.rendezvous_period_secs,

The fm_rendezvous task executes every five minutes. As long as the previous sitrep is still current, and hasn't been superseded, any resources that were deleted in the intervening time will be periodically recreated. So I think the proposal to gate resource creation on "generation is less than or equal to that of our currently-executing sitrep" isn't sufficient to guard against this case. I think a transaction wouldn't do it either. I think we need tombstones.

Here's my strawman proposal for how GC'ing tombstones would work: we have a fm_case_id column on support bundles, and a case_id field on alerts. These resources can be hard-deleted when there is no longer any matching row in the fm_case table. In other words, when we're deleting old sitreps (#9384) we can query for any resources that reference their cases and clean up anything we find dangling. Would this work?