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.
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.
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
Require extra 8 GiB free space on the destination Storage Domain in the LSM flows #624
Require extra 8 GiB free space on the destination Storage Domain in the LSM flows #624
Changes from all commits
e6784cd
408a636
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bennyz @ahadas
IMHO the whole
LiveMigrateDiskCommand::validateDestDomainsSpaceRequirements()
can be removed, sinceMoveOrCopyDiskCommand::validateSpaceRequirements()
runs on both source & destination Storage Domains and checks bothisDomainWithinThresholds()
and Storage Domains available space.I already removed
isDomainWithinThresholds()
check above, since it's exactly what theMoveOrCopyDiskCommand::validateSpaceRequirements()
does (for both source and destination SDs).But I want to be careful and be sure before I remove the rest of the method, there are 2 slight differences between what
MoveOrCopyDiskCommand::validateSpaceRequirements()
performs on both source and destination SDs and what this method does on destination only:In the
MoveOrCopyDiskCommand::validateSpaceRequirements()
, theDiskImage
that is used to retrieve the snapshots is initialized using agetImage()
call, that uses theCopyImageGroupCommand::getImage()
implementation:ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/CopyImageGroupCommand.java
Line 91 in b54471d
While in
LiveMigrateDiskCommand::validateDestDomainsSpaceRequirements()
thegetDiskImageByImageId()
is called:ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/lsm/LiveMigrateDiskCommand.java
Line 674 in b54471d
But at the end it looks like they are returning pretty much the same
diskImageDao.get(imageId);
...In the
MoveOrCopyDiskCommand::validateSpaceRequirements()
, the available free space validation is performed by thehasSpaceForDisksWithSnapshots()
:ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/storage/StorageDomainValidator.java
Line 245 in b54471d
While in
LiveMigrateDiskCommand::validateDestDomainsSpaceRequirements()
thehasSpaceForClonedDisks()
is called:ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/storage/StorageDomainValidator.java
Line 222 in b54471d
Trying to compare them very carefully, I see they are almost the same with very minor nuances, and though maybe both methods need to stay, but anyway, in current implementation both are called and I don't think that we should verify in 2 different ways the same disk & target domain to allow Live Migration.
So which version is correct? The one in
LiveMigrateDiskCommand::validateDestDomainsSpaceRequirements()
or theMoveOrCopyDiskCommand::validateSpaceRequirements()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, they are similar yet different as you pointed out. I guess the validation in LiveMigrateDiskCommand should stay, but need to take a deeper look at the differences. @bennyz can you answer this from the top of you head? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bennyz is there some code that can be united - moved to the
MoveOrCopyDiskCommand
base class?For me it looks like
LiveMigrateDiskCommand::validateDestDomainsSpaceRequirements()
is redundant, but there are 2 slight potential differences.DiskImage
, used to retrieve the snapshots, is retrieved.hasSpaceForDisksWithSnapshots
vs.hasSpaceForClonedDisks()
, that are currently used both to test the destination SD free space, and I added 3 chunks' addition only to one of them.