Repository navigation
Add machine initialization script for e2e CI runners - #75
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Warning Review limit reached
More reviews will be available in 49 minutes and 14 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughA new Bash script ChangesBare-metal OSAC CI provisioning script
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/machine-init.sh`:
- Around line 304-307: The pkill -x dnsmasq command indiscriminately kills all
dnsmasq processes, including the instance managed by libvirt for VM networking,
which breaks DNS/DHCP for any running virtual machines. Remove the pkill -x
dnsmasq line and the subsequent sleep 1 command, allowing NetworkManager to
manage its own dnsmasq lifecycle when systemctl restart NetworkManager is
executed. This way, only NetworkManager's dnsmasq instance will be restarted
while libvirt's instance remains unaffected.
- Around line 45-47: The --data-path argument parsing lacks validation to ensure
that a value is actually provided after the flag. When the flag is used without
a following argument, $2 will be unset causing an unclear failure with set -u.
Before assigning $2 to DATA_PATH in the --data-path case, add a check to verify
that $2 exists and is not empty. If the argument is missing, the script should
display a clear error message indicating that --data-path requires a value and
then exit with an appropriate error code.
- Around line 200-207: The curl download piped directly to tar silently fails if
the network request errors or returns an error status, since the -s flag
suppresses output and tar will attempt extraction on partial or empty data.
Instead of piping curl to tar, download the file to a temporary location first
within the TMP_OC directory, verify the download succeeded (by checking the curl
exit status or file size), and then extract from the downloaded file. This
ensures download failures are caught before attempting to install the oc and
kubectl binaries from potentially corrupted data.
- Around line 257-260: The DATA_PATH variable is being written unquoted to the
config file in the echo statement that writes CLUSTER_TOOL_DATA. If DATA_PATH
contains spaces, this will result in a malformed config file. Quote the
DATA_PATH variable expansion using double quotes when writing it to ensure the
value is properly preserved even if it contains spaces.
- Around line 310-316: The DNS verification loop that checks for 127.0.0.1 in
/etc/resolv.conf using grep within the for loop iterating 30 times does not
provide any warning if the condition is never satisfied after all iterations
complete. Add a flag variable before the for loop to track whether DNS was
successfully configured, set this flag to true when the grep condition succeeds
(inside the if block), and after the loop completes, add a conditional check
that logs a warning or error message if the flag indicates DNS configuration
failed. This ensures that DNS misconfiguration issues are surfaced to the user
instead of silently allowing the script to continue.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9151d88e-5021-448f-ac56-ca45fd0a9eaa
📒 Files selected for processing (1)
scripts/machine-init.sh
| --data-path) | ||
| DATA_PATH="$2" | ||
| shift 2 |
There was a problem hiding this comment.
Missing argument validation for --data-path.
If a user runs ./machine-init.sh --data-path without providing a value, $2 will be unset, causing an unclear failure with set -u. Validate that the argument exists before using it.
Proposed fix
--data-path)
+ if [[ -z "${2:-}" ]]; then
+ echo "ERROR: --data-path requires a value." >&2
+ exit 1
+ fi
DATA_PATH="$2"
shift 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.
| --data-path) | |
| DATA_PATH="$2" | |
| shift 2 | |
| --data-path) | |
| if [[ -z "${2:-}" ]]; then | |
| echo "ERROR: --data-path requires a value." >&2 | |
| exit 1 | |
| fi | |
| DATA_PATH="$2" | |
| shift 2 | |
| ;; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/machine-init.sh` around lines 45 - 47, The --data-path argument
parsing lacks validation to ensure that a value is actually provided after the
flag. When the flag is used without a following argument, $2 will be unset
causing an unclear failure with set -u. Before assigning $2 to DATA_PATH in the
--data-path case, add a check to verify that $2 exists and is not empty. If the
argument is missing, the script should display a clear error message indicating
that --data-path requires a value and then exit with an appropriate error code.
| OC_URL="https://mirror.openshift.com/pub/openshift-v4/clients/ocp/stable/openshift-client-linux.tar.gz" | ||
| echo " Downloading from ${OC_URL}..." | ||
| TMP_OC=$(mktemp -d) | ||
| curl -sL "${OC_URL}" | tar xz -C "${TMP_OC}" | ||
| install -m 0755 "${TMP_OC}/oc" /usr/local/bin/oc | ||
| install -m 0755 "${TMP_OC}/kubectl" /usr/local/bin/kubectl 2>/dev/null || true | ||
| rm -rf "${TMP_OC}" | ||
| echo " oc installed: $(oc version --client 2>/dev/null | head -1)" |
There was a problem hiding this comment.
Download failure not detected when piping curl to tar.
If the download fails (network error, 404, etc.), curl -s silently fails and tar receives partial/empty data. This could result in a missing or corrupted oc binary. Download to a file first and verify success.
Proposed fix
OC_URL="https://mirror.openshift.com/pub/openshift-v4/clients/ocp/stable/openshift-client-linux.tar.gz"
echo " Downloading from ${OC_URL}..."
TMP_OC=$(mktemp -d)
- curl -sL "${OC_URL}" | tar xz -C "${TMP_OC}"
+ if ! curl -fSL "${OC_URL}" -o "${TMP_OC}/oc.tar.gz"; then
+ echo "ERROR: Failed to download OpenShift CLI." >&2
+ rm -rf "${TMP_OC}"
+ exit 1
+ fi
+ tar xz -C "${TMP_OC}" -f "${TMP_OC}/oc.tar.gz"
install -m 0755 "${TMP_OC}/oc" /usr/local/bin/oc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/machine-init.sh` around lines 200 - 207, The curl download piped
directly to tar silently fails if the network request errors or returns an error
status, since the -s flag suppresses output and tar will attempt extraction on
partial or empty data. Instead of piping curl to tar, download the file to a
temporary location first within the TMP_OC directory, verify the download
succeeded (by checking the curl exit status or file size), and then extract from
the downloaded file. This ensures download failures are caught before attempting
to install the oc and kubectl binaries from potentially corrupted data.
| # Write config (only if not already present) | ||
| if [[ ! -f "${CT_CONFIG_DIR}/config" ]]; then | ||
| echo "CLUSTER_TOOL_DATA=${DATA_PATH}" > "${CT_CONFIG_DIR}/config" | ||
| fi |
There was a problem hiding this comment.
DATA_PATH is written unquoted to config file.
If DATA_PATH contains spaces (unlikely but possible), the config file will be malformed. Quote the value for robustness.
Proposed fix
# Write config (only if not already present)
if [[ ! -f "${CT_CONFIG_DIR}/config" ]]; then
- echo "CLUSTER_TOOL_DATA=${DATA_PATH}" > "${CT_CONFIG_DIR}/config"
+ echo "CLUSTER_TOOL_DATA=\"${DATA_PATH}\"" > "${CT_CONFIG_DIR}/config"
fi📝 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.
| # Write config (only if not already present) | |
| if [[ ! -f "${CT_CONFIG_DIR}/config" ]]; then | |
| echo "CLUSTER_TOOL_DATA=${DATA_PATH}" > "${CT_CONFIG_DIR}/config" | |
| fi | |
| # Write config (only if not already present) | |
| if [[ ! -f "${CT_CONFIG_DIR}/config" ]]; then | |
| echo "CLUSTER_TOOL_DATA=\"${DATA_PATH}\"" > "${CT_CONFIG_DIR}/config" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/machine-init.sh` around lines 257 - 260, The DATA_PATH variable is
being written unquoted to the config file in the echo statement that writes
CLUSTER_TOOL_DATA. If DATA_PATH contains spaces, this will result in a malformed
config file. Quote the DATA_PATH variable expansion using double quotes when
writing it to ensure the value is properly preserved even if it contains spaces.
| # Kill stale dnsmasq processes before restart | ||
| pkill -x dnsmasq 2>/dev/null || true | ||
| sleep 1 | ||
| systemctl restart NetworkManager |
There was a problem hiding this comment.
pkill -x dnsmasq kills all dnsmasq processes, including libvirt's.
Libvirt runs its own dnsmasq instance for VM networking (default network). Killing all dnsmasq processes will break DNS/DHCP for any running VMs and may cause libvirt network issues. NetworkManager should manage its own dnsmasq lifecycle on restart.
Proposed fix — let NetworkManager handle dnsmasq
- # Kill stale dnsmasq processes before restart
- pkill -x dnsmasq 2>/dev/null || true
- sleep 1
systemctl restart NetworkManager
+ sleep 2 # Give NetworkManager time to spawn dnsmasq📝 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.
| # Kill stale dnsmasq processes before restart | |
| pkill -x dnsmasq 2>/dev/null || true | |
| sleep 1 | |
| systemctl restart NetworkManager | |
| systemctl restart NetworkManager | |
| sleep 2 # Give NetworkManager time to spawn dnsmasq |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/machine-init.sh` around lines 304 - 307, The pkill -x dnsmasq command
indiscriminately kills all dnsmasq processes, including the instance managed by
libvirt for VM networking, which breaks DNS/DHCP for any running virtual
machines. Remove the pkill -x dnsmasq line and the subsequent sleep 1 command,
allowing NetworkManager to manage its own dnsmasq lifecycle when systemctl
restart NetworkManager is executed. This way, only NetworkManager's dnsmasq
instance will be restarted while libvirt's instance remains unaffected.
| for _ in $(seq 1 30); do | ||
| if grep -q '127.0.0.1' /etc/resolv.conf 2>/dev/null; then | ||
| echo " DNS configured (resolv.conf -> 127.0.0.1)" | ||
| break | ||
| fi | ||
| sleep 0.5 | ||
| done |
There was a problem hiding this comment.
DNS verification loop silently continues on timeout.
If /etc/resolv.conf never points to 127.0.0.1 after 30 iterations (15 seconds), the script continues without warning. This masks a DNS misconfiguration that will cause cluster-tool failures later.
Proposed fix
# Verify resolv.conf points to localhost
+ dns_ok=false
for _ in $(seq 1 30); do
if grep -q '127.0.0.1' /etc/resolv.conf 2>/dev/null; then
echo " DNS configured (resolv.conf -> 127.0.0.1)"
+ dns_ok=true
break
fi
sleep 0.5
done
+ if [[ "${dns_ok}" != "true" ]]; then
+ echo " WARNING: resolv.conf does not point to 127.0.0.1. DNS may not work correctly."
+ fi
fi📝 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.
| for _ in $(seq 1 30); do | |
| if grep -q '127.0.0.1' /etc/resolv.conf 2>/dev/null; then | |
| echo " DNS configured (resolv.conf -> 127.0.0.1)" | |
| break | |
| fi | |
| sleep 0.5 | |
| done | |
| dns_ok=false | |
| for _ in $(seq 1 30); do | |
| if grep -q '127.0.0.1' /etc/resolv.conf 2>/dev/null; then | |
| echo " DNS configured (resolv.conf -> 127.0.0.1)" | |
| dns_ok=true | |
| break | |
| fi | |
| sleep 0.5 | |
| done | |
| if [[ "${dns_ok}" != "true" ]]; then | |
| echo " WARNING: resolv.conf does not point to 127.0.0.1. DNS may not work correctly." | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/machine-init.sh` around lines 310 - 316, The DNS verification loop
that checks for 127.0.0.1 in /etc/resolv.conf using grep within the for loop
iterating 30 times does not provide any warning if the condition is never
satisfied after all iterations complete. Add a flag variable before the for loop
to track whether DNS was successfully configured, set this flag to true when the
grep condition succeeds (inside the if block), and after the loop completes, add
a conditional check that logs a warning or error message if the flag indicates
DNS configuration failed. This ensures that DNS misconfiguration issues are
surfaced to the user instead of silently allowing the script to continue.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/machine-init.sh`:
- Around line 229-230: The curl command downloading the OSAC binary to
/usr/local/bin/osac uses the `-s` flag which suppresses all output including
error messages, making debugging download failures difficult. Modify the curl
command by adding the `-S` flag to the existing flags (changing `-Lsfo` to
`-LSfo`) so that error messages are displayed while progress indicators remain
hidden, allowing the script to show what went wrong if the download fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e695587b-e1b2-45c0-9799-584ea8c7629e
📒 Files selected for processing (1)
scripts/machine-init.sh
| curl -Lsfo /usr/local/bin/osac "${OSAC_URL}" | ||
| chmod +x /usr/local/bin/osac |
There was a problem hiding this comment.
Add -S flag to show errors on download failure.
The -s flag suppresses all output, including error messages. If the download fails (404, network error), the script exits with no indication of what went wrong. Add -S to re-enable error messages while keeping progress hidden.
Proposed fix
- curl -Lsfo /usr/local/bin/osac "${OSAC_URL}"
+ curl -fsSLo /usr/local/bin/osac "${OSAC_URL}"📝 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.
| curl -Lsfo /usr/local/bin/osac "${OSAC_URL}" | |
| chmod +x /usr/local/bin/osac | |
| curl -fsSLo /usr/local/bin/osac "${OSAC_URL}" | |
| chmod +x /usr/local/bin/osac |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/machine-init.sh` around lines 229 - 230, The curl command downloading
the OSAC binary to /usr/local/bin/osac uses the `-s` flag which suppresses all
output including error messages, making debugging download failures difficult.
Modify the curl command by adding the `-S` flag to the existing flags (changing
`-Lsfo` to `-LSfo`) so that error messages are displayed while progress
indicators remain hidden, allowing the script to show what went wrong if the
download fails.
Adds scripts/machine-init.sh which prepares a bare-metal machine for OSAC e2e CI: system packages, services (libvirtd, HAProxy, podman), OpenShift CLI, osac CLI (auto-detects latest release), cluster-tool in local mode, Vault CLI, and a verification step. Supports selective steps and --data-path for cluster-tool storage.
b44b28e to
12845bd
Compare
|
🏗️ CI Triage: Root cause: The boot step failed to pull the cluster snapshot image from quay.io due to a registry or network connection issue. Explanation: During the 'osac-project-cluster-tool-boot' step, the cluster-tool attempted to pull the 'vmaas-kustomize' flavor from quay.io. The skopeo copy command failed with multiple 'unexpected EOF' errors while reading blob bodies, indicating a network interruption or registry unavailability. This is an infrastructure issue unrelated to the PR's code. Evidence: Suggestion: Retrigger the job. This is a transient infrastructure issue with the Quay.io registry or network connectivity. Prow job | Build For deeper investigation, use the |
2 participants
Summary
Single script (
scripts/machine-init.sh) to initialize a bare-metal machine for OSAC e2e CI. Installs all dependencies, configures cluster-tool in local mode, sets up DNS and HAProxy.Steps
packagesservicesoccluster-toolvaultverifyUsage
Design
--host local(runner IS the bare-metal machine)osac-ci-1: tested step-by-step on the actual runner machineSummary by CodeRabbit