Lvms timeout - #50
Lvms timeout#50
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe LVMCluster readiness task now polls ChangesLVMCluster readiness polling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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: 3
🤖 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/lvms/tasks/main.yml`:
- Around line 63-64: Configure the ansible.builtin.shell task containing the
`set -euo pipefail` loop to use `/bin/bash` via `args.executable`, preserving
the existing strict-shell behavior and retry loop.
- Line 65: Update the LVM cluster state check around the oc command to stop
unconditionally masking failures with || true. Capture and inspect oc’s exit
status, retry only the expected transient or not-found condition, and
immediately surface authentication, RBAC, context, and other API errors instead
of treating them as an empty status state.
- Line 65: Bound every oc request in the readiness probe, including the state
check and failure-path oc get ... -o yaml, with an explicit --request-timeout
value so no individual API call can block the retry loop indefinitely. Update
the relevant shell commands in the task while preserving the existing retry and
error-handling behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: cf3be300-ee77-4e24-98d6-bc1cbd0d07b0
📒 Files selected for processing (1)
ansible/roles/lvms/tasks/main.yml
| set -euo pipefail | ||
| for i in $(seq 1 360); do |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -n --hidden -g '*.yml' -g '*.yaml' -g 'ansible.cfg' \
'ansible_shell_executable|module_defaults|executable:' . || trueRepository: redhat-performance/JetBrew
Length of output: 644
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== ansible/roles/lvms/tasks/main.yml =="
cat -n ansible/roles/lvms/tasks/main.yml | sed -n '1,180p'
echo
echo "== shell/executable config hits in lvms role =="
rg -n --hidden -g 'ansible/roles/lvms/**' -g 'ansible.cfg' \
'ansible_shell_executable|module_defaults|executable:' .
echo
echo "== ansible.cfg if present =="
fd -a 'ansible.cfg' .Repository: redhat-performance/JetBrew
Length of output: 3569
Pin the shell to bash here. ansible.builtin.shell runs under /bin/sh by default, and set -euo pipefail fails on shells without pipefail support. Add args.executable: /bin/bash or remove pipefail.
🤖 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/lvms/tasks/main.yml` around lines 63 - 64, Configure the
ansible.builtin.shell task containing the `set -euo pipefail` loop to use
`/bin/bash` via `args.executable`, preserving the existing strict-shell behavior
and retry loop.
Source: Path instructions
| poll: 5 | ||
| set -euo pipefail | ||
| for i in $(seq 1 360); do | ||
| state="$(oc get lvmcluster lvmcluster -n openshift-storage -o jsonpath='{.status.state}' 2>/dev/null || true)" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -n -C3 -- 'status\.state|2>/dev/null|\|\| true' \
ansible/roles/lvms/tasks/main.ymlRepository: redhat-performance/JetBrew
Length of output: 1053
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Show the full wait loop around the flagged line.
sed -n '58,82p' ansible/roles/lvms/tasks/main.yml
printf '\n--- shell semantics probe ---\n'
bash -lc 'set -euo pipefail; state="$(false 2>/dev/null || true)"; rc=$?; printf "rc=%s state=%q\n" "$rc" "$state"'
printf '\n--- masked-failure probe with command substitution ---\n'
bash -lc 'set -euo pipefail; state="$(sh -c '\''echo auth-error >&2; exit 1'\'' 2>/dev/null || true)"; printf "state=%q\n" "$state"'Repository: redhat-performance/JetBrew
Length of output: 1048
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Confirm whether the retry loop is 30 minutes or 60 minutes.
python3 - <<'PY'
attempts = 360
sleep_seconds = 5
print(attempts * sleep_seconds)
PYRepository: redhat-performance/JetBrew
Length of output: 169
Do not swallow oc failures here. || true makes auth, RBAC, missing-context, and API errors look like an empty .status.state, so the loop will keep retrying for up to 30 minutes instead of surfacing the real failure. Capture the exit code and only retry the expected transient/not-found case.
🤖 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/lvms/tasks/main.yml` at line 65, Update the LVM cluster state
check around the oc command to stop unconditionally masking failures with ||
true. Capture and inspect oc’s exit status, retry only the expected transient or
not-found condition, and immediately surface authentication, RBAC, context, and
other API errors instead of treating them as an empty status state.
Source: Path instructions
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -n -C2 -- '--request-timeout|oc get' \
ansible/roles/lvms/tasks/main.yml
if command -v oc >/dev/null 2>&1; then
oc options 2>&1 | rg -- '--request-timeout' || true
fiRepository: redhat-performance/JetBrew
Length of output: 1998
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '55,80p' ansible/roles/lvms/tasks/main.yml
printf '\n---\n'
rg -n -- '--request-timeout|timeout|until:|retries:|delay:' ansible/roles/lvms/tasks/main.ymlRepository: redhat-performance/JetBrew
Length of output: 1441
🌐 Web query:
oc request-timeout default oc get --request-timeout kubernetes client command line
💡 Result:
The default value for the --request-timeout flag in both the oc and kubectl command-line tools is "0" [1][2][3]. In the context of these tools, a value of "0" signifies that there is no timeout, meaning the client will not give up on a server request based on a duration limit [4][1][5]. Key technical details: - Behavior: When the flag is set to "0", the command will wait indefinitely for a response, which is the standard behavior for long-running operations like watching resources [5]. - Usage: If a non-zero value is provided (e.g., 1s, 2m, 3h), the client will enforce that duration as a timeout for the server request [4][1]. - Implementation: The kubernetes/client-go library manages these timeouts [6][7]. While the flag defaults to "0", internal client configurations and specific API request types (such as watches vs. standard GET requests) may involve nuanced handling to balance responsiveness and reliability [8][9]. If you encounter unexpected timeouts, ensure you are not passing a conflicting value in your configuration or environment variables, as the CLI flag explicitly overrides default behaviors when set [5].
Citations:
- 1: https://kubernetes.io/docs/reference/kubectl/kubectl/
- 2: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_options/
- 3: https://kubernetes.io/docs/reference/kubectl/generated/kubectl/
- 4: https://manpages.opensuse.org/Tumbleweed/oc/oc-login.1.en.html
- 5: Add global timeout flag kubernetes/kubernetes#33958
- 6: https://github.com/kubernetes/kubernetes/blob/63b36867/staging/src/k8s.io/client-go/rest/request.go
- 7: client-go does not propagate updated timeout in request query parameter on retries kubernetes/kubernetes#117313
- 8: client-go: changes config.Timeout field semantic kubernetes/kubernetes#101022
- 9: kubectl: fix timeout=32s for some rest APIs when --request-timeout=0 kubernetes/kubernetes#103619
Bound each oc request with a timeout.
oc get defaults to no client-side timeout, so a stuck API call can block the 360-attempt loop forever. Apply --request-timeout to the readiness check and the failure-path oc get ... -o yaml, or wrap the whole probe in an outer wall-clock deadline.
🤖 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/lvms/tasks/main.yml` at line 65, Bound every oc request in the
readiness probe, including the state check and failure-path oc get ... -o yaml,
with an explicit --request-timeout value so no individual API call can block the
retry loop indefinitely. Update the relevant shell commands in the task while
preserving the existing retry and error-handling behavior.
Source: Path instructions
1 participant
No description provided.