Skip to content

Claude: Generate a runbook to have zero downtime upgrades with code - #18

Open
sharma-tapas wants to merge 1 commit into
main_24_03from
zero-downtime-upgrades
Open

Claude: Generate a runbook to have zero downtime upgrades with code #18
sharma-tapas wants to merge 1 commit into
main_24_03from
zero-downtime-upgrades

Conversation

@sharma-tapas

Copy link
Copy Markdown
Collaborator

PR just for review if Claude agents can look in right direction

…nd scripts

Signed-off-by: Tapas Sharma <tapas@platform9.com>

@kgunjikar kgunjikar 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.

a) You are assuming we can get safe snapshots here. I doubt that's possible.
b) for some time, the ovn-controller is reconciling, and you are replaying old values. Are you sure that is safe?
c) Are counts enough to check for the correctness of programming?

I won't go into details of commands etc here.

Basically, Claude doesn't show an innate understanding of the OVN design. I would limit it to smaller problems

@sharma-tapas

Copy link
Copy Markdown
Collaborator Author

a) You are assuming we can get safe snapshots here. I doubt that's possible. b) for some time, the ovn-controller is reconciling, and you are replaying old values. Are you sure that is safe? c) Are counts enough to check for the correctness of programming?

I won't go into details of commands etc here.

Basically, Claude doesn't show an innate understanding of the OVN design. I would limit it to smaller problems

This is great feedback, this agent before suggesting this did not have OVN and Neutron documentation awareness, I have added it and also added the entire OVN codebase with Linux Kernel, TCP/IP RFC and other networking standards.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Comment on lines +28 to +33
if [ "$COUNT" -lt "$THRESHOLD" ] && [ "$RESTORED" -eq 0 ]; then
log "flow table cleared (count=$COUNT < threshold=$THRESHOLD), restoring snapshot"
"$OVS_OFCTL" add-groups br-int "$GROUPS_FILE" 2>/dev/null || true
"$OVS_OFCTL" add-flows br-int "$FLOWS_FILE" 2>/dev/null || true
RESTORED=1
log "snapshot restored"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Flow-restore safety net permanently disables itself if its first attempt fails silently

The restore-complete flag is set unconditionally (RESTORED=1 at container/flow-watchdog.sh:32) even when the preceding restore commands fail silently (due to || true), so a later genuine flow-table wipe is never corrected.

Impact: During an upgrade, if the restore commands fail for any transient reason (e.g., the OVS bridge is momentarily unavailable), the watchdog becomes permanently inert and cannot protect against the actual flow-table clear it was designed to catch.

Mechanism: RESTORED flag set without verifying restore success

The watchdog polls ovs-ofctl dump-flows br-int in a loop (container/flow-watchdog.sh:25-26). When the flow count drops below the threshold, it attempts to restore flows and groups:

"$OVS_OFCTL" add-groups br-int "$GROUPS_FILE" 2>/dev/null || true
"$OVS_OFCTL" add-flows  br-int "$FLOWS_FILE"  2>/dev/null || true
RESTORED=1

Because both commands suppress errors with || true, any failure (bridge not yet created, OVS socket not ready, permission issue) is silently swallowed. RESTORED is then set to 1 unconditionally.

The guard at container/flow-watchdog.sh:28 ([ "$RESTORED" -eq 0 ]) means the restore branch can only execute once. If the first attempt was a no-op failure, the watchdog will never attempt restoration again, even when the real flow-table clear occurs moments later.

A concrete trigger: the sidecar container starts and the snapshot file already exists on the HostPath volume. ovs-ofctl dump-flows br-int fails because br-int hasn't been re-created yet by ovn-controller, producing empty output → COUNT=0 → threshold check passes → restore commands fail silently → RESTORED=1. When ovn-controller later clears the flow table during its startup, the watchdog is already spent.

Fix: either check the exit code of add-flows before setting RESTORED=1, or verify the flow count actually increased after the restore attempt.

Suggested change
if [ "$COUNT" -lt "$THRESHOLD" ] && [ "$RESTORED" -eq 0 ]; then
log "flow table cleared (count=$COUNT < threshold=$THRESHOLD), restoring snapshot"
"$OVS_OFCTL" add-groups br-int "$GROUPS_FILE" 2>/dev/null || true
"$OVS_OFCTL" add-flows br-int "$FLOWS_FILE" 2>/dev/null || true
RESTORED=1
log "snapshot restored"
if [ "$COUNT" -lt "$THRESHOLD" ] && [ "$RESTORED" -eq 0 ]; then
log "flow table cleared (count=$COUNT < threshold=$THRESHOLD), restoring snapshot"
"$OVS_OFCTL" add-groups br-int "$GROUPS_FILE" 2>/dev/null || true
if "$OVS_OFCTL" add-flows br-int "$FLOWS_FILE" 2>/dev/null; then
RESTORED=1
log "snapshot restored"
else
log "restore failed, will retry"
fi
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

2 participants