chore(ansible): add RHEL 9.6 and RHEL 10 ppc64le boot images - #3390
chore(ansible): add RHEL 9.6 and RHEL 10 ppc64le boot images#3390mdafsanhossain wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3390 +/- ##
==========================================
+ Coverage 27.34% 27.43% +0.08%
==========================================
Files 95 94 -1
Lines 5420 5406 -14
Branches 2545 2535 -10
==========================================
+ Hits 1482 1483 +1
+ Misses 3211 3198 -13
+ Partials 727 725 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
/retest |
Signed-off-by: mdafsanhossain <Mdafsan.Hossain@ibm.com>
Signed-off-by: mdafsanhossain <Mdafsan.Hossain@ibm.com>
Signed-off-by: mdafsanhossain <Mdafsan.Hossain@ibm.com>
Signed-off-by: mdafsanhossain <Mdafsan.Hossain@ibm.com>
This reverts commit 564dfef.
Signed-off-by: mdafsanhossain <Mdafsan.Hossain@ibm.com>
Signed-off-by: mdafsanhossain <Mdafsan.Hossain@ibm.com>
23e5fc0 to
6d98049
Compare
|
Hi @mdafsanhossain, sorry for the delay on the review for this, it seems the tests are failing because Civet web server is unable to bind port 8080 Since this only happens on the updated images, I assume there might be something else needed in them to address this, do you need help debugging this issue? |
Thanks @Molter73, I think I am seeing this only on RHEL 10. I am trying to check locally if the image has something else running on that port. I will ask you for help if needed. |
Signed-off-by: Afsan Hossain <Mdafsan.Hossain@ibm.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates PPC64LE RHEL images, inventory registration, VM provisioning, container image pull privilege handling, and Podman authentication file selection. ChangesPPC64LE test setup
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 4
🤖 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 `@ansible/roles/provision-vm/tasks/redhat.yml`:
- Around line 67-73: Update the cockpit.socket task to remove broad
ignore_errors handling and tolerate only an explicitly absent unit; ensure other
stop or disable failures from the ansible.builtin.systemd operation fail
provisioning, while preserving the existing ppc64le condition.
In `@ansible/roles/run-test-target/tasks/test-docker.yml`:
- Line 66: Replace the direct copy in the Docker authentication task with an
idempotent merge of AUTH_FILE into the existing ~/.docker/config.json,
preserving credentials already configured by earlier tasks while updating Podman
entries. Remove reliance on the creates guard, generate one explicit merged auth
file, and ensure the resulting config is used for the test artifact container.
- Line 66: Update the credential-copy step around AUTH_FILE to enforce mode 0600
on ~/.docker/config.json after copying, ensuring group and world users cannot
read the registry credentials while preserving the existing copy behavior.
- Around line 58-60: Update the auth-file checks in the shell block around
AUTH_FILE to use POSIX-compatible [ -f ... ] test syntax instead of
Bash-specific [[ ... ]], preserving the existing XDG_RUNTIME_DIR and
/run/containers/0 paths and fallback behavior.
🪄 Autofix
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 15455fc0-021c-417e-9802-680995ca3e9c
📒 Files selected for processing (7)
ansible/group_vars/all.ymlansible/group_vars/platform_rhel-ppc64le.ymlansible/roles/create-vm/tasks/create-ppc64le-vm.ymlansible/roles/provision-vm/tasks/redhat.ymlansible/roles/run-test-target/tasks/pull-images.ymlansible/roles/run-test-target/tasks/test-docker.ymlansible/vars/ppc64le.yml
| - name: Disable cockpit to free port 9090 for collector metrics | ||
| ansible.builtin.systemd: | ||
| name: cockpit.socket | ||
| state: stopped | ||
| enabled: false | ||
| when: vm_arch == "ppc64le" | ||
| ignore_errors: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not ignore failure of the cockpit state change.
ignore_errors: true allows provisioning to continue when cockpit.socket was not stopped or disabled. The collector can then start with the same port conflict that this task is intended to prevent. Tolerate only an explicitly absent unit, and fail on other systemd errors.
As per path instructions, focus on major issues impacting performance, readability, maintainability and security and avoid nitpicks.
🤖 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 `@ansible/roles/provision-vm/tasks/redhat.yml` around lines 67 - 73, Update the
cockpit.socket task to remove broad ignore_errors handling and tolerate only an
explicitly absent unit; ensure other stop or disable failures from the
ansible.builtin.systemd operation fail provisioning, while preserving the
existing ppc64le condition.
Source: Path instructions
| if [[ -f "${XDG_RUNTIME_DIR:-}/containers/auth.json" ]]; then | ||
| AUTH_FILE="${XDG_RUNTIME_DIR:-}/containers/auth.json" | ||
| elif [[ -f "/run/containers/0/auth.json" ]]; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
readlink -f /bin/sh 2>/dev/null || true
/bin/sh -c '[[ -f /etc/hosts ]]'Repository: stackrox/collector
Length of output: 170
🏁 Script executed:
#!/bin/sh
set -eu
echo "Relevant task snippet:"
sed -n '1,130p' ansible/roles/run-test-target/tasks/test-docker.yml
echo
echo "Ansible shell usages in target file:"
rg -n "executable:|/bin/(sh|bash)|shell:" ansible/roles/run-test-target/tasks/test-docker.yml
echo
echo "Target file size:"
wc -l ansible/roles/run-test-target/tasks/test-docker.ymlRepository: stackrox/collector
Length of output: 4640
🌐 Web query:
Ansible shell module default executable /bin/sh documentation
💡 Result:
The Ansible shell module, specifically ansible.builtin.shell, is designed to execute commands on a remote node by running them through a shell [1][2]. By default, this module uses /bin/sh as the shell executable to interpret and run the provided commands [1][3][4]. If you need to use a different shell or an alternative interpreter, you can utilize the executable parameter [1][2][5]. This parameter allows you to specify the absolute path to the desired shell or command interpreter [1][2]. For example, to use /bin/bash instead of the default /bin/sh to support non-POSIX shell features, you would define it in your task as follows [1][2]: - name: Run a command using bash ansible.builtin.shell: cat < /tmp/*txt args: executable: /bin/bash The executable parameter is a flexible option that can also point to other interpreters, such as /usr/bin/expect, to execute scripts written for specific environments [5].
Citations:
- 1: https://docs.ansible.com/projects/ansible/latest/collections/ansible/builtin/shell_module.html
- 2: https://docs.ansible.com/projects/ansible-core/devel/collections/ansible/builtin/shell_module.html
- 3: https://docs.ansible.com/projects/ansible-core/2.19/collections/ansible/builtin/shell%5Fmodule.html
- 4: https://docs.ansible.com/projects/ansible/11/collections/ansible/builtin/shell_module.html
- 5: https://docs.ansible.com/projects/ansible/8/collections/ansible/builtin/shell_module.html
🌐 Web query:
Ubuntu 24.04 /bin/sh dash [[ -f ]] behavior
💡 Result:
In Ubuntu 24.04, the system shell /bin/sh is a symbolic link to dash (the Debian Almquist Shell) [1][2][3]. Dash does not support the [[... ]] keyword, which is an extension found in Bash, KornShell (ksh), and Zsh [4][5][6][7]. If a script uses [[... ]] while being executed by /bin/sh (dash), the following behavior occurs: 1. Failure to Recognize Keyword: Dash treats [[ as a literal command name rather than a shell keyword [8][5][6]. It attempts to locate a command named [[ in the system's PATH [8]. 2. Execution Result: Since the command [[ is not found in the PATH, the shell typically reports that the command was not found (e.g., "[[: not found") and returns a non-zero exit status [8]. If the script logic relies on the exit status of this "command," it will fail as expected, though sometimes developers mistakenly interpret the lack of a syntax error as successful execution [8]. 3. Portability Requirement: To ensure scripts are portable and compatible with /bin/sh, you must use the standard [... ] (test) command [2][5][7]. When converting [[... ]] to the portable [... ] syntax, keep the following differences in mind: - Quoting: Always quote variables inside [ ] to prevent word splitting and globbing issues (e.g., [ "$var" = "value" ]) [5][9][10]. - Logical Operators: [ ] uses -a and -o for AND/OR, or preferably, the shell's && and || operators outside of the brackets (e.g., [ condition1 ] && [ condition2 ]) [9][11]. - Equality: Use the single = operator instead of the == extension supported by Bash [2][5].
Citations:
- 1: https://askubuntu.com/questions/1527266/ubuntu-24-04-i-changed-bin-sh-link-to-point-to-bash-instead-of-dash-now-some-th
- 2: https://wiki.ubuntu.com/DashAsBinSh
- 3: https://learn.microsoft.com/en-us/answers/questions/2262631/vm-default-shell-sh-vs-bash-in-ubuntu-22-04-and-24
- 4: https://askubuntu.com/questions/1277922/what-are-syntax-differences-between-bash-and-dash
- 5: https://shellmap.eversources.app/compare/dash-vs-bash
- 6: https://shellmap.eversources.app/cmd/double-bracket
- 7: https://unix.stackexchange.com/questions/168255/is-test-or-or-more-portable-both-between-bash-shells-and-between-other-shel
- 8: https://unix.stackexchange.com/questions/650942/shell-script-returns-0-exit-status-despite-syntax-error
- 9: https://unix.stackexchange.com/questions/784363/is-posix-compliant-while-is-not
- 10: https://unix.stackexchange.com/questions/49007/when-should-i-use-vs-in-bash-single-vs-double-brackets
- 11: https://www.baeldung.com/linux/bash-single-vs-double-brackets
Use shell syntax supported by Ansible’s default shell.
Ansible shell tasks run through /bin/sh by default. Lines 58 and 60 use Bash [[ ... ]], which fails on /bin/sh implementations such as Dash, causing this task to exit 1 even when an auth file exists. Use POSIX [ -f ... ] tests, or add executable: /bin/bash to this task.
🤖 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 `@ansible/roles/run-test-target/tasks/test-docker.yml` around lines 58 - 60,
Update the auth-file checks in the shell block around AUTH_FILE to use
POSIX-compatible [ -f ... ] test syntax instead of Bash-specific [[ ... ]],
preserving the existing XDG_RUNTIME_DIR and /run/containers/0 paths and fallback
behavior.
Source: Path instructions
| echo >&2 "No valid auth.json file found" | ||
| exit 1 | ||
| fi | ||
| cp "${AUTH_FILE}" ~/.docker/config.json |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '(^|/)ansible/(roles/(run-test-target/tasks/test-docker\.yml|provision-vm/tasks/garden\.yml)|roles/.*)\.yml$' | head -200
echo
echo "== test-docker.yml outline/size =="
wc -l ansible/roles/run-test-target/tasks/test-docker.yml
sed -n '1,140p' ansible/roles/run-test-target/tasks/config-docker.yml 2>/dev/null || true
sed -n '1,140p' ansible/roles/run-test-target/tasks/test-docker.yml
echo
echo "== garden.yml relevant =="
rg -n -C 5 'config\.json|quay|auth|docker|containers' ansible/roles/provision-vm/tasks/garden.yml
sed -n '100,150p' ansible/roles/provision-vm/tasks/garden.yml
echo
echo "== search config.json lifecycle =="
rg -n -C 3 '~?/\.docker/config\.json|COPY_FILE|creates|quay\.io|XDG_RUNTIME_DIR|AUTH_FILE' ansible/rolesRepository: stackrox/collector
Length of output: 14276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dependency pins for Ansible =="
git ls-files | rg '(^|/)(ansible|requirements|environment|galaxy|defaults).*ya?ml$|(^|/)galaxy_install|ansible.cfg' | sort
for f in $(git ls-files | rg '(^|/)(galaxy\.(yml|yaml|json)|requirements\.(yml|yaml|json)|galaxy_install\.sh|ansible\.cfg|environment.yml)$'); do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo
echo "== ansible-role ordering / include order =="
rg -n -C 4 'roles/provision-vm|roles/run-test-target|include_tasks|import_tasks|provision-vm|run-test-target' ansible/roles ansible -g '*.yml'
echo
echo "== local ansible behavior probe for shell creates and cp overwrite =="
python3 - <<'PY'
import os, stat, json
from pathlib import Path
tmp = Path('/tmp/coderabbit_config_merge_probe')
tmp.mkdir(exist_ok=True)
(src, dst) = (tmp / 'src_auth.json', tmp / 'dst_config.json')
for p in (src, dst):
p.write_text('{}')
os.chmod(dst, 0o644)
cfg = json.load(src)
print('src=', cfg, 'dst_exists=', dst.exists(), 'dst_stat=', oct(stat.S_IMODE(dst.stat().st_mode)))
with open(dst, 'w') as f:
json.dump(cfg, f)
print('dst=', json.load(dst))
src = tmp / 'src_auth2.json'
dst = tmp / 'dst_config2.json'
src.write_text('{"auths": {"registry.example.test": {"auth": "a"}}}')
dst.write_text('{"auths": {"quay.io": {"auth": "q"}}}')
with open(dst, 'w') as f:
json.dump(json.load(src), f)
print('after overwrite dst=', json.load(dst))
PYRepository: stackrox/collector
Length of output: 28809
🌐 Web query:
Ansible shell module creates option documentation prechecks destination exists
💡 Result:
The Ansible shell module (ansible.builtin.shell) includes two specific parameters, creates and removes, designed to manage idempotent execution by performing pre-checks on the destination [1][2][3]. The creates parameter takes a filename as an argument [1][2]. If the specified file already exists, the task will be skipped [2][3]. This is useful for ensuring a command is only run when a specific output file or artifact has not yet been generated [3]. The removes parameter also takes a filename as an argument [1][2]. In this case, if the specified file does not exist, the task will be skipped [1][2]. This is useful for scenarios where you only want to run a command if a certain prerequisite file is present or if you want to ensure the command only executes if a previous process has not yet removed a file [1][2]. These options allow the shell module to behave more like a declarative module by checking the state of the target system before executing the shell command [3].
Citations:
- 1: https://docs.ansible.com/projects/ansible-core/2.19/collections/ansible/builtin/shell%5Fmodule.html
- 2: https://docs.ansible.com/projects/ansible-core/devel/collections/ansible/builtin/shell_module.html
- 3: https://docs.ansible.com/projects/ansible/8/collections/ansible/builtin/shell_module.html
Merge Podman auth into the Docker config instead of copying it as a single file.
~/.docker/config.json can already exist before this task runs, so the creates guard skips the update and can leave stale Podman credentials in the test artifact container. When it does run, cp "${AUTH_FILE}" ~/.docker/config.json replaces the whole file and can remove credentials set earlier by tasks such as ansible/roles/provision-vm/tasks/garden.yml. Build one explicit merged auth file and update it idempotently instead.
🤖 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 `@ansible/roles/run-test-target/tasks/test-docker.yml` at line 66, Replace the
direct copy in the Docker authentication task with an idempotent merge of
AUTH_FILE into the existing ~/.docker/config.json, preserving credentials
already configured by earlier tasks while updating Podman entries. Remove
reliance on the creates guard, generate one explicit merged auth file, and
ensure the resulting config is used for the test artifact container.
Source: Path instructions
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg 'ansible/roles/(run-test-target|provision-vm)/tasks/(test-docker|garden)\.yml' || true
echo "== target file context =="
if [ -f ansible/roles/run-test-target/tasks/test-docker.yml ]; then
nl -ba ansible/roles/run-test-target/tasks/test-docker.yml | sed -n '1,110p'
fi
echo "== upstream config context =="
if [ -f ansible/roles/provision-vm/tasks/garden.yml ]; then
nl -ba ansible/roles/provision-vm/tasks/garden.yml | sed -n '100,145p'
fi
echo "== cp behavior for permissions =="
tmp="$(mktemp -d)"
mkdir -p "$tmp/src" "$tmp/dst"
printf '{}' > "$tmp/src/config.json"
printf '{}' > "$tmp/dst/config.json"
chmod 0644 "$tmp/dst/config.json"
ls -l "$tmp/src/config.json" "$tmp/dst/config.json"
cp "$tmp/src/config.json" "$tmp/dst/config.json"
ls -l "$tmp/dst/config.json"
install -m 0600 "$tmp/src/config.json" "$tmp/dst/config.json"
ls -l "$tmp/dst/config.json"
rm -rf "$tmp"Repository: stackrox/collector
Length of output: 337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file context =="
cat -n ansible/roles/run-test-target/tasks/test-docker.yml | sed -n '1,110p'
echo "== upstream config context =="
cat -n ansible/roles/provision-vm/tasks/garden.yml | sed -n '100,145p'
echo "== cp behavior for permissions =="
tmp="$(mktemp -d)"
mkdir -p "$tmp/src" "$tmp/dst"
printf '{}' > "$tmp/src/config.json"
printf '{}' > "$tmp/dst/config.json"
chmod 0644 "$tmp/dst/config.json"
stat -c '%a' "$tmp/dst/config.json"
cat "$tmp/src/config.json" "$tmp/dst/config.json" > /tmp/old_dupe; cp "$tmp/src/config.json" "$tmp/dst/config.json"
stat -c '%a' "$tmp/dst/config.json"
install -m 0600 "$tmp/src/config.json" "$tmp/dst/config.json"
stat -c '%a' "$tmp/dst/config.json"
rm -rf "$tmp"Repository: stackrox/collector
Length of output: 6183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== copy/permission tools =="
command -v python3 || true
command -v perl || true
command -v install || true
echo "== python copy semantics probe =="
python3 - <<'PY'
import os, tempfile, shutil
old_umask = os.umask(0o022)
try:
with tempfile.TemporaryDirectory() as tmp:
os.makedirs(os.path.join(tmp, "src"))
os.makedirs(os.path.join(tmp, "dst"))
src = os.path.join(tmp, "src", "config.json")
dst = os.path.join(tmp, "dst", "config.json")
with open(dst, "w") as f:
f.write("{}")
os.chmod(dst, 0o644)
with open(src, "w") as f:
f.write("{}")
initial = oct(os.stat(dst).st_mode)[-4:]
shutil.copy(src, dst)
after = oct(os.stat(dst).st_mode)[-4:]
print(f"io_error={False}")
print(f"initial_perm={initial}")
print(f"after_perm={after}")
finally:
os.umask(old_umask)
PY
echo "== install -m 0600 available =="
if command -v install >/dev/null 2>&1; then
python3 - <<'PY'
import os, tempfile, shutil
old_umask = os.umask(0o022)
try:
with tempfile.TemporaryDirectory() as tmp:
os.makedirs(os.path.join(tmp, "src"))
os.makedirs(os.path.join(tmp, "dst"))
src = os.path.join(tmp, "src", "config.json")
dst = os.path.join(tmp, "dst", "config.json")
with open(dst, "w") as f:
f.write("{}")
os.chmod(dst, 0o644)
with open(src, "w") as f:
f.write("{}")
shutil.copy(src, dst)
print(f"pre_install_perm={oct(os.stat(dst).st_mode)[-4:]}")
os.system(f"install -m 0600 {src} {dst}")
print(f"post_install_perm={oct(os.stat(dst).st_mode)[-4:]}")
finally:
os.umask(old_umask)
PY
fiRepository: stackrox/collector
Length of output: 330
Other
Reachability: Internal
Enforce private permissions on copied registry credentials.
/root/.docker/config.json is created with mode 0644, and cp preserves destination permissions when overwriting. Keep the copied auth file from inheriting group/world-readable access; use a file operation that sets mode 0600 after the copy.
Suggested fix
- cp "${AUTH_FILE}" ~/.docker/config.json
+ install -m 0600 "${AUTH_FILE}" ~/.docker/config.json<security_category:sensitive_data_exposure>
cwe:CWE-732
📝 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.
| cp "${AUTH_FILE}" ~/.docker/config.json | |
| install -m 0600 "${AUTH_FILE}" ~/.docker/config.json |
🤖 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 `@ansible/roles/run-test-target/tasks/test-docker.yml` at line 66, Update the
credential-copy step around AUTH_FILE to enforce mode 0600 on
~/.docker/config.json after copying, ensuring group and world users cannot read
the registry credentials while preserving the existing copy behavior.
Source: Path instructions
|
/retest |
|
@Molter73 I have fixed the RHEL 10 test issues. There are some optimization that can be made to create-vm step but I was wondering if I should open a separate PR for that after this is merged. Thoughts? |
3 participants
Description
Updates boot images for ppc64le vm to RHEL 9.6 and RHEL 10.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
CI is sufficient.
For more details, ref the Confluence page about this section.