feat(reboot): add --wait/--expect-up to verify a rebooted VC member - #164
Conversation
Closes #161. `reboot --member N --now` issued the reboot and returned; verifying the member came back meant hand-polling `show virtual-chassis status`, `show chassis fpc` and `show interfaces terse` for minutes — which is what the operator actually waits for before the next maintenance step. - `--wait SEC` (only with `--member --now`): after issuing, reconnect until the member is Prsnt with a role *and* its FPC slot is Online. `--expect-up IFACE[,IFACE...]` additionally requires those ports to be up/up: a member can be Prsnt while its PFE is not forwarding yet, which is when traffic hashed to it is black-holed. Connection failures are "not yet" — the whole VC can be unreachable while one member reboots if the management path transits its uplink. - Exit code 10 (`error=member_not_back`) when it does not come back; `wait`, `after`, `fpc_state` and `interfaces` land in the result and the --json row. Scheduled (`--at`) reboots and dry-runs verify nothing. - vc.py gains `get_fpc_state()`, `get_interface_states()` (terse text nodes are newline-wrapped, hence the strip) and `wait_for_member()`; the reconnect loop is now shared with `wait_for_master()` via `_poll_device()` (bounded probes, sleep/monotonic via the module so tests can patch them). XML shapes taken from a live two-member QFX5110 VC. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Kotm5Xha6X2hu3S2u2RAT
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| last_problem = None | ||
| while True: | ||
| result["attempts"] += 1 | ||
| remaining = max(1, int(deadline - time.monotonic())) |
There was a problem hiding this comment.
R1F1 (blocking)
A member reboot RPC can return after enqueueing the reboot but before the member leaves Prsnt or its FPC leaves Online. Because polling reconnects immediately and accepts that first healthy snapshot, the command can report confirmed and exit 0 just before the member actually goes down, allowing the next maintenance step to run during the reboot.
Advisory AI review — verify before acting. Round summary and ledger are in the ai-review comment.
| logger.debug(f"get_interface_states: {type(e).__name__}: {e}") | ||
| return states | ||
| if rsp is None or isinstance(rsp, bool): | ||
| return states |
There was a problem hiding this comment.
R1F2 (advisory)
For --expect-up ge-0/0/40.0 or irb.0, Junos reports the requested name under a <logical-interface> element rather than <physical-interface>. This loop leaves the state as None, so an interface that is actually up/up causes polling to time out and return exit code 10.
Advisory AI review — verify before acting. Round summary and ledger are in the ai-review comment.
| deadline = start + timeout | ||
| last_problem = None | ||
| while True: | ||
| result["attempts"] += 1 |
There was a problem hiding this comment.
R1F3 (advisory)
If an unsuccessful probe finishes before the deadline, line 575 sleeps for the entire remaining interval and the loop then re-enters at the deadline. max(1, ...) starts one more connection attempt with a one-second probe budget, so an unreachable or not-yet-NETCONF-ready device can make --wait SEC exceed SEC rather than terminating at the requested deadline.
Advisory AI review — verify before acting. Round summary and ledger are in the ai-review comment.
codex review of #164 (critical): `request system reboot member N` returns while the member is still up, so the first probe found the pre-reboot member Prsnt/Online and reported success without a reboot having happened. - cmd_reboot reads the member's boot timestamp (and the VC master, for the localre/fpcN mapping) on the still-open connection *before* issuing, and wait_for_member now requires that timestamp to change. When it cannot be read, recovery requires having observed the member go away (connection failures count, via _poll_device's new on_unreachable callback) and the result carries a warning. - get_interface_states matches logical interface names too, so --expect-up ae0.0 works, not only physical names. - _poll_device caps dev.timeout as well as auto_probe with the remaining window: bounding only the connect left the RPCs able to run past --wait. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Kotm5Xha6X2hu3S2u2RAT
| "fpc_state": last.get("fpc_state"), | ||
| "interfaces": last.get("interfaces"), | ||
| "booted": last.get("booted"), | ||
| "rebooted": bool(polled["ok"]), |
There was a problem hiding this comment.
R2F1 (advisory)
If the boot timestamp changes but an expected interface remains down until timeout, the reboot was observed but this field returns False because polled["ok"] also includes FPC and interface readiness. Library callers therefore cannot distinguish “reboot occurred but readiness failed” from “reboot never occurred”; track reboot evidence independently from the final gate.
Advisory AI review — verify before acting. Round summary and ledger are in the ai-review comment.
…, tighten the fallback codex review follow-up on #164: - boot_time_is_newer() compares parsed instants (falling back to string inequality), so a reformatted or re-zoned rendering of the same boot is not read as a reboot, and only a later timestamp counts. - the member's uptime block is resolved with the master seen in *this* probe: mastership can move while the member reboots, which changes whether it is localre or fpcN. - a VC-status RPC failure no longer counts as 'the member went down' for the no-baseline fallback; only an unreachable device or a member-level not-ready state does. - dev.timeout is capped by the probe interval rather than the whole window, so one slow RPC cannot consume the budget; --wait is documented as a between-probes budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Kotm5Xha6X2hu3S2u2RAT
| if not booted or not baseline: | ||
| return False | ||
| fmt = "%Y-%m-%d %H:%M:%S" | ||
| try: |
There was a problem hiding this comment.
R3F1 (advisory)
For baseline 2026-06-16 19:11:55 UTC and post-probe value 2026-06-17 04:11:55 JST, both timestamps represent the same instant, but slicing to 19 characters makes the latter appear newer. If the same boot is rendered in a different timezone, wait_for_member() can therefore falsely confirm that a reboot occurred.
Advisory AI review — verify before acting. Round summary and ledger are in the ai-review comment.
…recovery /code-review findings on #164, plus regression tests: - get_fpc_state() returns None both for "slot not reported" and for an RPC failure; treating that as "the member went down" let a transient error on the no-baseline path confirm a member that never rebooted. Only a known non-Online state counts as a transition now. - get_member_boot_time() accepted a reply with no per-RE blocks for any member, but such a reply describes only the RE the session is on; rebooting member 1 from a session on member 0 would baseline against member 0 and always time out. It is accepted only when the master is unknown or is the member asked for. - the verification-failure step was tagged "error", which the formatter prints above the reboot line; it is "verify_error" now. - `rebooted` reported overall success rather than boot evidence, so a member that came back with a port still down said rebooted=false. - docstring/CLAUDE.md still claimed the boot timestamp is never parsed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Kotm5Xha6X2hu3S2u2RAT
AI review (gpt-5.6-sol)Verdict: no blocking findings — 2 advisory. Advisory findings do not need to be resolved before merge; open an issue for the ones worth keeping. Scope: new commits since No findings clear the reporting bar this round. Still open from earlier rounds:
Findings ledger (all rounds)
Advisory per-push review (round 4) generated by ai-review — verify findings before acting. |
1 participant
Closes #161.
Why
reboot --member N --nowissued the reboot and returned. Verifying the member came back meant hand-pollingshow virtual-chassis status,show chassis fpcandshow interfaces terseevery 30 s for minutes — that wait is the actual gate before the next maintenance step.What
--wait SEC(only with--member --now): the member's boot timestamp is read on the still-open connection before the reboot is issued; verification then reconnects until that timestamp is newer and the member isPrsntwith a role and its FPC slot isOnline. The reboot RPC returns while the member is still up, so "healthy right now" is not evidence that anything happened — without the baseline (unreadable) recovery instead requires having observed the member go away, and the result says so.--expect-up IFACE[,IFACE…]: also require those interfaces to beup/up. A member can bePrsntwhile its PFE is not forwarding yet — precisely when traffic hashed to it is black-holed — so the single-homed ports belong in the condition.error=member_not_back/unreachable) when it does not come back in time.wait,after,fpc_stateandinterfacesare in the result dict and the--jsonrow; the text output gains aconfirmed: member N is back (FPC Online, K port(s) up) after Ns / M probe(s)line.--at) reboots and--dry-runverify nothing. Guards:--waitrequires--member --now,--expect-uprequires--wait,--wait >= 0.vc.pygainsget_fpc_state(),get_interface_states(),get_member_boot_time(),boot_time_is_newer()andwait_for_member(), built on a new_poll_device()reconnect loop (probes bounded by the remaining window, RPCs bounded by the interval,time.sleep/monotonicthrough the module so tests patch them).wait_for_master()keeps its own loop for now — converting it would change itsafter/errorsemantics. XML shapes were read off a live two-member QFX5110 VC — note the terse reply wraps every text node in newlines, hence thestrip().Tests
581 passed (+22).
vc.py97 %.wait_for_member: comes back after an unreachable window (probe bounds,gather_facts=False, device closed each probe);Prsntbut FPCPresent→ not back;--expect-upgates success and names the offending port on timeout; member absent; never reachable.cmd_reboot: confirmed → 0;--expect-upparsed (whitespace tolerated) and rendered; not back → 10;--wait 0, a failed reboot, a dry-run and a scheduled reboot verify nothing; CLI guard combinations.🤖 Generated with Claude Code
https://claude.ai/code/session_011Kotm5Xha6X2hu3S2u2RAT