OCPBUGS-104851: podman-etcd: add restart_no_leave CRM attribute guard to skip member removal - #2197
Conversation
|
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-2197/1/input |
d7b0400 to
8a83d19
Compare
|
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-2197/2/input |
8a83d19 to
bb81fe0
Compare
|
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-2197/3/input |
bb81fe0 to
2778ba1
Compare
|
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-2197/4/input |
2778ba1 to
6ec2636
Compare
|
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-2197/5/input |
|
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-2197/6/input |
62e98fd to
44db83d
Compare
|
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-2197/7/input |
|
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-2197/8/input |
oalbrigt
changed the title
lucaconsalvi
left a comment
lucaconsalvi
left a comment
There was a problem hiding this comment.
Thanks Francesco — clean PR, the core mechanism is well-designed. The transient attribute as a one-shot guard mirrors the existing force_new_cluster pattern nicely, and the permanent/transient helper refactoring is a good cleanup.
A few items below, mostly around clear_restart_no_leave error handling.
| if ! crm_attribute --delete --lifetime reboot --node "$NODENAME" --name "restart_no_leave" 2>/dev/null; then | ||
| ocf_log warn "could not clear restart_no_leave attribute" | ||
| fi | ||
| ocf_log info "$NODENAME: restart_no_leave attribute cleared" |
There was a problem hiding this comment.
Bug: unconditional success log + stale attribute risk
This ocf_log info runs unconditionally — including on the failure path. If the delete fails, the logs show both:
WARN: could not clear restart_no_leave attribute
INFO: master-0: restart_no_leave attribute cleared
This contradicts itself and would mislead operators during incident triage.
Also worth considering: if the clear fails, the attribute persists. Since it is transient (--lifetime reboot), it survives a crm_resource --restart (no reboot). The next podman_stop would then silently skip leave_etcd_member_list — an unintended non-destructive stop.
The parallel clear_force_new_cluster handles this by returning $OCF_ERR_GENERIC on failure before reaching the success log. Suggestion:
clear_restart_no_leave()
{
if ! crm_attribute --delete --lifetime reboot --node "$NODENAME" --name "restart_no_leave" 2>/dev/null; then
ocf_log warn "could not clear restart_no_leave attribute"
return
fi
ocf_log info "$NODENAME: restart_no_leave attribute cleared"
}There was a problem hiding this comment.
Good catch on the contradictory log, I fixed with an early return.
On the stale attribute risk: agreed it's worth noting, but it is well bounded. The attribute is --lifetime reboot, so any fencing event (the primary "other stop" on TNF) clears it automatically.
It's also only ever set intentionally by CEO before a pcs resource restart, there's no path where it gets set implicitly or repeatedly.
Returning OCF_ERR_GENERIC here risks reintroducing the Pacemaker escalation pattern from OCPBUGS-86897, which the bug description explicitly calls out as a constraint. The warn log gives operators visibility if it ever does fail.
|
|
||
| clear_restart_no_leave() | ||
| { | ||
| if ! crm_attribute --delete --lifetime reboot --node "$NODENAME" --name "restart_no_leave" 2>/dev/null; then |
There was a problem hiding this comment.
Nit: 2>/dev/null hides diagnostic info on delete failure
The parallel clear_force_new_cluster does not suppress stderr from crm_attribute, allowing Pacemaker's error details (connection refused, permission denied, etc.) to appear in logs. Removing 2>/dev/null here would give operators actionable context when the delete fails — especially important since the warn message is generic.
| # Read a transient node attribute from CIB (auto-cleared on reboot). | ||
| get_cib_transient_attribute() | ||
| { | ||
| crm_attribute --query --lifetime reboot --node "$1" --name "$2" 2>/dev/null | awk -F"value=" '{print $2}' | tr -d "'" |
There was a problem hiding this comment.
Observation: pipeline masks crm_attribute exit code
Without set -o pipefail, the exit code comes from tr (last in the pipeline), which almost never fails. So this function always returns 0, regardless of whether crm_attribute succeeded.
This makes the error check in get_force_new_cluster at the if ! value=$(get_cib_transient_attribute ...) call site effectively dead code — that branch can never be entered.
For is_restart_no_leave, a CIB failure (pacemaker not running, node name wrong) is silently treated as "attribute not set", which defaults to destructive behavior. The fail-safe direction is arguably correct (better to unnecessarily leave+rejoin than skip it), but the operator gets no indication their flag was ignored.
The pattern is pre-existing (the old inline code had the same semantics), but extracting it into a named reusable helper cements it as the API contract. If the function is meant to be reusable, consider propagating the exit code:
get_cib_transient_attribute()
{
local output rc
output=$(crm_attribute --query --lifetime reboot --node "$1" --name "$2" 2>/dev/null)
rc=$?
[ $rc -ne 0 ] && return $rc
echo "$output" | awk -F"value=" '{print $2}' | tr -d "'"
}There was a problem hiding this comment.
That's a super valid concern, I also applied a similar pattern to the get_cib_pesistent_attribute().
Thanks for cathing this one!
|
|
||
| # Read a node attribute from CIB (not local disk) for symmetric evaluation. | ||
| get_cib_attribute() | ||
| is_restart_no_leave() |
There was a problem hiding this comment.
Nit: missing doc comments for consistency
The parallel functions is_force_new_cluster and clear_force_new_cluster both have doc comments. Adding one-liners here would help future readers:
# Return 0 if 'restart_no_leave' is set on the current node, 1 otherwise.
is_restart_no_leave()and:
# Delete the one-shot restart_no_leave flag after consumption in start().
clear_restart_no_leave()
|
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-2197/10/input |
73e9cf6 to
0e98ddd
Compare
|
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-2197/11/input |
fonta-rh
left a comment
fonta-rh
left a comment
There was a problem hiding this comment.
Nice work addressing Luca's feedback in the squash — the clear_restart_no_leave error handling and get_cib_transient_attribute pipeline fix look good.
Three additional items below. Finding 1 is the most important — a correctness issue in the attribute lifecycle.
Finding 3 (cross-repo): cluster-restore-tnf.sh doesn't clear restart_no_leave
cleanup_podman_etcd_attributes() in cluster-etcd-operator/bindata/etcd/cluster-restore-tnf.sh (lines 104-119) clears standalone_node, learner_node, and force_new_cluster but not restart_no_leave. Since is_restart_no_leave is checked before the FNC branch in podman_start (line 2331), a lingering restart_no_leave attribute during a restore would preempt the force_new_cluster recovery that the restore script sets up.
Suggested fix — add to cleanup_podman_etcd_attributes():
crm_attribute --delete --name "restart_no_leave" --lifetime reboot --node "${NODENAME}" || true(and the equivalent for the peer node, matching the force_new_cluster pattern)
| if ocf_is_true "$pod_was_running"; then | ||
| if is_restart_no_leave; then | ||
| ocf_log notice "restart_no_leave set: starting normally (non-destructive restart)" | ||
| clear_restart_no_leave |
There was a problem hiding this comment.
Bug: guard consumed before start succeeds
clear_restart_no_leave fires here, ~350 lines before the container actually starts (~line 2681). If start fails after this point (port still bound, config generation error, podman run failure), the attribute is gone. Pacemaker calls stop before retrying, and since restart_no_leave was already cleared, stop calls leave_etcd_member_list — the exact destructive action the feature was designed to prevent.
The established pattern in this same function handles it correctly: clear_force_new_cluster is called at line 2719, after monitor_cmd_exec confirms the container started successfully (line 2716). The attribute should persist through failed start attempts and only be consumed on success.
Suggested fix — use a local flag, clear after confirmed success:
# Line ~2286: add local variable
local restart_no_leave_flag=false
# Line 2331: replace clear with flag
if is_restart_no_leave; then
ocf_log notice "restart_no_leave set: starting normally (non-destructive restart)"
restart_no_leave_flag=true
elif ocf_is_true "$pod_was_running"; then
...
# Line ~2719: clear after confirmed success, alongside force_new_cluster
if is_force_new_cluster; then
clear_force_new_cluster
...
fi
if ocf_is_true "$restart_no_leave_flag"; then
clear_restart_no_leave
fi
return $OCF_SUCCESSThis follows the existing clear_force_new_cluster pattern, uses a local variable (no script-level init needed), and ensures the attribute persists through failed start retries.
| fi | ||
|
|
||
| if ocf_is_true "$pod_was_running"; then | ||
| if is_restart_no_leave; then |
There was a problem hiding this comment.
Logic gap: stale attribute after orphaned stop
If CEO sets restart_no_leave and stop runs (skipping leave), but start never happens — node banned by admin, resource disabled — the --lifetime reboot attribute persists indefinitely without reboot. When eventually unbanned, is_restart_no_leave fires here and bypasses the entire FNC/learner decision tree (lines 2337+). If the peer did force_new_cluster during the ban (creating a new single-member cluster), the local etcd attempts to start with stale cluster membership and fails.
The --lifetime reboot scope handles the reboot case naturally, but ban-without-reboot is a normal admin operation.
Worth considering: a staleness guard (e.g., verify the local member is still known to etcd before trusting restart_no_leave), or documenting that CEO should clear the attribute if the restart doesn't complete.
On TNF, CA signer rotation updates the trust bundle on disk but podman-etcd is never restarted, so etcd keeps the old CA pool in memory and kube-apiserver fails with "tls: unknown certificate authority". CEO needs a way to trigger a Pacemaker restart without the destructive leave/rejoin cycle. Add a restart_no_leave transient CRM attribute (--lifetime reboot) that podman_stop() checks before calling leave_etcd_member_list(). When set by CEO, the stop skips member removal and clears the attribute, allowing a non-destructive restart that re-reads the CA bundle. Same pattern as force_new_cluster. Refactor CIB attribute access into get_cib_permanent_attribute (--type nodes) and get_cib_transient_attribute (--lifetime reboot) for clarity. Migrate get_force_new_cluster to the new transient helper.
|
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-2197/12/input |
95db491 to
1f556f6
Compare
|
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-2197/13/input |
fonta-rh
left a comment
fonta-rh
left a comment
There was a problem hiding this comment.
Looks good after the latest push — F1 (early clear), F4 (error handling), and F5 (pipeline exit code) are all resolved cleanly. The restart_no_leave_flag approach mirrors the established force_new_cluster pattern at line ~2719, which is the right call.
Two design-level items for follow-up (not blocking):
-
F2 (stale attribute after orphaned stop): If stop runs with
restart_no_leavebut start never happens (node banned without reboot), the--lifetime rebootattribute persists. If the peer doesforce_new_clusterduring the ban, the eventual start uses stale membership. Low likelihood (requires ban-without-reboot during an active CEO restart), but worth a note in the agent's operational assumptions. -
F3 (cluster-restore-tnf.sh):
cleanup_podman_etcd_attributes()doesn't clearrestart_no_leave— cross-repo, tracked separately.
|
@fonta-rh those two design-level items should be already covered by last commits on the CEO side:
|
|
/lgtm |
|
Thanks. |
4 participants
On TNF, CA signer rotation updates the bundle on disk but podman-etcd is never restarted, so etcd keeps old CA pool in memory and kube-apiserver fails with "tls: unknown certificate authority". CEO needs a way to trigger a Pacemaker restart withoutthe destructive leave/rejoin cycle.