-
Notifications
You must be signed in to change notification settings - Fork 261
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
Conversation
0295038
to
187a534
Compare
.../modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/MoveOrCopyDiskCommand.java
Outdated
Show resolved
Hide resolved
2b95d46
to
61dc003
Compare
if (!isStorageDomainWithinThresholds(getDstStorageDomain())) { | ||
return false; | ||
} | ||
|
||
DiskImage diskImage = getDiskImageByImageId(getParameters().getImageId()); |
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, since MoveOrCopyDiskCommand::validateSpaceRequirements()
runs on both source & destination Storage Domains and checks both isDomainWithinThresholds()
and Storage Domains available space.
I already removed isDomainWithinThresholds()
check above, since it's exactly what the MoveOrCopyDiskCommand::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:
Line 91 in b54471d
protected DiskImage getImage() {
While inLiveMigrateDiskCommand::validateDestDomainsSpaceRequirements()
thegetDiskImageByImageId()
is called:
Line 674 in b54471d
private DiskImage getDiskImageByImageId(Guid imageId) {
But at the end it looks like they are returning pretty much the samediskImageDao.get(imageId);
... -
In the
MoveOrCopyDiskCommand::validateSpaceRequirements()
, the available free space validation is performed by thehasSpaceForDisksWithSnapshots()
:
Line 245 in b54471d
public ValidationResult hasSpaceForDisksWithSnapshots(Collection<DiskImage> diskImages) {
While in LiveMigrateDiskCommand::validateDestDomainsSpaceRequirements()
the hasSpaceForClonedDisks()
is called:
Line 222 in b54471d
public ValidationResult hasSpaceForClonedDisks(Collection<DiskImage> diskImages) { |
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 the MoveOrCopyDiskCommand::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.
- The way how
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.
8714048
to
4d2d120
Compare
/ost |
.../modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/MoveOrCopyDiskCommand.java
Outdated
Show resolved
Hide resolved
.../modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/MoveOrCopyDiskCommand.java
Outdated
Show resolved
Hide resolved
/ost |
7e65a6e
to
3605a58
Compare
/ost |
@barpavel can you please keep only the parts crucial for the fix? there are far too many changes here for what is actually needed |
.../modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/lsm/LiveMigrateDiskCommand.java
Outdated
Show resolved
Hide resolved
This PR was verified in the following scenarios.
There are a few open question (above). The code refactoring that I did "on the way" allows to improve some old code, or even remove current duplicate checks. |
3605a58
to
8b1e2c4
Compare
.../modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/MoveOrCopyDiskCommand.java
Show resolved
Hide resolved
.../modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/MoveOrCopyDiskCommand.java
Outdated
Show resolved
Hide resolved
.../modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/lsm/LiveMigrateDiskCommand.java
Outdated
Show resolved
Hide resolved
c190bc9
to
c72349d
Compare
Changing "createMultipleStorageDomainsValidator()" to be "protected" in the "MoveOrCopyDiskCommand" class and overriding it in its derived "LiveMigrateDiskCommand" class to add also the source Storage Domain ID. Thus no need to check in the parent "MoveOrCopyDiskCommand" class whether it's an LSM flow or not, in order to add also the source Storage Domain ID to be checked by the created "MultipleStorageDomainsValidator". Also removing the redundant "isDomainWithinThresholds()" destination Storage Domain validation from the "LiveMigrateDiskCommand" class, since it is correctly performed by the above "MultipleStorageDomainsValidator" that validates both source & destination Storage Domains from its parent's "MoveOrCopyDiskCommand::validateSpaceRequirements()". Signed-off-by: Pavel Bar <pbar@redhat.com> Bug-Url: https://bugzilla.redhat.com/2116309
…SM flow Modify destination "StorageDomainValidator" getter to return a dedicated validator that its "getTotalSizeForClonedDisk()" method requires the additional 3 chunks (approximately 8 GiB) extra space on the destination Storage Domain. This is relevant only in LSM flow for cluster version 4.7 or above. Signed-off-by: Pavel Bar <pbar@redhat.com> Bug-Url: https://bugzilla.redhat.com/2116309
c72349d
to
408a636
Compare
/ost |
Update Live Storage Migration flow validation to require additional 3 chunks (approximately 8 GiB) of free space on the destination Storage Domain.
Bug-Url: https://bugzilla.redhat.com/2116309