Skip to content

ROX-36925: VM papercuts after guest OS search - #22840

Draft
vikin91 wants to merge 2 commits into
masterfrom
piotr/vm-papercuts
Draft

ROX-36925: VM papercuts after guest OS search#22840
vikin91 wants to merge 2 commits into
masterfrom
piotr/vm-papercuts

Conversation

@vikin91

@vikin91 vikin91 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Description

Small follow-ups after the Guest OS search fix landed on master.

  • Scan stamping still looks up facts["guestOS"], not the display guest_os column. The v2 "unknown guest OS" unit case now puts unknown in that fact as well, so it exercises the sentinel filter instead of passing because the fact was absent.
  • VMScraper logs when it first queues a VM for an index pull (delay and spread window) and when a KubeVirt recreate changes VM identity and forces a re-queue.

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

The unknown-OS lookup case is an existing table row. The scraper change is info logs only.

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

  • CI

AI-Assisted: cursor, test fixture and scraper log lines, user supplied the scraper diff

@vikin91
ROX-36925: set Facts in unknown-OS v2 lookup test
82a3cea 
lookupGuestOS reads facts["guestOS"], not GuestOs. The v2 unknown
case now matches v1 so it hits the sentinel filter instead of passing
because the fact was missing.

Associated request: follow-up branch piotr/vm-papercuts for PR #22806
review (set Facts on the unknown guest OS test).

Partially generated by AI.
@vikin91
ROX-36925: log first VMScraper index-pull schedule
541fb46 
Log when a VM is first queued (delay and spread window) and when
identity change forces a re-queue, so operators can see scrape
scheduling without tracing.

Associated request: include scraper first-queue / identity-change
logs on piotr/vm-papercuts.

Partially generated by AI.
@openshift-ci

openshift-ci Bot commented Sep 15, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@vikin91 vikin91 mentioned this pull request Sep 15, 2026
Merged
14 tasks
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: ff19577b-1e51-49c5-88cc-b03ecb347435

📥 Commits

Reviewing files that changed from the base of the PR and between 4ce5f0f and 541fb46.

📒 Files selected for processing (2)
  • central/sensor/service/pipeline/virtualmachineindex/pipeline_test.go
  • sensor/common/virtualmachine/vmscraper/scraper.go

Included review availability: Your plan provides up to 12 included reviews per hour; 8 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation for virtual machines with unknown guest operating systems.

  • Operational Improvements

    • Added clearer diagnostic logging when virtual machines are first queued or re-queued after an identity change.
    • VM scheduling behavior remains unchanged.

Walkthrough

The V2 unknown-guest-OS fixture now populates both guest OS fields. VM reconciliation now logs initial queueing and identity replacement re-queueing. Scheduling and state-reset behavior remain unchanged.

Changes

VM indexing updates

Layer / File(s) Summary
Unknown guest OS fixture
central/sensor/service/pipeline/virtualmachineindex/pipeline_test.go		 The V2 fixture provides pkgVM.UnknownGuestOS in both guest OS fields and still expects an empty scan OS.
VM queue logging
sensor/common/virtualmachine/vmscraper/scraper.go		 Reconciliation logs initial VM queueing and re-queueing after VM identity changes. Scheduling and state-reset behavior remain unchanged.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Other

Suggested reviewers: guzalv

Merge Risk: ⚪ Minimal · up to 541fb

The change corrects the unknown-OS test fixture and adds safe VM queueing visibility without altering scheduling or state-reset behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title identifies the ticket and describes the follow-up changes after the guest OS search fix. The term "papercuts" is broad, but the title remains related and sufficiently clear.
Description check ✅ Passed The description explains the test-fixture correction and VMScraper logging changes. It addresses documentation, production readiness, automated testing, and validation. The unchecked CI inspection ite…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch piotr/vm-papercuts

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.79%. Comparing base (4ce5f0f) to head (541fb46).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #22840      +/-   ##
==========================================
- Coverage   51.82%   51.79%   -0.04%     
==========================================
  Files        2901     2901              
  Lines      182819   182823       +4     
==========================================
- Hits        94755    94695      -60     
- Misses      79777    79822      +45     
- Partials     8287     8306      +19
Flag Coverage Δ
go-unit-tests 51.79% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

🚀 Build Images Ready

Images are ready for commit 541fb46. To use with deploy scripts:

export MAIN_IMAGE_TAG=5.0.x-302-g541fb46afb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

ai-assisted ai-review area/central area/sensor coderabbit-review do-not-merge/work-in-progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@vikin91