Trust Quorum: Handle prepare messages + Alarms #8062
Conversation
This builds on #8052. Node's now handle `PrepareMsg`s from coordinators. The coordinator proptest was updated to generate prepares from non-existent test only nodes and send them to the coordinator. Additionally, protocol invariant violations are now detected in a few cases and recorded to the `PersistentState`. This is for debugging and support purposes. The goal is to test the code well enough that we never actually see an alarm in production.
Instead we return them wherever they can arise. This has a couple of benefits. * Higher level code can stop accepting requests when it sees an alarm to prevent endless logging and raise an alert to Nexus. * After support resolves the issue there is not necessarily any reason to manually mutate persistent state, unless that was the cause of the alarm. This also allows more straightforward/rusty error handling. Persistent state is now only returned in success cases.
| use omicron_uuid_kinds::RackUuid; | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| /// A critical invariant violation that should never occur. |
There was a problem hiding this comment.
Alarm is a nice name for this, going to steal it
| if latest_prepare.config.epoch < epoch { | ||
| // We haven't seen this prepare yet, but Nexus thinks we have. | ||
| // This is essentially the same case as above. | ||
| let latest_seen_epoch = Some(latest_prepare.config.epoch); | ||
| let alarm = Alarm::MissingPrepare { epoch, latest_seen_epoch }; | ||
| error!(self.log, "{alarm}"); | ||
| return Err(alarm); | ||
| } | ||
|
|
||
| if latest_prepare.config.epoch > epoch { | ||
| // Only commit if we have a `PrepareMsg` and it's the latest |
There was a problem hiding this comment.
what if it's the same? (it's a no-op I guess?) worth noting explicitly in a comment for intrepid readers in between the blocks
| // | ||
| // This is a less serious error than other invariant violations | ||
| // since it can be recovered from. However, it is still worthy of an | ||
| // alarm, as the most likely case is a disk/ ledger failure. |
There was a problem hiding this comment.
| // alarm, as the most likely case is a disk/ ledger failure. | |
| // alarm, as the most likely case is a disk/ledger failure. |
Hmm, so I thought alarms were unrecoverable errors, but this is recoverable? do we need a categorization of alarms?
| if self.persistent_state.last_committed_epoch() == Some(epoch) { | ||
| info!( |
There was a problem hiding this comment.
So I was following along fine until now, but I'm having trouble wrapping my head around all the cases here.
I presume that the general invariant here is that last_committed_epoch < latest_prepare.epoch. What are all the cases here? Currently there's:
- epoch to commit < latest prepare (
OutOfOrderCommitalarm) - epoch to commit == latest prepare (good)
- epoch to commit > latest prepare (
MissingPreparealarm)
What if epoch to commit > latest prepare, but also it is an idempotent commit? That case seems fine at first glance but it would result in an alarm here I think?
Would it be worth writing this as an explicit match statement? (Maybe even abstracting a combined prepare/commit epoch comparison out into a function?)
| if msg_last_committed_epoch != last_committed_epoch { | ||
| // If the msg contains an older last_committed_epoch than what | ||
| // we have, then out of order commits have occurred, as we know | ||
| // this prepare is later than what we've seen. This is a critical | ||
| // protocol invariant that has been violated. | ||
| // | ||
| // If the msg contains a newer last_committed_epoch than what | ||
| // we have, then we have likely missed a commit and are behind | ||
| // by more than one reconfiguration. The protocol currently does | ||
| // not allow this. Future protocol implementations may provide a | ||
| // capability to "jump" configurations. | ||
| // |
There was a problem hiding this comment.
worth modelling as separate enum variants similar to MissingPrepare/OutOfOrderCommit above?
| // Idempotent request | ||
| if msg.config == latest_prepare.config { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
worth noting that just calling Eq is fine here because there's no ancillary local-only data attached to this config -- this is deserialized data.
But also is it worth checking that the raw bytes are the same via e.g. a digest? Or do we consider byte sequences that deserialize to the same value to be the same
| let coordinating_epoch = cs.reconfigure_msg().epoch(); | ||
| if coordinating_epoch > msg.config.epoch { | ||
| warn!(self.log, "Received stale prepare while coordinating"; | ||
| "from" => %from, | ||
| "msg_epoch" => %msg.config.epoch, | ||
| "epoch" => %cs.reconfigure_msg().epoch() | ||
| ); | ||
| return Ok(None); | ||
| } | ||
| if coordinating_epoch == msg.config.epoch { | ||
| let alarm = Alarm::DifferentNodesCoordinatingSameEpoch { | ||
| epoch: coordinating_epoch, | ||
| them: from, | ||
| us: self.platform_id.clone(), | ||
| }; | ||
| error!(self.log, "{alarm}"); | ||
| return Err(alarm); | ||
| } |
There was a problem hiding this comment.
apologies, having trouble following this too. again, it seems like there are 3-4 epochs at play here, and it's not clear to me what the relationship is among all of them. Maybe just an ASCII diagram at the top of the file/function would help.
| // to check for an alarm and pull it out. We could also return either | ||
| // an `Alarm` or a `ReconfigurationError` inside `Result::Err`. This is | ||
| // probably the best approach, but I'm open to other structures. |
There was a problem hiding this comment.
Tricky! Yeah I'd probably return an error enum with Alarm and ReconfigurationError as variants, and maybe even consider not implementing std::error::Error or fmt::Display to ensure that people don't accidentally convert it to anyhow::Error.
This builds on #8052.
Nodes now handle
PrepareMsgs from coordinators. The coordinator proptest was updated to generate prepares from non-existent test only nodes and send them to the coordinator.Additionally, protocol invariant violations are now detected in a few cases and recorded to the
PersistentState. This is for debugging and support purposes. The goal is to test the code well enough that we never actually see an alarm in production.