Tomas Jelinek 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 migrati I don't really think so. Especially not since it is called from the MigrateVmCommand's canDoAction which is not executed internally. But anyway, I think it should do the checks and if some class needs some additional ones, it should do them by itself. 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 s well, I think it is better to have the parent passed explicitly, because I can use things like addCanDoActionMessages, failCanDoAction, validate etc. instead of manipulating a list passed here directly and duplicating the logic from the command. Also from usage perspective it is more nice to pass the parent than the messages etc. Line 29: this.targetClusterId = targetClusterId; Line 30: } Line 31: Line 32: protected boolean validate() { 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 f Done 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 comma Done 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(); > +1 Done Line 199: } Line 200: Line 201: private int getMaximumMigrationDowntime() { Line 202: if (getVm().getMigrationDowntime() != null) { Line 469: Line 470: @Override Line 471: public List<PermissionSubject> getPermissionCheckSubjects() { Line 472: List<PermissionSubject> permissionList = super.getPermissionCheckSubjects(); Line 473: if (getParameters().getTargetVdsGroupId() != null) { > change cluster also requires EDIT_VM_PROPERTIES (it is got from VdcActionTy Done Line 474: // needs to have create VM on the target cluster in case migrating out from the cluster Line 475: permissionList.add(new PermissionSubject(getParameters().getTargetVdsGroupId(), VdcObjectType.VdsGroups, ActionGroup.CREATE_VM)); Line 476: } Line 477: 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: } > I think this check and the previous one should be removed. if I explicitly - migrating to different cluster is not something we want to promote, and also we want to avoid that the user will do it accidentally. So we want the user to send the different cluster id explicitly. - we check that the VM stays in the DC in ChangeVmClusterValidator - ok, adding back the prev check 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? I did not change the order - I have just removed one and added one :) Anyway, adding back the original, so now we will have 3. 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 63: } Line 64: Line 65: public Guid getTargetVdsGroupId() { Line 66: return targetVdsGroupId; Line 67: } > please add setter (some day this parameters class will be persisted and jso Done 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 Because it is the child of MigrateVmParameters and that one expects to get also the target cluster if going off cluster. And the MigrateVmCommand is using this parameter. It could be done that the child will get less params and do some calculations, but it would get messy soon. And also, migrating to different cluster is not something we want to promote, and also we want to avoid that the user will do it accidentally. So we want the user to send the different cluster id explicitly. 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
