Allon Mureinik has posted comments on this change.
Change subject: core: support multiple concurrent disks migration
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(15 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());
Not a fan of this pattern.
Why not have it private static final, and have each class that really needs a
log define one?
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;
Seems as though part of this should be generalized to CommandBase, no?
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) {
Wasn't this already validated by the calling command?
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) {
possible optimization - this can be checked per unique SD instead of per disk.
Not sure it's worth the hassle.
Line 266: DiskImage diskImage = getDiskImageById(imageId);
Line 267: StorageDomainValidator validator = new StorageDomainValidator(
Line 268: getStorageDomainById(sourceDomainId,
diskImage.getstorage_pool_id().getValue()));
Line 269:
Line 269:
Line 270: return
validator.isDomainExistAndActive(getReturnValue().getCanDoActionMessages());
Line 271: }
Line 272:
Line 273: private boolean validateDestStorage(Guid imageId, Guid
destDomainId) {
same here
Line 274: DiskImage diskImage = getDiskImageById(imageId);
Line 275: StorageDomainValidator validator = new StorageDomainValidator(
Line 276: getStorageDomainById(destDomainId,
diskImage.getstorage_pool_id().getValue()));
Line 277:
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());
I'd use ceil instead of round, just to be on the super-safe side.
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/lsm/LiveMigrateDisksTaskHandler.java
Line 10: import org.ovirt.engine.core.common.action.VdcActionType;
Line 11: import org.ovirt.engine.core.common.action.VdcReturnValueBase;
Line 12: import org.ovirt.engine.core.common.asynctasks.AsyncTaskType;
Line 13:
Line 14: public class LiveMigrateDisksTaskHandler implements
SPMAsyncTaskHandler {
please document the empty blocks
Line 15:
Line 16: private final TaskHandlerCommand<? extends
LiveMigrateDisksParameters> enclosingCommand;
Line 17:
Line 18: public LiveMigrateDisksTaskHandler(TaskHandlerCommand<? extends
LiveMigrateDisksParameters> enclosingCommand) {
....................................................
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);
The error message for this BllMessage is: "Cannot ${action} ${type}. The
specified disk does not exist".
I don't think it's appropriate - you need a new message for something like
"Cannot ${action} ${type}. No disk moves were requested."
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());
why is this required?
Can you add a comment here?
Line 84: if (diskImage == null) {
Line 85: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_NOT_EXIST);
Line 86: }
Line 87:
Line 105: return true;
Line 106: }
Line 107:
Line 108: private boolean isVmRunning(VM vm) {
Line 109: return vm != null && vm.getStatus() == VMStatus.Up &&
you can't get here with vm == null, so it should be removed from here.
Line 110: vm.getRunOnVds() != null &&
!vm.getRunOnVds().equals(Guid.Empty);
Line 111: }
Line 112:
Line 113: private boolean isVmDown(VM vm) {
Line 110: vm.getRunOnVds() != null &&
!vm.getRunOnVds().equals(Guid.Empty);
Line 111: }
Line 112:
Line 113: private boolean isVmDown(VM vm) {
Line 114: return vm != null && vm.getStatus() == VMStatus.Down;
you can't get here with vm == null, so it should be removed from here.
Line 115: }
Line 116:
Line 117: private LiveMigrateDiskParameters
getLiveMigrateDiskParameters(MoveDiskParameters moveDiskParameters, Guid vmId) {
Line 118: return new
LiveMigrateDiskParameters(moveDiskParameters.getImageId(),
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) {
this is more of a "createXYZ" method than a "getXYZ" method
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);
You're iterating over the keyset, and calling get from the map in the first
line.
Just iterate over the entrySet instead
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() {
IMHO, this class doesn't need an MLA check - if it fails, it will fail the
entire command, before pivoting to the underlying commands.
Is this the behaviour we want?
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),
you sure about the "NONE" here?
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 <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
