[STORGE][GATING] Add retry for virtctl download to handle transient SSL errors#5346
[STORGE][GATING] Add retry for virtctl download to handle transient SSL errors#5346Ahmad-Hafe wants to merge 1 commit into
Conversation
The virtctl binary download from the cluster CLI route can fail with SSLEOFError during TLS handshake, causing all tests depending on the virtctl_binary fixture to fail in setup. Wrap the download in TimeoutSampler to retry on SSLError and ConnectionError for up to 2 minutes (10s between attempts). Signed-off-by: Ahmad Hafe <ahafe@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (5)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5346 published |
|
/verified |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR METADATA ==="
gh pr view 5346 --json number,headRefOid,baseRefName,headRefName,files --jq '{number, headRefOid, baseRefName, headRefName, files: [.files[] | {path, additions, deletions}]}'
echo
echo "=== CHANGED FILES ==="
git diff --name-status origin/main...HEAD || git diff --name-status HEAD~1...HEAD
echo
echo "=== FIRST CHANGED FILE/PATCH INFO ==="
first_file="$(git diff --name-only origin/main...HEAD | head -n1 || true)"
if [[ -z "${first_file}" ]]; then
first_file="$(git diff --name-only HEAD~1...HEAD | head -n1 || true)"
fi
echo "first_file=${first_file}"
if [[ -n "${first_file}" ]]; then
git diff --unified=0 origin/main...HEAD -- "${first_file}" || git diff --unified=0 HEAD~1...HEAD -- "${first_file}"
fi
echo
echo "=== FULL DIFF UNIFIED=80 ==="
git diff --unified=80 origin/main...HEAD || git diff --unified=80 HEAD~1...HEAD
echo
echo "=== MODIFIED SYMBOLS IN UTILITIES/LIBS (if any) ==="
for f in $(git diff --name-only origin/main...HEAD | grep -E '^(utilities|libs)/' || true); do
echo "--- $f"
git diff --unified=0 origin/main...HEAD -- "$f" | grep -E '^[+-][[:space:]]*(def|class)[[:space:]]+' || true
done
echo
echo "=== SEARCH virtctl_binary DEFINITIONS/USES ==="
rg -n -C3 '\bvirtctl_binary\b' . || true
echo
echo "=== SEARCH download/import/retry symbols around virtctl ==="
rg -n -C3 '\b(virtctl|download|TimeoutSampler|SSLError|ConnectionError)\b' tests utilities libs conftest.py 2>/dev/null || true
echo
echo "=== PYTEST MARKER FILES ==="
echo "--- smoke files"
rg -n -l '`@pytest`\.mark\.smoke' tests || true
echo "--- gating files"
rg -n -l '`@pytest`\.mark\.gating' tests || true
echo
echo "=== MARKERS IN FILES MENTIONING virtctl_binary ==="
while IFS= read -r f; do
[[ -f "$f" ]] || continue
echo "--- $f"
rg -n -C2 '`@pytest`\.mark\.(smoke|gating)|virtctl_binary|def test_|class Test' "$f" || true
done < <(rg -l '\bvirtctl_binary\b' tests conftest.py utilities libs 2>/dev/null || true)
echo
echo "=== CONFTSTS WITH virtctl/download-related SYMBOLS ==="
fd '^conftest\.py$' . -x sh -c 'rg -n -C3 "\b(virtctl_binary|virtctl|TimeoutSampler|download)\b" "$1" && echo "--- $1"' sh {} 2>/dev/null || true
echo
echo "=== COLLECT TEST FUNCTIONS USING virtctl_binary PARAM/FIXTURE ==="
python - <<'PY'
import os, re
for root, dirs, files in os.walk("tests"):
dirs[:] = [d for d in dirs if d not in {".git", "__pycache__"}]
for name in files:
if not name.endswith(".py"):
continue
path=os.path.join(root,name)
try:
text=open(path,encoding="utf-8").read()
except Exception:
continue
if "virtctl_binary" not in text:
continue
print(f"--- {path}")
lines=text.splitlines()
current_class=None
decorators=[]
for i,line in enumerate(lines, start=1):
mcls=re.match(r'^class\s+(\w+)', line)
if mcls:
current_class=mcls.group(1)
if line.strip().startswith("@"):
decorators.append((i,line.strip()))
continue
m=re.match(r'^\s*def\s+(test_\w+)\s*\(([^)]*)', line)
if m:
func=m.group(1); params=m.group(2)
block="\n".join(lines[max(0,i-8):min(len(lines),i+25)])
if "virtctl_binary" in params or "virtctl_binary" in block:
q=f"{path}::{current_class+'::' if current_class else ''}{func}"
marks=[d for _,d in decorators if "pytest.mark" in d]
print(f"{i}: {q} params=({params}) marks={marks}")
decorators=[]
elif line.strip() and not line.strip().startswith("#"):
decorators=[]
PYLength of output: 202021 🏁 Script executed: #!/bin/bash
set -euo pipefail
BASE="$(git merge-base origin/main HEAD 2>/dev/null || git rev-parse HEAD~1)"
echo "BASE=$BASE"
echo "HEAD=$(git rev-parse HEAD)"
echo "=== PR META ==="
gh pr view 5346 --json headRefOid,files --jq '{headRefOid, files: [.files[] | {path, additions, deletions}]}'
echo "=== CHANGED FILES ==="
git diff --name-status "$BASE"...HEAD
echo "=== FIRST FILE PATCH ==="
first_file="$(git diff --name-only "$BASE"...HEAD | head -n1)"
echo "FIRST_FILE=$first_file"
git diff --unified=0 "$BASE"...HEAD -- "$first_file" | sed -n '1,120p'
echo "=== FULL TARGETED DIFF ==="
git diff --unified=30 "$BASE"...HEAD
echo "=== MODIFIED SYMBOLS ==="
for f in $(git diff --name-only "$BASE"...HEAD | grep -E '^(utilities|libs|tests)/|^conftest\.py$' || true); do
echo "--- $f"
git diff --unified=0 "$BASE"...HEAD -- "$f" | grep -E '^[+-][[:space:]]*(def|class)[[:space:]]+' || true
done
echo "=== virtctl_binary DEFINITIONS/USES CONCISE ==="
rg -n -C2 '\bvirtctl_binary\b' tests conftest.py utilities libs 2>/dev/null || true
echo "=== run_virtctl_command DEFINITIONS/USES CONCISE ==="
rg -n -C2 '\brun_virtctl_command\b' tests conftest.py utilities libs 2>/dev/null || true
echo "=== REQUESTS/URLRETRIEVE/TimeoutSampler IN CHANGED FILES ==="
for f in $(git diff --name-only "$BASE"...HEAD); do
echo "--- $f"
rg -n -C3 '\b(TimeoutSampler|SSLError|ConnectionError|requests|urlretrieve|urlopen|download|virtctl)\b' "$f" || true
done
echo "=== TEST FUNCTIONS AND MARKERS IN FILES WITH virtctl_binary ==="
python - <<'PY'
import ast, os, re
paths=[]
for root, dirs, files in os.walk("tests"):
dirs[:] = [d for d in dirs if d not in {".git","__pycache__"}]
for name in files:
if name.endswith(".py"):
p=os.path.join(root,name)
try:
txt=open(p, encoding="utf-8").read()
except Exception:
continue
if "virtctl_binary" in txt:
paths.append(p)
for p in sorted(paths):
print(f"--- {p}")
txt=open(p, encoding="utf-8").read()
tree=ast.parse(txt)
module_marks=[]
for node in tree.body:
if isinstance(node, ast.Assign):
pass
def mark_names(decos):
out=[]
for d in decos:
s=ast.unparse(d) if hasattr(ast, "unparse") else ""
if "pytest.mark." in s:
out.append(s)
return out
for node in tree.body:
if isinstance(node, ast.FunctionDef) and node.name.startswith("test_"):
params=[a.arg for a in node.args.args]
body=ast.get_source_segment(txt,node) or ""
if "virtctl_binary" in params or "virtctl_binary" in body:
print(f"{p}::{node.name} params={params} marks={mark_names(node.decorator_list)}")
if isinstance(node, ast.ClassDef):
cmarks=mark_names(node.decorator_list)
for item in node.body:
if isinstance(item, ast.FunctionDef) and item.name.startswith("test_"):
params=[a.arg for a in item.args.args]
body=ast.get_source_segment(txt,item) or ""
if "virtctl_binary" in params or "virtctl_binary" in body:
print(f"{p}::{node.name}::{item.name} params={params} class_marks={cmarks} marks={mark_names(item.decorator_list)}")
PY
echo "=== MARKERS IN virtctl_binary FILES ==="
for f in $(rg -l '\bvirtctl_binary\b' tests 2>/dev/null || true); do
echo "--- $f"
rg -n '`@pytest`\.mark\.(smoke|gating|polarion|tier|destructive|s390x)|pytestmark|virtctl_binary|def test_|class Test' "$f" || true
done
echo "=== CONFTST HIERARCHY FOR SMOKE/GATING FILES WITH changed symbol terms ==="
python - <<'PY'
import os, subprocess
smoke = subprocess.run(["rg","-l","`@pytest`\\.mark\\.smoke","tests"], text=True, capture_output=True).stdout.splitlines()
gating = subprocess.run(["rg","-l","`@pytest`\\.mark\\.gating","tests"], text=True, capture_output=True).stdout.splitlines()
terms=["virtctl_binary","download_virtctl_binary","TimeoutSampler","requests.get"]
for label, files in [("smoke",smoke),("gating",gating)]:
print(f"-- {label} count={len(files)}")
for f in files:
d=os.path.dirname(f)
seen=[]
while True:
cf=os.path.join(d,"conftest.py") if d else "conftest.py"
if os.path.exists(cf) and cf not in seen:
txt=open(cf,encoding="utf-8").read(errors="ignore")
hits=[t for t in terms if t in txt]
if hits:
print(f"{label}: {f} hierarchy_conftest={cf} hits={hits}")
seen.append(cf)
if d in ("", "."):
break
nd=os.path.dirname(d)
if nd==d:
break
d=nd
PYLength of output: 27846 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== utilities/infra.py changed call chain snippets ==="
sed -n '665,725p' utilities/infra.py
echo "=== tests/conftest.py virtctl fixtures/imports snippets ==="
sed -n '130,170p' tests/conftest.py
sed -n '1455,1480p' tests/conftest.py
sed -n '1900,1950p' tests/conftest.py
rg -n -C2 '\b(get_and_extract_file_from_cluster|download_and_extract_file_from_cluster|virtctl_binary|bin_directory_to_os_path)\b' tests/conftest.py
echo "=== console cli download conftest/test snippets ==="
sed -n '1,130p' tests/install_upgrade_operators/console_cli_download/conftest.py
sed -n '1,140p' tests/install_upgrade_operators/console_cli_download/test_disconnected_virtctl.py
echo "=== direct callers of changed and transitive symbols ==="
rg -n -C2 '\b(download_and_extract_file_from_cluster|get_and_extract_file_from_cluster)\b' tests utilities libs conftest.py 2>/dev/null || true
echo "=== smoke/gating conftest hierarchy hits for exact changed/transitive symbols ==="
python - <<'PY'
import os, subprocess
terms=["download_and_extract_file_from_cluster","get_and_extract_file_from_cluster"]
for label, pattern in [("smoke", "`@pytest`\\.mark\\.smoke"), ("gating", "`@pytest`\\.mark\\.gating")]:
files=subprocess.run(["rg","-l",pattern,"tests"], text=True, capture_output=True).stdout.splitlines()
print(f"-- {label} files={len(files)}")
for f in files:
d=os.path.dirname(f)
while True:
cf=os.path.join(d,"conftest.py") if d else "conftest.py"
if os.path.exists(cf):
txt=open(cf,encoding="utf-8",errors="ignore").read()
hits=[t for t in terms if t in txt]
if hits:
print(f"{label}: {f} hierarchy_conftest={cf} hits={hits}")
if d in ("", "."):
break
nd=os.path.dirname(d)
if nd == d:
break
d = nd
PY
echo "=== markers for affected console cli download tests ==="
rg -n '`@pytest`\.mark\.(smoke|gating|polarion|arm64|tier)|pytestmark|class Test|def test_' tests/install_upgrade_operators/console_cli_download/test_disconnected_virtctl.py tests/install_upgrade_operators/console_cli_download/conftest.pyLength of output: 21956 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr_number="5346"
commit_id="4eadff212d10c02b338502fdaaf6418d46cd603f"
path="utilities/infra.py"
line="692"
body="$(cat <<'EOF'
**Test Execution Plan**
- **Run smoke tests: False** — Verified smoke-marked files exist, but no smoke file or smoke parent-directory `conftest.py` imports/calls `download_and_extract_file_from_cluster` or its transitive caller `get_and_extract_file_from_cluster`.
- **Run gating tests: True** — `tests/install_upgrade_operators/console_cli_download/test_disconnected_virtctl.py` is module-marked `gating`; affected path: test → `downloaded_and_extracted_virtctl_binary_for_os` fixture → `utilities.infra.get_and_extract_file_from_cluster` → changed `utilities.infra.download_and_extract_file_from_cluster` retry wrapper around `requests.get`.
**Affected tests to run**
- `tests/install_upgrade_operators/console_cli_download/test_disconnected_virtctl.py::TestDisconnectedVirtctlDownload::test_download_virtcli_binary` (gating)
- `tests/install_upgrade_operators/console_cli_download/test_disconnected_virtctl.py::TestDisconnectedVirtctlDownloadAndExecute::test_download_and_execute_virtcli_binary_linux` (gating)
- `tests/install_upgrade_operators/console_cli_download/test_custom_console_cli_download.py` — direct import/use of `download_and_extract_file_from_cluster`.
- A lightweight session-fixture regression using any small non-installing test that requires `bin_directory_to_os_path`, because `tests/conftest.py` session fixture `virtctl_binary` uses `download_file_from_cluster` → `get_and_extract_file_from_cluster` → changed `download_and_extract_file_from_cluster`.
**Real tests (cluster required)**
Error path (the fix):
`pytest tests/install_upgrade_operators/console_cli_download/test_disconnected_virtctl.py::TestDisconnectedVirtctlDownload::test_download_virtcli_binary -k linux_amd64`
Expected: transient `requests.exceptions.SSLError` / `requests.exceptions.ConnectionError` from the cluster CLI download route is retried for up to 2 minutes instead of failing fixture setup immediately.
Happy path (regression):
`pytest tests/install_upgrade_operators/console_cli_download/test_disconnected_virtctl.py::TestDisconnectedVirtctlDownloadAndExecute::test_download_and_execute_virtcli_binary_linux -k linux_amd64`
Expected: archive download, extraction, and `virtctl version` execution still succeed with the new `TimeoutSampler` wrapper.
EOF
)"
payload="$(jq -n \
--arg body "$body" \
--arg commit_id "$commit_id" \
--arg path "$path" \
--argjson line "$line" \
'{body:$body, commit_id:$commit_id, path:$path, line:$line, side:"RIGHT"}')"
for attempt in 1 2; do
if gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"repos/${repo}/pulls/${pr_number}/comments" \
--input - <<<"$payload" >/tmp/inline-comment-result.json; then
exit 0
fi
sleep 2
done
gh pr comment "$pr_number" --repo "$repo" --body "$body" >/tmp/fallback-pr-comment-result.txtLength of output: 3163 |
|
/retest tox |
|
/retest all |
1 similar comment
|
/retest all |
|
Shouldn't this point to main? |
|
/approve |
|
@ema-aka-young main have problem in Run Utilities Unit Tests CI |
|
/approve cancel missed that this is on 4.22 |
10 participants
What this PR does / why we need it:
The
virtctlbinary download from the cluster CLI route can fail withSSLEOFErrorduring TLS handshake, causing all tests depending on thevirtctl_binaryfixture to fail in setup.Wraps the download in
TimeoutSamplerto retry onSSLErrorandConnectionErrorfor up to 2 minutes (10s between attempts).Which issue(s) this PR fixes:
Cherry-pick source for https://github.com/RedHatQE/cnv-tests/pull/3497
Special notes for reviewer:
jira-ticket:
https://redhat.atlassian.net/browse/CNV-83631