core: MBS / Cinder code path parity - #1153
Merged
Merged
Conversation
- Engine has many places that need to treat both Cinder and
Managed Block Storage as one case
(isCinderDomain() || isManagedBlockStorage()) because the two
share the relevant traits: one volume per disk, no qcow2 chain
to aggregate, vendor-side capacity accounting. Cinder and MBS
are sequential rather than parallel features (MBS is the
successor); both consistently take the same code path when the
engine knows about both.
- Many places historically checked only isCinderDomain() and
never got the parallel isManagedBlockStorage() added when MBS
landed, causing MBS domains to fall through to SPM-style code
paths that don't match their semantics.
- Add StorageType.isVendorManagedBlock() naming the combined case
as a single concept; replace 7 partially-applied checks with the
helper:
* 6 methods in StorageDomainValidator (hasSpaceFor*/threshold
methods that previously skipped engine-side space checks
for Cinder but ran the check on MBS)
* 1 check in GetAllDisksByStorageDomainIdQuery (previously
ran SPM-style snapshot aggregation on MBS instead of the
simple list path)
- Update StorageDomainValidatorFreeSpaceTest to recognize MBS
as a short-circuit case for the cloned/snapshots/all-disks
checks, matching the validator's actual behavior after the
helper widens the skip from Cinder-only to vendor-managed.
- When Cinder is fully removed (CentOS Stream 10 already dropped
it) the helper collapses to isManagedBlockStorage() and the
isCinderDomain machinery can be deleted in a single follow-up.
Signed-off-by: Ryan Ronnander <rronnander@linbit.com>
peter-boden
self-requested a review
June 2, 2026 13:29
dupondje
approved these changes
Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
More MBS quality-of-life improvements:
case (
isCinderDomain() || isManagedBlockStorage()) because the twoshare the relevant traits: one volume per disk, no qcow2 chain to
aggregate, vendor-side capacity accounting
isCinderDomain()and never gotthe parallel
isManagedBlockStorage()added when MBS landed, causingMBS domains to fall through to SPM-style code paths that don't match
their semantics
StorageType.isVendorManagedBlock()to name that combined case asa single concept; replace 7 partially-applied checks with the helper:
StorageDomainValidator(hasSpaceFor*/ thresholdmethods that previously skipped engine-side space checks for
Cinder but ran the check on MBS)
GetAllDisksByStorageDomainIdQuery(previously ranSPM-style snapshot aggregation on MBS instead of the simple list
path)
StorageDomainValidatorFreeSpaceTestto recognize MBS as ashort-circuit case for the cloned / snapshots / all-disks checks,
matching the validator's actual behavior after the helper widens the
skip from Cinder-only to vendor-managed
the helper collapses to
isManagedBlockStorage()and theisCinderDomainmachinery can be deleted in a single follow-upFound and addressed while improving oVirt's Managed Block Storage support. No user-visible bug fixed — this is code-hygiene and latent-correctness for shared engine code paths.
Part of an ongoing series of MBS improvements:
Changes introduced with this PR
StorageType: new helperisVendorManagedBlock()returning true for either Cinder or MBS, with a doc comment explaining the shared semanticsStorageDomainValidator: 6 methods changed fromisCinderDomain()(orisCinderDomain() || isManagedBlockStorage()) toisVendorManagedBlock()GetAllDisksByStorageDomainIdQuery: 1 check changed similarly, with a comment noting the simple list path applies to vendor-managed-block domains generally, not just CinderStorageDomainValidatorFreeSpaceTest: parameterization aligned so MBS is recognized as a short-circuit case in the cloned / snapshots / all-disks tests, matching the validator's actual behaviorAre you the owner of the code you are sending in, or do you have permission of the owner?
Yes.