fix: avoid RPM lock issue #44
Conversation
…rraform-aws-tailscale into feature/add-extra-flags
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (3)
userdata.sh.tmpl (3)
7-9: Consider using /etc/sysctl.d/ for modular configurationWhile the current approach works, it's considered a best practice to place custom sysctl configurations in
/etc/sysctl.d/for better organization and maintainability.-echo 'net.ipv4.ip_forward = 1' >> /etc/sysctl.conf -echo 'net.ipv6.conf.all.forwarding = 1' >> /etc/sysctl.conf -sysctl -p /etc/sysctl.conf +echo 'net.ipv4.ip_forward = 1' > /etc/sysctl.d/99-tailscale.conf +echo 'net.ipv6.conf.all.forwarding = 1' >> /etc/sysctl.d/99-tailscale.conf +sysctl -p /etc/sysctl.d/99-tailscale.conf
17-19: Add error handling for package installationThe transition to
dnfis appropriate for Amazon Linux 2023. Consider adding error handling for package installation failures.+set +e # Temporarily disable exit on error dnf install -y dnf-utils +if [ $? -ne 0 ]; then + echo "Failed to install dnf-utils" + exit 1 +fi dnf config-manager --add-repo https://pkgs.tailscale.com/stable/amazon-linux/2/tailscale.repo +if [ $? -ne 0 ]; then + echo "Failed to add Tailscale repository" + exit 1 +fi dnf install -y tailscale +if [ $? -ne 0 ]; then + echo "Failed to install Tailscale" + exit 1 +fi +set -e # Re-enable exit on error
Line range hint
36-44: Validate Tailscale configuration before applyingThe Tailscale configuration looks correct, but it would be beneficial to validate the inputs before applying them.
+# Validate required inputs +if [ -z "${routes}" ]; then + echo "Error: routes must be specified" + exit 1 +fi +if [ -z "${tags}" ]; then + echo "Error: tags must be specified" + exit 1 +fi +if [ -z "${hostname}" ]; then + echo "Error: hostname must be specified" + exit 1 +fi +if [ -z "${authkey}" ]; then + echo "Error: authkey must be specified" + exit 1 +fi + tailscale up \ %{ if ssh_enabled == true }--ssh%{ endif } \ %{ if exit_node_enabled == true }--advertise-exit-node%{ endif } \
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
userdata.sh.tmpl(1 hunks)variables.tf(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- variables.tf
userdata.sh.tmpl
Outdated
| echo "Waiting for the RPM lock to be released..." | ||
| while fuser /var/lib/rpm/.rpm.lock >/dev/null 2>&1 ; do | ||
| sleep 1 | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and error handling to RPM lock check
While the lock check is a good addition, it could benefit from a timeout to prevent infinite waiting and proper error handling.
echo "Waiting for the RPM lock to be released..."
+TIMEOUT=300 # 5 minutes timeout
+start_time=$(date +%s)
+if ! command -v fuser >/dev/null 2>&1; then
+ echo "fuser command not found, installing psmisc..."
+ dnf install -y psmisc
+fi
while fuser /var/lib/rpm/.rpm.lock >/dev/null 2>&1 ; do
+ current_time=$(date +%s)
+ if (( current_time - start_time > TIMEOUT )); then
+ echo "Timeout waiting for RPM lock to be released"
+ exit 1
+ fi
sleep 1
done📝 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.
| echo "Waiting for the RPM lock to be released..." | |
| while fuser /var/lib/rpm/.rpm.lock >/dev/null 2>&1 ; do | |
| sleep 1 | |
| done | |
| echo "Waiting for the RPM lock to be released..." | |
| TIMEOUT=300 # 5 minutes timeout | |
| start_time=$(date +%s) | |
| if ! command -v fuser >/dev/null 2>&1; then | |
| echo "fuser command not found, installing psmisc..." | |
| dnf install -y psmisc | |
| fi | |
| while fuser /var/lib/rpm/.rpm.lock >/dev/null 2>&1 ; do | |
| current_time=$(date +%s) | |
| if (( current_time - start_time > TIMEOUT )); then | |
| echo "Timeout waiting for RPM lock to be released" | |
| exit 1 | |
| fi | |
| sleep 1 | |
| done |
There was a problem hiding this comment.
This might be a good one.
There was a problem hiding this comment.
@gberenice, glad you think this could be useful! Incorporating a timeout and error handling will ensure your script doesn't hang indefinitely if the RPM lock isn't released, improving its robustness. If you need any assistance implementing it, feel free to ask.
There was a problem hiding this comment.
@gberenice yeah, I think this is good. Make the timeout 30 seconds -- the lock should never be that long and we want to have this user-data script be speedy if we can.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@Gowiem I've researched more about this issue, and it's recommended the following:
when using user data via cloud-init it is possible that other processes will be running simultaneously. If those processes are out of your control, it is best to just implement the required retry mechanisms in your own scripts.
I'll add a retry tomorrow and ping you for another round of review.
There was a problem hiding this comment.
@Gowiem PR updated with retry logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
userdata.sh.tmpl (2)
40-42: Optimize package installation checksThe installation commands are properly sequenced and use the correct package manager (dnf) for AL2023. Consider adding existence checks to avoid unnecessary reinstallation attempts.
+if ! rpm -q dnf-utils >/dev/null 2>&1; then retry_command "dnf install -y dnf-utils" 5 +fi +if ! rpm -q tailscale >/dev/null 2>&1; then retry_command "dnf config-manager --add-repo https://pkgs.tailscale.com/stable/amazon-linux/2/tailscale.repo" 5 retry_command "dnf install -y tailscale" 5 +fi
46-46: Add error handling for FLAGS configurationThe sed command could fail silently if the file or pattern doesn't exist.
-sed -i "s|^FLAGS=.*|FLAGS=\"${tailscaled_extra_flags}\"|" /etc/default/tailscaled +if [ -f /etc/default/tailscaled ]; then + sed -i "s|^FLAGS=.*|FLAGS=\"${tailscaled_extra_flags}\"|" /etc/default/tailscaled || { + echo "Failed to update tailscaled FLAGS" + exit 1 + } +else + echo "ERROR: /etc/default/tailscaled not found" + exit 1 +fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
userdata.sh.tmpl(1 hunks)
🔇 Additional comments (1)
userdata.sh.tmpl (1)
7-9: LGTM: IP forwarding configuration is correct
The IP forwarding setup is properly configured for both IPv4 and IPv6, and the removal of sudo is appropriate since the script runs with root privileges.
| # Function to retry a command up to a maximum number of attempts | ||
| retry_command() { | ||
| local cmd="$1" | ||
| local max_attempts="$2" | ||
| local attempt=1 | ||
| local exit_code=0 | ||
|
|
||
| while [ $attempt -le $max_attempts ]; do | ||
| echo "Attempt $attempt of $max_attempts: $cmd" | ||
| eval "$cmd" | ||
| exit_code=$? | ||
| if [ $exit_code -eq 0 ]; then | ||
| echo "Command succeeded: $cmd" | ||
| return 0 | ||
| else | ||
| echo "Command failed with exit code $exit_code: $cmd" | ||
| attempt=$((attempt + 1)) | ||
| if [ $attempt -le $max_attempts ]; then | ||
| echo "Retrying in 2 seconds..." | ||
| sleep 2 | ||
| fi | ||
| fi | ||
| done | ||
|
|
||
| echo "Command failed after $max_attempts attempts: $cmd" | ||
| return $exit_code | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and cleanup handling to retry mechanism
While the retry logic is well-implemented, consider these enhancements:
- Add the agreed-upon 30-second timeout
- Include cleanup handling for interrupted execution
retry_command() {
local cmd="$1"
local max_attempts="$2"
local attempt=1
local exit_code=0
+ local start_time=$(date +%s)
+ local timeout=30 # 30-second timeout
+
+ # Setup cleanup trap
+ trap 'echo "Command interrupted: $cmd"; return 1' INT TERM
while [ $attempt -le $max_attempts ]; do
+ current_time=$(date +%s)
+ if (( current_time - start_time > timeout )); then
+ echo "Timeout after ${timeout} seconds: $cmd"
+ return 124 # Standard timeout exit code
+ fi
echo "Attempt $attempt of $max_attempts: $cmd"📝 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.
| # Function to retry a command up to a maximum number of attempts | |
| retry_command() { | |
| local cmd="$1" | |
| local max_attempts="$2" | |
| local attempt=1 | |
| local exit_code=0 | |
| while [ $attempt -le $max_attempts ]; do | |
| echo "Attempt $attempt of $max_attempts: $cmd" | |
| eval "$cmd" | |
| exit_code=$? | |
| if [ $exit_code -eq 0 ]; then | |
| echo "Command succeeded: $cmd" | |
| return 0 | |
| else | |
| echo "Command failed with exit code $exit_code: $cmd" | |
| attempt=$((attempt + 1)) | |
| if [ $attempt -le $max_attempts ]; then | |
| echo "Retrying in 2 seconds..." | |
| sleep 2 | |
| fi | |
| fi | |
| done | |
| echo "Command failed after $max_attempts attempts: $cmd" | |
| return $exit_code | |
| } | |
| # Function to retry a command up to a maximum number of attempts | |
| retry_command() { | |
| local cmd="$1" | |
| local max_attempts="$2" | |
| local attempt=1 | |
| local exit_code=0 | |
| local start_time=$(date +%s) | |
| local timeout=30 # 30-second timeout | |
| # Setup cleanup trap | |
| trap 'echo "Command interrupted: $cmd"; return 1' INT TERM | |
| while [ $attempt -le $max_attempts ]; do | |
| current_time=$(date +%s) | |
| if (( current_time - start_time > timeout )); then | |
| echo "Timeout after ${timeout} seconds: $cmd" | |
| return 124 # Standard timeout exit code | |
| fi | |
| echo "Attempt $attempt of $max_attempts: $cmd" | |
| eval "$cmd" | |
| exit_code=$? | |
| if [ $exit_code -eq 0 ]; then | |
| echo "Command succeeded: $cmd" | |
| return 0 | |
| else | |
| echo "Command failed with exit code $exit_code: $cmd" | |
| attempt=$((attempt + 1)) | |
| if [ $attempt -le $max_attempts ]; then | |
| echo "Retrying in 2 seconds..." | |
| sleep 2 | |
| fi | |
| fi | |
| done | |
| echo "Command failed after $max_attempts attempts: $cmd" | |
| return $exit_code | |
| } |
🤖 I have created a release *beep* *boop* --- ## [1.5.1](v1.5.0...v1.5.1) (2024-11-25) ### Bug Fixes * avoid RPM lock issue ([#44](#44)) ([30b0aca](30b0aca)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
what
This:
sudocommands since the user-data script runs as the root user.dnfinstead of yum: Amazon Linux 2023 uses dnf as the default package manager.dnfis the successor toyum.why
references
Summary by CodeRabbit
New Features
Bug Fixes
sudocalls for a cleaner execution process.Documentation
ssm_state_enabledvariable description in the configuration file.