Arik Hadas has posted comments on this change. Change subject: core: enable migration to a different cluster ......................................................................
Patch Set 5: (10 comments) http://gerrit.ovirt.org/#/c/34316/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVMClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVMClusterCommand.java: Line 23: } Line 24: Line 25: @Override Line 26: protected boolean canDoAction() { Line 27: if (getVm() != null && !ObjectIdentityChecker.CanUpdateField(getVm(), "vdsGroupId", getVm().getStatus())) { I guess you didn't put this one inside the validator because of the migration flow right? if so, I think it is better to move it to the validator and actually execute the check only if it is invoked directly from REST/UI, assuming that in case of internal execution the caller is responsible to check if the VM is in the right state Line 28: addCanDoActionMessage(VdcBllMessages.VM_STATUS_NOT_VALID_FOR_UPDATE); Line 29: return false; Line 30: } Line 31: http://gerrit.ovirt.org/#/c/34316/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVmClusterValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVmClusterValidator.java: Line 24: Line 25: private VDSGroup targetCluster; Line 26: Line 27: public ChangeVmClusterValidator(VmCommand parentCommand, Guid targetClusterId) { Line 28: this.parentCommand = parentCommand; it is better not to pass the parent command, but to pass all the required state to the validator to keep the validator object independent from the context it is invoked from Line 29: this.targetClusterId = targetClusterId; Line 30: } Line 31: Line 32: protected boolean validate() { Line 31: Line 32: protected boolean validate() { Line 33: // Set parameters for messaging. Line 34: parentCommand.addCanDoActionMessage(VdcBllMessages.VAR__ACTION__UPDATE); Line 35: parentCommand.addCanDoActionMessage(VdcBllMessages.VAR__TYPE__VM__CLUSTER); when it is called from the migration flow, don't we want to have the "cannot migration vm.." prefix for the errors? Line 36: Line 37: VM vm = parentCommand.getVm(); Line 38: if (vm == null) { Line 39: parentCommand.addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND); Line 39: parentCommand.addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND); Line 40: return false; Line 41: } else { Line 42: Line 43: if (!parentCommand.canRunActionOnNonManagedVm()) { if parentCommand is MigrateVmCommand, then the canRunActionOnNonManagedVm for migrate opreation is called which is wrong. I think this check should not be inside the validator Line 44: return false; Line 45: } Line 46: Line 47: targetCluster = DbFacade.getInstance().getVdsGroupDao().get(targetClusterId); Line 55: parentCommand.addCanDoActionMessage(VdcBllMessages.VM_CANNOT_MOVE_TO_CLUSTER_IN_OTHER_STORAGE_POOL); Line 56: return false; Line 57: } Line 58: Line 59: List<VmNic> interfaces = parentCommand.getVmNicDao().getAllForVm(vm.getId()); we can get the dao from DbFacade directly instead of using the parent command Line 60: Line 61: Version clusterCompatibilityVersion = targetCluster.getcompatibility_version(); Line 62: if (!validateDestinationClusterContainsNetworks(interfaces) Line 63: || !validateNics(interfaces, clusterCompatibilityVersion)) { http://gerrit.ovirt.org/#/c/34316/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java: Line 194: return; Line 195: } Line 196: Line 197: ChangeVmClusterWorker worker = new ChangeVmClusterWorker(this, getParameters().getTargetVdsGroupId(), getVm()); Line 198: worker.execute(); > it might have been easier to just run changeVmCluster, no? +1 note that in this case you'll need to separate internal execution flow vs external execution flow in the validator Line 199: } Line 200: Line 201: private int getMaximumMigrationDowntime() { Line 202: if (getVm().getMigrationDowntime() != null) { http://gerrit.ovirt.org/#/c/34316/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmToServerCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmToServerCommand.java: Line 46: } Line 47: Line 48: if (getParameters().getTargetVdsGroupId() != null && !getParameters().getTargetVdsGroupId().equals(getDestinationVds().getVdsGroupId())) { Line 49: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DESTINATION_HOST_NOT_IN_DESTINATION_CLUSTER); Line 50: } > in case user didnt send different cluster, we still need to perform the pre I think this check and the previous one should be removed. if I explicitly say I want to migrate to specific host and we allow to migrate to different cluster, there's nothing to check (except that they are both reside in the same data-center, do we check this?) Line 51: Line 52: if (getVm().getRunOnVds() != null && getVm().getRunOnVds().equals(getDestinationVdsId())) { Line 53: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_MIGRATION_TO_SAME_HOST); Line 54: } Line 49: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DESTINATION_HOST_NOT_IN_DESTINATION_CLUSTER); Line 50: } Line 51: Line 52: if (getVm().getRunOnVds() != null && getVm().getRunOnVds().equals(getDestinationVdsId())) { Line 53: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_MIGRATION_TO_SAME_HOST); is there a reason to change the order of those checks? Line 54: } Line 55: Line 56: return true; Line 57: } http://gerrit.ovirt.org/#/c/34316/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/MigrateVmParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/MigrateVmParameters.java: Line 67: } please add setter (some day this parameters class will be persisted and json will complain that setter is missing) http://gerrit.ovirt.org/#/c/34316/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/MigrateVmToServerParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/MigrateVmToServerParameters.java: Line 5: public class MigrateVmToServerParameters extends MigrateVmParameters { Line 6: private static final long serialVersionUID = 2378358850714143232L; Line 7: private Guid vdsId; Line 8: Line 9: public MigrateVmToServerParameters(boolean forceMigration, Guid vmId, Guid serverId, Guid targetVdsGroupId) { why do we expect to receive the target cluster here? can't we take it from the destination server? I think there are 3 cases that should be handled: 1. migration within the same cluster (particular case of the next one) 2. migration to other cluster 3. migration to server - the server defined the target cluster Line 10: super(forceMigration, vmId, targetVdsGroupId); Line 11: vdsId = serverId; Line 12: } Line 13: -- To view, visit http://gerrit.ovirt.org/34316 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9eb4fd7f10d3b0e63455a81f0b5c1389586a43f4 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
