Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid RPM lock issue #44

Merged
merged 16 commits into from
Nov 25, 2024
Merged

fix: avoid RPM lock issue #44

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f714468
feat: support extra arguments for tailscaled
gberenice Aug 16, 2024
1ed0f2d
Merge branch 'main' into feature/add-extra-flags
gberenice Aug 19, 2024
2464faf
chore: adds prettierignore
gberenice Aug 20, 2024
7fb22e4
Merge branch 'feature/add-extra-flags' of github.com:masterpointio/te…
gberenice Aug 20, 2024
34a1a0a
chore: update trunk less often
gberenice Nov 15, 2024
9aef7ae
chore: update git ignore list
gberenice Nov 15, 2024
d9bc3b0
feat: support aws ssm state
gberenice Nov 15, 2024
66702c5
feat: adds CRabbit config
gberenice Nov 15, 2024
d179278
chore: fix merge conflicts
gberenice Nov 15, 2024
574177c
chore: remove the comment
gberenice Nov 15, 2024
09b372c
feat: support configuring ASG size and capacity
gberenice Nov 17, 2024
a497d78
feat: add SSM write permissions
gberenice Nov 20, 2024
a23768c
fix: remove sudo + switch to dnf + wait for rpm lock
gberenice Nov 21, 2024
bdafa2b
Merge branch 'main' into fix/avoid-rpm-lock-issue
gberenice Nov 21, 2024
5a5b8d2
chore: adds timeout
gberenice Nov 21, 2024
fb8e943
chore: replaces retries instead of checking RPM lock
gberenice Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions userdata.sh.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,31 +4,36 @@ exec > >(tee /var/log/user-data.log | logger -t user-data -s 2>/dev/console) 2>&
echo "Starting user-data script..."

echo "Enabling IP forwarding..."
echo 'net.ipv4.ip_forward = 1' | sudo tee -a /etc/sysctl.conf
echo 'net.ipv6.conf.all.forwarding = 1' | sudo tee -a /etc/sysctl.conf
sudo sysctl -p /etc/sysctl.conf
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 "Waiting for the RPM lock to be released..."
while fuser /var/lib/rpm/.rpm.lock >/dev/null 2>&1 ; do
sleep 1
done
Copy link

@coderabbitai coderabbitai bot Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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

Copy link
Member Author

Choose a reason for hiding this comment

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

This might be a good one.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@Gowiem PR updated with retry logic.


echo "Installing Tailscale..."
sudo yum install -y yum-utils
sudo yum-config-manager --add-repo https://pkgs.tailscale.com/stable/amazon-linux/2/tailscale.repo
sudo yum install -y tailscale
dnf install -y dnf-utils
dnf config-manager --add-repo https://pkgs.tailscale.com/stable/amazon-linux/2/tailscale.repo
dnf install -y tailscale

%{ if tailscaled_extra_flags_enabled == true }
echo "Exporting FLAGS to /etc/default/tailscaled..."
sudo sed -i "s|^FLAGS=.*|FLAGS=\"${tailscaled_extra_flags}\"|" /etc/default/tailscaled
sed -i "s|^FLAGS=.*|FLAGS=\"${tailscaled_extra_flags}\"|" /etc/default/tailscaled
%{ endif }

# Setup tailscale
# Setup Tailscale
echo "Enabling and starting tailscaled service..."
sudo systemctl enable --now tailscaled
systemctl enable --now tailscaled

echo "Waiting for tailscaled to initialize..."
sleep 5

# Start tailscale
# We pass --advertise-tags below even though the authkey being created with those tags should result
# in the same effect. This is to be more explicit because tailscale tags are a complicated topic.
sudo tailscale up \
tailscale up \
gberenice marked this conversation as resolved.
Show resolved Hide resolved
%{ if ssh_enabled == true }--ssh%{ endif } \
%{ if exit_node_enabled == true }--advertise-exit-node%{ endif } \
%{ if tailscale_up_extra_flags_enabled == true }${tailscale_up_extra_flags}%{ endif } \
Expand Down
4 changes: 3 additions & 1 deletion variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,9 @@ variable "ssm_state_enabled" {
default = false
type = bool
description = <<-EOT
Control if tailscaled state is stored in AWS SSM (including preferences and keys). This tells the Tailscale daemon to write + read state from SSM, which unlocks important features like retaining the existing tailscale machine name.
Control if tailscaled state is stored in AWS SSM (including preferences and keys).
This tells the Tailscale daemon to write + read state from SSM,
which unlocks important features like retaining the existing tailscale machine name.
See more in the [docs](https://tailscale.com/kb/1278/tailscaled#flags-to-tailscaled).
EOT
}
Loading