Gilad Chaplik has posted comments on this change. Change subject: core: prevent maintenance a host with hard affinity ......................................................................
Patch Set 1: (7 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? Done 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 added a description to the method. 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. Done 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? causes regression. when migrating hostMap doesn't contain the RunOnVds host, means that acceptableHosts will not add the host -> acceptableHosts remains empty, and in line 111, we add all hosts if it's empty. Preventing the maintenance, means that we don't need to handle it here. 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? handled that in the command 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 78: - Otherwise, either bring the node back up, or release the SPM resource.\n\ Line 79: To do so, verify that the node is really down by right clicking on the host and confirm that the node was shutdown manually. Line 80: VDS_CANNOT_MAINTENANCE_VDS_IS_NOT_OPERATIONAL=Cannot switch Host to Maintenance mode, Host is not operational. Line 81: VDS_CANNOT_MAINTENANCE_VDS_IS_IN_MAINTENANCE=Cannot switch Host to Maintenance mode. Host is already in Maintenance mode. Line 82: VDS_CANNOT_MAINTENANCE_VDS_HAS_AFFINITY_VMS=Cannot switch Host(s) to Maintenance mode.\nThe following Enforcing Affinity Groups (VMs) have running VMs and can break affinity rule.\n${AFFINITY_GROUPS_VMS}\nPlease manually migrate the VM(s), or change Affinity Group's enforcing to false. > the first "(Vms)" is confusing. can you improve or remove :)? Done Line 83: VDS_CANNOT_MAINTENANCE_SPM_WITH_RUNNING_TASKS=Cannot switch Host to Maintenance mode. Host has asynchronous running tasks,\n\ Line 84: wait for operation to complete and retry. Line 85: VDS_CANNOT_MAINTENANCE_SPM_CONTENDING=Cannot switch Host to Maintenance mode. Host is contending for Storage Pool Manager,\n\ Line 86: wait for operation to complete and retry. http://gerrit.ovirt.org/#/c/34060/1/packaging/dbscripts/affinity_groups_sp.sql File packaging/dbscripts/affinity_groups_sp.sql: Line 140: RETURN QUERY Line 141: SELECT DISTINCT affinity_groups_view.* FROM affinity_groups_view Line 142: INNER JOIN affinity_group_members ON id = affinity_group_members.affinity_group_id Line 143: INNER JOIN vm_dynamic ON affinity_group_members.vm_id = vm_dynamic.vm_guid AND vm_dynamic.run_on_vds = v_vds_id Line 144: WHERE enforcing; should be positive affinity Line 145: END; $procedure$ Line 146: LANGUAGE plpgsql; -- 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: 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
