Omer Frenkel has posted comments on this change.

Change subject: core: enable migration to a different cluster
......................................................................


Patch Set 5:

(4 comments)

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 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);
this should move to the command, please override the 
setActionMessageParameters() method
Line 36: 
Line 37:         VM vm = parentCommand.getVm();
Line 38:         if (vm == null) {
Line 39:             
parentCommand.addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND);


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?
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 
VdcActionType.ChangeVMCluster.getActionGroup()

you can extract a method in VmHandler to get the permissions needed for change 
cluster and call it here an in changeVmCluster command
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:         }
in case user didnt send different cluster, we still need to perform the 
previous check - current host and dest host are on the same cluster.
(for example user can send from rest api to migrate to host on different 
cluster, although targetCluster==null)
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:         }


-- 
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 <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to