[shiftstack] Use GitHub for shiftstack-qa clone and fix false-positive test results#3922
Conversation
|
Skipping CI for Draft Pull Request. |
22a3c0d to
4dd8af5
Compare
imatza-rh
changed the title
4dd8af5 to
b2ab112
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
b2ab112 to
487590c
Compare
|
This PR is stale because it has been for over 15 days with no activity. |
487590c to
2a0c27d
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 27m 41s |
|
recheck |
2a0c27d to
c01e57b
Compare
c01e57b to
b3ecf49
Compare
b3ecf49 to
609d972
Compare
ekuris-redhat
left a comment
ekuris-redhat
left a comment
There was a problem hiding this comment.
Review of the exec_command_in_pod.yml rewrite — 7 findings, focused on the nohup+polling approach. The GitHub→GerritHub migration and variable rename look clean.
| bash -c 'cat /tmp/cifmw_cmd_rc' | ||
| register: pod_exit_code | ||
| changed_when: false | ||
| until: pod_exit_code.rc == 0 |
There was a problem hiding this comment.
The polling condition pod_exit_code.rc == 0 treats every non-zero oc exec exit code as "command still running." But if the pod is evicted, OOM-killed, or the API server becomes unreachable, oc exec also returns non-zero , indistinguishable from "file not yet created." This means a dead pod causes 5760 retries (8 hours) of silent failure before the task finally times out.
Consider adding a pod-liveness check inside the polling loop so it fails fast when the pod is gone, e.g. wrapping the cat in a compound check:
bash -c 'if cat /tmp/cifmw_cmd_rc 2>/dev/null; then exit 0; elif oc get pod -n NS POD -o jsonpath="{.status.phase}" | grep -q Running; then exit 1; else echo "Pod is no longer running" >&2; exit 2; fi'Or add a separate task after the loop that checks pod phase when pod_exit_code.rc != 0.
| msg: '"{{ command }}" command execution failed with exit code "{{ pod_command_result.rc }}" and error: "{{ pod_command_result.stderr }}".' | ||
| msg: >- | ||
| "{{ command }}" failed. | ||
| oc exit code: {{ pod_command_result.rc | default('N/A') }}, |
There was a problem hiding this comment.
With the nohup approach, pod_command_result.stderr is from the backgrounding step (empty), and pod_exit_code.stderr is from cat /tmp/cifmw_cmd_rc (empty or "No such file"). The actual command output sits in /tmp/cifmw_cmd_output.log inside the pod but is never retrieved.
This is a regression from the old synchronous oc rsh which surfaced stderr directly in pod_command_result.stderr.
Consider fetching the output log before reporting the failure:
rescue:
- name: Retrieve command output from pod
ansible.builtin.command:
cmd: >-
oc exec -n {{ namespace }} {{ pod_name }} --
bash -c 'tail -c 8192 /tmp/cifmw_cmd_output.log 2>/dev/null || echo "(output not available)"'
register: pod_output_log
failed_when: false
changed_when: false
- name: Fail when the command module fails
ansible.builtin.fail:
msg: >-
"{{ command }}" failed.
pod exit code: {{ pod_exit_code.stdout | default('N/A') | trim }},
output: {{ pod_output_log.stdout | default('(not retrieved)') }}| changed_when: false | ||
| failed_when: false | ||
|
|
||
| - name: Write the command to a local script file |
There was a problem hiding this comment.
The five setup tasks (lines 17–68: kill previous, clear markers, write script, copy to pod, chmod) run before the block: at line 69. If any of them fails, e.g. a transient API-server error on "Copy the script into the pod" the rescue: diagnostic message and the always: log-saving block are both skipped entirely. The caller gets a raw Ansible error with no structured diagnostics and no log file written.
Consider moving these setup tasks inside the block: so that failures are caught by the existing rescue/always handlers.
| @@ -45,6 +126,7 @@ | |||
| pod_name: "{{ pod_name }}" | |||
| command: "{{ command }}" | |||
| pod_command_result: "{{ pod_command_result }}" | |||
There was a problem hiding this comment.
With the nohup approach, pod_command_result now contains only the background PID (e.g. {stdout: "1234", rc: 0}), not the actual command output. The saved log file loses its diagnostic value compared to the old synchronous approach.
Consider fetching /tmp/cifmw_cmd_output.log from the pod in the always: block and including it in the saved command info.
| cmd: >- | ||
| oc exec -n {{ namespace }} {{ pod_name }} -- | ||
| bash -c 'if [ -f /tmp/cifmw_cmd_pid ]; then | ||
| kill $(cat /tmp/cifmw_cmd_pid) 2>/dev/null; fi' |
There was a problem hiding this comment.
The kill and clear-markers run as two separate oc exec round-trips (lines 17–39). Between them there's a window where the killed process could still write /tmp/cifmw_cmd_rc if the script was already finishing, or where PID reuse could cause the kill to target an unrelated process in the pod.
Consider combining into a single atomic operation:
- name: Kill any previous background command and clear markers
ansible.builtin.command:
cmd: >-
oc exec -n {{ namespace }} {{ pod_name }} --
bash -c 'if [ -f /tmp/cifmw_cmd_pid ]; then
kill $(cat /tmp/cifmw_cmd_pid) 2>/dev/null;
wait $(cat /tmp/cifmw_cmd_pid) 2>/dev/null; fi;
rm -f /tmp/cifmw_cmd_rc /tmp/cifmw_cmd_pid
/tmp/cifmw_cmd_script.sh /tmp/cifmw_cmd_output.log'
changed_when: false
failed_when: false| content: | | ||
| #!/bin/bash | ||
| {{ command }} | ||
| dest: /tmp/cifmw_cmd_script.sh |
There was a problem hiding this comment.
The script is written to a hardcoded path /tmp/cifmw_cmd_script.sh on the Ansible controller. If two CI jobs run this role concurrently on the same executor host, they'd overwrite each other's scripts. Consider incorporating the pod name into the path to avoid collisions:
dest: "/tmp/cifmw_cmd_script_{{ pod_name }}.sh"| cifmw_shiftstack_project_name: "shiftstack" | ||
| cifmw_shiftstack_qa_gerrithub_change: "" | ||
| cifmw_shiftstack_qa_repo: "https://review.gerrithub.io/shiftstack/shiftstack-qa" | ||
| cifmw_shiftstack_qa_change_ref: "" |
There was a problem hiding this comment.
cifmw_shiftstack_exec_delay and cifmw_shiftstack_exec_retries are documented in the README with concrete defaults (5 and 5760), but they're not declared here. Every other role variable with a concrete default is defined in defaults/main.yml , the file header at line 18 states "All variables intended for modification should be placed in this file."
The inline | default() fallbacks work functionally, but adding them here makes them discoverable and overridable through the standard Ansible variable precedence:
cifmw_shiftstack_exec_delay: 5
cifmw_shiftstack_exec_retries: 5760
4f43ff7 to
f4d8917
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 10m 58s |
|
recheck |
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 41m 46s |
|
recheck |
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 39m 31s |
…e test results shiftstack-qa moved code review to GitHub PRs (shiftstack/ shiftstack-qa#2). The role default still clones from GerritHub, which is no longer maintained. - Change cifmw_shiftstack_qa_repo default to GitHub - Rename cifmw_shiftstack_qa_gerrithub_change to cifmw_shiftstack_qa_change_ref Also fix a false-positive bug in exec_command_in_pod.yml where oc rsh could return rc=0 before the inner command finished, causing the test to report PASS when the playbook actually failed. Observed in tp!2297 build d078ccbd - the shiftstack test ran for only 145s (vs typical 1.5-2h), OCP was never installed, yet the job reported SUCCESS. The fix runs the inner command detached via nohup and polls a marker file for the exit code. This survives connection drops. Uses kubernetes.core.k8s_exec and k8s_cp modules for setup tasks; the poll step uses oc exec for resilience to transient network errors. Process-crash detection via kill -0 inside the pod catches OOM without polling for 8h. Diagnostic output fetched in rescue/always blocks restores stderr visibility lost by the async approach. Configurable via cifmw_shiftstack_exec_retries (default 5760) and cifmw_shiftstack_exec_delay (default 5s) for an 8h timeout. Related-Issue: #OSPRH-30385 Assisted-By: Claude Code Signed-off-by: Itay Matza <imatza@redhat.com>
f4d8917 to
6397c6c
Compare
2 participants
Summary
Two fixes for the shiftstack CI role:
1. Clone shiftstack-qa from GitHub instead of GerritHub
shiftstack-qa moved code review to GitHub PRs (shiftstack/shiftstack-qa#2). The role default still clones from GerritHub, which is no longer maintained. GerritHub main is 3+ commits behind GitHub main - missing the
4-stableOCP channel switch,lb_testsdisable, and.gitreviewremoval.cifmw_shiftstack_qa_repodefault tohttps://github.com/shiftstack/shiftstack-qacifmw_shiftstack_qa_gerrithub_changetocifmw_shiftstack_qa_change_ref- supports both GitHub PR refs and GerritHub change refs2. Fix false-positive and connection-drop bugs in exec_command_in_pod.yml
oc exec/rsh connections to the shiftstackclient pod drop after ~38 minutes during long-running tests. The previous fix (marker file + polling) didn't solve it because the marker file write was chained to the same bash process that dies when the connection drops.
Root cause verified from tp!2297 build d349fb73 inner test log: the test ran for 1h 37m inside the pod (Manila StorageClass failure at 14:20 UTC), but oc exec returned after only 38m 50s. The marker file was never created - 720 retries over 2h all got No such file or directory.
Fix: writes the command to a script file, copies it into the pod, then runs it detached via nohup. The process survives oc exec connection drops. Also kills orphaned background processes from previous invocations to prevent marker file poisoning.
Default polling timeout set to 8h (5760 retries x 5s) to cover the full test suite duration. Configurable via
cifmw_shiftstack_exec_retriesandcifmw_shiftstack_exec_delay, documented in README.md.Verified on the live shiftstackclient pod (serval70) - 6-case Ansible test suite covering success, exit codes 1/42, dollar-sign commands, orphan kill, retry timeout, and nohup survival after oc exec termination.
Test plan