Roy Golan has posted comments on this change. Change subject: core: prevent maintenance a host with hard affinity ......................................................................
Patch Set 1: (6 comments) http://gerrit.ovirt.org/#/c/34060/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MaintenanceNumberOfVdssCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MaintenanceNumberOfVdssCommand.java: Line 245: addCanDoActionMessage(VdcBllMessages.VDS_CANNOT_MAINTENANCE_VDS_IS_NOT_RESPONDING_AND_IS_SPM); Line 246: } else if (vds.getSpmStatus() == VdsSpmStatus.SPM && vds.getStatus() == VDSStatus.Up && Line 247: getAsyncTaskDao().getAsyncTaskIdsByStoragePoolId(vds.getStoragePoolId()).size() > 0) { Line 248: addCanDoActionMessage(VdcBllMessages.VDS_CANNOT_MAINTENANCE_SPM_WITH_RUNNING_TASKS); Line 249: result = false; could you extract this to a boolean method as well? Line 250: } else { Line 251: if (!vms.isEmpty()) { Line 252: List<AffinityGroup> affinityGroups = Line 253: getDbFacade().getAffinityGroupDao() Line 246: } else if (vds.getSpmStatus() == VdsSpmStatus.SPM && vds.getStatus() == VDSStatus.Up && Line 247: getAsyncTaskDao().getAsyncTaskIdsByStoragePoolId(vds.getStoragePoolId()).size() > 0) { Line 248: addCanDoActionMessage(VdcBllMessages.VDS_CANNOT_MAINTENANCE_SPM_WITH_RUNNING_TASKS); Line 249: result = false; Line 250: } else { we should add a //TODO here that this limitation is due to scheduler who doesn't have knowledge of other VMs in-process. Line 251: if (!vms.isEmpty()) { Line 252: List<AffinityGroup> affinityGroups = Line 253: getDbFacade().getAffinityGroupDao() Line 254: .getEnforcingAffinityGroupsByRunningVmsOnVdsId(vdsId); Line 250: } else { Line 251: if (!vms.isEmpty()) { Line 252: List<AffinityGroup> affinityGroups = Line 253: getDbFacade().getAffinityGroupDao() Line 254: .getEnforcingAffinityGroupsByRunningVmsOnVdsId(vdsId); if there are 2 VMs and one is down then the affinity isn't broken really. Line 255: if (!affinityGroups.isEmpty()) { Line 256: List<Object> items = new ArrayList<>(); Line 257: for (AffinityGroup affinityGroup : affinityGroups) { Line 258: items.add(String.format("%1$s (%2$s)", http://gerrit.ovirt.org/#/c/34060/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/VmAffinityFilterPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/VmAffinityFilterPolicyUnit.java: Line 74: Set<Guid> acceptableHosts = new HashSet<>(); Line 75: // Group all hosts for VMs with positive affinity Line 76: for (Guid id : allVmIdsPositive) { Line 77: VM runVm = runningVMsMap.get(id); Line 78: if (runVm != null && runVm.getRunOnVds() != null) { can you explain this removal? Line 79: acceptableHosts.add(runVm.getRunOnVds()); Line 80: } Line 81: } Line 82: http://gerrit.ovirt.org/#/c/34060/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/scheduling/AffinityGroupDaoImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/scheduling/AffinityGroupDaoImpl.java: Line 60: getCustomMapSqlParameterSource().addValue("vm_id", vmId)); Line 61: } Line 62: Line 63: @Override Line 64: public List<AffinityGroup> getEnforcingAffinityGroupsByRunningVmsOnVdsId(Guid vdsId) { could possibly map the number of running vms per AG for the canDo? Line 65: return getCallsHandler().executeReadList("getEnforcingAffinityGroupsByRunningVmsOnVdsId", Line 66: createEntityRowMapper(), Line 67: getCustomMapSqlParameterSource().addValue("vds_id", vdsId)); Line 68: } http://gerrit.ovirt.org/#/c/34060/1/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 82: the first "(Vms)" is confusing. can you improve or remove :)? -- To view, visit http://gerrit.ovirt.org/34060 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idd93ddca562bf739b2cd423e907be1b6da861735 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
