Skip to content

Revert "virtio: Enable full disk caching" #379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Revert "virtio: Enable full disk caching" #379

wants to merge 1 commit into from

Conversation

cfergeau
Copy link
Collaborator

@cfergeau cfergeau commented Oct 7, 2025 •
edited by coderabbitai bot
Loading

This reverts commit 732de99. The goal is to run some experiments for containers/podman#27216

Summary by CodeRabbit

  • Refactor
    • Streamlined virtual disk image attachment to use platform defaults for caching and synchronization.
    • Reduces configuration complexity while maintaining existing behavior.
    • No changes to user workflows, settings, or public API.
    • Backward compatible with existing setups; no visible UI changes.
    • Update is expected to be seamless with no action required from users.

@cfergeau
Revert "virtio: Enable full disk caching"
3347072 
This reverts commit 732de99.
The goal is to run some experiments for containers/podman#27216

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
@openshift-ci openshift-ci bot requested review from nirs and praveenkumar October 7, 2025 12:34
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Oct 7, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign lstocchi for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cfergeau
Copy link
Collaborator Author

cfergeau commented Oct 7, 2025

/hold
I only want to use CI generate binaries for testing

@coderabbitai coderabbitai
Copy link

coderabbitai bot commented Oct 7, 2025 •
edited
Loading

Walkthrough

Updated the image attachment creation in pkg/vf/virtio.go: DiskStorageConfig.toVz now constructs DiskImageStorageDeviceAttachment directly with (config.ImagePath, config.ReadOnly), removing the previous WithCacheAndSync configuration that set fsync and cached modes. No function signatures or public APIs were changed.

Changes

Cohort / File(s) Summary
Virtio disk image attachment path
pkg/vf/virtio.go		 Replaced attachment creation using WithCacheAndSync (fsync + cached) with direct DiskImageStorageDeviceAttachment(config.ImagePath, config.ReadOnly), deferring cache/sync behavior to the constructor defaults.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant DiskStorageConfig
  participant VZ as VZ API

  rect rgb(235, 245, 255)
    note over DiskStorageConfig: Old flow (before)
    Caller->>DiskStorageConfig: toVz()
    DiskStorageConfig->>VZ: NewDiskImageStorageDeviceAttachment(imagePath, readOnly)\n.WithCacheAndSync(Fsync, Cached)
    VZ-->>DiskStorageConfig: Attachment
    DiskStorageConfig-->>Caller: Attachment
  end

  rect rgb(240, 255, 240)
    note over DiskStorageConfig: New flow (after)
    Caller->>DiskStorageConfig: toVz()
    DiskStorageConfig->>VZ: NewDiskImageStorageDeviceAttachment(imagePath, readOnly)
    VZ-->>DiskStorageConfig: Attachment (uses default cache/sync)
    DiskStorageConfig-->>Caller: Attachment
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I hop through code with whiskers keen,
Swapped a knob for defaults clean.
Cache and sync now softly glide,
Constructor leads, I’m just the guide.
Carrot commits, concise and bright—
One-line change, and onward flight! 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly conveys that the PR reverts the previous change enabling full disk caching in the virtio module, matching the core modification of removing explicit cache and sync settings. It is concise and specific, enabling maintainers to understand the primary change at a glance.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 88ab5cd and 3347072.

📒 Files selected for processing (1)
  • pkg/vf/virtio.go (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: build (macos-15)
  • GitHub Check: build (macos-13)
  • GitHub Check: build (macos-14)
  • GitHub Check: lint
🔇 Additional comments (1)
pkg/vf/virtio.go (1)

489-494: LGTM—Intentional revert for testing
Removes explicit Cached cache mode and Fsync sync mode for all disk attachments (NVMExpressController, VirtioBlk, USBMassStorage), defaulting to vz.NewDiskImageStorageDeviceAttachment. Confirm this default behavior meets your I/O durability and performance requirements.

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.

❤️ Share

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

@cfergeau cfergeau mentioned this pull request Oct 7, 2025
Open
@nirs
Copy link
Collaborator

nirs commented Oct 7, 2025

Poem

Can we disable this unhelpful poems?

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

@nirs nirs Awaiting requested review from nirs

@praveenkumar praveenkumar Awaiting requested review from praveenkumar

Assignees
No one assigned
Labels
do-not-merge/hold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cfergeau @nirs