Skip to content

Allow MBSs to upload disks - #1125

Open
slavkap wants to merge 4 commits into
oVirt:masterfrom
slavkap:upload-msb-disk
Open

Allow MBSs to upload disks#1125
slavkap wants to merge 4 commits into
oVirt:masterfrom
slavkap:upload-msb-disk

Conversation

@slavkap

@slavkap slavkap commented Mar 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Changes introduced with this PR

  • Enables uploading disks to Managed Block Storage domains. This change adds the full upload flow for Managed Block Storage using the existing NBD-based transfer path, including connect/attach on the host and cleanup on finish.

  • It was tested with StorPool and Ceph as managed block storage domains

  • The changes in this PR highly depend on vdsm PR and fixes Support upload-image to MBS domain #313

Are you the owner of the code you are sending in, or do you have permission of the owner?

[y]

@slavkap slavkap mentioned this pull request Mar 2, 2026
Open
@slavkap slavkap changed the title Allow MSBs to upload disk Allow MBSs to upload disks Mar 2, 2026
@slavkap
slavkap marked this pull request as draft March 6, 2026 07:36
@slavkap
slavkap marked this pull request as ready for review March 24, 2026 16:00
@ryan-ronnander

ryan-ronnander commented May 5, 2026

Copy link
Copy Markdown
Contributor

I've tested this PR on the LINSTOR/DRBD side. The engine side of the upload -> MBS feature has no LINSTOR-specific issues.

Looks great from over here 👍

@ryan-ronnander ryan-ronnander mentioned this pull request May 5, 2026
Merged
sp-viktori added a commit to sp-viktori/ovirt-engine that referenced this pull request May 18, 2026
@sp-viktori
Merge PR oVirt#1125 from slavkap/ovirt-engine:upload-msb-disk
234a6b1
peter-boden
peter-boden requested changes Jun 17, 2026
View reviewed changes

@peter-boden peter-boden left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, about commit messages:

  • "cleanup" is a bit minimal
  • "Allow MSBs to upload disk", MSB = MBS ?
  • 23b58f6 msg isn't really clear, maybe change it to something like:

Add support for converting QCOW2 images to RAW format when uploading
disks via the Web UI to Managed Block Storage (MBS).

Previously, the Web UI lacked an option to convert these images, which
caused issues since most MBS domains require volumes in RAW format.
Now, if a user uploads a QCOW2 disk to MBS, the system automatically
uses qemu-img convert to handle the conversion. Both the source and
converted volumes are created directly on the MBS.

Code wise:

The repointing thing feels a bit iffy. I had a go at trying to do it differently. Would this work? refactor-mbs-upload-conversion.patch

Also, I think I have the same comments here as that I had on #1138 (i.e. the MBS logic is woven through everything, we should create subclasses etc.)

Comment thread ...modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/AddDiskCommandCallback.java Outdated
private ImageDao imageDao;

@Inject
private StorageDomainDao storageDomainDao;

@peter-boden peter-boden Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

Comment thread ...bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/TransferDiskImageCommand.java Outdated
Guid childCmdId;
}
}
// DE72501108006231412815

@peter-boden peter-boden Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

slavkap added 4 commits July 30, 2026 16:35
@slavkap
core: enable disk upload to managed block storage domains
dfbd23b 
Add the initial engine and webadmin support for uploading disks to Managed
Block Storage (MBS) domains via the existing NBD image-transfer path.

Extend TransferDiskImageCommand to connect and attach MBS volumes on the
transfer host, validate MBS domains for TransferDiskImage, and tear down
volumes when the session ends. Update AddDiskCommand and
AddManagedBlockStorageDiskCommand for the MBS upload flow, and allow MBS
domains in the upload UI (UploadImageModel, AbstractDiskModel).

Depends on vdsm support for MBS upload-image.

Signed-off-by: Slavka Peleva <slavkap@storpool.com>
@slavkap
core: convert QCOW2 uploads to RAW on managed block storage
2f84649 
Add support for converting QCOW2 images to RAW format when uploading
disks via the Web UI to Managed Block Storage (MBS).
Previously, the Web UI lacked an option to convert these images, which
caused issues since most MBS domains require volumes in RAW format.
Now, if a user uploads a QCOW2 disk to MBS, the system automatically
uses qemu-img convert to handle the conversion. Both the source and
converted volumes are created directly on the MBS.

Signed-off-by: Slavka Peleva <slavkap@storpool.com>
@slavkap
core: use ManagedBlockExecutor in MBS upload conversion
3618c38 
Replace leftover cinderlib types with the managedblock executor and DAO in
TransferDiskImageCommand's upload conversion path.
Map the CONVERTING ImageTransferPhase in ImageTransferMapper so REST API
clients see a valid phase while conversion is in progress.

Signed-off-by: Slavka Peleva <slavkap@storpool.com>
@slavkap
core: split MBS disk image transfer into dedicated command
c4792d4 
Introduce MbsTransferDiskImageCommand and ActionType.MbsTransferDiskImage,
and move MBS-specific upload/transfer logic out of TransferDiskImageCommand.
Finish MBS upload conversion with a stable disk_id: update the images row
in place instead of repointing base_disks and related references. Dispatch
MbsTransferDiskImage from the web UI, REST API, and download flow for
managed block storage domains.

Signed-off-by: Slavka Peleva <slavkap@storpool.com>
@slavkap

slavkap commented Jul 30, 2026

Copy link
Copy Markdown
Author

Thanks for the review, @peter-boden — I think that I've managed to address all points:

Applied your conversion-finish patch: stable disk_id, swap the images row only (no repointing).
Split MBS logic into MbsTransferDiskImageCommand + ActionType (#1138 pattern); UI/REST/download dispatch updated.
Removed unused DAO and stray comment; commit messages reworded (including your QCOW2→RAW text).
Still depends on vdsm#459. Please take another look when you can.

sp-viktori added a commit to sp-viktori/ovirt-engine that referenced this pull request Jul 31, 2026
@sp-viktori
Merge PR oVirt#1125 from slavkap/ovirt-engine:upload-msb-disk
41651bb
sp-viktori added a commit to sp-viktori/ovirt-engine that referenced this pull request Jul 31, 2026
@sp-viktori
core: fix ActionType id collision between PR oVirt#1125 and oVirt#1138
1695b34 
Both PRs branched off master independently and each claimed the
then-next-free ActionType id 1054: MbsTransferDiskImage from oVirt#1125
(upload-msb-disk) and MbsExtractOva from oVirt#1138 (import-ova-mbs). They
add their entries in different parts of the enum, so git merges them
without a textual conflict and only ActionTypeTest notices.

This is not merely a failing test: ActionType.forValue(1054) returns
whichever entry comes first, so a command rebuilt from
command_entities.command_type could dispatch to the wrong one.

Move oVirt#1125's single entry to 1058 rather than oVirt#1138's contiguous
1054-1057 block, as the smaller change. Nothing references the numeric
value (lookups go through the enum name) and no dbscript mentions it.

Upstream still needs one of the two PRs renumbered; until that happens
this is carried as olvm/patches/ovirt-engine/ so each re-roll re-applies
it.

Generated-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sp-viktori added a commit to sp-viktori/ovirt-engine that referenced this pull request Aug 4, 2026
@sp-viktori
core: fix ActionType id collision between PR oVirt#1125 and oVirt#1138
ee84846 
Both PRs branched off master independently and each claimed the
then-next-free ActionType id 1054: MbsTransferDiskImage from oVirt#1125
(upload-msb-disk) and MbsExtractOva from oVirt#1138 (import-ova-mbs). They
add their entries in different parts of the enum, so git merges them
without a textual conflict and only ActionTypeTest notices.

This is not merely a failing test: ActionType.forValue(1054) returns
whichever entry comes first, so a command rebuilt from
command_entities.command_type could dispatch to the wrong one.

Move oVirt#1125's single entry to 1058 rather than oVirt#1138's contiguous
1054-1057 block, as the smaller change. Nothing references the numeric
value (lookups go through the enum name) and no dbscript mentions it.

Upstream still needs one of the two PRs renumbered; until that happens
this is carried as olvm/patches/ovirt-engine/ so each re-roll re-applies
it.

Generated-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peter-boden peter-boden peter-boden requested changes
@sgratch sgratch Awaiting requested review from sgratch
@didib didib Awaiting requested review from didib
@sandrobonazzola sandrobonazzola Awaiting requested review from sandrobonazzola
@bennyz bennyz Awaiting requested review from bennyz
@ahadas ahadas Awaiting requested review from ahadas
@emesika emesika Awaiting requested review from emesika
@michalskrivanek michalskrivanek Awaiting requested review from michalskrivanek
@smelamud smelamud Awaiting requested review from smelamud
@dupondje dupondje Awaiting requested review from dupondje dupondje is a code owner
@JasperB-TeamBlue JasperB-TeamBlue Awaiting requested review from JasperB-TeamBlue JasperB-TeamBlue is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support upload-image to MBS domain

3 participants

@slavkap @ryan-ronnander @peter-boden