numinnex commented on code in PR #3942:
URL: https://github.com/apache/iggy/pull/3942#discussion_r3861341222
##########
core/metadata/src/impls/metadata.rs:
##########
@@ -1170,21 +1175,48 @@ where
// guard, not here.
self.checkpoint_if_needed(consensus, journal).await;
- // Backup: gap check (op == current_op + 1).
- // Primary: sequencer pre-advanced by push_prepare_entry (guards
- // sibling on_request races during journal.append await).
- // TODO: promote the backup gap warn below to a hard assert or a
- // repair-session trigger (message repair has landed; the drop-and-
- // wait-for-retransmit path is the last soft handling left here).
+ // Backup: gap check against the JOURNAL head, not the sequencer.
+ //
+ // The two frontiers can disagree. The sequencer is pre-advanced on the
+ // primary by `push_prepare_entry` and re-synced on a backup only
after a
+ // successful append, so a replica can carry a sequencer one ahead of
what
+ // its WAL holds. Gating admission on it then rejects the very prepare
that
+ // would heal the log: a backup with `last_op = 44` refused op 45
because
+ // its sequencer said to expect 46. The primary retransmits that op
for the
+ // life of the process, every backup logs an out-of-order gap, it never
+ // reaches a commit quorum, and its client is never answered.
+ //
+ // `max(last_op, snapshot_op)`, never `last_op` alone. A state transfer
+ // installs a snapshot that IS ops `..=snapshot_op` applied and
truncates the
+ // WAL above that floor rather than refilling below it, so `last_op`
reads the
+ // receiver as needing an op the snapshot already contains and no peer
will
+ // send again. That drop
+ // never heals: an offer built on a quiet cluster carries `commit_op ==
+ // snapshot_seq`, so the install lands `commit_min == commit_max`, and
+ // `maybe_request_metadata_repair`, the only path that refills the
head,
+ // arms on `commit_min < commit_max`. With the other backup down the
primary
+ // needs this replica's ack to commit anything, so the plane stops on a
+ // cluster still inside its quorum.
+ //
+ // The journal is the only frontier that answers "what can be appended
+ // next", which is what this check is for, and the hash-chain
verification
+ // below is stated against it too. A prepare at or below the head that
this
+ // replica already holds was re-acked and returned above. What reaches
HERE
+ // is the next op or a gap, and not every gap is fillable: metadata
repair
+ // covers only `commit_min + 1 ..= commit_max`, so an interior hole
below
+ // the head and a forward gap above `commit_max` both sit outside it.
let is_backup = consensus.is_follower();
if is_backup {
- if header.op != current_op + 1 {
+ let handle = journal.handle();
+ let journal_head =
handle.last_op().unwrap_or(0).max(handle.snapshot_op());
+ if header.op != journal_head + 1 {
Review Comment:
This drop arms no repair, and the PR's own fault config wedges behind it.
Repro: `workload-fuzz --plane metadata --crash-prob 0.01 --restart-prob 0.05
--ticks 30000 --seed 1` (also seeds 2 and 5) fails `did not drain`.
Shape: a restarted backup adopts StartView at head N and arms repair up to
N; live prepares N+1.. arrive during the repair and are dropped here. The other
backup crashes, so `commit_max` is pinned (the primary needs this ack) and
`on_commit` never sees `CommitOutcome::Advanced` to re-arm. The stalled-repair
retry at `core/shard/src/lib.rs:5077` has no else branch for `from_op > to_op`,
so the finished session is never cleared and the `is_none()` gate at `:5379`
blocks a new one. The primary retransmits only `commit_max + 1`, which lands
here forever. 2/3 live, zero progress.
Pre-existing (the old sequencer gate drops the same op), so not a defect of
this PR, but the advertised config is red on roughly a third of seeds. Worth an
issue and a mention in the PR body. Fix looks small: clear the session when
`from_op > to_op`, and level-trigger the arm in `tick_metadata` when
`commit_min < commit_max` with no session.
##########
core/simulator/src/workload/oracle.rs:
##########
@@ -117,29 +139,227 @@ pub fn drive_to_quiesce(sim: &mut Simulator, workload:
&mut Workload, max_ticks:
true
}
-/// Post-drain consensus checks that hold today.
+/// Why the run did not drain, as a multi-line report.
///
-/// Asserts no live replica is ahead of the leader, and (on a serial run) that
-/// the shadow equals the metadata committed on the leader. See the module docs
-/// for why full cross-replica equality is deferred.
+/// A failed drain is either a wedge or a merely slow cluster, and the bare
boolean
+/// [`drive_to_quiesce`] returns cannot tell them apart. With crashes,
restarts and
+/// packet loss in play that distinction is the whole diagnosis, so name what
is
+/// outstanding and what every live replica believes.
+#[must_use]
+pub fn quiesce_failure_report(sim: &Simulator, workload: &Workload) -> String {
+ let mut report = format!(
+ "did not drain: {} request(s) still outstanding (seed={:#x})\n",
+ workload.total_in_flight(),
+ workload.options.seed,
+ );
+ for row in workload.outstanding_summary() {
+ let _ = writeln!(
+ report,
+ " outstanding client={} request={} action={:?}
last_target=replica {} \
+ attempts={}",
+ row.client, row.request, row.action, row.target, row.attempts,
+ );
+ }
+ let _ = writeln!(report, " resends issued: {}", workload.resends());
+ for replica_idx in 0..sim.replica_count {
+ if sim.is_crashed(replica_idx) {
+ let _ = writeln!(report, " replica {replica_idx}: CRASHED");
+ continue;
+ }
+ let _ = write!(report, " replica {replica_idx}: live");
+ // Metadata plane first: a rejoining replica is quorum-invisible until
its
+ // view probe completes, so its status separates "the cluster is slow"
from
+ // "the cluster has no quorum despite enough live replicas".
+ if let Some(consensus) =
sim.replicas[usize::from(replica_idx)].shards[0]
+ .plane
+ .metadata()
+ .consensus
+ .as_ref()
+ {
+ // The three ways `is_caught_up_primary` stays shut on what
otherwise
+ // reads as a healthy Normal primary, silently dropping every
request.
+ let _ = write!(
+ report,
+ " | metadata status={:?} view={} log_view={} commit={}..{}
barrier={} \
+ transferring={} primary={}",
+ consensus.status(),
+ consensus.view(),
+ consensus.log_view(),
+ consensus.commit_min(),
+ consensus.commit_max(),
Review Comment:
Consider printing the metadata repair session here too (armed or not,
`from_op..to_op`). On the seed-1 wedge this shows `commit=1278..1300` for the
stuck backup, and the session state is what points at the shard retry path
rather than the gap drop.
##########
core/simulator/src/lib.rs:
##########
@@ -2078,909 +2015,1228 @@ mod tests {
sim.init_partition(ns_a);
sim.register_client_with_primary(&client);
- // Phase 1: Create-heavy to populate the shadow.
- let mut options = WorkloadOptions::new(0x5EED_0002, replica_count,
vec![ns_a]);
- options.weights = ActionWeights::new(&[(Action::CreateStream, 100)]);
+ let mut options = WorkloadOptions::new(seed, replica_count,
vec![ns_a]);
+ options.weights = ActionWeights::partition_only();
let mut wl = Workload::new(options);
- for _tick in 0..3_000u32 {
- if let Some((target, msg)) = wl.build_request(&client) {
- sim.submit_request(client.client_id(), target,
msg.into_generic());
- }
- for reply in sim.step() {
- let cmds = wl.on_reply(&reply);
- apply_sim_commands(&mut sim, &cmds);
- }
- }
- let created =
wl.auditor.stats().commits_per_action[Action::CreateStream as usize];
- assert!(created > 0, "Create-only workload produced no commits");
- assert_eq!(
- wl.shadow.stream_names.len() as u64,
- created,
- "shadow stream count diverged from CreateStream commits"
- );
- // Phase 2: Create/Delete mix. DeleteStream sample succeeds only if
- // shadow.pick_stream_name returns Some (the wiring under test).
- wl.options.weights =
- ActionWeights::new(&[(Action::CreateStream, 30),
(Action::DeleteStream, 70)]);
- for _tick in 0..3_000u32 {
- if let Some((target, msg)) = wl.build_request(&client) {
- sim.submit_request(client.client_id(), target,
msg.into_generic());
- }
- for reply in sim.step() {
- let cmds = wl.on_reply(&reply);
- apply_sim_commands(&mut sim, &cmds);
- }
- }
- let deleted =
wl.auditor.stats().commits_per_action[Action::DeleteStream as usize];
- let created_total =
wl.auditor.stats().commits_per_action[Action::CreateStream as usize];
+ let clients = [client];
+ let replies = workload::run(&mut sim, &mut wl, &clients, 3_000,
u64::MAX);
+ assert!(replies > 0, "lossy workload produced no replies");
assert!(
- deleted > 0,
- "DeleteStream never committed; shadow-driven sampling is broken \
- (sample would return None unless pick_stream_name found a live
name)"
+ wl.resends() > 0,
+ "no request timed out at 5% packet loss, so the resend path never
ran; \
+ raise the loss rate or lower request_timeout_ticks"
);
- let expected_live = created_total.saturating_sub(deleted);
- assert_eq!(
- wl.shadow.stream_names.len() as u64,
- expected_live,
- "shadow.stream_names.len() ({}) != creates ({}) - deletes ({}) =
{}",
- wl.shadow.stream_names.len(),
- created_total,
- deleted,
- expected_live,
+
+ assert!(
+ oracle::drive_to_quiesce(&mut sim, &mut wl, 20_000),
+ "{}",
+ oracle::quiesce_failure_report(&sim, &wl),
);
+ oracle::assert_converged(&sim, &mut wl);
}
- /// Drive workload with 4 concurrent clients over two namespaces; assert:
- ///
- /// - Every client observes at least one commit (no starvation).
- /// - Per-(client, namespace) commit-monotonic invariant holds.
- /// - Commits interleave across clients.
- #[test]
- fn multi_client_interleaves_commits() {
+ fn workload_hash_for_seed(seed: u64) -> u64 {
+ workload_hash(seed, 1).0
+ }
+
+ /// Reply-trace and executor-schedule hashes for a full workload run at
+ /// `shards_per_replica` shards. Shared by the single-shard locked
+ /// baseline and the multi-shard replay tests.
+ fn workload_hash(seed: u64, shards_per_replica: u16) -> (u64, u64) {
Review Comment:
`workload_hash` never touches `FaultInjector`, `run_with_faults` or
`resubmit_due`, so the new fault path has no same-seed replay test; an injector
drawing from `rand::random` would pass `cargo test -p simulator`.
Deleted without a replacement: both `*_solo_replica_restarts_*` tests (the
only asserts that restart replay rebuilds the STM and client table),
`prepare_timestamps_replay_with_seed`, `multi_shard_replay_is_deterministic`
(the schedule hash this fn returns is now discarded),
`shell_on_consensus_schedule_matches_shell_off`, and
`crash_injection_spares_primary_and_keeps_quorum` (`spare_primary` /
`min_survivors` are now unasserted). `SimSuperblock::set_yield_writes` is
orphaned by the same deletion.
Suggest restoring the solo-restart pair and adding one test that runs
`run_with_faults` twice on a seed and compares reply hash, `schedule_hash()`,
crashes and restarts.
##########
core/simulator/src/lib.rs:
##########
@@ -854,7 +1145,11 @@ impl Simulator {
.collect();
materialised.sort_unstable_by_key(IggyNamespace::inner);
for namespace in materialised {
- materialise_partition(&self.replicas[idx], namespace);
+ materialise_partition(
Review Comment:
Ordering on restart: `new_shard` replays the WAL before this call seeds the
namespace, while a fresh boot seeds first (`:519`). `CreateStream::apply` takes
`vacant_key()` at apply time (`core/metadata/src/stm/stream.rs:1607`), so a
restarted replica ends up with `wl-*` streams at slabs 0-1 and the fillers
above, the reverse of its peers. Same committed log, different slab ids.
Nothing observes it today (the state checker compares headers, the entity
oracle filters by name), but a `DeleteStream` hitting the impersonating slab
would surface as a drain failure blamed on the cluster. Seeding before the
replay loop in `new_shard`, or journaling the seed, closes it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
