Skip to content

OCPBUGS-100303: podman-etcd: resolve force_new_cluster race during simultaneous dual-start - #2194

Merged
oalbrigt merged 2 commits into
ClusterLabs:mainfrom
Neilhamza:OCPBUGS-100303/fnc-race-resolution
Aug 10, 2026
Merged

OCPBUGS-100303: podman-etcd: resolve force_new_cluster race during simultaneous dual-start#2194
oalbrigt merged 2 commits into
ClusterLabs:mainfrom
Neilhamza:OCPBUGS-100303/fnc-race-resolution

Conversation

@Neilhamza

@Neilhamza Neilhamza commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Problem

When both TNF control-plane nodes restart simultaneously (dual power loss, certificate expiry), the podman-etcd resource agent can enter a permanent deadlock. Both nodes set the force_new_cluster CIB attribute due to two race vectors:

  1. CIB replication delay: Both nodes update their revision in CIB then call compare_revision(), which reads local disk for self but CIB for peer. If the peer's update hasn't propagated, both see stale peer revision and both determine they're "newer."

  2. Inconsistent start_resources_count: During simultaneous boot, OCF_RESKEY_CRM_meta_notify_start_resource can disagree between nodes, causing both to enter the case 1 path which sets force_new_cluster for any revision that isn't "older" (including "equal").

Once both hold force_new_cluster, the multi-holder safety check returns OCF_ERR_CONFIGURED -- a fatal error with no automatic retry -- causing total etcd outage requiring manual intervention.

Considered alternative

An atomic claim via cibadmin --create (create-fails-if-exists) would eliminate the verdict, guard, and stale-claim machinery in one stroke. However it sacrifices --lifetime reboot auto-cleanup and requires staleness machinery (boot-id, dead-claimant detection) with similar net complexity and higher migration risk for clusters already running with per-node FNC attributes.

Solution

This PR addresses OCPBUGS-100303 (force_new_cluster race during simultaneous dual-start). Fixing the bug correctly required addressing three additional aspects beyond the race itself: the fatal abort that prevents self-healing, the stale-claim cleanup for banned peers, and the active-peer guard that prevents split-brain.

The PR contains 2 commits:

  1. The entire fix: conflict verdict, safety guard, stale-claim cleanup, helpers.
  2. Test hooks only (separable -- drop if maintainers prefer keeping test instrumentation downstream).

Conflict verdict (fnc_conflict_verdict)

When both nodes hold FNC, the verdict function determines the outcome using CIB-sourced revision and cluster_id data (symmetric inputs -- both nodes evaluate the same data):

Verdict Condition Action
Winner Higher CIB revision, or only node with revision Winner force-new-clusters, loser yields
EQUAL Same revision AND same non-empty cluster_id Both clear FNC, joint normal start (no learner rebuild)
ADMIN Same revision but different/empty cluster_id, or no revision data Soft-fail with actual holder names in exit reason; never guess

Alphabetic node-name guessing was explicitly rejected -- when there is no data to distinguish the nodes, the agent reports the conflict and lets an administrator inspect the etcd data directories rather than risking a wrong choice.

Post-set safety guard

After the start-flow decision logic but before container creation, a one-shot guard sleeps 3s (load-bearing: lets a peer's in-flight FNC write propagate through attrd/CIB) then verifies this node is the sole visible holder. On any anomaly -- two holders (race), zero holders (own write not yet visible), or sole holder is the peer -- it soft-fails with OCF_ERR_GENERIC and lets Pacemaker retry. The early multi-holder check resolves the conflict on the retry with fresh notify vars.

Stale-claim cleanup

The data-backed winner in the early multi-holder check clears a stale peer FNC attribute when the peer's resource is neither starting, active, nor has an in-flight start in the CIB. This handles the banned-peer scenario where reboot-lifetime attributes persist until membership loss and no agent ever runs to yield. peer_has_pending_start queries the CIB status section for pending ops (accepts both exit code 105/CRM_EX_NOSUCH for Pacemaker 2.x and 6/ENXIO for legacy 1.x -- both verified empirically on Pacemaker 2.1.10; xpath positive-match against real pending start ops also confirmed). Query errors are treated as "pending" to prevent deletion during CIB instability.

Other design decisions

  • Active-peer guard checks for voter peers only (is_peer_active_voter) -- yielding to a learner-only peer would leave the cluster with no voter
  • All error paths return OCF_ERR_GENERIC (soft, retriable), never OCF_ERR_CONFIGURED (fatal)
  • Dual-claim always logged at err level -- even when auto-resolution succeeds, the evidence that a startup race occurred is preserved for post-mortem
  • fnc_yield clears standalone marker -- prevents stale is_standalone from re-triggering FNC on next restart
  • fnc_equal_clear_and_confirm re-checks after clearing -- handles CIB divergence where one node sees EQUAL while the other sees a winner

New helper functions

  • fnc_conflict_verdict: four-outcome verdict (winner/EQUAL/ADMIN)
  • fnc_equal_clear_and_confirm: clear-and-verify for EQUAL outcome
  • fnc_admin_escalate: exit reason with holder names and recovery command
  • fnc_yield: loser-side cleanup (FNC + standalone marker + JOIN_AS_LEARNER)
  • is_peer_active_voter: voter-only active-peer check
  • peer_has_pending_start: CIB status xpath for in-flight starts (exit codes 105 + 6)
  • is_numeric, get_cib_revision: POSIX-safe revision helpers

OCF_RESKEY_test_race_delay is an intentionally undeclared test hook for deterministic race reproduction (commit 2, separable). Both injection points are inert when the variable is unset.

What this PR touches beyond the race fix

  • set_force_new_cluster(): test hook added before the crm_attribute write
  • Early multi-holder check: OCF_ERR_CONFIGURED (fatal) replaced with verdict dispatch + stale-claim cleanup + OCF_ERR_GENERIC (retriable)
  • Single-holder-is-me path: active-peer voter guard added
  • Both start-path set_force_new_cluster call sites: return code now checked; on failure, return OCF_ERR_GENERIC
  • New code between decision logic and container creation: post-set safety guard (one-shot, +3s on FNC starts only)

Testing

Live 4.23 TNF cluster (two-node DualReplica, OCP 4.23.0-0.nightly-2026-08-01-032511, Pacemaker 2.1.10, dev-scripts on AWS). Tests run across two cluster deployments. All tests ran without manual intervention -- the cluster self-healed in every scenario. All 35 Cluster Operators verified healthy after each test.

Test 1: Conflict resolution resolves existing deadlock

  • Pre-set FNC on both nodes, stopped both etcd, exited maintenance
  • Both nodes detected dual FNC, computed same verdict: master-1 wins (revision 40221 > 40210)
  • master-0 yielded (cleared FNC), joined as learner
  • master-1 kept FNC through startup, cleared after etcd started successfully (normal post-start clear, not a yield)
  • Exercises: early multi-holder verdict, fnc_yield(), if/elif/else structural change
  • Result: PASSED -- self-healed, all COs recovered

Test 2: Quorum-loss regression (kill peer VM)

  • Destroyed master-1 VM via virsh destroy
  • master-0's detect_cluster_leadership_loss() fired -> set FNC + standalone
  • master-0 restarted etcd with force_new_cluster (+3s guard) -> cleared FNC -> added master-1 as learner after fencing
  • master-1 promoted to voter
  • Exercises: standalone marker lifecycle, post-set safety guard on uncontested FNC start, fencing + learner rejoin
  • Result: PASSED

Test 3: Normal single-node failover (fence master-0)

  • Destroyed master-0 VM. Same flow as Test 2, reversed roles.
  • master-1 survived, set FNC+standalone, master-0 rejoined after fencing
  • Result: PASSED

Test 4: Equal-revision EQUAL verdict (joint normal start)

  • Both nodes had genuinely equal disk revisions and same cluster_id (natural state on healthy cluster pre-fencing)
  • Pre-set FNC on both, stopped etcd, exited maintenance
  • Both: "FNC conflict inputs: master-0=52806 master-1=52806" -> both cleared FNC -> "equal revision and cluster_id - joint normal start"
  • Neither node rebuilt as learner -- both started normally from existing data
  • Disk revisions were 52546 at pre-stop check; the cluster committed between the check and container stops, so the RA read 52806 at start. Post-fencing revision divergence prevented re-running this test on the second cluster; the code and verdict logic have not changed between deployments.
  • Exercises: fnc_conflict_verdict EQUAL path, fnc_equal_clear_and_confirm, cluster_id comparison
  • Result: PASSED

Test 5: ADMIN verdict (code-trace verified)

  • The ADMIN path triggers when revision data is genuinely unavailable or cluster_ids diverge after split-brain. podman_start() writes revision and cluster_id from disk before the FNC check, making ADMIN unreachable without actual data corruption.
  • Code-trace verified: both no-revision branches and the different-cluster_id branch echo "ADMIN"; callers return OCF_ERR_GENERIC with fnc_admin_escalate showing actual holder names.
  • Result: VERIFIED BY CODE TRACE

Test 6: Race window exercised via test hook

  • OCF_RESKEY_test_race_delay=8 set on both nodes, stopped both etcd, exited maintenance
  • Both hooks fired simultaneously. Revision asymmetry (post-fencing) produced a single-set: master-0 decided to set FNC (higher revision), master-1 decided JOIN_AS_LEARNER (lower revision)
  • master-1's both-yield guard fired: "both-yield detected: no FNC holder and no active peer - retrying" (master-0's FNC write hadn't propagated yet due to hook delay)
  • On retry, master-0 was active, master-1 joined as learner
  • The dual-FNC variant of this race resolves via the identical verdict logic proven live in Test 1, with the post-set guard as code-trace-verified backstop
  • Exercises: test hooks, both-yield guard (first live firing), race-window convergence
  • Result: PASSED -- both-yield guard caught transient state, converged in one retry

Test 7: Stale-claim deletion (banned loser)

  • Set FNC on both nodes, master-0's CIB revision set artificially low (1000 -- banned node's RA never overwrites), master-0 banned
  • master-1's early check: dual FNC -> verdict master-1 wins (55074 > 1000) -> detected master-0 not starting/active/pending -> deleted master-0's stale FNC attribute
  • Logs: "FNC conflict inputs: master-1=55074 master-0=1000" -> "master-1 wins FNC conflict resolution" -> "peer master-0 not starting/active - clearing its stale force_new_cluster claim"
  • After unban: master-0 rejoined as learner, promoted to voter
  • Exercises: stale-claim deletion, peer_has_pending_start negative path, winner-only deletion
  • Result: PASSED

Verification: cibadmin exit code and peer_has_pending_start xpath

  • No-match exit code: 105 (CRM_EX_NOSUCH) confirmed on Pacemaker 2.1.10
  • Positive match: during a real pending start, the xpath //node_state[@uname='master-1']//lrm_rsc_op[@operation='start'][@op-status='-1'] returned exit code 0 with the pending op element. The xpath is correct.
  • Negative match: Test 7 (banned peer, no pending start) returned exit code 105. Deletion proceeded correctly.
  • peer_has_pending_start verified in both directions: match -> deletion blocked; no-match -> deletion allowed.

Final cluster state

  • Both nodes: Ready (v1.35.3)
  • All 35 Cluster Operators: Available=True, Progressing=False, Degraded=False
  • PCS: Both etcd Started on both nodes, no FNC or standalone attributes set
  • Zero manual interventions for cluster recovery across all tests

Related: OCPBUGS-86897 (fixed by #2181), OCPBUGS-100303

@knet-jenkins

knet-jenkins Bot commented Aug 2, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/1/input

@knet-jenkins

knet-jenkins Bot commented Aug 2, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/2/input

@knet-jenkins

knet-jenkins Bot commented Aug 2, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/3/input

@knet-jenkins

knet-jenkins Bot commented Aug 2, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/4/input

@Neilhamza
Neilhamza force-pushed the OCPBUGS-100303/fnc-race-resolution branch from 03eae0f to e76cd92 Compare August 2, 2026 09:29
@knet-jenkins

knet-jenkins Bot commented Aug 2, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/5/input

@Neilhamza
Neilhamza force-pushed the OCPBUGS-100303/fnc-race-resolution branch from e76cd92 to 5f25c1a Compare August 3, 2026 08:21
@knet-jenkins

knet-jenkins Bot commented Aug 3, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/6/input

@Neilhamza
Neilhamza force-pushed the OCPBUGS-100303/fnc-race-resolution branch from 5f25c1a to 02a6f02 Compare August 3, 2026 08:42
@knet-jenkins

knet-jenkins Bot commented Aug 3, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/7/input

@Neilhamza
Neilhamza force-pushed the OCPBUGS-100303/fnc-race-resolution branch from 02a6f02 to 742f408 Compare August 3, 2026 08:47
@knet-jenkins

knet-jenkins Bot commented Aug 3, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/8/input

@Neilhamza
Neilhamza force-pushed the OCPBUGS-100303/fnc-race-resolution branch from 742f408 to 561b994 Compare August 3, 2026 08:52
@knet-jenkins

knet-jenkins Bot commented Aug 3, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/9/input

@Neilhamza
Neilhamza force-pushed the OCPBUGS-100303/fnc-race-resolution branch from 561b994 to 6596483 Compare August 3, 2026 08:59
@knet-jenkins

knet-jenkins Bot commented Aug 3, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/10/input

@fonta-rh fonta-rh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid design -- the conflict verdict + post-set guard + stale-claim cleanup is the right approach over the old OCF_ERR_CONFIGURED deadlock. Four items below, three blocking.

Comment thread heartbeat/podman-etcd
ocf_log err "force_new_cluster holders changed after decision ($fnc_guard_holders): retrying"
return "$OCF_ERR_GENERIC"
fi
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: two requests on this guard.

  1. Add a comment documenting the theoretical failure mode: if CPG attribute propagation exceeds 3s (no hard SLA from attrd), both nodes pass this guard, both force-new-cluster, and the learner wipe at line 2550 destroys the loser's data. Probability is very low (CPG is typically sub-100ms, so this is a 30x+ margin), but documenting it helps future debugging.

  2. The guard is silent on the success path. Add a log after the check passes:

ocf_log notice "post-set guard: $NODENAME is sole FNC holder ($fnc_guard_holders), proceeding with force-new-cluster"

If this message appears on both nodes within the same 3s window, that is conclusive evidence of a double-FNC event. The current err-level log at line 2504 only fires when the guard catches a conflict -- we need the symmetric case logged too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed. Added the residual risk comment describing the attrd propagation window and the learner data wipe consequence. Also added the success path notice log so both nodes passing within 3s is visible evidence.

Comment thread heartbeat/podman-etcd Outdated
Comment on lines +1296 to +1298
clear_force_new_cluster
sleep 2
holders=$(get_force_new_cluster 2>/dev/null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: both error paths in this function are swallowed.

Line 1296: clear_force_new_cluster return is unchecked. If the delete fails, FNC persists, the post-set guard at line 2495 fires and proceeds with FNC start -- but the EQUAL verdict intended a normal start. Same data on both nodes so no data loss, but unnecessary learner rebuild.

Line 1298: get_force_new_cluster 2>/dev/null discards the return code. On CIB failure, holders is empty, grep does not match the peer, function concludes "joint normal start." Since FNC was already cleared at line 1296, is_force_new_cluster at 2495 returns false and the post-set guard is entirely bypassed -- proceeding blind.

Fix: check both return codes, return OCF_ERR_GENERIC on failure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed. Both clear_force_new_cluster and get_force_new_cluster return codes are now checked, returning OCF_ERR_GENERIC on failure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated: the caller in the EQUAL case branch now propagates the return code (if ! fnc_equal_clear_and_confirm; then return OCF_ERR_GENERIC), matching the ADMIN branch style. Both the in-function checks and the caller propagation are in place so the full error chain is covered.

Comment thread heartbeat/podman-etcd Outdated
Comment on lines +1252 to +1272
# All inputs read from CIB (symmetric); each node wrote revision and
# cluster_id before setting FNC, so both are visible wherever both
# FNC attributes are.
fnc_conflict_verdict()
{
local local_rev peer_rev peer_node_name

peer_node_name=$(get_peer_node_name)
local_rev=$(get_cib_revision "$NODENAME")
peer_rev=$(get_cib_revision "$peer_node_name")
ocf_log info "FNC conflict inputs: $NODENAME=$local_rev $peer_node_name=$peer_rev"

if is_numeric "$local_rev" && is_numeric "$peer_rev"; then
if [ "$local_rev" -gt "$peer_rev" ]; then
echo "$NODENAME"
elif [ "$local_rev" -lt "$peer_rev" ]; then
echo "$peer_node_name"
else
local local_cid peer_cid
local_cid=$(attribute_node_cluster_id get)
peer_cid=$(attribute_node_cluster_id_peer)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: the comment at line 1252 states "All inputs read from CIB (symmetric)" but attribute_node_cluster_id get reads from disk (jq on $ETCD_REVISION_JSON, line 915), while attribute_node_cluster_id_peer reads from CIB (crm_attribute --query). Since attribute_node_cluster_id update swallows CIB write failures (lines 929-934, returns OCF_SUCCESS on error), a silent write failure produces asymmetric verdicts: one node sees EQUAL, the other sees ADMIN.

Self-heals on retry (ADMIN returns OCF_ERR_GENERIC), not a data-loss path, but the comment is wrong and the asymmetry is avoidable.

Fix: add a get_cib_cluster_id helper analogous to get_cib_revision and use it for both local and peer reads here. Correct the comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, this mirrors the original bug's disk vs CIB asymmetry. Added get_cib_cluster_id helper (same pattern as get_cib_revision) and both sides now read from CIB. Comment corrected.

Comment thread heartbeat/podman-etcd Outdated
peer_has_pending_start()
{
local rc
cibadmin -Q --xpath "//node_state[@uname='$1']//lrm_rsc_op[@operation='start'][@op-status='-1']" >/dev/null 2>&1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this xpath matches pending starts on any resource on the peer, not just the etcd resource. The // descendant-axis from node_state hits lrm_rsc_op across all lrm_resource elements.

During cold boot (the exact scenario this PR targets), co-located resources (VIP, STONITH) routinely have pending starts. A pending VIP start causes peer_has_pending_start to return 0, blocking stale-claim cleanup. The post-set guard then sees 2 holders and aborts -- delaying recovery by one or more retry cycles.

Fix: scope to the etcd resource:

cibadmin -Q --xpath "//node_state[@uname='$1']//lrm_resource[@id='$OCF_RESOURCE_INSTANCE']/lrm_rsc_op[@operation='start'][@op-status='-1']" >/dev/null 2>&1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed. Scoped to the etcd resource using lrm_resource[@id] with the instance suffix stripped (${OCF_RESOURCE_INSTANCE%%:*}) for clone safety. Re-verified the xpath on a live cluster in both directions: exit 0 during a real pending etcd start, exit 105 when healthy.

@Neilhamza
Neilhamza force-pushed the OCPBUGS-100303/fnc-race-resolution branch from 6596483 to 5ce1355 Compare August 4, 2026 12:12
@knet-jenkins

knet-jenkins Bot commented Aug 4, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/11/input

@Neilhamza
Neilhamza force-pushed the OCPBUGS-100303/fnc-race-resolution branch from 5ce1355 to 8c338eb Compare August 4, 2026 12:18
@knet-jenkins

knet-jenkins Bot commented Aug 4, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/12/input

@clobrano
clobrano self-requested a review August 5, 2026 12:11

@clobrano clobrano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is sounds to me. It makes much more sense to let both nodes read from the same source.
I only left a couple of comments and an idea, but LGTM

Comment thread heartbeat/podman-etcd Outdated
}

# Read a node's revision from CIB (not local disk) for symmetric evaluation.
get_cib_revision()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick (it would help maintenance)
I think this function could replace attribute_node_revision_peer, or be used inside it to avoid multiple calls of the same line crm_attribute ... --name "revision".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, delegated attribute_node_revision_peer to get_cib_revision so the query lives in one place.

Comment thread heartbeat/podman-etcd Outdated
}

# Read a node's cluster_id from CIB (not local disk) for symmetric evaluation.
get_cib_cluster_id()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it the same as attribute_node_cluster_id get?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They look similar but read from different sources: attribute_node_cluster_id get reads local disk (jq on the revision JSON file), get_cib_cluster_id reads from the CIB. That disk vs CIB asymmetry was the same class of bug that caused the original race, which is why the helper exists. Added a NOTE comment above attribute_node_cluster_id to make this clear for future readers.

Comment thread heartbeat/podman-etcd
fnc_admin_escalate "$fnc_holders"
return "$OCF_ERR_GENERIC"
;;
*)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment here might help a newcomer to understand what's happening here without reading through the full logic :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed, added a comment explaining the verdict named the peer and we yield.

Comment thread heartbeat/podman-etcd Outdated
Comment on lines +2513 to +2516
# Residual risk: if attrd propagation exceeds 3s (no hard SLA, but
# typically sub-100ms), both nodes pass this guard and both
# force-new-cluster; the loser's data directory is wiped during
# learner rejoin. Probability is very low given the 30x+ margin.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make revision and cluster-id lifetime reboot, and let each node write them to CIB at startup, isn't reading the data correctly from CIB (i.e., matching what we have locally) a sign that attrd have been propagated?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took your round-trip idea and applied it to the FNC write itself so it covers the actual race window. The revision/cluster_id round-trip would prove propagation up to a point before either node claims, but the FNC write happens later, so both could still pass unaware. With the FNC attribute as the barrier, CIB total ordering guarantees the later claimant always sees the conflict. Faster than the fixed 3s on healthy clusters and robust under load. Thanks for the insight.

Comment thread heartbeat/podman-etcd Outdated
Comment on lines +2513 to +2516
# Residual risk: if attrd propagation exceeds 3s (no hard SLA, but
# typically sub-100ms), both nodes pass this guard and both
# force-new-cluster; the loser's data directory is wiped during
# learner rejoin. Probability is very low given the 30x+ margin.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make revision and cluster-id lifetime reboot, and let each node write them to CIB at startup, isn't reading the data correctly from CIB (i.e., matching what we have locally) a sign that the attrd have been propagated?
This should let the resource agent pass this step quicker than 3s most of the times and also deal with longer network delays in heavy network load scenarios.

@Neilhamza
Neilhamza force-pushed the OCPBUGS-100303/fnc-race-resolution branch from 8c338eb to c11b5d7 Compare August 6, 2026 08:57
@knet-jenkins

knet-jenkins Bot commented Aug 6, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/13/input

@fonta-rh

fonta-rh commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

/lgtm

@oalbrigt oalbrigt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few changes, and it should be good to go.

Comment thread heartbeat/podman-etcd Outdated
return 1
}

is_numeric()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use ocf_is_decimal() instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, switched to ocf_is_decimal and removed the custom is_numeric function.

Comment thread heartbeat/podman-etcd Outdated
}

# Read a node's revision from CIB (not local disk) for symmetric evaluation.
get_cib_revision()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you merge these 2 functions into get_attribute() or a similarly named function? Seems like the only difference is the attribute name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What @oalbrigt is suggesting would be useful also for another PR we have in queue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, merged both into get_cib_attribute(node, attr_name). Also updated attribute_node_revision_peer to delegate to it.

Comment thread heartbeat/podman-etcd Outdated

peer_node_name=$(get_peer_node_name)
if ! clear_force_new_cluster; then
ocf_log err "fnc_equal_clear_and_confirm: failed to clear own FNC"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to use ocf_exit_reason() for these errors.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, switched both to ocf_exit_reason.

Comment thread heartbeat/podman-etcd Outdated
attribute_node_cluster_id update
attribute_node_revision update

if [ -n "${OCF_RESKEY_test_race_delay:-}" ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldnt be any reason to do :- in a if [ -n ... ]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, removed the :- at both hook sites.

Comment thread heartbeat/podman-etcd Outdated
# bounded by CIB ordering correctness, not timing.
if is_force_new_cluster; then
local fnc_guard_holders fnc_guard_wait=0
while true; do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while :; is probably more effective here, as it's an sh internal, and not a separate command.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, switched to while :; do.

…multaneous dual-start

When both TNF nodes restart simultaneously, CIB replication delay can
cause both to set the force_new_cluster attribute before either sees
the other's write. The existing multi-holder check returned
OCF_ERR_CONFIGURED (fatal, no retry), causing permanent etcd outage.

Replace the fatal abort with a four-outcome conflict verdict
(fnc_conflict_verdict) that reads both revisions and cluster_ids from
CIB via get_cib_attribute for symmetric evaluation:

- Winner (higher revision): force-new-clusters, loser yields
- EQUAL (same revision + same cluster_id): both clear FNC and start
  normally without learner rebuild
- ADMIN (no data, or equal revision with different cluster_id): never
  guess -- soft-fail with recovery guidance showing actual holder names
- Active voter peer: always yield regardless of verdict

Add an adaptive post-set safety guard that waits for the local FNC
write to be visible in the CIB replica (write round-trip through the
DC), then verifies this node is the sole holder. CIB total ordering
guarantees that once our write is visible, any peer claim ordered
before ours is visible too -- the later claimant always detects the
conflict. Adaptive: passes as soon as propagation completes instead
of a fixed sleep. On any anomaly it soft-fails and lets Pacemaker
retry.

The data-backed winner in the early check clears stale peer FNC when
the peer is not starting, not active, and has no pending start in the
CIB. peer_has_pending_start queries the CIB status section scoped to
the etcd resource (avoids false positives from co-located resources
during cold boot). Accepts exit codes 105 (Pacemaker 2.x) and 6
(legacy 1.x). Query errors treated as "pending" (fail-safe).

Dual-claim always logged at err level even when auto-resolved.
All error paths return OCF_ERR_GENERIC (soft, retriable).

Signed-off-by: Neil Hamza <nhamza@redhat.com>
Add OCF_RESKEY_test_race_delay env-gated sleep hooks at two injection
points for deterministic race reproduction. Both hooks are inert when
the variable is unset. Intentionally not declared in RA metadata.

This commit is separable -- drop it if maintainers prefer keeping
test instrumentation downstream only.

Signed-off-by: Neil Hamza <nhamza@redhat.com>
@Neilhamza
Neilhamza force-pushed the OCPBUGS-100303/fnc-race-resolution branch from c11b5d7 to 6638f20 Compare August 9, 2026 06:51
@knet-jenkins

knet-jenkins Bot commented Aug 9, 2026

Copy link
Copy Markdown

Can one of the project admins check and authorise this run please: https://haci.fast.eng.rdu2.dc.redhat.com/job/resource-agents/job/resource-agents-pipeline/job/PR-2194/14/input

@fonta-rh fonta-rh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@oalbrigt
oalbrigt merged commit 4f47af4 into ClusterLabs:main Aug 10, 2026
1 check passed
@oalbrigt

Copy link
Copy Markdown
Contributor

Thanks.

Neilhamza added a commit to Neilhamza/resource-agents that referenced this pull request Aug 23, 2026
…ce_new_cluster

set_force_new_cluster() writes the CIB attribute without checking whether
the peer already holds it.  In the start path a lost race is caught by the
post-set guard (OCPBUGS-100303, PR ClusterLabs#2194) but costs a full failed-start
retry cycle; in the monitor path (detect_cluster_leadership_loss) there is
no protection at all.

Add a holder pre-check: query get_force_new_cluster() before writing and
return rc=2 if any non-self node holds the attribute.  This narrows the
CHECK->SET window from the caller's decision point down to the primitive
itself, where the test_race_delay hook is now positioned.

Return-code contract:
  0 - attribute set (holders were empty or self-only)
  1 - crm/CIB error (caller should soft-fail)
  2 - another node already holds force_new_cluster (caller should defer)

Self-holding still writes (idempotent attrd update, keeps recovery robust
if a prior cycle died between set and restart).

This is a churn-reducing first layer, not a correctness fix.  The merged
post-set guard and multi-holder tiebreaker remain the correctness mechanism
(CIB total ordering: once our write is visible, any peer claim ordered
before ours is visible too).

Signed-off-by: Neil Hamza <nhamza@redhat.com>
Neilhamza added a commit to Neilhamza/resource-agents that referenced this pull request Aug 24, 2026
…ce_new_cluster

set_force_new_cluster() writes the CIB attribute without checking whether
the peer already holds it.  The start path already has an outer holder
check (~line 2370) that catches the common case, and a lost race past that
check is caught by the post-set guard (OCPBUGS-100303, PR ClusterLabs#2194), but the
guard costs a full failed-start retry cycle.  The monitor path
(detect_cluster_leadership_loss) has no protection at all: in a symmetric
"no leader" partition both nodes write FNC and assert standalone_node,
converging only after restart churn through the tiebreaker.

Add a holder pre-check inside set_force_new_cluster(): query
get_force_new_cluster() before writing and return rc=2 if any non-self
node holds the attribute.  In the start path this covers the residual
window between the outer holder check and the write.  In the monitor path
it is the sole defense — the bigger vector, since the monitor had no
holder awareness at all.  The test_race_delay hook is now positioned
between pre-check and write to exercise exactly this window.

Return-code contract:
  0 - attribute set (holders were empty or self-only)
  1 - crm/CIB error (caller should soft-fail)
  2 - another node already holds force_new_cluster (caller should defer)

Self-holding still writes (idempotent attrd update, keeps recovery robust
if a prior cycle died between set and restart).

This is a churn-reducing first layer, not a correctness fix.  The merged
post-set guard and multi-holder tiebreaker remain the correctness mechanism
(CIB total ordering: once our write is visible, any peer claim ordered
before ours is visible too).

Signed-off-by: Neil Hamza <nhamza@redhat.com>
Neilhamza added a commit to Neilhamza/resource-agents that referenced this pull request Aug 24, 2026
…ce_new_cluster

set_force_new_cluster() writes the CIB attribute without checking whether
the peer already holds it.  The start path already has an outer holder
check (~line 2370) that catches the common case, and a lost race past that
check is caught by the post-set guard (OCPBUGS-100303, PR ClusterLabs#2194), but the
guard costs a full failed-start retry cycle.  The monitor path
(detect_cluster_leadership_loss) has no protection at all: in a symmetric
"no leader" partition both nodes write FNC and assert standalone_node,
converging only after restart churn through the tiebreaker.

Add a holder pre-check inside set_force_new_cluster(): query
get_force_new_cluster() before writing and return rc=2 if any non-self
node holds the attribute.  In the start path this covers the residual
window between the outer holder check and the write.  In the monitor path
it is the sole defense — the bigger vector, since the monitor had no
holder awareness at all.  The test_race_delay hook is now positioned
between pre-check and write to exercise exactly this window.

Return-code contract:
  0 - attribute set (holders were empty or self-only)
  1 - crm/CIB error (caller should soft-fail)
  2 - another node already holds force_new_cluster (caller should defer)

Self-holding still writes (idempotent attrd update, keeps recovery robust
if a prior cycle died between set and restart).

This is a churn-reducing first layer, not a correctness fix.  The merged
post-set guard and multi-holder tiebreaker remain the correctness mechanism
(CIB total ordering: once our write is visible, any peer claim ordered
before ours is visible too).

Signed-off-by: Neil Hamza <nhamza@redhat.com>
Neilhamza added a commit to Neilhamza/resource-agents that referenced this pull request Aug 24, 2026
set_force_new_cluster() writes the CIB attribute without checking
whether the peer already holds it.  The start path has an outer holder
check that catches the common case, and a lost race past it is caught
by the post-set guard (OCPBUGS-100303, PR ClusterLabs#2194), but the guard costs a
full failed-start retry cycle.  The monitor path
(detect_cluster_leadership_loss) has no protection at all: in a
symmetric "no leader" partition both nodes write FNC and assert
standalone_node, converging only after restart churn through the
tiebreaker.

Add a holder pre-check inside set_force_new_cluster(): query
get_force_new_cluster() before writing and return rc=2 if any non-self
node holds the attribute.  In the start path this covers the residual
window between the outer holder check and the write; in the monitor
path it is the sole defense.  The test_race_delay hook moves between
pre-check and write to exercise exactly this window.

Return-code contract:
  0 - attribute set (holders were empty or self-only)
  1 - crm/CIB error (caller should soft-fail)
  2 - another node already holds force_new_cluster (caller should defer)

Handle rc=2 at the three call sites.  Start path
(start_resources_count=1 and =2): set JOIN_AS_LEARNER=true and continue
— the peer holding FNC is driving recovery, and converting in place
avoids one guard-triggered retry cycle; the post-set guard (gated on
is_force_new_cluster) stays inert since this node never set FNC.  In
the =2 "newer" arm this means first-claimant-wins in the residual
window: for the peer to claim first despite our "newer" verdict its
comparison must have used stale saved attributes — the same data
fnc_conflict_verdict would read, so the tiebreaker is no more
authoritative there.  Monitor path: skip set_standalone_node, log the
deferral, and still return OCF_ERR_GENERIC (contract unchanged); the
restart lands in podman_start which joins as learner.  No post-set
guard on the monitor path: it is timeout-sensitive, and the restart ->
start-path tiebreaker is the backstop.

Self-holding still writes (idempotent attrd update, keeps recovery
robust if a prior cycle died between set and restart).  If the peer's
claim is stale (it never starts), the learner wait burns its timeout
and retries through the existing single-peer-holder path — a
pre-existing exposure this change converges to rather than introduces.

This is a churn-reducing first layer, not a correctness fix.  The
merged post-set guard and multi-holder tiebreaker remain the
correctness mechanism (CIB total ordering: once our write is visible,
any peer claim ordered before ours is visible too).

Signed-off-by: Neil Hamza <nhamza@redhat.com>
Neilhamza added a commit to Neilhamza/resource-agents that referenced this pull request Aug 24, 2026
The function is named try_set_force_new_cluster to reflect that it may
decline to write.

set_force_new_cluster() writes the CIB attribute without checking
whether the peer already holds it.  The start path has an outer holder
check that catches the common case, and a lost race past it is caught
by the post-set guard (OCPBUGS-100303, PR ClusterLabs#2194), but the guard costs a
full failed-start retry cycle.  The monitor path
(detect_cluster_leadership_loss) has no protection at all: in a
symmetric "no leader" partition both nodes write FNC and assert
standalone_node, converging only after restart churn through the
tiebreaker.

Add a holder pre-check inside try_set_force_new_cluster(): query
get_force_new_cluster() before writing and return rc=2 if any non-self
node holds the attribute.  In the start path this covers the residual
window between the outer holder check and the write; in the monitor
path it is the sole defense.  The test_race_delay hook moves between
pre-check and write to exercise exactly this window.

Return-code contract:
  0 - attribute set (holders were empty or self-only)
  1 - crm/CIB error (caller should soft-fail)
  2 - another node already holds force_new_cluster (caller should defer)

Handle rc=2 at the three call sites.  Start path
(start_resources_count=1 and =2): set JOIN_AS_LEARNER=true and continue
— the peer holding FNC is driving recovery, and converting in place
avoids one guard-triggered retry cycle; the post-set guard (gated on
is_force_new_cluster) stays inert since this node never set FNC.  In
the =2 "newer" arm this means first-claimant-wins in the residual
window: for the peer to claim first despite our "newer" verdict its
comparison must have used stale saved attributes — the same data
fnc_conflict_verdict would read, so the tiebreaker is no more
authoritative there.  Monitor path: skip set_standalone_node, log the
deferral, and still return OCF_ERR_GENERIC (contract unchanged); the
restart lands in podman_start which joins as learner.  No post-set
guard on the monitor path: it is timeout-sensitive, and the restart ->
start-path tiebreaker is the backstop.

Self-holding still writes (idempotent attrd update, keeps recovery
robust if a prior cycle died between set and restart).  If the peer's
claim is stale (it never starts), the learner wait burns its timeout
and retries through the existing single-peer-holder path — a
pre-existing exposure this change converges to rather than introduces.

This is a churn-reducing first layer, not a correctness fix.  The
merged post-set guard and multi-holder tiebreaker remain the
correctness mechanism (CIB total ordering: once our write is visible,
any peer claim ordered before ours is visible too).

Signed-off-by: Neil Hamza <nhamza@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants