[CNV-82132] Add STP for annotation-based GuestAgentPing probe pausing - #128
[CNV-82132] Add STP for annotation-based GuestAgentPing probe pausing#128geetikakay wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 21 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 complete Software Test Plan for annotation-based pausing of GuestAgentPing probes, covering scope, QE review criteria, test strategy, environment requirements, risks, traceability scenarios, and approval assignments. ChangesGuest Agent Probe Pause
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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. |
|
|
||
| ### **Feature Overview** | ||
|
|
||
| OpenShift Virtualization 4.22 introduces annotation-based GuestAgentPing probe pausing, designed per VEP 207. VM administrators can temporarily pause GuestAgentPing-based liveness and readiness probes on running VirtualMachineInstances by setting the `kubevirt.io/pause-guest-agent-probes` annotation. While paused, probes report success, preventing unwanted VM restarts or readiness transitions during planned maintenance such as guest OS updates, driver installations, or agent upgrades. Removing the annotation resumes normal probe behavior. The feature requires no feature gate and operates independently of existing probe suppression mechanisms for live migration and VM pause. |
There was a problem hiding this comment.
right, i saw epic is for CNV v5.0.0 so i will keep 5.0 and 4.23 both
|
|
||
| ### **Feature Overview** | ||
|
|
||
| OpenShift Virtualization 4.22 introduces annotation-based GuestAgentPing probe pausing, designed per VEP 207. VM administrators can temporarily pause GuestAgentPing-based liveness and readiness probes on running VirtualMachineInstances by setting the `kubevirt.io/pause-guest-agent-probes` annotation. While paused, probes report success, preventing unwanted VM restarts or readiness transitions during planned maintenance such as guest OS updates, driver installations, or agent upgrades. Removing the annotation resumes normal probe behavior. The feature requires no feature gate and operates independently of existing probe suppression mechanisms for live migration and VM pause. |
There was a problem hiding this comment.
I would change probes report success to -> guest agent probe report success
|
|
||
| - [ ] **Acceptance Criteria** | ||
| - Ensured acceptance criteria are **defined clearly** (clear user stories; D/S requirements clearly defined in Jira). | ||
| - Seven criteria in Jira epic, detailed in VEP 207 design document. Implementation PR (kubevirt/kubevirt#17664) merged with unit and e2e tests covering core pause/resume lifecycle. |
There was a problem hiding this comment.
Would it make sense to write the criteria here? Btw. I can see only 3 criterias in epic
There was a problem hiding this comment.
I think adding a reference (link) to them in the merged VEP file is enough.
The above PR link is the link to the implementation, not sure it should be here, or it should be under a new criteria -
|
|
||
| #### **2. Known Limitations** | ||
|
|
||
| - No automatic triggering by internal operations (e.g., snapshots, live migration). The annotation is strictly user-controlled. |
There was a problem hiding this comment.
probably worth to mention, that there is no atomatic mechanism to enable the probe functionality again.
There was a problem hiding this comment.
that there is no atomatic mechanism to enable the probe functionality again.
it can set any time by users?
There was a problem hiding this comment.
One of the feature user stories mentioned to use a scheduled job - pause GuestAgentPing probes on specific VMIs during scheduled maintenance windows
This sounds like they are updating all the drivers once every x time, so a CronJob might need to do it.
I am not sure if this counts as automatic/internal or not, but it is a bit misleading to have such a goal, while saying we have no automatic way of doing it.
There was a problem hiding this comment.
hmm...what this means is: this annotation is never set by any operations and an admin/owner of VM could add it manually or via scripts.
| #### **3. Test Environment** | ||
|
|
||
| - **Cluster Topology:** Multi-node cluster with at least 2 schedulable worker nodes (e.g., 3 control-plane + 2 worker compact or standard topology) | ||
| - **OCP & OpenShift Virtualization Version(s):** OCP 4.22+ / OpenShift Virtualization 4.22+ |
| - **Owning SIG:** sig-infra | ||
| - **Participating SIGs:** N/A | ||
|
|
||
| **Document Conventions (if applicable):** VMI = VirtualMachineInstance, VEP = Virtualization Enhancement Proposal |
There was a problem hiding this comment.
Can you write them in new lines ? hard to read them like this.
There was a problem hiding this comment.
i saw network team using this pattern but AGENTS.md does not enforce bullet-point formatting. It says "no walls of text" but doesn't prescribe bullets over prose and that's single line.
There was a problem hiding this comment.
Ok, I will try to work on it in the template.md file at a later time, it might only be an issue for me.
There was a problem hiding this comment.
@RoniKishner I think we should get it checked if we want it to be enforced. let's do it!!#133
|
|
||
| - [ ] **Acceptance Criteria** | ||
| - Ensured acceptance criteria are **defined clearly** (clear user stories; D/S requirements clearly defined in Jira). | ||
| - Seven criteria in Jira epic, detailed in VEP 207 design document. Implementation PR (kubevirt/kubevirt#17664) merged with unit and e2e tests covering core pause/resume lifecycle. |
There was a problem hiding this comment.
I think adding a reference (link) to them in the merged VEP file is enough.
The above PR link is the link to the implementation, not sure it should be here, or it should be under a new criteria -
|
|
||
| #### **2. Known Limitations** | ||
|
|
||
| - No automatic triggering by internal operations (e.g., snapshots, live migration). The annotation is strictly user-controlled. |
There was a problem hiding this comment.
One of the feature user stories mentioned to use a scheduled job - pause GuestAgentPing probes on specific VMIs during scheduled maintenance windows
This sounds like they are updating all the drivers once every x time, so a CronJob might need to do it.
I am not sure if this counts as automatic/internal or not, but it is a bit misleading to have such a goal, while saying we have no automatic way of doing it.
9aeea94 to
d3a4067
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md`:
- Around line 115-121: Update the “Out of Scope (Testing Scope Exclusions)”
checklist so every item uses the required “Item / Rationale: / PM/Lead
Agreement:” structure, including a clear rationale and PM/Lead name and date for
each entry. Preserve the existing exclusions and their intended scope while
adding the missing fields.
- Around line 83-85: Update the API Extensions checklist entry to remove the
internal source path reference and describe only the user-facing VMI annotation
kubevirt.io/pause-guest-agent-probes, including its accepted truthy and falsy
values and observable behavior.
- Around line 59-64: Add “*Sign-off:* [Name/Date]” beneath each limitation in
the “Known Limitations” section, including the duplicate live-migration entries,
so every listed limitation has an explicit approval sign-off.
- Around line 163-174: Update the Platform entry in the Test Environment section
to use the repository’s standard platform-agnostic wording, replacing the fixed
“bare metal or cloud provider” list while leaving the other environment
requirements unchanged.
- Around line 101-113: Rewrite the Testing Goals list as SMART, actionable
scenarios that specify VMI state, probe configuration, annotation value,
guest-agent state, migration topology, and observable outcomes for each goal.
Replace generic “Verify” wording with concrete setup, action, and
expected-result criteria, and split the combined annotation persistence,
snapshot independence, and concurrent-safety item into three separate goals.
- Around line 46-53: Add traceability for every acceptance criterion in the
document, especially dedicated scenarios validating unchanged Pod probe
specifications and live-migration suppression taking precedence over the user
annotation. Map the existing scenarios in the referenced sections to each
criterion, or add scenarios where coverage is missing, including criteria for
manual pause/unpause, backward compatibility, unaffected non-GuestAgentPing
probes, and annotation removal.
- Around line 271-291: Reclassify the cross-feature scenarios in the CNV-82392
and CNV-82132 entries, along with the referenced entries near the access
credential, snapshot, and pause/unpause scenarios, from Tier 1 to Tier 2. Update
only the affected Tier fields while preserving each scenario’s wording,
priority, and other metadata.
- Around line 28-57: Update the checklist sections in this document, including
the referenced sections, by marking items with completed evidence as [x]. Remove
unresolved template prompts such as the unanswered customer-value question, and
leave [ ] only for genuinely pending or non-applicable items with an explicit
justification.
- Line 57: Update the documentation readiness status in
CNV-82132-pause-guest-agent-probes.md so the upstream documentation state and
the testing entry criterion agree. Revise the references around the upstream
documentation note and the entry criterion near the testing requirements to
reflect whether the documentation is actually merged, without changing unrelated
readiness items.
- Around line 243-250: Update the CNV-82132 resume scenario to explicitly state
the guest agent’s state before annotation removal. Clarify that removing the
annotation resumes normal probe behavior without inherently restarting the VMI,
and make the liveness-triggered restart expectation conditional on the guest
agent remaining stopped.
- Line 1: Resolve all nine outstanding review conversations for this test plan,
including the conversations associated with lines 14, 81, 107, and 114, so the
required can-be-merged check passes.
- Around line 68-70: Replace the template Developer Handoff/QE Kickoff checklist
entry with a completed record containing the actual handoff date, participants,
decisions, and takeaways. Expand the existing VEP 207 review information into
concrete outcomes, including what was decided and any remaining action items or
testing implications, rather than only listing covered topics.
- Around line 151-152: Update the Upgrade Testing section in
CNV-82132-pause-guest-agent-probes.md to state that the VMI annotation remains
persisted on the Kubernetes object until explicitly removed, while
SyncVirtualMachine gRPC is only its delivery mechanism. Replace the N/A details
with upgrade/restart coverage that verifies the annotation is retained and
continues to function across virt-launcher rollbacks or upgrades.
- Around line 142-143: Update the Usability Testing section in
CNV-82132-pause-guest-agent-probes.md to document the PM/UX decision and
customer-value rationale for omitting usability and accessibility testing,
rather than citing only the no-UI labels.
- Around line 5-18: Add the required structured DP, TP, and GA Feature Maturity
metadata block near the existing Metadata & Tracking section, and state both the
feature’s current phase and the phase covered by this STP in Feature Overview.
Revise Feature Overview to describe only user-visible probe-pausing behavior and
remove implementation details such as feature-gate status.
🪄 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: ca3ecb94-e972-4758-aad8-93e2b0ca03d1
📒 Files selected for processing (1)
stps/sig-infra/CNV-82132-pause-guest-agent-probes.md
| - [ ] **Review Requirements** | ||
| - Reviewed the relevant requirements. | ||
| - VEP 207 ([kubevirt/enhancements#208](https://github.com/kubevirt/enhancements/pull/208)) defines clear scope: GuestAgentPing probes only, annotation-based manual control. Seven acceptance criteria are documented in the Jira epic CNV-82132. | ||
|
|
||
| - [ ] **Understand Value and Customer Use Cases** | ||
| - Confirmed clear user stories and intent. | ||
| - Understand the difference between U/S and D/S requirements. | ||
| - **What is the value of the feature for RH customers**. | ||
| - Ensured requirements contain relevant **customer use cases**. | ||
| - User stories: | ||
| - VM owner pauses probes during individual VM maintenance (e.g., guest OS update, driver installation) | ||
| - Cluster admin pauses probes across multiple VMIs during scheduled maintenance windows | ||
| - Value: Prevents unwanted VM restarts during planned guest agent downtime, reducing maintenance disruption for customers | ||
|
|
||
| - [ ] **Testability** | ||
| - Confirmed requirements are **testable and unambiguous**. | ||
| - All seven acceptance criteria are directly testable: annotation set/remove behavior, probe type isolation, annotation value parsing and migration propagation. | ||
|
|
||
| - [ ] **Acceptance Criteria** | ||
| - Ensured acceptance criteria are **defined clearly** (clear user stories, D/S requirements clearly defined in Jira). | ||
| - Derived from VEP 207 goals: | ||
| - Manual pause/unpause of GuestAgentPing probes via VMI annotations without Pod restart | ||
| - Strict backward compatibility maintained (Pod probe specs unchanged) | ||
| - Internal actions that trigger probe suppression (e.g., live migration) have higher priority than the user annotation | ||
| - Non-GuestAgentPing probes (TCP, HTTP, Exec) are unaffected | ||
| - Annotation removal resumes normal probe behavior | ||
|
|
||
| - [ ] **Non-Functional Requirements (NFRs)** | ||
| - Confirmed coverage for NFRs, including Performance, Security, Usability, Downtime, Connectivity, Monitoring (alerts/metrics), Scalability, Portability (e.g., cloud support), and Docs. | ||
| - No UI impact (no-ui, no-ux labels). No new metrics or alerts required. Annotation check is an O(1) early-return guard with negligible performance impact. Upstream documentation PR in progress (kubevirt/user-guide#990). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
HIGH — Mark completed checklist items as completed.
Many sections contain completed evidence but leave the corresponding checklist items as [ ]. Mark reviewed/applicable items [x]; reserve [ ] for pending or non-applicable items with justification. Remove remaining template prompts such as the unanswered customer-value question.
Also applies to: 127-161, 192-218
🧰 Tools
🪛 LanguageTool
[grammar] ~30-~30: Ensure spelling is correct
Context: ...cements/pull/208)) defines clear scope: GuestAgentPing probes only, annotation-based manual control. Seven acceptance criteria are documente...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md` around lines 28 - 57,
Update the checklist sections in this document, including the referenced
sections, by marking items with completed evidence as [x]. Remove unresolved
template prompts such as the unanswered customer-value question, and leave [ ]
only for genuinely pending or non-applicable items with an explicit
justification.
Source: Coding guidelines
There was a problem hiding this comment.
@coderabbitai we are in process of finalizing how we want to handle it. My proposal is we should handle what in scope of PM reviews should be handle in PR template.PM would still give /lgtm but they can optionally checkbox/read items from PR template. A skill should be added to get automatic PM attention if we are not testing or marking something out of scope.
PM might just want to check below in all time
goals
out of scope items
risks
Non functional requirements that are out of scope for this STP. An agreement with PM is needed if separate tasks are required that needs to handled by performance teams.
We will be confirming same from PM
There was a problem hiding this comment.
@geetikakay Thanks for outlining the direction here — moving PM-scoped review items (goals, out-of-scope items, risks, NFR exceptions) into the PR template with checkboxes, backed by an automated skill to flag untested/out-of-scope items for PM attention, sounds like a solid long-term improvement and would reduce boilerplate in the STP itself.
That said, since this is still being finalized and confirmed with PM, I'll continue flagging inline checklist/justification gaps in STPs per the current convention until the new process is documented and adopted. Once it's finalized, let me know and I'll update how I review these sections (e.g., no longer require inline justification for items that move to the PR template).
For this PR specifically, the original comment about marking completed checklist items as [x] still stands as-is for now.
(๑•̀ㅁ•́๑)✧ noted — will keep an eye out for the process update
🧠 Learnings used
Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests-design-docs PR: 128
File: stps/sig-infra/CNV-82132-pause-guest-agent-probes.md:142-143
Timestamp: 2026-07-16T00:58:10.501Z
Learning: Until the STP review process changes (moving PM-review-scoped items into a PR template with checkboxes) are finalized and documented, continue requiring inline PM/UX justification inside STP documents. In particular, if you mark items as out of scope or skip Usability Testing (and similar PM/UX-scoped exceptions), include the required inline justification in the STP text rather than relying on the PR template/checkbox mechanism.
| - [ ] **Usability Testing** -- Validates user experience and accessibility requirements | ||
| - *Details:* N/A. Feature has no UI (no-ui, no-ux labels). Annotation-based interface documented in upstream user guide (kubevirt/user-guide#990). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
HIGH — Provide PM/UX justification for skipping usability testing.
“No UI” labels alone are insufficient justification. Record the PM/UX decision explaining why usability or accessibility testing has no customer value for this feature.
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md` around lines 142 - 143,
Update the Usability Testing section in CNV-82132-pause-guest-agent-probes.md to
document the PM/UX decision and customer-value rationale for omitting usability
and accessibility testing, rather than citing only the no-UI labels.
Source: Coding guidelines
There was a problem hiding this comment.
@coderabbitai we are in process of finalizing how we want to handle it. My proposal is we should handle what in scope of PM reviews should be handle in PR template.PM would still give /lgtm but they can optionally checkbox/read items from PR template. A skill should be added to get automatic PM attention if we are not testing or marking something out of scope.
PM might just want to check below in all time
- goals
- out of scope items
- risks
- Non functional requirements that are out of scope for this STP. An agreement with PM is needed if separate tasks are required that needs to handled by performance teams.
We will be confirming same from PM
There was a problem hiding this comment.
@geetikakay, that's a sensible direction — moving PM-scoped concerns (goals, out-of-scope, risks, NFR exclusions) into the PR template as explicit checkboxes gives PM a lightweight, consistent surface to review without requiring every STP author to write out full justification prose each time. Automating attention when something is left untested or marked out-of-scope is a good complement to that.
A couple of thoughts as you finalize this with PM:
- Until the PR template change lands and PM sign-off process is confirmed, guideline-driven checks (like this Usability Testing justification) will still flag inline in STPs, since that's the current source of truth for reviewers.
- Once the template exists, it'd help to update the coding guidelines/STP template docs to reflect that NFR-exclusion rationale (e.g., Usability, Performance) can live in the PR template rather than the STP body — otherwise reviewers (including me) will keep asking for it in-document.
- For the "Non-Functional Requirements that are out of scope" bullet, worth clarifying whether that PM checkbox is meant to replace the STP's own NFR section entirely, or just serve as a secondary confirmation gate — this affects whether STPs still need inline justification text.
Happy to adjust review behavior once this process is confirmed with PM. For now, I'll leave this comment open since the underlying concern (missing PM/UX rationale for skipping usability testing) isn't yet resolved in this specific STP.
🐇💭 curious to see how this template-based process shapes up
✏️ Learnings added
Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests-design-docs PR: 128
File: stps/sig-infra/CNV-82132-pause-guest-agent-probes.md:142-143
Timestamp: 2026-07-16T00:58:10.501Z
Learning: Repo RedHatQE/openshift-virtualization-tests-design-docs: The team (per geetikakay) is exploring moving PM-review-scoped items (Testing Goals, Out of Scope items, Risks, and Non-Functional Requirements marked out of scope) out of individual STP justification text and into a PR template with checkboxes, so PM can give /lgtm while optionally reviewing these items via the template rather than requiring inline justification in every STP. This is still pending confirmation with PM and not yet implemented as of PR `#128`. Until this process change is finalized and documented, continue enforcing inline PM/UX justification requirements (e.g., for skipping Usability Testing) directly in STP documents per existing coding guidelines.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kshvaika
Repo: RedHatQE/openshift-virtualization-tests-design-docs PR: 106
File: stps/sig-virt/dual-stream-cluster-rhcos9-rhcos10/storage.md:153-190
Timestamp: 2026-05-25T09:57:32.773Z
Learning: When reviewing STP documents under `stps/`, follow the Test Strategy checklist convention in Section II.2: use `[x]` only when the checklist item is applicable and has been addressed, and use `[ ]` when the item is NOT applicable. For `[ ]` items, require a clear justification in the corresponding `Details:` field. Do not treat `[ ]` as an incorrect omission solely because the `Details:` field contains text—such text is the required non-applicability explanation, not evidence that the item was completed.
Learnt from: RoniKishner
Repo: RedHatQE/openshift-virtualization-tests-design-docs PR: 34
File: stps/sig-infra/virtual-machine-template.md:64-69
Timestamp: 2026-05-27T18:26:25.780Z
Learning: When reviewing STP documents in the `stps/` directory, do not require NFR categories like Monitoring, Performance, and Scalability to be re-enumerated in the Section I.1 “Non-Functional Requirements (NFRs)” block if they are already explicitly addressed (with justification) elsewhere in the same STP—specifically in a dedicated “Out of Scope” section. In this case, treating duplication as redundant is acceptable. Also, Observability may be treated as a subset of Monitoring rather than a separate NFR category. Do not flag the I.1 NFR block as incomplete solely because these categories are missing from it when they are covered elsewhere in the document.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests-design-docs PR: 34
File: stps/sig-infra/virtual-machine-template.md:133-168
Timestamp: 2026-06-02T10:06:00.601Z
Learning: When reviewing any STP document under `stps/`, check Section II.2 (“Test Strategy”). It must explicitly enumerate every testing type required by the STP template. No testing type may be omitted: if a testing type is not applicable, it still must be listed and accompanied by a clear justification/rationale. Do not treat Section II.2 as complete if any required testing types are missing—missing entries indicate the template’s testing types were not documented as considered (a review violation).
d3a4067 to
4be8c7a
Compare
|
@geetikakay I think this thread should be unresolved: https://github.com/RedHatQE/openshift-virtualization-tests-design-docs/pull/128/changes#r3591751859 |
4be8c7a to
c1baefa
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
stps/sig-infra/CNV-82132-pause-guest-agent-probes.md (3)
176-176: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLOW — Use the standard platform-agnostic wording.
Replace the fixed platform list with:
Platform-agnostic — no platform-specific behavior; applicable to all CI platforms.🤖 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-infra/CNV-82132-pause-guest-agent-probes.md` at line 176, Update the Platform entry in CNV-82132-pause-guest-agent-probes.md to use the standard platform-agnostic wording: “Platform-agnostic — no platform-specific behavior; applicable to all CI platforms.” Remove the fixed OpenShift platform list.Source: Learnings
246-253: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH — Make VMI restart conditional in the resume scenario.
Removing the annotation resumes normal probe behavior; it does not inherently restart the VMI. State whether the guest agent remains stopped, and expect a liveness-triggered restart only when the agent remains stopped and the probe subsequently fails.
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md` around lines 246 - 253, Update the CNV-82132 resume scenario to avoid requiring an unconditional VMI restart after removing the annotation. Specify the expected guest-agent state after removal, and require a liveness-triggered VMI restart only if the agent remains stopped and the probe subsequently fails.Source: Coding guidelines
274-279: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH — Classify cross-feature scenarios as Tier 2.
These scenarios combine probe pausing with live migration, migration suppression, credential injection, snapshots, or VM pause/unpause. Under the stated tier definitions, they are Tier 2 integration workflows. If separate Tier 1 coverage exists elsewhere, document that coverage explicitly rather than leaving these rows classified as Tier 1.
Based on learnings, confirm any externally maintained Tier 1 coverage before changing the classification.
Also applies to: 289-294, 307-312, 341-354
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md` around lines 274 - 279, Update the cross-feature test scenarios in this document, including CNV-82392 and the rows at the referenced sections, from Tier 1 to Tier 2 because they combine probe pausing with other workflows. Before changing each classification, confirm any externally maintained Tier 1 coverage and document it explicitly where applicable.Sources: Coding guidelines, Learnings
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md`:
- Around line 64-67: Add an explicit “Sign-off: [Name/Date]” entry beneath each
of the four listed limitations, replacing the placeholders with the actual
approver name and approval date.
- Around line 120-124: Rewrite each out-of-scope checklist entry in the document
using the exact “Item / Rationale: / PM/Lead Agreement:” structure. Preserve
each item’s scope while adding a specific rationale and recording the actual
agreeing PM/Lead name and agreement date for every entry.
- Around line 145-146: Update the Usability Testing checklist entry in
CNV-82132-pause-guest-agent-probes.md to include an inline PM/UX justification
for skipping testing, explicitly stating the decision and why
usability/accessibility testing provides no customer value for this
annotation-only workflow. Retain the existing no-UI context while adding the
required rationale.
- Around line 71-73: Expand the “Developer Handoff/QE Kickoff” section in
CNV-82132-pause-guest-agent-probes.md beyond the GuestAgentPingFailed note by
recording the meeting date, participants, decisions, concrete takeaways, and
remaining action items, preserving the existing monitoring-gap detail.
- Around line 266-271: Update the CNV-82132 traceability scenario to validate
all non-GuestAgentPing probe types named by the acceptance criterion: Exec, TCP,
and HTTP. Either expand the existing test scenario to cover each probe or add
separate traceability rows, while preserving the stated pause-annotation
behavior.
- Around line 5-21: The feature document’s metadata and overview need
maturity-phase coverage and a customer-focused scope. Add the structured DP, TP,
and GA maturity metadata using the document’s established conventions,
explicitly identify the current phase and the phase covered by this STP, and
remove the implementation detail about requiring no feature gate from the
“Feature Overview” while preserving its observable pause/resume behavior.
- Line 60: Resolve the documentation readiness contradiction in the dependency,
Entry Criteria, and related sections: either list the upstream documentation PR
as a dependency while retaining its merged-documentation gate, or explicitly
classify it as docs-team-owned and remove it from QE Entry Criteria as
non-blocking. Apply the same consistent treatment to the referenced
documentation statements.
- Around line 33-56: Update the acceptance-criteria section of the CNV-82132
document to contain all seven explicitly numbered or otherwise traceable
criteria. Add dedicated criteria covering unchanged Pod probe specifications and
live-migration/internal suppression taking precedence over the user annotation,
and ensure the remaining listed behaviors are individually traceable; if any are
out of scope, document the rationale, PM/Lead agreement, and upstream coverage
references.
- Around line 31-60: Update the checklist sections in
CNV-82132-pause-guest-agent-probes.md, including the referenced ranges, by
changing completed items from [ ] to [x] and removing leftover template prompts,
including the unanswered “What is the value...” prompt. Retain unchecked boxes
only for genuinely pending or non-applicable items, and provide justification
for any non-applicable entries.
---
Duplicate comments:
In `@stps/sig-infra/CNV-82132-pause-guest-agent-probes.md`:
- Line 176: Update the Platform entry in CNV-82132-pause-guest-agent-probes.md
to use the standard platform-agnostic wording: “Platform-agnostic — no
platform-specific behavior; applicable to all CI platforms.” Remove the fixed
OpenShift platform list.
- Around line 246-253: Update the CNV-82132 resume scenario to avoid requiring
an unconditional VMI restart after removing the annotation. Specify the expected
guest-agent state after removal, and require a liveness-triggered VMI restart
only if the agent remains stopped and the probe subsequently fails.
- Around line 274-279: Update the cross-feature test scenarios in this document,
including CNV-82392 and the rows at the referenced sections, from Tier 1 to Tier
2 because they combine probe pausing with other workflows. Before changing each
classification, confirm any externally maintained Tier 1 coverage and document
it explicitly where applicable.
🪄 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: 93c88810-470e-483b-a7d2-430aa33d1559
📒 Files selected for processing (1)
stps/sig-infra/CNV-82132-pause-guest-agent-probes.md
| - No automatic triggering by KubeVirt-internal operations (e.g., snapshots, live migration). The annotation must be set by the user, either manually or through external tooling (e.g., shell scripts, CronJobs). | ||
| - Live migration probe suppression is handled separately (kubevirt/kubevirt#17235) and operates independently of this annotation. | ||
| - The annotation does not suppress guest-exec commands. Access credential injection continues to work while probes are paused. | ||
| - Windows guest OS update scenarios use the same annotation mechanism but OS-specific validation is not included in this test plan. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
HIGH — Add sign-offs to every known limitation.
Each listed limitation needs an explicit *Sign-off:* [Name/Date] entry with the actual approver and date.
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md` around lines 64 - 67,
Add an explicit “Sign-off: [Name/Date]” entry beneath each of the four listed
limitations, replacing the placeholders with the actual approver name and
approval date.
Source: Coding guidelines
| - [x] **Developer Handoff/QE Kickoff** | ||
| - *Key takeaways and concerns:* | ||
| - GuestAgentPingFailed events are suppressed during pause — monitoring gap identified. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
HIGH — Record the actual developer-handoff outcomes.
Add the meeting date, participants, decisions, concrete takeaways, and remaining action items. The current “GuestAgentPingFailed events” note alone is not a handoff record.
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md` around lines 71 - 73,
Expand the “Developer Handoff/QE Kickoff” section in
CNV-82132-pause-guest-agent-probes.md beyond the GuestAgentPingFailed note by
recording the meeting date, participants, decisions, concrete takeaways, and
remaining action items, preserving the existing monitoring-gap detail.
Source: Coding guidelines
| - [ ] HTTP, TCP, and Exec probe pausing -- VEP 207 explicitly limits scope to GuestAgentPing probes only | ||
| - [ ] Automatic probe scheduling or timeout -- VEP 207 defines annotation as manual-only with no automatic expiry | ||
| - [ ] Live migration probe fix (kubevirt/kubevirt#17235) -- separate feature with independent implementation and test coverage | ||
| - [ ] Upstream Kubernetes probe suspension (KEP 5002) -- upstream proposal not yet implemented in Kubernetes | ||
| - [ ] Windows guest OS update scenarios -- same annotation mechanism applies but OS-specific validation deferred |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
HIGH — Add rationale and PM/Lead agreement to every out-of-scope item.
Rewrite each entry using the required format: Item / Rationale: / PM/Lead Agreement: with the actual name and date.
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md` around lines 120 - 124,
Rewrite each out-of-scope checklist entry in the document using the exact “Item
/ Rationale: / PM/Lead Agreement:” structure. Preserve each item’s scope while
adding a specific rationale and recording the actual agreeing PM/Lead name and
agreement date for every entry.
Source: Coding guidelines
There was a problem hiding this comment.
@coderabbitai that is already discussed at several places
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='stps/sig-infra/CNV-82132-pause-guest-agent-probes.md'
printf '%s\n' '--- Relevant document outline ---'
ast-grep outline "$file" --items all | sed -n '1,180p'
printf '%s\n' '--- Scope exclusions and approval-related references ---'
rg -n -i -C 3 \
'out of scope|scope exclusion|rationale|PM/Lead|PM/UX|agreement|sign-off|approved|Windows|HTTP|TCP|Exec|migration probe|KEP 5002' \
"$file"
printf '%s\n' '--- Target section ---'
sed -n '112,135p' "$file"Length of output: 11170
@geetikakay, the existing sections do explain most rationales, so no additional technical justification needs to be invented.
However, the finding remains valid: Lines 120-124 do not record a PM/Lead Agreement with an actual name and date for any exclusion, and the required per-item structure is absent. Discussion elsewhere cannot substitute for that traceable approval record under the current STP rules.
Please minimally reformat each item as:
- [ ] **Item:** … / *Rationale:* … / *PM/Lead Agreement:* [Name/Date]You can reuse the existing rationale text and only add the corresponding approver/date.
🧠 Learnings used
Learnt from: RoniKishner
Repo: RedHatQE/openshift-virtualization-tests-design-docs PR: 34
File: stps/sig-infra/virtual-machine-template.md:64-69
Timestamp: 2026-05-27T18:26:25.780Z
Learning: When reviewing STP documents in the `stps/` directory, do not require NFR categories like Monitoring, Performance, and Scalability to be re-enumerated in the Section I.1 “Non-Functional Requirements (NFRs)” block if they are already explicitly addressed (with justification) elsewhere in the same STP—specifically in a dedicated “Out of Scope” section. In this case, treating duplication as redundant is acceptable. Also, Observability may be treated as a subset of Monitoring rather than a separate NFR category. Do not flag the I.1 NFR block as incomplete solely because these categories are missing from it when they are covered elsewhere in the document.
Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests-design-docs PR: 128
File: stps/sig-infra/CNV-82132-pause-guest-agent-probes.md:142-143
Timestamp: 2026-07-16T00:58:10.501Z
Learning: Until the STP review process changes (moving PM-review-scoped items into a PR template with checkboxes) are finalized and documented, continue requiring inline PM/UX justification inside STP documents. In particular, if you mark items as out of scope or skip Usability Testing (and similar PM/UX-scoped exceptions), include the required inline justification in the STP text rather than relying on the PR template/checkbox mechanism.
| - [ ] **Usability Testing** -- Validates user experience and accessibility requirements | ||
| - *Details:* N/A. Feature has no UI (no-ui, no-ux labels). Annotation-based interface documented in upstream user guide (kubevirt/user-guide#990). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
HIGH — Add PM/UX justification for skipping usability testing.
Document the PM/UX decision and why usability/accessibility testing provides no customer value for this annotation-only workflow; “no UI” and labels alone are insufficient.
Based on learnings, inline PM/UX justification remains required until the process change is finalized.
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md` around lines 145 - 146,
Update the Usability Testing checklist entry in
CNV-82132-pause-guest-agent-probes.md to include an inline PM/UX justification
for skipping testing, explicitly stating the decision and why
usability/accessibility testing provides no customer value for this
annotation-only workflow. Retain the existing no-UI context while adding the
required rationale.
Source: Learnings
| - **[CNV-82132]** -- As a VM admin, I want non-GuestAgentPing probes (exec, TCP, HTTP) unaffected by the annotation | ||
| - *Test Scenario:* Verify exec probe fires normally with pause annotation set | ||
| - *Tier:* Tier 1 (Functional) | ||
| - *Priority:* P1 | ||
| - *Upstream coverage:* none -- not in analyzed PR(s) | ||
| - *Downstream required:* no |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
HIGH — Cover TCP and HTTP probes as well as Exec.
The acceptance criterion names three unaffected probe types, but the traceability row validates only Exec. Expand the scenario to verify Exec, TCP, and HTTP, or create separate rows.
🤖 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-infra/CNV-82132-pause-guest-agent-probes.md` around lines 266 - 271,
Update the CNV-82132 traceability scenario to validate all non-GuestAgentPing
probe types named by the acceptance criterion: Exec, TCP, and HTTP. Either
expand the existing test scenario to cover each probe or add separate
traceability rows, while preserving the stated pause-annotation behavior.
Source: Coding guidelines
There was a problem hiding this comment.
@coderabbitai it is already identified as a gap and we plan to fix it
There was a problem hiding this comment.
@geetikakay, understood. HIGH: the gap is acknowledged, but this STP still needs complete traceability for the stated acceptance criterion. Please either add TCP/HTTP coverage in this PR or link a tracked follow-up that explicitly covers both probe types and their expected unaffected behavior.
Would you like me to open a GitHub follow-up issue for it?
🧠 Learnings used
Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests-design-docs PR: 128
File: stps/sig-infra/CNV-82132-pause-guest-agent-probes.md:101-113
Timestamp: 2026-07-16T01:23:24.642Z
Learning: For STP documentation under stps/ (e.g., *.md files), keep Section II.1 “Testing Goals” as high-level, priority-ordered “what we’re trying to achieve” statements (e.g., P0/P1/P2). Do not require Section II.1 to enumerate every configuration dimension (such as VMI state, probe configuration, annotation values, guest-agent state, migration topology, or detailed observable/expected outcomes). Instead, verify that all such detailed dimensions are covered via traceability in Section III (“Test Scenarios & Traceability”), where each scenario maps to the corresponding Jira Requirement ID, Tier, and Priority.
There was a problem hiding this comment.
@coderabbitai it will be generated by STP writter once we review this STP
There was a problem hiding this comment.
@geetikakay, understood. I’ll keep this HIGH finding open for the STP-writer output: the final reviewed STP must include traceability for Exec, TCP, and HTTP probes remaining unaffected by the pause annotation.
This matters because Section III is the testable evidence for the acceptance criterion; a stated coverage gap alone does not establish coverage or ownership of the missing TCP/HTTP validation.
🐇💭
✏️ Learnings added
Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests-design-docs PR: 128
File: stps/sig-infra/CNV-82132-pause-guest-agent-probes.md:269-274
Timestamp: 2026-07-20T13:19:16.186Z
Learning: For `stps/` documentation in RedHatQE/openshift-virtualization-tests-design-docs, traceability additions identified during review may be generated by the STP writer after the STP review. The final generated STP must still contain the required Section III test scenarios before approval.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests-design-docs PR: 128
File: stps/sig-infra/CNV-82132-pause-guest-agent-probes.md:101-113
Timestamp: 2026-07-16T01:23:24.642Z
Learning: For STP documentation under stps/ (e.g., *.md files), keep Section II.1 “Testing Goals” as high-level, priority-ordered “what we’re trying to achieve” statements (e.g., P0/P1/P2). Do not require Section II.1 to enumerate every configuration dimension (such as VMI state, probe configuration, annotation values, guest-agent state, migration topology, or detailed observable/expected outcomes). Instead, verify that all such detailed dimensions are covered via traceability in Section III (“Test Scenarios & Traceability”), where each scenario maps to the corresponding Jira Requirement ID, Tier, and Priority.
c1baefa to
a61b0e0
Compare
Signed-off-by: Geetika Kapoor <gkapoor@redhat.com>
a61b0e0 to
1afe367
Compare
|
@geetikakay what is the status of this STP? Is it ready for another round of reviews? |
@ksimon1 sure |
|
@geetikakay the PR has |
| - *Downstream required:* no | ||
|
|
||
|
|
||
| - **[CNV-82392]** -- As a VM admin, I want the pause annotation to propagate during live migration so probes remain paused on the target node |
There was a problem hiding this comment.
Should be the CNV-82392 tracked in metadata paragraph? Only CNV-82132 is tracked there
| - Ensured requirements contain relevant **customer use cases**. | ||
| - User stories: | ||
| - VM owner pauses probes during individual VM maintenance (e.g., guest OS update, driver installation) | ||
| - Cluster admin pauses probes across multiple VMIs during scheduled maintenance windows |
There was a problem hiding this comment.
For the cluster admin use case - what do you mean by multiple machines? Is it for all VMs in the cluster or selected ones?
There was a problem hiding this comment.
@ronensdeor yes multiple VM's. I will rewrite it
10 participants
STP Metadata
kubevirt/enhancements#208
What this PR does
Special notes for your reviewer
Summary by CodeRabbit