Conversation
Adds a script to simulate losing a host. This script has three different ways of simulating that loss to enable us to develop recovery steps for Swarm and Control Plane/Etcd in parallel.
📝 WalkthroughWalkthroughIntroduces a new Bash script Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds an option to reset the Lima E2E fixture back to its initial state without tearing it down entirely. This can save a significant amount of time between tests.
- Fix etcd simulation for client-mode servers - Remove database services in etcd simulation - Rebuild control-plane in reset - Remove `set -x`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/simulate-disaster.sh`:
- Around line 171-173: The dispatch case labeled "full" calls a nonexistent
function simulate_full_node_loss; update that case to call the actual function
simulate_full_loss (replace simulate_full_node_loss with simulate_full_loss) so
the "full" branch invokes the defined function and won't fail at runtime.
- Around line 121-125: Update the usage text in the simulate-disaster.sh header:
fix the duplicated word "different different" and include the missing `reset`
option in the synopsis string (change "Usage: $1 <swarm|etcd|full> <host-id>
[host-id ...]" to include `reset`, e.g. "Usage: $1 <swarm|etcd|full|reset>
<host-id> [host-id ...]") and adjust the descriptive paragraph to remove the
duplicate word so it reads "three different types of disasters" (or similar).
Ensure you update both the usage line and the description near that header.
- Line 17: Fix the unquoted $@ expansions in the for-loops and correct the
misnamed function call: change the three loops that iterate host_id (the ones
using for host_id in $@) to use quoted expansion (for host_id in "$@") to
prevent word-splitting and globbing, update the calls that forward args to use
quoted slices where shown (simulate_swarm_node_loss "${@:2}",
simulate_etcd_node_loss "${@:2}"), replace the invalid simulate_full_node_loss
call with the actual function name simulate_full_loss and pass quoted args
(simulate_full_loss "${@:2}"), and ensure the script's entry call uses main "$@"
instead of unquoted arguments.
| simulate_swarm_node_loss() { | ||
| local host_id | ||
|
|
||
| for host_id in $@; do |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n hack/simulate-disaster.shRepository: pgEdge/control-plane
Length of output: 6082
🌐 Web query:
shell script quoting $@ best practices word splitting globbing
💡 Result:
Best practices for $@ (and avoiding word splitting + globbing)
-
Forward all arguments exactly:
Use"$@"(quoted). In bash,"$@"expands to separate words:"$1" "$2" ...(preserves spaces, tabs, newlines, and literal* ? [characters in each argument). [1][2] -
Don’t use
$*for forwarding:
$*(especially unquoted) merges args and then undergoes splitting/globbing;"$*"turns all args into one string separated by the first char ofIFS. Prefer"$@". [1][3] -
Loop over args safely:
Prefer either:for arg in "$@"; do ...; done
or (bash) simply:
for arg; do ...; done
Avoid
for arg in $@; ...because unquoted$@triggers word splitting and pathname expansion (globbing). [2] -
Quote variable expansions by default:
Unquoted expansions are subject to field splitting and then pathname expansion (globbing). Quoting suppresses both. [2][4] -
When mixing variables with globs, quote only the variable parts:
for f in "${dir}"/*.txt; do ...; done
not
"${dir}/*.txt"(that would disable the glob). [5] -
If you actually want splitting, do it explicitly (not via unquoted
$var/$@):
Useread -r -a/mapfile(bash) or controlledIFShandling, rather than relying on “sloppy” splitting + globbing. [2]
Sources: Bash Reference Manual on special parameters @/* and quoting [1], ShellCheck guidance on quoting and "$@" [2][3][5], POSIX shell quoting/expansion rules [4].
Fix quoting of $@ expansions and correct the function name on line 172.
Unquoted $@ expansions in this script cause word-splitting and globbing issues, making them fragile with inputs containing spaces or glob characters. Additionally, line 172 calls simulate_full_node_loss which doesn't exist; the function is defined as simulate_full_loss on line 70, causing a runtime error.
Apply these fixes:
- Line 17:
for host_id in "$@"; do - Line 39:
for host_id in "$@"; do - Line 73:
for host_id in "$@"; do - Line 166:
simulate_swarm_node_loss "${@:2}" - Line 169:
simulate_etcd_node_loss "${@:2}" - Line 172:
simulate_full_loss "${@:2}"(corrected function name and quoted) - Line 187:
main "$@"
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 17-17: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/simulate-disaster.sh` at line 17, Fix the unquoted $@ expansions in the
for-loops and correct the misnamed function call: change the three loops that
iterate host_id (the ones using for host_id in $@) to use quoted expansion (for
host_id in "$@") to prevent word-splitting and globbing, update the calls that
forward args to use quoted slices where shown (simulate_swarm_node_loss
"${@:2}", simulate_etcd_node_loss "${@:2}"), replace the invalid
simulate_full_node_loss call with the actual function name simulate_full_loss
and pass quoted args (simulate_full_loss "${@:2}"), and ensure the script's
entry call uses main "$@" instead of unquoted arguments.
| Usage: $1 <swarm|etcd|full> <host-id> [host-id ...] | ||
|
|
||
| Simulates disasters against the Lima test fixtures. Supports three different | ||
| different types of disasters to enable us to develop some recovery steps in | ||
| parallel: |
There was a problem hiding this comment.
Update usage text (typo + include reset).
The synopsis omits reset, and the description says “different different”.
✏️ Suggested fix
-Usage: $1 <swarm|etcd|full> <host-id> [host-id ...]
+Usage: $1 <swarm|etcd|full|reset> <host-id> [host-id ...]
@@
-Simulates disasters against the Lima test fixtures. Supports three different
-different types of disasters to enable us to develop some recovery steps in
+Simulates disasters against the Lima test fixtures. Supports three different
+types of disasters to enable us to develop some recovery steps in📝 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.
| Usage: $1 <swarm|etcd|full> <host-id> [host-id ...] | |
| Simulates disasters against the Lima test fixtures. Supports three different | |
| different types of disasters to enable us to develop some recovery steps in | |
| parallel: | |
| Usage: $1 <swarm|etcd|full|reset> <host-id> [host-id ...] | |
| Simulates disasters against the Lima test fixtures. Supports three different | |
| types of disasters to enable us to develop some recovery steps in | |
| parallel: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/simulate-disaster.sh` around lines 121 - 125, Update the usage text in
the simulate-disaster.sh header: fix the duplicated word "different different"
and include the missing `reset` option in the synopsis string (change "Usage: $1
<swarm|etcd|full> <host-id> [host-id ...]" to include `reset`, e.g. "Usage: $1
<swarm|etcd|full|reset> <host-id> [host-id ...]") and adjust the descriptive
paragraph to remove the duplicate word so it reads "three different types of
disasters" (or similar). Ensure you update both the usage line and the
description near that header.
| full) | ||
| simulate_full_node_loss ${@:2} | ||
| ;; |
There was a problem hiding this comment.
Fix the full dispatch: undefined function call.
full currently calls simulate_full_node_loss, but the defined function is simulate_full_loss, so the full path will fail at runtime.
✅ Suggested fix
- full)
- simulate_full_node_loss ${@:2}
+ full)
+ simulate_full_loss ${@:2}
;;📝 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.
| full) | |
| simulate_full_node_loss ${@:2} | |
| ;; | |
| full) | |
| simulate_full_loss ${@:2} | |
| ;; |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 172-172: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/simulate-disaster.sh` around lines 171 - 173, The dispatch case labeled
"full" calls a nonexistent function simulate_full_node_loss; update that case to
call the actual function simulate_full_loss (replace simulate_full_node_loss
with simulate_full_loss) so the "full" branch invokes the defined function and
won't fail at runtime.
2 participants
Summary
Adds a script to simulate losing a host. This script has three different ways of simulating that loss to enable us to develop recovery steps for Swarm and Control Plane/Etcd in parallel.
Testing
NOTE - You can get this script without checking out this whole branch by doing:
Then to use the script: