Liron Aravot has posted comments on this change.
Change subject: core,ui: support multiple concurrent disks migration
......................................................................
Patch Set 5: (15 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java
Line 119: if (vm != null) {
Line 120: return vm;
Line 121: }
Line 122:
Line 123: vm = getVmDAO().get(getParameters().getVmId());
super.getVm() will do dao call...
Line 124: if (vm != null) {
Line 125: setVm(vm);
Line 126: setVmId(vm.getId());
Line 127: }
Line 152: if (storageDomainsMap.containsKey(storageDomainId)) {
Line 153: return storageDomainsMap.get(storageDomainId);
Line 154: }
Line 155:
Line 156: storage_domains storageDomain =
getStorageDomainDao().getForStoragePool(storageDomainId, storagePoolId);
why not by id? we should have index on that column..
Line 157: storageDomainsMap.put(storageDomainId, storageDomain);
Line 158:
Line 159: return storageDomain;
Line 160: }
Line 222:
Line 223: return true;
Line 224: }
Line 225:
Line 226: protected boolean isImageNotLocked() {
name is a bit misleading...
Line 227: return ImagesHandler.checkImagesLocked(getVm(),
getReturnValue().getCanDoActionMessages());
Line 228: }
Line 229:
Line 230: private boolean isLiveMigrationEnabled() {
Line 253:
Line 254: if (!ImagesHandler.BlankImageTemplateId.equals(templateId)) {
Line 255: DiskImage templateImage =
getDiskImageDao().get(templateId);
Line 256: if
(!templateImage.getstorage_ids().contains(sourceDomainId)) {
Line 257:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_TEMPLATE_NOT_FOUND_ON_DESTINATION_DOMAIN);
failCanDo..
Line 258: return false;
Line 259: }
Line 260: }
Line 261:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveDisksCommand.java
Line 30:
Line 31: private List<VdcReturnValueBase> vdcReturnValues = new
ArrayList<VdcReturnValueBase>();
Line 32: private List<MoveDiskParameters> moveParametersList = new
ArrayList<MoveDiskParameters>();
Line 33: private Map<Guid, List<LiveMigrateDiskParameters>>
vmsLiveMigrateParametersMap =
Line 34: new HashMap<Guid, List<LiveMigrateDiskParameters>>();
do we need a map? seems to me like we are just iternating over it and the vm id
is a member of the LiveMigrateDiskParameters as well..
Line 35:
Line 36: public MoveDisksCommand(Guid commandId) {
Line 37: super(commandId);
Line 38: }
Line 44: @Override
Line 45: protected void executeCommand() {
Line 46: if (!moveParametersList.isEmpty()) {
Line 47:
vdcReturnValues.addAll(Backend.getInstance().RunMultipleActions(VdcActionType.MoveOrCopyDisk,
Line 48: new
ArrayList<VdcActionParametersBase>(getMoveDisksParametersList()), false));
1. do we need to create new arraylist here?
2. do we really need the getter? this member is accessed internally only..
Line 49: }
Line 50:
Line 51: if (!vmsLiveMigrateParametersMap.isEmpty()) {
Line 52:
vdcReturnValues.addAll(Backend.getInstance().RunMultipleActions(VdcActionType.LiveMigrateVmDisks,
Line 49: }
Line 50:
Line 51: if (!vmsLiveMigrateParametersMap.isEmpty()) {
Line 52:
vdcReturnValues.addAll(Backend.getInstance().RunMultipleActions(VdcActionType.LiveMigrateVmDisks,
Line 53: new
ArrayList<VdcActionParametersBase>(getLiveMigrateDisksParametersList()),
false));
same question regarding the arraylist...
by the way- if the two if's are true, we will create two arraylists..
Line 54: }
Line 55:
Line 56: handleChildReturnValue();
Line 57: setSucceeded(true);
Line 59:
Line 60: private void handleChildReturnValue() {
Line 61: for (VdcReturnValueBase vdcReturnValue : vdcReturnValues) {
Line 62:
getReturnValue().getCanDoActionMessages().addAll(vdcReturnValue.getCanDoActionMessages());
Line 63:
getReturnValue().setCanDoAction(getReturnValue().getCanDoAction() &&
vdcReturnValue.getCanDoAction());
i think that those action happen here a lot of times, though it's uneeded..
i'd perform the set and add operations only in case
vdcReturnValue.getCanDoAction() == false, otherwise they are needless...we
could also skip the getReturnValue().setCanDoAction if it false,,
Line 64: }
Line 65: }
Line 66:
Line 67: @Override
Line 94:
Line 95: List<VM> allVms =
getVmDAO().getVmsListForDisk(diskImage.getId());
Line 96: VM vm = !allVms.isEmpty() ? allVms.get(0) : null;
Line 97:
Line 98: if (vm == null || isVmDown(vm)) {
vm can't be null...
Line 99: moveParametersList.add(moveDiskParameters);
Line 100: }
Line 101: else if (isVmRunning(vm)) {
Line 102: MultiValueMapUtils.addToMap(vm.getId(),
Line 97:
Line 98: if (vm == null || isVmDown(vm)) {
Line 99: moveParametersList.add(moveDiskParameters);
Line 100: }
Line 101: else if (isVmRunning(vm)) {
we can't garunatee that the vm runs when we execute, so is this check help us
somehow?
Line 102: MultiValueMapUtils.addToMap(vm.getId(),
Line 103:
createLiveMigrateDiskParameters(moveDiskParameters, vm.getId()),
Line 104: vmsLiveMigrateParametersMap);
Line 105: }
Line 111:
Line 112: return true;
Line 113: }
Line 114:
Line 115: private boolean isVmRunning(VM vm) {
don't we have such method on VmHandler? maybe it can be added there?
Line 116: return vm.getStatus() == VMStatus.Up &&
Line 117: vm.getRunOnVds() != null &&
!vm.getRunOnVds().equals(Guid.Empty);
Line 118: }
Line 119:
Line 116: return vm.getStatus() == VMStatus.Up &&
Line 117: vm.getRunOnVds() != null &&
!vm.getRunOnVds().equals(Guid.Empty);
Line 118: }
Line 119:
Line 120: private boolean isVmDown(VM vm) {
don't we have such method on VmHandler? maybe it can be added there?
Line 121: return vm.getStatus() == VMStatus.Down;
Line 122: }
Line 123:
Line 124: private LiveMigrateDiskParameters
createLiveMigrateDiskParameters(MoveDiskParameters moveDiskParameters, Guid
vmId) {
Line 130: }
Line 131:
Line 132: protected List<MoveDiskParameters> getMoveDisksParametersList() {
Line 133: for (MoveDiskParameters moveDiskParameters :
moveParametersList) {
Line 134:
moveDiskParameters.setSessionId(getParameters().getSessionId());
I'd separate or rename the method as it's not a true 'getter'.
why do we set the session it only on move disk and not on live migrate?
Line 135: }
Line 136:
Line 137: return moveParametersList;
Line 138: }
Line 137: return moveParametersList;
Line 138: }
Line 139:
Line 140: protected List<LiveMigrateVmDisksParameters>
getLiveMigrateDisksParametersList() {
Line 141: List<LiveMigrateVmDisksParameters>
liveMigrateDisksParametersList = new ArrayList<LiveMigrateVmDisksParameters>();
arraylist is created and returned from here..and a new arraylist is created
from it..line 53..
Line 142:
Line 143: for (Map.Entry<Guid, List<LiveMigrateDiskParameters>> entry :
vmsLiveMigrateParametersMap.entrySet()) {
Line 144: LiveMigrateVmDisksParameters liveMigrateDisksParameters =
Line 145: new
LiveMigrateVmDisksParameters(entry.getValue(), entry.getKey());
Line 142:
Line 143: for (Map.Entry<Guid, List<LiveMigrateDiskParameters>> entry :
vmsLiveMigrateParametersMap.entrySet()) {
Line 144: LiveMigrateVmDisksParameters liveMigrateDisksParameters =
Line 145: new
LiveMigrateVmDisksParameters(entry.getValue(), entry.getKey());
Line 146:
why do we need to create each time new parameters when we already have the
parameters in the map?
Line 147:
liveMigrateDisksParameters.setParentCommand(VdcActionType.MoveDisks);
Line 148:
liveMigrateDisksParameters.setSessionId(getParameters().getSessionId());
Line 149:
Line 150:
liveMigrateDisksParametersList.add(liveMigrateDisksParameters);
--
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: 5
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: Liron Aravot <[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
