stp, sig-virt: Introduce the PCI Topology Stability STP - #138
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Software Test Plan for PCI topology stability, defining PCI fingerprint verification across VM lifecycle and upgrade scenarios, execution requirements, limitations, traceability, and approvals. ChangesPCI topology stability
Estimated code review effort: 2 (Simple) | ~10 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 |
|
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
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. |
|
mtessun can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@stps/sig-virt/pci-topology-stability.md`:
- Line 124: Update the version entries in the upgrade scenario table to specify
explicit source and target OCP and OpenShift Virtualization/CNV versions,
including the supported upgrade path between them. Apply the same correction to
the corresponding entry referenced near line 171, preserving the table’s
existing format.
- Line 140: Remove lspci from the “Other Tools” entry in Section II.3.1, since
it is a standard guest utility; retain it only as an environment prerequisite if
that documentation is needed, and leave the table limited to new or non-standard
tooling introduced by the plan.
- Line 45: Add explicit PM/UX rationale to the NFRs entry explaining why
usability testing is out of scope for this topology-assignment feature, rather
than relying solely on “No UI component.” Update the UI/usability coverage
statement while preserving the existing non-functional requirement
classifications.
- Line 45: Revise the STP narrative sections at the NFR entry and the referenced
locations to describe only user-observable topology behavior and outcomes.
Remove internal implementation details such as VM annotations, SSH or parsing
mechanics, virt-launcher, and domain-definition logic; retain those details only
in associated test-case verification steps.
- Line 45: Update the scalability statement in the Non-Functional Requirements
table to document concrete boundaries of the topology assignment mechanism,
including relevant VM/device counts and migration concurrency limits where
applicable, or explicitly explain why it introduces no additional platform
constraints. Replace the generic “per-VM; no scale concerns” wording while
preserving the existing scope of the NFR.
- Line 44: Add acceptance criteria for live migration and CNV upgrade that
verify workload continuity throughout each operation, such as the guest
application continuing successfully without interruption or restart. Keep the
existing PCI address stability criteria, and ensure the new conditions can only
pass when no disruption occurs.
- Around line 49-53: Replace every unresolved “TBD” sign-off in the PCI topology
stability document, including the known/test limitation entries and all
Out-of-Scope PM/Lead Agreement rows, with named approvers and approval dates.
Add links to relevant open bugs for affected limitations where applicable, and
ensure no approval placeholders remain before approval.
- Around line 168-171: Update the post-operation steps in CNV-16326 through
CNV-16329 to require explicit stability gates before the second PCI fingerprint
capture: verify the VM is running after restart, confirm migration and snapshot
restore completion, and confirm the CNV upgrade is complete with a healthy
cluster. Specify the concrete status or health checks used for each gate while
preserving the existing fingerprint comparison.
- Line 20: Update the PCI fingerprint and related stability checks to hash
stable guest-visible device identity/address pairs rather than only the sorted
address set, ensuring swapped addresses between devices are detected. Apply the
same mapping-based validation to the scenarios and assertions referenced in the
document, and keep the requirement explicitly focused on per-device address
stability.
- Line 13: Update the Feature Maturity entry in the overview table to include
structured DP, TP, and GA entries with their target versions instead of N/A, and
revise the Feature Overview section to explicitly identify the phase covered by
this STP. Preserve the existing metadata structure and use the project’s
established phase terminology.
- Around line 115-116: Update the Cross Integrations row in the PCI topology
stability test strategy to reflect the storage operator integration already
identified in Dependencies: mark it applicable and document the required
snapshot/restore coverage and responsible component or owner. Do not leave it as
N/A or claim the feature is entirely within sig-virt scope.
- Line 63: Update the “Topology Considerations” entry to make arm64 coverage
auditable by naming the responsible multi-arch test suite, owning team, and Jira
requirement, or move the arm64 test case into this plan with equivalent coverage
details. Preserve the existing amd64/arm64 support claim only when the
referenced ownership and tracking information is explicit.
🪄 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: 8d4516cb-2639-4c51-b6b9-9e2bd2378b66
📒 Files selected for processing (1)
stps/sig-virt/pci-topology-stability.md
| | **Feature Tracking** | N/A | | ||
| | **Epic Tracking** | N/A | | ||
| | **Jira Tracking** | [CNV-81270](https://redhat.atlassian.net/browse/CNV-81270) | | ||
| | **Feature Maturity** | N/A | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
HIGH: Complete phase metadata and state the covered phase.
Feature Maturity is only N/A, and the overview never identifies the current phase or which phase this STP covers. Add structured DP, TP, and GA entries with target versions, then state the covered phase in the overview.
As per coding guidelines, Feature Maturity must use the structured DP/TP/GA format and Feature Overview must identify the covered phase.
Also applies to: 22-30
🤖 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 `@stps/sig-virt/pci-topology-stability.md` at line 13, Update the Feature
Maturity entry in the overview table to include structured DP, TP, and GA
entries with their target versions instead of N/A, and revise the Feature
Overview section to explicitly identify the phase covered by this STP. Preserve
the existing metadata structure and use the project’s established phase
terminology.
Source: Coding guidelines
| | **Understand Value** | [x] | Customers depend on stable device addresses so the guest continues to recognize disks, network interfaces, and other devices after lifecycle operations. Address shifts cause data unavailability and application failures. | | | ||
| | **Customer Use Cases** | [x] | 1. As a VM administrator, I want my VM's device addresses to remain unchanged after a restart so that the guest OS continues to recognize all devices.<br>2. As a VM administrator, I want my VM's device addresses to remain unchanged after live migration so that applications continue to function.<br>3. As a VM administrator, I want my VM's device addresses to remain unchanged after restoring from a snapshot so that applications resume with the same device layout.<br>4. As a cluster administrator, I want my VMs' device addresses to remain unchanged after a CNV upgrade so that the upgrade does not disrupt running workloads. | | | ||
| | **Testability** | [x] | All requirements are testable by capturing PCI device addresses from the guest before and after each operation. | | | ||
| | **Acceptance Criteria** | [x] | 1. PCI device addresses visible to the guest are identical before and after VM restart.<br>2. PCI device addresses visible to the guest are identical before and after live migration.<br>3. PCI device addresses visible to the guest are identical before and after snapshot restore.<br>4. PCI device addresses visible to the guest are identical before and after CNV upgrade. | | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
HIGH: Make acceptance criteria prove non-disruption.
The use cases claim that applications continue functioning and upgrades do not disrupt workloads, but these criteria only compare PCI fingerprints before and after the operation. A workload could be interrupted and still produce the same fingerprint. Add a high-level continuity/no-disruption criterion for live migration and upgrade, or narrow those user-facing claims.
As per coding guidelines, non-disruptive behavior requires at least one acceptance condition that can only pass when disruption did not occur.
🧰 Tools
🪛 LanguageTool
[style] ~44-~44: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... before and after live migration.
3. PCI device addresses visible to the guest a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~44-~44: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...efore and after snapshot restore.
4. PCI device addresses visible to the guest a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 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 `@stps/sig-virt/pci-topology-stability.md` at line 44, Add acceptance criteria
for live migration and CNV upgrade that verify workload continuity throughout
each operation, such as the guest application continuing successfully without
interruption or restart. Keep the existing PCI address stability criteria, and
ensure the new conditions can only pass when no disruption occurs.
Source: Coding guidelines
| | **Customer Use Cases** | [x] | 1. As a VM administrator, I want my VM's device addresses to remain unchanged after a restart so that the guest OS continues to recognize all devices.<br>2. As a VM administrator, I want my VM's device addresses to remain unchanged after live migration so that applications continue to function.<br>3. As a VM administrator, I want my VM's device addresses to remain unchanged after restoring from a snapshot so that applications resume with the same device layout.<br>4. As a cluster administrator, I want my VMs' device addresses to remain unchanged after a CNV upgrade so that the upgrade does not disrupt running workloads. | | | ||
| | **Testability** | [x] | All requirements are testable by capturing PCI device addresses from the guest before and after each operation. | | | ||
| | **Acceptance Criteria** | [x] | 1. PCI device addresses visible to the guest are identical before and after VM restart.<br>2. PCI device addresses visible to the guest are identical before and after live migration.<br>3. PCI device addresses visible to the guest are identical before and after snapshot restore.<br>4. PCI device addresses visible to the guest are identical before and after CNV upgrade. | | | ||
| | **Non-Functional Requirements (NFRs)** | [x] | Monitoring: No new metrics or alerts required. Observability: No new observability requirements; topology version is visible via existing VM annotations. UI: No UI component. Documentation: Covered by upstream documentation. Performance: No performance targets; topology assignment is a one-time operation during VM startup. Security: No security implications. Scalability: Feature is per-VM; no scale concerns. | | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
HIGH: Add PM/UX justification for skipping UI and usability testing.
“No UI component” is not the required customer-value justification for marking usability out of scope. Add inline PM/UX rationale, or mark usability applicable and define the coverage.
As per coding guidelines, UI testing cannot be dismissed solely because no code changes or UI component are present.
Also applies to: 111-111
🤖 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 `@stps/sig-virt/pci-topology-stability.md` at line 45, Add explicit PM/UX
rationale to the NFRs entry explaining why usability testing is out of scope for
this topology-assignment feature, rather than relying solely on “No UI
component.” Update the UI/usability coverage statement while preserving the
existing non-functional requirement classifications.
Source: Coding guidelines
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
HIGH: Remove internal implementation details from the STP narrative.
References to VM annotations, SSH/parsing mechanics, virt-launcher, and domain-definition logic describe implementation rather than user experience. Reword these sections around observable outcomes and keep implementation-specific verification details in the associated test cases.
As per coding guidelines, STPs must describe what users experience and avoid internal component references and implementation mechanisms.
Also applies to: 60-61, 160-160
🤖 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 `@stps/sig-virt/pci-topology-stability.md` at line 45, Revise the STP narrative
sections at the NFR entry and the referenced locations to describe only
user-observable topology behavior and outcomes. Remove internal implementation
details such as VM annotations, SSH or parsing mechanics, virt-launcher, and
domain-definition logic; retain those details only in associated test-case
verification steps.
Source: Coding guidelines
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
HIGH: Replace the generic scalability claim with concrete boundaries.
“Feature is per-VM; no scale concerns” does not acknowledge platform limits or define the tested scale. Document relevant limits such as VM/device counts, migration concurrency, or explain why the existing mechanism introduces none.
As per coding guidelines, scalability NFRs must acknowledge constraints imposed by the underlying platform mechanism.
Also applies to: 109-109
🤖 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 `@stps/sig-virt/pci-topology-stability.md` at line 45, Update the scalability
statement in the Non-Functional Requirements table to document concrete
boundaries of the topology assignment mechanism, including relevant VM/device
counts and migration concurrency limits where applicable, or explicitly explain
why it introduces no additional platform constraints. Replace the generic
“per-VM; no scale concerns” wording while preserving the existing scope of the
NFR.
Source: Coding guidelines
| | **Technology Challenges** | [x] | Verification requires running commands inside the guest OS to observe the assigned addresses — there is no host-side API that exposes the guest-visible topology. Tests must SSH into the guest and parse device enumeration output. | | | ||
| | **API Extensions** | [x] | No new user-facing APIs. Topology version is tracked via existing VM annotations. Annotation presence and values are covered by upstream tests; downstream tests focus on the user-visible outcome (stable addresses). | | | ||
| | **Test Environment Needs** | [x] | See environment requirements in Section II.3 and testing tools in Section II.3.1 | | | ||
| | **Topology Considerations** | [x] | At least 2 worker nodes required for migration tests. Feature applies only to amd64 and arm64 architectures. Migration test requires multi-worker cluster. Restart and snapshot/restore tests work on any topology including SNO. | | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the document and inspect the relevant section with line numbers
wc -l stps/sig-virt/pci-topology-stability.md
sed -n '1,180p' stps/sig-virt/pci-topology-stability.md | cat -n
# Find all references to compatibility / arm64 / multi-arch / external suites
rg -n "Compatibility|arm64|amd64|multi-arch|multiarch|architecture|external|owner|requirement|requirements|N/A" stps/sig-virt/pci-topology-stability.mdRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 34084
HIGH: Make the arm64 compatibility coverage traceable. Section II.2 says arm64 is handled separately by the multi-arch team, but it does not name the suite, owner, or Jira requirement. Add that traceability here or move the arm64 case into this plan so the supported-architecture claim is auditable.
🤖 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 `@stps/sig-virt/pci-topology-stability.md` at line 63, Update the “Topology
Considerations” entry to make arm64 coverage auditable by naming the responsible
multi-arch test suite, owning team, and Jira requirement, or move the arm64 test
case into this plan with equivalent coverage details. Preserve the existing
amd64/arm64 support claim only when the referenced ownership and tracking
information is explicit.
| | CNV-16326 | As a VM administrator, I want my VM's device addresses to remain stable after restart. | Boot a VM, capture PCI fingerprint, stop and start the VM, capture fingerprint again, verify they match. | Tier 2 | P0 | | ||
| | CNV-16327 | As a VM administrator, I want my VM's device addresses to remain stable after live migration. | Boot a VM, capture PCI fingerprint, live-migrate the VM to another node, capture fingerprint again, verify they match. | Tier 2 | P0 | | ||
| | CNV-16328 | As a VM administrator, I want my VM's device addresses to remain stable after snapshot restore. | Boot a VM, capture PCI fingerprint, take a snapshot, restore the VM from the snapshot, capture fingerprint again, verify they match. | Tier 2 | P0 | | ||
| | CNV-16329 | As a cluster administrator, I want my VMs' device addresses to remain stable after a CNV upgrade. | Capture PCI fingerprints for all upgrade VMs before CNV upgrade, perform the upgrade, capture fingerprints again, verify they match. | Tier 2 | P0 | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
HIGH: Add stable-state gates before post-operation captures.
Each scenario says to capture the fingerprint after an operation but does not require the VM or cluster to reach a stable state. Capturing during restart, restore, migration, or upgrade transitions can create false failures. Specify concrete completion gates, such as VM running, migration/restore complete, and upgrade complete with a healthy cluster.
As per coding guidelines, post-operation scenarios must justify and define the concrete mechanism used to establish stable state.
🤖 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 `@stps/sig-virt/pci-topology-stability.md` around lines 168 - 171, Update the
post-operation steps in CNV-16326 through CNV-16329 to require explicit
stability gates before the second PCI fingerprint capture: verify the VM is
running after restart, confirm migration and snapshot restore completion, and
confirm the CNV upgrade is complete with a healthy cluster. Specify the concrete
status or health checks used for each gate while preserving the existing
fingerprint comparison.
Source: Coding guidelines
44915ef to
5d1385c
Compare
|
mtessun can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
5d1385c to
ac17c7e
Compare
|
mtessun can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
ac17c7e to
ff84473
Compare
|
mtessun can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
dbe1830 to
320ed7f
Compare
|
mtessun can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
rnetser
left a comment
rnetser
left a comment
There was a problem hiding this comment.
Code Review
Found 3 issue(s) in this PR:
💡 Suggestions (3)
| File | Line | Issue |
|---|---|---|
stps/sig-virt/pci-topology-stability.md |
63 | Several sections use internal implementation terms: `kubevirt.io/pciTopologyVers |
stps/sig-virt/pci-topology-stability.md |
222 | Timeline/Schedule, Test Environment, Untestable Aspects, and Resource Constraint |
stps/sig-virt/pci-topology-stability.md |
10 | Two nits on this line: |
Review generated by pi
Assisted-by: PI (claude-opus-4-6-1m)
| - [x] **Non-Functional Requirements (NFRs)** | ||
| - *List applicable NFRs and their targets:* | ||
| - Monitoring: No new metrics or alerts required. | ||
| - Observability: No dedicated observability tooling. Topology version can be inspected via the `kubevirt.io/pciTopologyVersion` VM annotation, but this is not exposed in dashboards or metrics. |
There was a problem hiding this comment.
Several sections use internal implementation terms: kubevirt.io/pciTopologyVersion annotation (NFR), VMI creation and VM template creation (Out of Scope), virt-launcher and domain definition logic (Risks). Technology/Design sections are more lenient, but NFR Observability and Out of Scope rationale should use user-facing language where possible. For example: "topology version tracked via a VM annotation" instead of naming the annotation key.
Assisted-by: PI (claude-opus-4-6-1m)
| - **Enhancement(s):** N/A — downstream regression tests for PCI topology stability | ||
| - **Feature Tracking:** N/A | ||
| - **Epic Tracking:** N/A | ||
| - **Jira Tracking:** [CNV-81270](https://redhat.atlassian.net/browse/CNV-81270) |
There was a problem hiding this comment.
Two nits on this line:
- Jira link uses old
redhat.atlassian.netURL — please update toissues.redhat.com Jira Trackingis not a standard template field. Template usesFeature TrackingandEpic Tracking. Since both are N/A for regression tests, consider usingFeature Tracking: N/A — regression testsand moving the Jira link to Epic Tracking or a note.
Assisted-by: PI (claude-opus-4-6-1m)
Add Software Test Plan for PCI topology stability regression tests, covering PCI address fingerprint verification across VM restart, live migration, snapshot/restore, and CNV upgrade. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Samuel Albershtein <salbersh@redhat.com>
320ed7f to
2a939a5
Compare
|
mtessun can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
| - QE Architect (OCP-V): [Ruth Netser](@rnetser) | ||
| - QE Members (OCP-V): [Akriti Gupta](@akri3i), [Samuel Alberstein](@SamAlber) | ||
| - Principal QE (OCP-V): [Den Shchedrivyi](@dshchedr), [Vasiliy Sibirskiy](@vsibirsk) | ||
| - Principal Developer (OCP-V): [Jean-Edouard Babin](@jean-edouard) |
There was a problem hiding this comment.
Babin? :)
| - Principal Developer (OCP-V): [Jean-Edouard Babin](@jean-edouard) | |
| - Principal Developer (OCP-V): [Jed Lejosne](@jean-edouard) |
| - **Participating SIGs:** sig-virt | ||
|
|
||
| **Document Conventions (if applicable):** | ||
| - PCI fingerprint: An md5 hash of the sorted PCI device addresses visible to the guest, used to detect topology changes across lifecycle operations. |
There was a problem hiding this comment.
Why an md5 hash? Even if you want to compare character-per-character you can do that on the PCI data itself instead of hashes. My concern here is that you can't investigate failures with just hashes. Also, a hash is fragile, maybe some unimportant data can be allowed to change...
Either way, make sure you consult @mhenriks about the level of detail we should capture (i.e. the minimum lspci params we need to make sure nothing important changed). In fact, it'd be nice if he could have a look at this document before merge.
There was a problem hiding this comment.
Thanks @jean-edouard for the valuable feedback.
I've dropped the MD5 hash in the test implementation. The test now captures full lspci output lines (BDF address + device description) and compares them directly as a list. This means:
Debuggability -- on failure you see exactly which device moved, appeared, or disappeared (e.g. "05:00.0 SCSI storage controller: Red Hat, Inc. Virtio 1.0 SCSI" was replaced by something else), instead of two opaque hashes.
Device swap detection -- comparing full lines (address + device type) catches two devices swapping addresses, which the old BDF-only hash could not detect.
@mhenriks, would be great to get your input on whether plain lspci output is the right granularity, or if we should filter to specific fields. The current approach captures what a customer would see running lspci inside the guest, which seemed like the natural baseline.
Will update the STP's Document Conventions to reflect this change.
There was a problem hiding this comment.
For the tests described in here I think lspci will be fine. If you ever do like a start + stop + upgrade + start scenario then you may want to lspci -Dmmn (machine-readable numeric output) and compare BDF + class/vendor/device ID fields only
|
mtessun can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
Remove internal implementation terms and rewrite in user-facing language. Replace MD5 fingerprint with direct PCI device line comparison. Restructure Developer Handoff into concrete takeaways. Fix reviewer name, approvers list, and risk formatting. Signed-off-by: Samuel Albershtein <salbersh@redhat.com> Assisted-by: Claude <noreply@anthropic.com>
d0b4ca2 to
92e22e5
Compare
|
Clean rebase detected — no code changes compared to previous head ( |
|
mtessun can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests-design-docs repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
11 participants
Add Software Test Plan for PCI topology stability regression tests, covering PCI address verification across VM restart, live migration, snapshot/restore, and CNV upgrade.
Assisted-by: Claude noreply@anthropic.com
STP Metadata
VEP issue:
What this PR does
Special notes for your reviewer
Summary by CodeRabbit