Dudi Maroshi has posted comments on this change. Change subject: core: use NumaNodePinning instead of weird Pairs ......................................................................
Patch Set 9: Code-Review-1 (16 comments) We might wish to refactor the tables and views for NUMA node. We might discard the many-to-many association table vm_vds_numa_node_map and fix the associated views. Identified relationships: there is many-to-one (few nume-node to one vds) and one-to-many association (one numa-node to many vms). There is also NUMA node entity relationship to pinned cpu list. This CPU list can be retrieved from vm_static.cpu_pinning in the view. Later the list can be Java computed like the distance map. Note that there is no testing if the cpu_pinning is breaking the NUMA architecture. Libvirt might flag a warning? As for NUMA GUI rendering, I fixed it to use indexes only. We can also refactor NUMA-node primary key (vds_id + index). And discard the GUID. https://gerrit.ovirt.org/#/c/38048/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java: Line 899: // check single node pinned Line 900: if (vmNumaNodes != null Line 901: && vmNumaNodes.size() == 1 Line 902: && vmNumaNodes.get(0) != null Line 903: && vmNumaNodes.get(0).getNumaNodePinnings().size() == 1 I suggest getNumaNodePinnings().size() > 1. Following the rational for preferred mode. (see http://man7.org/linux/man-pages/man8/numactl.8.html option --preferred=node) If getNumaNodePinnings().size() == 1. This is strict mode, there is only one node, and failed allocation attempt will fault. (see option --membind=nodes in the link above) Line 904: && vmNumaNodes.get(0).getNumaNodePinnings().iterator().next().isPinned()) { Line 905: return ValidationResult.VALID; Line 906: } Line 907: https://gerrit.ovirt.org/#/c/38048/9/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 67: if (vmNumaNodes == null || vmNumaNodes.size() == 0) { Line 68: // if VM do not contain any NUMA node, skip checking Line 69: return true; Line 70: } Line 71: Confusing var name. Refrain from verbs to describe state. I suggest pinningAllowed Line 72: boolean pinHost = !Config.<Boolean> getValue(ConfigValues.SupportNUMAMigration); Line 73: Guid vdsId = getDedicatedHost(); Line 74: if (pinHost && vdsId == null && getMigrationSupport() != MigrationSupport.PINNED_TO_HOST) { Line 75: return failCanDoAction(VdcBllMessages.VM_NUMA_PINNED_VDS_NOT_EXIST); https://gerrit.ovirt.org/#/c/38048/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/UpdateVmNumaNodesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/UpdateVmNumaNodesCommand.java: Line 32: for (NumaNodePinning pinning : vmNumaNode.getNumaNodePinnings()) { Line 33: if (pinning.isPinned()) { Line 34: for (VdsNumaNode vdsNumaNode : vdsNumaNodes) { Line 35: if (vdsNumaNode.getIndex() == pinning.getIndex()) { Line 36: pinning.setPhysicalNodeId(vdsNumaNode.getId()); 1. It is way improved in readability then before. 2. Reminder: we want to discard the GUIDs and use indexes Line 37: break; Line 38: } Line 39: } Line 40: } https://gerrit.ovirt.org/#/c/38048/9/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsNumaNodeDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsNumaNodeDAODbFacadeImpl.java: Line 47: numaNodesCpusMap.get(pair.getFirst()).add(pair.getSecond()); Line 48: } Line 49: Line 50: for (VdsNumaNode node : vdsNumaNodes) { Line 51: if (numaNodesCpusMap.containsKey(node.getId())) { Reminder: banishing GUID, using index Line 52: node.setCpuIds(numaNodesCpusMap.get(node.getId())); Line 53: } Line 54: } Line 55: Line 70: numaNodeCpusExecutions.add(createNumaNodeCpusParametersMapper(node, cpuId)); Line 71: } Line 72: if (node instanceof VmNumaNode) { Line 73: for (NumaNodePinning pinning : ((VmNumaNode) node).getNumaNodePinnings()) { Line 74: vNodeToPnodeExecutions.add(createVnodeToPnodeParametersMapper(node.getId(), pinning)); Reminder: banishing GUID, using index Line 75: } Line 76: } Line 77: } Line 78: Line 108: } Line 109: if (node instanceof VmNumaNode) { Line 110: vNodeToPnodeDeletions.add(getCustomMapSqlParameterSource().addValue("vm_numa_node_id", node.getId())); Line 111: for (NumaNodePinning pinning : ((VmNumaNode)node).getNumaNodePinnings()) { Line 112: vNodeToPnodeInsertions.add(createVnodeToPnodeParametersMapper(node.getId(), pinning)); Reminder: banishing GUID, using index Line 113: } Line 114: } Line 115: } Line 116: getCallsHandler().executeStoredProcAsBatch("UpdateNumaNode", executions); Line 133: getCallsHandler().executeStoredProcAsBatch("DeleteNumaNode", executions); Line 134: } Line 135: Line 136: private MapSqlParameterSource createNumaNodeParametersMapper(VdsNumaNode node) { Line 137: return getCustomMapSqlParameterSource() Reminder: banishing GUID, using index Line 138: .addValue("numa_node_id", node.getId()) Line 139: .addValue("numa_node_index", node.getIndex()) Line 140: .addValue("mem_total", node.getMemTotal()) Line 141: .addValue("cpu_count", node.getCpuIds().size()) Line 142: .addValue("distance", getDistanceString(node.getNumaNodeDistances())); Line 143: } Line 144: Line 145: private MapSqlParameterSource createNumaNodeStatisticsParametersMapper(VdsNumaNode node) { Line 146: return getCustomMapSqlParameterSource() Reminder: banishing GUID, using index Line 147: .addValue("numa_node_id", node.getId()) Line 148: .addValue("mem_free", node.getNumaNodeStatistics().getMemFree()) Line 149: .addValue("usage_mem_percent", node.getNumaNodeStatistics().getMemUsagePercent()) Line 150: .addValue("cpu_sys", node.getNumaNodeStatistics().getCpuSys()) Line 163: protected MapSqlParameterSource createVnodeToPnodeParametersMapper(Guid vNodeId, NumaNodePinning pinning) { Line 164: return getCustomMapSqlParameterSource() Line 165: .addValue("id", Guid.newGuid()) Line 166: .addValue("vm_numa_node_id", vNodeId) Line 167: .addValue("vds_numa_node_id", pinning.getPhysicalNodeId()) Reminder: banishing GUID, using index Line 168: .addValue("vds_numa_node_index", pinning.getIndex()) Line 169: .addValue("is_pinned", pinning.isPinned()); Line 170: } Line 171: Line 175: public VdsNumaNode mapRow(ResultSet rs, int rowNum) Line 176: throws SQLException { Line 177: VdsNumaNode entity = new VdsNumaNode(); Line 178: NumaNodeStatistics stat = new NumaNodeStatistics(); Line 179: entity.setId(getGuid(rs, "numa_node_id")); Reminder: banishing GUID, using index Line 180: entity.setIndex(rs.getInt("numa_node_index")); Line 181: entity.setMemTotal(rs.getLong("mem_total")); Line 182: stat.setMemFree(rs.getLong("mem_free")); Line 183: stat.setMemUsagePercent(rs.getInt("usage_mem_percent")); Line 189: entity.setNumaNodeDistances(getDistanceMap(rs.getString("distance"))); Line 190: return entity; Line 191: } Line 192: }; Line 193: Reminder: banishing GUID, using index Line 194: private static final RowMapper<Pair<Guid, Integer>> vdsNumaNodeCpusRowMapper = Line 195: new RowMapper<Pair<Guid, Integer>>() { Line 196: Line 197: @Override https://gerrit.ovirt.org/#/c/38048/9/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmNumaNodeDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmNumaNodeDAODbFacadeImpl.java: Line 47: } Line 48: Line 49: Map<Guid, Set<NumaNodePinning>> vmNumaNodesPinMap = getAllVmNumaNodePinInfo(); Line 50: Line 51: for (VmNumaNode node : vmNumaNodes) { we might might wish to assign the index instead of the GUID Line 52: if (numaNodesCpusMap.containsKey(node.getId())) { Line 53: node.setCpuIds(numaNodesCpusMap.get(node.getId())); Line 54: } Line 55: if (vmNumaNodesPinMap.containsKey(node.getId())) { Line 49: Map<Guid, Set<NumaNodePinning>> vmNumaNodesPinMap = getAllVmNumaNodePinInfo(); Line 50: Line 51: for (VmNumaNode node : vmNumaNodes) { Line 52: if (numaNodesCpusMap.containsKey(node.getId())) { Line 53: node.setCpuIds(numaNodesCpusMap.get(node.getId())); Reminder: If we want to banish the GUID from NUMA pinning, we will have to banish it from NUMA cpu as well. Or leave it null. Line 54: } Line 55: if (vmNumaNodesPinMap.containsKey(node.getId())) { Line 56: node.setNumaNodePinnings(vmNumaNodesPinMap.get(node.getId())); Line 57: } Line 59: Line 60: return vmNumaNodes; Line 61: } Line 62: Line 63: @Override Reminder: if we banish the GUID, this function will be obsolete as well Line 64: public List<VmNumaNode> getAllPinnedVmNumaNodeByVdsNumaNodeId(Guid vdsNumaNodeId) { Line 65: MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() Line 66: .addValue("vds_numa_node_id", vdsNumaNodeId) Line 67: .addValue("is_pinned", true); Line 125: new RowMapper<VmNumaNode>() { Line 126: @Override Line 127: public VmNumaNode mapRow(ResultSet rs, int rowNum) Line 128: throws SQLException { Line 129: VmNumaNode entity = new VmNumaNode(); Reminder: banishing GUID, require us to think about omitting, nullifying or falsifying GUID. Line 130: entity.setId(getGuid(rs, "numa_node_id")); Line 131: entity.setIndex(rs.getInt("numa_node_index")); Line 132: entity.setMemTotal(rs.getLong("mem_total")); Line 133: return entity; Line 132: entity.setMemTotal(rs.getLong("mem_total")); Line 133: return entity; Line 134: } Line 135: }; Line 136: Reminder: banishing the GUID from the code is more difficult with the RowMapper returning Map with few null keys. We might need to think about falsifying GUID or refactoring all the RowMappers bellow to key on (int index). Refactoring RowMappers is not risky, since all its clients are in 2 public method, not returning any GUID. Line 137: private static final RowMapper<Pair<Guid, VmNumaNode>> vmNumaNodeInfoWithVdsGroupRowMapper = Line 138: new RowMapper<Pair<Guid, VmNumaNode>>() { Line 139: @Override Line 140: public Pair<Guid, VmNumaNode> mapRow(ResultSet rs, int rowNum) -- To view, visit https://gerrit.ovirt.org/38048 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I27a8f369e9896faeddd1e361ec884bf2c049b98f Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Dudi Maroshi <d...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Roy Golan <rgo...@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
