Daniel Erez has posted comments on this change.
Change subject: core: support multiple concurrent disks migration
......................................................................
Patch Set 2: (11 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractSPMAsyncTaskHandler.java
Line 14: import org.ovirt.engine.core.utils.log.LogFactory;
Line 15:
Line 16: public abstract class AbstractSPMAsyncTaskHandler<C extends
TaskHandlerCommand<?>> implements SPMAsyncTaskHandler {
Line 17:
Line 18: protected Log log = LogFactory.getLog(getClass());
Why not? We have the same line in CommandBase which all command can
conveniently use.
Line 19:
Line 20: private final C cmd;
Line 21:
Line 22: public AbstractSPMAsyncTaskHandler(C cmd) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateDiskCommand.java
Line 130: case END_FAILURE:
Line 131: return AuditLogType.USER_MOVED_VM_DISK_FINISHED_FAILURE;
Line 132: }
Line 133:
Line 134: return AuditLogType.UNASSIGNED;
Probably only 'endSuccessfully' and 'endWithFailure' implementations...
Since it requires some further seat infrastructure design in CommandBase,
I prefer to defer it to a separate patch.
Line 135: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateDisksCommand.java
Line 228:
Line 229: return true;
Line 230: }
Line 231:
Line 232: private boolean isDiskExist(Guid imageId) {
Done.
Yeah, I preferred to keep it here for future use. But since it's an internal
command it should be safe enough to remove it.
Line 233: if (getDiskImageById(imageId) == null) {
Line 234: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_NOT_EXIST);
Line 235: }
Line 236:
Line 261:
Line 262: return true;
Line 263: }
Line 264:
Line 265: private boolean validateSourceStorageDomain(Guid imageId, Guid
sourceDomainId) {
For complexity reasons, I'll address it on a future patch.
Line 266: DiskImage diskImage = getDiskImageById(imageId);
Line 267: StorageDomainValidator validator = new StorageDomainValidator(
Line 268: getStorageDomainById(sourceDomainId,
diskImage.getstorage_pool_id().getValue()));
Line 269:
Line 304: List<DiskImage> allImageSnapshots =
Line 305:
ImagesHandler.getAllImageSnapshots(diskImage.getImageId(), templateId);
Line 306:
Line 307: diskImage.getSnapshots().addAll(allImageSnapshots);
Line 308: totalImagesSize +=
Math.round(diskImage.getActualDiskWithSnapshotsSize());
If needed, I prefer to do it on a separate patch, since the same exact logic is
used at MoveOrCopyDiskCommand::validateSpaceRequirements()
Line 309: }
Line 310:
Line 311: if
(!StorageDomainSpaceChecker.hasSpaceForRequest(destDomain, totalImagesSize)) {
Line 312: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveDisksCommand.java
Line 65:
Line 66: @Override
Line 67: protected boolean canDoAction() {
Line 68: if (getParameters().getParametersList().isEmpty()) {
Line 69: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_NOT_EXIST);
Done.
Changed to: "Cannot ${action} ${type}. No disks have been specified."
Line 70: }
Line 71:
Line 72: return updateParameters();
Line 73: }
Line 79: }
Line 80:
Line 81: private boolean updateParameters() {
Line 82: for (MoveDiskParameters moveDiskParameters :
getParameters().getParametersList()) {
Line 83: DiskImage diskImage =
getDbFacade().getDiskImageDao().getAncestor(moveDiskParameters.getImageId());
Done
Line 84: if (diskImage == null) {
Line 85: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_NOT_EXIST);
Line 86: }
Line 87:
Line 113: private boolean isVmDown(VM vm) {
Line 114: return vm != null && vm.getStatus() == VMStatus.Down;
Line 115: }
Line 116:
Line 117: private LiveMigrateDiskParameters
getLiveMigrateDiskParameters(MoveDiskParameters moveDiskParameters, Guid vmId) {
Done
Line 118: return new
LiveMigrateDiskParameters(moveDiskParameters.getImageId(),
Line 119: moveDiskParameters.getSourceDomainId(),
Line 120: moveDiskParameters.getStorageDomainId(),
Line 121: vmId,
Line 133: private List<LiveMigrateDisksParameters>
getLiveMigrateDisksParametersList() {
Line 134: List<LiveMigrateDisksParameters>
liveMigrateDisksParametersList = new ArrayList<LiveMigrateDisksParameters>();
Line 135:
Line 136: for (Guid vmId : vmsLiveMigrateParametersMap.keySet()) {
Line 137: List<LiveMigrateDiskParameters> parametersList =
vmsLiveMigrateParametersMap.get(vmId);
Done
Line 138: LiveMigrateDisksParameters liveMigrateDisksParameters =
Line 139: new LiveMigrateDisksParameters(parametersList,
vmId);
Line 140:
Line 141:
liveMigrateDisksParameters.setParentCommand(VdcActionType.MoveDisks);
Line 147: return liveMigrateDisksParametersList;
Line 148: }
Line 149:
Line 150: @Override
Line 151: public List<PermissionSubject> getPermissionCheckSubjects() {
CommandBase would skip MLA check for internal action.
Line 152: List<PermissionSubject> permissionList = new
ArrayList<PermissionSubject>();
Line 153:
Line 154: for (MoveOrCopyImageGroupParameters parameters :
getParameters().getParametersList()) {
Line 155: permissionList.add(new
PermissionSubject(parameters.getImageId(),
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 224: ConnectAllHostsToLun(1008, QuotaDependency.NONE),
Line 225: AddPosixFsStorageDomain(1009, ActionGroup.CREATE_STORAGE_DOMAIN,
QuotaDependency.NONE),
Line 226: LiveMigrateDisk(1010, QuotaDependency.NONE),
Line 227: LiveMigrateDisks(1011, false, QuotaDependency.STORAGE),
Line 228: MoveDisks(1012, false, QuotaDependency.NONE),
Yes, since MoveDisksCommand doesn't implement QuotaStorageDependent
(MoveOrCopyDiskCommand and LiveMigrateDisksCommand do implement it).
Line 229: // Event Notification
Line 230: AddEventSubscription(1100, QuotaDependency.NONE),
Line 231: RemoveEventSubscription(1101, QuotaDependency.NONE),
Line 232:
--
To view, visit http://gerrit.ovirt.org/10154
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadcffa5748b58b1af40535b0447487dde6c2d6cb
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
