Dudi Maroshi has posted comments on this change. Change subject: core: enable pinning to multiple hosts ......................................................................
Patch Set 11: (7 comments) https://gerrit.ovirt.org/#/c/41962/11/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 84: <Guid> > emptyList is generic method so the cast it unnecessary. vm.setDedicatedVmForVdsList(Collections.emptyList()); Is a compilation error: The method setDedicatedVmForVdsList(List<Guid>) in the type VM is not applicable for the arguments (List<Object>) https://gerrit.ovirt.org/#/c/41962/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java: Line 923: // vmList.equals(paramList) not good enough, the lists order could change Line 924: if (vmList.size() != paramList.size()){ Line 925: return true; Line 926: } Line 927: for (Guid origGuid : vmList) { > need to make sure we ignore nulls Done Line 928: if (paramList.contains(origGuid) == false){ Line 929: return true; Line 930: } Line 931: } https://gerrit.ovirt.org/#/c/41962/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/AbstractVmNumaNodeCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/AbstractVmNumaNodeCommand.java: Line 72: numaMigrationSupported > this rename is really awkward, really. Done https://gerrit.ovirt.org/#/c/41962/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PinToHostPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PinToHostPolicyUnit.java: Line 39: } Line 40: } Line 41: Line 42: // if flow reaches here, the VM is pinned but there is no dedicated host. Line 43: return Collections.<VDS>emptyList(); > missing a space there and the Generic VDS decleration is redundant as empty Done Line 44: } Line 45: Line 46: return hosts; Line 47: } https://gerrit.ovirt.org/#/c/41962/11/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/BaseDAODbFacade.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/BaseDAODbFacade.java: Line 218: Line 219: return new ArrayList<String>(Arrays.asList(str.split(SEPARATOR))); Line 220: } Line 221: Line 222: static public List<Guid> convertStringListToGuidList(List<String> stringList){ > see GuidUtils.getGuidListFromStringArray Done Line 223: List<Guid> guidsList = new ArrayList<>(); Line 224: if (stringList == null || stringList.isEmpty()){ Line 225: return guidsList; Line 226: } https://gerrit.ovirt.org/#/c/41962/11/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 1445: ACTION_TYPE_FAILED_SCSI_RESERVATION_NOT_VALID_FOR_FLOATING_DISK=Cannot ${action} ${type}. SCSI reservation cannot be set when adding floating disks. Line 1446: ACTION_TYPE_FAILED_SGIO_IS_FILTERED=Cannot ${action} ${type}. SCSI reservation can be set only when SGIO is unfiltered. Line 1447: Line 1448: ACTION_TYPE_FAILED_VM_NOT_PINNED_TO_HOST=Cannot ${action} ${type}. VM must be pinned to a host. Line 1449: ACTION_TYPE_FAILED_VM_PINNED_TO_MULTIPLE_HOSTS=Cannot ${action} ${type}. VM must be pinned to a single host. > we need to add why its not allowed to multi-pin (numa|host-device) The failed action is stated in ${action} ${type} The affected commands are: 1. AddVmHostDeviceCommand 2. AddVmNumaNodesCommand 3. UpdateVmNumaNodeCommand Yet AddVmCommand and UpdateVmCommand allow adding/updating NUMA nodes. We need to think about duplicate functionality. Line 1450: ACTION_TYPE_FAILED_HOST_DEVICE_NOT_FOUND=Cannot ${action} ${type}. One or more of specified host devices not found. Line 1451: ACTION_TYPE_FAILED_HOST_DEVICE_NOT_AVAILABLE=Cannot ${action} ${type}. One or more configured host devices are unavailable. Line 1452: Line 1453: # vm icons https://gerrit.ovirt.org/#/c/41962/11/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java: Line 1655: } Line 1656: }); Line 1657: } Line 1658: Line 1659: public static VDS findHostByIdFromIdList(Collection<VDS> items, List<Guid> hostIdList) { > consider changing this to varargs so the API still remains the same for cal Refactored all callers (precisely 1) to this function, to use List<Guid> Line 1660: for (VDS host : items) { Line 1661: for (Guid hostId : hostIdList) { Line 1662: if (host.getId().equals(hostId)) { Line 1663: return host; -- To view, visit https://gerrit.ovirt.org/41962 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I710bd8d3505552a2a8d6060194d94c68c05445db Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Dudi Maroshi <d...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Dudi Maroshi <d...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Tomer Saban <tsa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
