Refactor upgrade options and timeouts #24
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant modifications to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
talos-bootstrap (1)
Line range hint
470-492: Enhance error handling in status monitoring.While the status monitoring has been improved with better timeouts and stage verification, the error handling could be enhanced.
Consider these improvements:
status=$(timeout 3 talosctl --talosconfig=talosconfig -e "${node}" -n "${node}" get machinestatus ${OPTS} -o jsonpath={.spec.stage} 2>/dev/null) if [ $? = 124 ]; then + echo "Warning: Timeout while checking machine status" >&2 old_is_up=0 fi if [ "$status" = upgrading ] || [ "$status" = rebooting ]; then + echo "Info: Machine is in ${status} state" >&2 continue fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
talos-bootstrap(4 hunks)
🔇 Additional comments (1)
talos-bootstrap (1)
453-453: Verify upgrade command behavior with --wait=false.The upgrade command is executed with
--wait=false, which means it will return immediately. Ensure this is the intended behavior and won't cause issues with the subsequent status monitoring.Run this script to check the documentation and usage of the
--waitflag:✅ Verification successful
The --wait=false flag usage is correct and properly implemented.
The upgrade command is intentionally non-blocking, as the script implements its own comprehensive status monitoring using
machinestatuschecks to track the upgrade progress through various stages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check documentation for the upgrade command's --wait flag talosctl upgrade --help | grep -A 2 "\-\-wait"Length of output: 147
Script:
#!/bin/bash # Search for documentation files and examples related to upgrade command fd -e md -e txt | xargs rg -l "upgrade.*wait" || true # Search for specific mentions of wait flag in upgrade contexts rg -i "upgrade.*--wait" -A 2 -B 2 || true # Search for upgrade command usage patterns rg "talosctl.*upgrade" -A 2 -B 2 || trueLength of output: 915
Script:
#!/bin/bash # Look for status monitoring after upgrade rg -A 10 "Scheduling upgrade" || true # Check for any health checks or status monitoring patterns rg "health|status|ready" talos-bootstrap || trueLength of output: 1168
talos-bootstrap
Outdated
| upgrade_opt="" | ||
| case ${upgrade_option} in | ||
| 1) preserve_opt="--preserve" ;; | ||
| 2) preserve_opt="" ;; | ||
| 3) should_upgrade="0" ;; | ||
| 1) should_upgrade=0 ;; | ||
| 3) upgrade_opt="--reboot-mode default" ;; | ||
| 3) upgrade_opt="--reboot-mode powercycle" ;; | ||
| 4) upgrade_opt="--force --reboot-mode default" ;; | ||
| 4) upgrade_opt="--force --reboot-mode powercycle" ;; | ||
| 6) upgrade_opt="--stage" ;; |
There was a problem hiding this comment.
Fix duplicate case numbers and incorrect mode assignments.
The case statement contains several critical issues:
- Duplicate case numbers (3 and 4) will cause unexpected behavior
- Incorrect reboot mode assignments
Apply this diff to fix the case statement:
upgrade_opt=""
case ${upgrade_option} in
1) should_upgrade=0 ;;
- 3) upgrade_opt="--reboot-mode default" ;;
- 3) upgrade_opt="--reboot-mode powercycle" ;;
- 4) upgrade_opt="--force --reboot-mode default" ;;
- 4) upgrade_opt="--force --reboot-mode powercycle" ;;
+ 2) upgrade_opt="--reboot-mode default" ;;
+ 3) upgrade_opt="--reboot-mode powercycle" ;;
+ 4) upgrade_opt="--force --reboot-mode default" ;;
+ 5) upgrade_opt="--force --reboot-mode powercycle" ;;
6) upgrade_opt="--stage" ;;
esac📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| upgrade_opt="" | |
| case ${upgrade_option} in | |
| 1) preserve_opt="--preserve" ;; | |
| 2) preserve_opt="" ;; | |
| 3) should_upgrade="0" ;; | |
| 1) should_upgrade=0 ;; | |
| 3) upgrade_opt="--reboot-mode default" ;; | |
| 3) upgrade_opt="--reboot-mode powercycle" ;; | |
| 4) upgrade_opt="--force --reboot-mode default" ;; | |
| 4) upgrade_opt="--force --reboot-mode powercycle" ;; | |
| 6) upgrade_opt="--stage" ;; | |
| upgrade_opt="" | |
| case ${upgrade_option} in | |
| 1) should_upgrade=0 ;; | |
| 2) upgrade_opt="--reboot-mode default" ;; | |
| 3) upgrade_opt="--reboot-mode powercycle" ;; | |
| 4) upgrade_opt="--force --reboot-mode default" ;; | |
| 5) upgrade_opt="--force --reboot-mode powercycle" ;; | |
| 6) upgrade_opt="--stage" ;; | |
| esac |
| 1 "apply config on live and skip upgrade" \ | ||
| 2 "apply config and perform upgrade (mode: default)" \ | ||
| 3 "apply config and perform upgrade (mode: powercycle)" \ | ||
| 4 "apply config and perform upgrade with force (mode: default)" \ | ||
| 5 "apply config and perform upgrade with force (mode: powercycle)" \ | ||
| 6 "apply config and schedule upgrade to perform it after a reboot" 3>&1 1>&2 2>&3) || exit 0 |
There was a problem hiding this comment.
Fix misalignment between menu options and case handling.
The menu options (1-6) don't align with their corresponding case statements, which could lead to incorrect option handling.
Apply this diff to align the descriptions with their handling:
- 1 "apply config on live and skip upgrade" \
- 2 "apply config and perform upgrade (mode: default)" \
- 3 "apply config and perform upgrade (mode: powercycle)" \
- 4 "apply config and perform upgrade with force (mode: default)" \
- 5 "apply config and perform upgrade with force (mode: powercycle)" \
- 6 "apply config and schedule upgrade to perform it after a reboot" 3>&1 1>&2 2>&3) || exit 0
+ 1 "Apply config on live and skip upgrade" \
+ 2 "Apply config and perform upgrade" \
+ 3 "Apply config and perform upgrade (powercycle mode)" \
+ 4 "Apply config and perform forced upgrade" \
+ 5 "Apply config and perform forced upgrade (powercycle mode)" \
+ 6 "Apply config and schedule upgrade for next reboot" 3>&1 1>&2 2>&3) || exit 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1 "apply config on live and skip upgrade" \ | |
| 2 "apply config and perform upgrade (mode: default)" \ | |
| 3 "apply config and perform upgrade (mode: powercycle)" \ | |
| 4 "apply config and perform upgrade with force (mode: default)" \ | |
| 5 "apply config and perform upgrade with force (mode: powercycle)" \ | |
| 6 "apply config and schedule upgrade to perform it after a reboot" 3>&1 1>&2 2>&3) || exit 0 | |
| 1 "Apply config on live and skip upgrade" \ | |
| 2 "Apply config and perform upgrade" \ | |
| 3 "Apply config and perform upgrade (powercycle mode)" \ | |
| 4 "Apply config and perform forced upgrade" \ | |
| 5 "Apply config and perform forced upgrade (powercycle mode)" \ | |
| 6 "Apply config and schedule upgrade for next reboot" 3>&1 1>&2 2>&3) || exit 0 |
Signed-off-by: Andrei Kvapil <[email protected]>
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit
New Features
Improvements