Automation test for kubevirt_vmi_sync_total metric#5246
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a ChangesVMI Sync Total Metric Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-4
requested review from
albarker-rh and
dshchedr
openshift-virtualization-qe-bot-4
requested review from
RoniKishner,
rlobillo and
vsibirsk
|
/build-and-push-container |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/observability/metrics/conftest.py`:
- Around line 668-671: The initial_vmi_sync_total_values function captures a
single snapshot via prometheus.query_sampler without waiting for all Prometheus
series to be exposed and stable, resulting in an incomplete baseline that causes
downstream migration validations to be flaky. Modify the function to poll or
wait for stable metrics before returning the baseline, ensuring that all
expected series for the given VM are present and their values have stabilized
before capturing the snapshot. This ensures reliable migration comparison.
In `@tests/observability/metrics/utils.py`:
- Around line 831-839: The pod-level validation using
`all(float(result["value"][1]) > 0 for result in sample)` is too strict and
causes false failures when replica churn or standby pods exist. Instead of
validating every pod in the sample, change the logic to only check that the
specific component pods (those starting with VIRT_CONTROLLER or VIRT_HANDLER)
have positive metric values. Modify the all_positive check to filter the sample
results to only those pods that match the required components before checking if
their values are positive, ensuring the test validates component-level behavior
rather than all pod series. This pattern appears in multiple locations in the
codebase and should be fixed consistently wherever the same validation pattern
exists.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f34ace6-4f6e-459f-8053-c2a8ab35c144
📒 Files selected for processing (4)
tests/observability/metrics/conftest.pytests/observability/metrics/constants.pytests/observability/metrics/test_vms_metrics.pytests/observability/metrics/utils.py
|
/retest build-container |
|
Verification failed for PR #5246. Execution details |
OhadRevah
changed the title
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5246 published |
|
/verified All tests passed for PR #5246. Execution details |
Signed-off-by: Ohad <orevah@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/observability/metrics/constants.py (1)
48-48: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH: Scope this metric query by namespace too.
Both the PR objective and the test docstrings describe a namespace+name lookup, but this template filters only by
name. Becausevm_for_migration_metrics_testuses a fixed VM name, another namespace with the same VM name can contribute extra series and break the “2 entries”, migration-baseline, and clear-after-delete assertions against the wrong VM.Suggested direction
-KUBEVIRT_VMI_SYNC_TOTAL = "kubevirt_vmi_sync_total{{name='{vm_name}'}}" +KUBEVIRT_VMI_SYNC_TOTAL = "kubevirt_vmi_sync_total{{namespace='{namespace}',name='{vm_name}'}}"Then update callers to format both
namespaceandvm_name.🤖 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 `@tests/observability/metrics/constants.py` at line 48, The KUBEVIRT_VMI_SYNC_TOTAL metric template currently filters only by vm_name, which can pick up series from other namespaces with the same VM name. Update the constant in constants.py and all callers that use it to include namespace plus name formatting, using the relevant namespace variable alongside vm_name in the metric query. This should be applied where the metric is consumed in the migration metrics tests so the assertions target the correct VM series.
🤖 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 `@tests/observability/metrics/conftest.py`:
- Around line 681-683: The deleted_vmi_sync_total_vm fixture currently only
performs the delete side effect and returns None, so update it to yield or
return vm_for_migration_metrics_test after calling delete(wait=True). Keep the
fixture behavior in place but make the resource available to callers, matching
the existing fixture pattern used in tests/observability/metrics/conftest.py.
---
Duplicate comments:
In `@tests/observability/metrics/constants.py`:
- Line 48: The KUBEVIRT_VMI_SYNC_TOTAL metric template currently filters only by
vm_name, which can pick up series from other namespaces with the same VM name.
Update the constant in constants.py and all callers that use it to include
namespace plus name formatting, using the relevant namespace variable alongside
vm_name in the metric query. This should be applied where the metric is consumed
in the migration metrics tests so the assertions target the correct VM series.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ccf0f88-3a88-4195-bcb0-26e094dca534
📒 Files selected for processing (4)
tests/observability/metrics/conftest.pytests/observability/metrics/constants.pytests/observability/metrics/test_vms_metrics.pytests/observability/metrics/utils.py
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5224. Overlapping filestests/observability/metrics/conftest.py |
|
/build-and-push-container |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/observability/metrics/test_vms_metrics.py (1)
597-612: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLOW: Update the Expected blocks to match the shared helper checks.
The migration test no longer asserts that both current series are greater than the initial sample; it checks that the virt-controller value increased and that a new virt-handler pod reports a positive value. The deletion test also accepts either no samples or zero-valued samples. Keeping the old wording makes failures harder to interpret.
Suggested fix
Expected: - - Metric values from both virt-controller and virt-handler - are greater than the values recorded before migration + - The virt-controller value is greater than the value recorded before migration + - A new virt-handler pod reports a value greater than 0 @@ Expected: - - Metric value is None + - The query returns no samples, or all returned values are 0Also applies to: 623-636
🤖 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 `@tests/observability/metrics/test_vms_metrics.py` around lines 597 - 612, The Expected blocks in the migration and deletion metric tests are out of sync with the shared helper behavior in the vms metrics test suite. Update the docstrings/comments in the relevant test cases to reflect that the migration check now verifies an increased virt-controller value plus a positive virt-handler series, and that the deletion check accepts either no samples or zero-valued samples. Use the existing test helper context in the VMS metrics tests to align the wording with the actual assertions so failures are described accurately.
🤖 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 `@tests/observability/metrics/conftest.py`:
- Around line 710-712: The VM fixture starts the guest and yields it
immediately, which can race guest initialization in downstream
NAD-swap/migration tests. In the fedora_vm fixture, after vm.start(wait=True) in
the context managed block, wait for readiness with vm.wait_for_agent_connected()
before yielding the VM so callers only receive a fully connected guest.
---
Outside diff comments:
In `@tests/observability/metrics/test_vms_metrics.py`:
- Around line 597-612: The Expected blocks in the migration and deletion metric
tests are out of sync with the shared helper behavior in the vms metrics test
suite. Update the docstrings/comments in the relevant test cases to reflect that
the migration check now verifies an increased virt-controller value plus a
positive virt-handler series, and that the deletion check accepts either no
samples or zero-valued samples. Use the existing test helper context in the VMS
metrics tests to align the wording with the actual assertions so failures are
described accurately.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2706b827-10f8-47a3-98c3-da65cc7a4eab
📒 Files selected for processing (3)
tests/observability/metrics/conftest.pytests/observability/metrics/test_vms_metrics.pytests/observability/metrics/utils.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/observability/metrics/test_vms_metrics.py (1)
597-612: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLOW: Update the Expected blocks to match the shared helper checks.
The migration test no longer asserts that both current series are greater than the initial sample; it checks that the virt-controller value increased and that a new virt-handler pod reports a positive value. The deletion test also accepts either no samples or zero-valued samples. Keeping the old wording makes failures harder to interpret.
Suggested fix
Expected: - - Metric values from both virt-controller and virt-handler - are greater than the values recorded before migration + - The virt-controller value is greater than the value recorded before migration + - A new virt-handler pod reports a value greater than 0 @@ Expected: - - Metric value is None + - The query returns no samples, or all returned values are 0Also applies to: 623-636
🤖 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 `@tests/observability/metrics/test_vms_metrics.py` around lines 597 - 612, The Expected blocks in the migration and deletion metric tests are out of sync with the shared helper behavior in the vms metrics test suite. Update the docstrings/comments in the relevant test cases to reflect that the migration check now verifies an increased virt-controller value plus a positive virt-handler series, and that the deletion check accepts either no samples or zero-valued samples. Use the existing test helper context in the VMS metrics tests to align the wording with the actual assertions so failures are described accurately.
🤖 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 `@tests/observability/metrics/conftest.py`:
- Around line 710-712: The VM fixture starts the guest and yields it
immediately, which can race guest initialization in downstream
NAD-swap/migration tests. In the fedora_vm fixture, after vm.start(wait=True) in
the context managed block, wait for readiness with vm.wait_for_agent_connected()
before yielding the VM so callers only receive a fully connected guest.
---
Outside diff comments:
In `@tests/observability/metrics/test_vms_metrics.py`:
- Around line 597-612: The Expected blocks in the migration and deletion metric
tests are out of sync with the shared helper behavior in the vms metrics test
suite. Update the docstrings/comments in the relevant test cases to reflect that
the migration check now verifies an increased virt-controller value plus a
positive virt-handler series, and that the deletion check accepts either no
samples or zero-valued samples. Use the existing test helper context in the VMS
metrics tests to align the wording with the actual assertions so failures are
described accurately.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2706b827-10f8-47a3-98c3-da65cc7a4eab
📒 Files selected for processing (3)
tests/observability/metrics/conftest.pytests/observability/metrics/test_vms_metrics.pytests/observability/metrics/utils.py
📜 Review details
🔇 Additional comments (3)
tests/observability/metrics/utils.py (1)
39-42: LGTM!Also applies to: 603-617, 823-932
tests/observability/metrics/conftest.py (1)
18-23: LGTM!Also applies to: 36-36, 117-117, 715-740
tests/observability/metrics/test_vms_metrics.py (1)
20-31: LGTM!Also applies to: 470-495, 574-590, 614-618, 638-641
🛑 Comments failed to post (1)
tests/observability/metrics/conftest.py (1)
710-712: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
HIGH: Wait for the guest agent before yielding the VM.
This fixture hands the VM straight to the NAD-swap/migration flow after
start(wait=True). If the guest is still booting, the subsequent mutation and metric polling can race initialization and make the test flaky on slower backends.Suggested fix
with fedora_vm(namespace=namespace.name, name=vm_name, client=unprivileged_client, spec=spec) as vm: vm.start(wait=True) + vm.wait_for_agent_connected() yield vmBased on learnings, after
vm.start()in VM-related fixtures, prefervm.wait_for_agent_connected()as the readiness gate.📝 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.with fedora_vm(namespace=namespace.name, name=vm_name, client=unprivileged_client, spec=spec) as vm: vm.start(wait=True) vm.wait_for_agent_connected() yield vm🤖 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 `@tests/observability/metrics/conftest.py` around lines 710 - 712, The VM fixture starts the guest and yields it immediately, which can race guest initialization in downstream NAD-swap/migration tests. In the fedora_vm fixture, after vm.start(wait=True) in the context managed block, wait for readiness with vm.wait_for_agent_connected() before yielding the VM so callers only receive a fully connected guest.Source: Learnings
|
Verification failed for PR #5246. Execution details |
|
/build-and-push-container |
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 `@tests/observability/metrics/constants.py`:
- Line 48: The KUBEVIRT_VMI_SYNC_TOTAL metric template is only filtering by
vm_name, which can match VMIs from other namespaces and break the “this VM only”
assumption in the observability helpers. Update the template in constants.py to
include namespace alongside vm_name, then adjust the call sites in
vm_for_migration_metrics_test and related helpers to format both namespace=...
and vm_name=... when building the query.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48e90460-8a38-4a61-9317-8d78e7a75c82
📒 Files selected for processing (4)
tests/observability/metrics/conftest.pytests/observability/metrics/constants.pytests/observability/metrics/test_vms_metrics.pytests/observability/metrics/utils.py
for more information, see https://pre-commit.ci Signed-off-by: Ohad <orevah@redhat.com>
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5246 published |
|
Verification failed for PR #5246. Execution details |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5246 published |
|
/verified All tests passed for PR #5246. Execution details |
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: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5246"
echo "== Current changed files =="
gh api "/repos/$repo/pulls/$pr/files?per_page=100" --jq '.[].filename'
files_json="$(mktemp)"
gh api "/repos/$repo/pulls/$pr/files?per_page=100" > "$files_json"
first_file="$(jq -r '.[0].filename' "$files_json")"
echo "FIRST_FILE=$first_file"
echo
echo "== First file patch =="
jq -r '.[0].patch // ""' "$files_json" | sed -n '1,120p'
first_line="$(
jq -r '.[0].patch // ""' "$files_json" | python - <<'PY'
import re, sys
cur = None
for raw in sys.stdin:
line = raw.rstrip("\n")
m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@', line)
if m:
cur = int(m.group(1))
continue
if cur is None:
continue
if line.startswith('+') and not line.startswith('+++'):
print(cur)
raise SystemExit(0)
if line.startswith('-') and not line.startswith('---'):
continue
cur += 1
raise SystemExit("no added line found")
PY
)"
echo "FIRST_CHANGED_LINE=$first_line"
echo
echo "== Outline changed Python files =="
while IFS= read -r f; do
echo "--- $f ---"
ast-grep outline "$f" --lang python || true
done < <(jq -r '.[].filename' "$files_json" | grep '^tests/observability/metrics/.*\.py$')
echo
echo "== Relevant symbol usage in observability metrics =="
rg -n -C2 \
'KUBEVIRT_VMI_SYNC_TOTAL|initial_vmi_sync_total_values|deleted_vmi_sync_total_vm|validate_vmi_sync_total_reported_and_positive|validate_vmi_sync_total_after_migration|validate_metric_value_cleared|TestVmiSyncTotal|`@pytest`\.mark\.(smoke|gating|tier2)' \
tests/observability/metrics
echo
echo "== Global usage of changed fixtures/helpers =="
rg -n -C2 \
'initial_vmi_sync_total_values|deleted_vmi_sync_total_vm|validate_vmi_sync_total_reported_and_positive|validate_vmi_sync_total_after_migration|validate_metric_value_cleared' \
tests
echo
echo "== Smoke-marked tests (actual paths) =="
rg -n -l '`@pytest`\.mark\.smoke' tests | sed -n '1,120p'
echo
echo "== Gating-marked tests (actual paths) =="
rg -n -l '`@pytest`\.mark\.gating' tests | sed -n '1,120p'
echo
echo "== Any smoke/gating markers under observability metrics =="
rg -n '`@pytest`\.mark\.(smoke|gating|tier2)' tests/observability/metrics || true
echo
echo "== Parent conftest references for smoke suites =="
python <<'PY'
from pathlib import Path
import subprocess
changed = [
"KUBEVIRT_VMI_SYNC_TOTAL",
"initial_vmi_sync_total_values",
"deleted_vmi_sync_total_vm",
"validate_vmi_sync_total_reported_and_positive",
"validate_vmi_sync_total_after_migration",
"validate_metric_value_cleared",
]
smoke_files = subprocess.check_output(
["bash", "-lc", r"rg -l '`@pytest`\.mark\.smoke' tests || true"],
text=True,
).splitlines()
repo_root = Path(".").resolve()
seen = set()
for smoke in smoke_files:
cur = (repo_root / smoke).parent
while True:
cf = cur / "conftest.py"
if cf.exists() and cf not in seen:
seen.add(cf)
text = cf.read_text()
hits = [s for s in changed if s in text]
if hits:
print(f"{cf.relative_to(repo_root)} :: {', '.join(hits)}")
if cur == repo_root:
break
cur = cur.parent
PY
body_file="$(mktemp)"
cat > "$body_file" <<'EOF'
## Test Execution Plan
**Run smoke tests: False**
No smoke-marked test has a verified dependency path to the changed symbols. `KUBEVIRT_VMI_SYNC_TOTAL`, `initial_vmi_sync_total_values`, `deleted_vmi_sync_total_vm`, `validate_vmi_sync_total_reported_and_positive`, `validate_vmi_sync_total_after_migration`, and `validate_metric_value_cleared` are only referenced within `tests/observability/metrics/`, specifically by `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal`. No smoke-marked file or parent `conftest.py` for smoke suites imports or calls these symbols.
**Run gating tests: False**
No gating-marked test is in the affected set. The changed fixtures/helpers have no callers outside `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal`, and there are no gating-marked tests under `tests/observability/metrics/`.
**Affected tests to run**
- `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total` *(tier2)*
- `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_increases_after_migration` *(tier2)*
- `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_cleared_after_vm_deletion` *(tier2)*
**Real tests (cluster required)**
Error/fix path:
`pytest tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_increases_after_migration -s -o log_cli=true -m tier2 --jira`
Expected: after live migration, the `virt-controller` metric value increases, a new `virt-handler` pod is observed, and the updated fixture/helper chain in `tests/observability/metrics/conftest.py` and `tests/observability/metrics/utils.py` succeeds.
Happy path (regression):
`pytest tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total -s -o log_cli=true -m tier2 --jira && pytest tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_cleared_after_vm_deletion -s -o log_cli=true -m tier2 --jira`
Expected: the metric is reported by both `virt-controller` and `virt-handler` before deletion, and the metric is cleared (absent or zero) after VM deletion.
**Why this scope:** `tests/observability/metrics/conftest.py` and `tests/observability/metrics/utils.py` are shared files, so real-cluster commands are required, but the dependency chain remains local: `TestVmiSyncTotal` → `initial_vmi_sync_total_values` / `deleted_vmi_sync_total_vm` → `validate_vmi_sync_total_reported_and_positive` / `validate_vmi_sync_total_after_migration` / `validate_metric_value_cleared`.
EOF
commit_id="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo
echo "== Attempt inline PR review comment via direct endpoint =="
set +e
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/comments" \
-f commit_id="$commit_id" \
-f path="$first_file" \
-F line="$first_line" \
-f side="RIGHT" \
-f body="$(cat "$body_file")"
status_inline=$?
set -e
echo "INLINE_STATUS=$status_inline"
if [ "$status_inline" -ne 0 ]; then
echo
echo "== Fallback regular PR comment =="
set +e
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/issues/$pr/comments" \
-f body="$(cat "$body_file")"
status_fallback=$?
set -e
echo "FALLBACK_STATUS=$status_fallback"
fi
rm -f "$body_file" "$files_json"Length of output: 8871 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5246"
path="tests/observability/metrics/conftest.py"
line="30"
echo "== Sanity check: current changed files =="
gh api "/repos/$repo/pulls/$pr/files?per_page=100" --jq '.[].filename'
echo
echo "== Sanity check: verify target patch context =="
gh api "/repos/$repo/pulls/$pr/files?per_page=100" \
--jq '.[] | select(.filename=="tests/observability/metrics/conftest.py") | .patch' \
| sed -n '1,40p'
echo
echo "== Verify affected symbols remain local to TestVmiSyncTotal =="
rg -n -C1 \
'initial_vmi_sync_total_values|deleted_vmi_sync_total_vm|validate_vmi_sync_total_reported_and_positive|validate_vmi_sync_total_after_migration|validate_metric_value_cleared|KUBEVIRT_VMI_SYNC_TOTAL|TestVmiSyncTotal' \
tests
echo
echo "== Verify smoke/gating markers under observability metrics =="
rg -n '`@pytest`\.mark\.(smoke|gating|tier2)' tests/observability/metrics || true
echo
echo "== Verify global smoke/gating file paths exist =="
rg -n -l '`@pytest`\.mark\.smoke' tests | sed -n '1,40p'
rg -n -l '`@pytest`\.mark\.gating' tests | sed -n '1,40p'
body_file="$(mktemp)"
cat > "$body_file" <<'EOF'
## Test Execution Plan
**Run smoke tests: False**
No smoke-marked test has a verified dependency path to the changed symbols. `KUBEVIRT_VMI_SYNC_TOTAL`, `initial_vmi_sync_total_values`, `deleted_vmi_sync_total_vm`, `validate_vmi_sync_total_reported_and_positive`, `validate_vmi_sync_total_after_migration`, and `validate_metric_value_cleared` are only referenced within `tests/observability/metrics/`, specifically by `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal`. No smoke-marked file or parent `conftest.py` for smoke suites imports or calls these symbols.
**Run gating tests: False**
No gating-marked test is in the affected set. The changed fixtures/helpers have no callers outside `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal`, and there are no gating-marked tests under `tests/observability/metrics/`.
**Affected tests to run**
- `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total` *(tier2)*
- `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_increases_after_migration` *(tier2)*
- `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_cleared_after_vm_deletion` *(tier2)*
**Real tests (cluster required)**
Error/fix path:
`pytest tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_increases_after_migration -s -o log_cli=true -m tier2 --jira`
Expected: after live migration, the `virt-controller` metric value increases, a new `virt-handler` pod is observed, and the updated fixture/helper chain in `tests/observability/metrics/conftest.py` and `tests/observability/metrics/utils.py` succeeds.
Happy path (regression):
`pytest tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total -s -o log_cli=true -m tier2 --jira && pytest tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_cleared_after_vm_deletion -s -o log_cli=true -m tier2 --jira`
Expected: the metric is reported by both `virt-controller` and `virt-handler` before deletion, and the metric is cleared (absent or zero) after VM deletion.
**Why this scope:** `tests/observability/metrics/conftest.py` and `tests/observability/metrics/utils.py` are shared files, so real-cluster commands are required, but the dependency chain remains local: `TestVmiSyncTotal` → `initial_vmi_sync_total_values` / `deleted_vmi_sync_total_vm` → `validate_vmi_sync_total_reported_and_positive` / `validate_vmi_sync_total_after_migration` / `validate_metric_value_cleared`.
EOF
commit_id="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo
echo "== Attempt 1: direct inline PR review comment endpoint =="
set +e
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/comments" \
-f commit_id="$commit_id" \
-f path="$path" \
-F line="$line" \
-f side="RIGHT" \
-f body="$(cat "$body_file")"
status_inline=$?
set -e
echo "INLINE_STATUS=$status_inline"
if [ "$status_inline" -ne 0 ]; then
echo
echo "== Attempt 2: review API with inline comment payload =="
set +e
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/reviews" \
-f event=COMMENT \
-f commit_id="$commit_id" \
-f body= \
-f "comments[][path]=$path" \
-F "comments[][line]=$line" \
-f "comments[][side]=RIGHT" \
-f "comments[][body]=$(cat "$body_file")"
status_review=$?
set -e
echo "REVIEW_STATUS=$status_review"
else
status_review=0
fi
if [ "${status_inline:-1}" -ne 0 ] && [ "${status_review:-1}" -ne 0 ]; then
echo
echo "== Fallback: regular PR comment =="
set +e
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/issues/$pr/comments" \
-f body="$(cat "$body_file")"
status_fallback=$?
set -e
echo "FALLBACK_STATUS=$status_fallback"
fi
rm -f "$body_file"Length of output: 16338
CRITICAL: Retry completed, but GitHub rejected all write paths with Why this matters: your workflow requires the plan to be posted as a PR comment. I can analyze the PR, but without comment/review write permission, I cannot complete the delivery step. Verified target
Verified findings
Ready-to-post content ## Test Execution Plan
**Run smoke tests: False**
No smoke-marked test has a verified dependency path to the changed symbols. `KUBEVIRT_VMI_SYNC_TOTAL`, `initial_vmi_sync_total_values`, `deleted_vmi_sync_total_vm`, `validate_vmi_sync_total_reported_and_positive`, `validate_vmi_sync_total_after_migration`, and `validate_metric_value_cleared` are only referenced within `tests/observability/metrics/`, specifically by `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal`. No smoke-marked file or parent `conftest.py` for smoke suites imports or calls these symbols.
**Run gating tests: False**
No gating-marked test is in the affected set. The changed fixtures/helpers have no callers outside `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal`, and there are no gating-marked tests under `tests/observability/metrics/`.
**Affected tests to run**
- `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total` *(tier2)*
- `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_increases_after_migration` *(tier2)*
- `tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_cleared_after_vm_deletion` *(tier2)*
**Real tests (cluster required)**
Error/fix path:
`pytest tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_increases_after_migration -s -o log_cli=true -m tier2 --jira`
Expected: after live migration, the `virt-controller` metric value increases, a new `virt-handler` pod is observed, and the updated fixture/helper chain in `tests/observability/metrics/conftest.py` and `tests/observability/metrics/utils.py` succeeds.
Happy path (regression):
`pytest tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total -s -o log_cli=true -m tier2 --jira && pytest tests/observability/metrics/test_vms_metrics.py::TestVmiSyncTotal::test_kubevirt_vmi_sync_total_cleared_after_vm_deletion -s -o log_cli=true -m tier2 --jira`
Expected: the metric is reported by both `virt-controller` and `virt-handler` before deletion, and the metric is cleared (absent or zero) after VM deletion.
**Why this scope:** `tests/observability/metrics/conftest.py` and `tests/observability/metrics/utils.py` are shared files, so real-cluster commands are required, but the dependency chain remains local: `TestVmiSyncTotal` → `initial_vmi_sync_total_values` / `deleted_vmi_sync_total_vm` → `validate_vmi_sync_total_reported_and_positive` / `validate_vmi_sync_total_after_migration` / `validate_metric_value_cleared`.HIGH: Once the integration has PR comment/review write permission on |
|
/lgtm |
|
/approve |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-5246. |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
11 participants
What this PR does / why we need it:
Verify that kubevirt_vmi_sync_total metric is reported by both virt-controller and virt-handler, that after live migration the virt-controller value increases and the virt-handler pod changes, and that the metric is cleared after VM deletion.
Assisted-by: claude code claude-opus-4-6
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
https://redhat.atlassian.net/browse/CNV-89660
Summary by CodeRabbit
kubevirt_vmi_sync_totalPrometheus metric constant to improve VM migration observability coverage.kubevirt_vmi_sync_totalmigration tests to rely on shared polling-based helpers that validate: the metric is reported and positive, values increase after live migration across controller/handler components, and the metric is cleared after VM deletion (including re-enabling the previously skipped check).