Martin Betak has posted comments on this change. Change subject: backend: Add HostDev passthrough support #1 ......................................................................
Patch Set 12: (12 comments) http://gerrit.ovirt.org/#/c/35892/12//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-12-03 17:09:16 +0100 Line 4: Commit: Martin Betak <mbe...@redhat.com> Line 5: CommitDate: 2015-01-15 16:25:53 +0100 Line 6: Line 7: backend: Add HostDev passthrough support #1 > what does the '#1' stands for? just that this patch is the first in the PCI series, I don't have any follow-up patches due to lack of vdsm impl at the moment though, I can drop it for now Line 8: Line 9: Preliminary steps for providing the ability to pass through Line 10: selected devices from Hosts to VMs. Line 11: http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandsFactory.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandsFactory.java: Line 135: (QueriesCommandBase<?>) findCommandConstructor(type, parameters.getClass(), EngineContext.class).newInstance(parameters, Line 136: engineContext); Line 137: Line 138: } Line 139: Injector.injectMembers(result); > how about replacing it with "return Injector.injectMembers(result);" in ord Done Line 140: return result; Line 141: } catch (Exception e) { Line 142: log.error("Command Factory: Failed to create command '{}' using reflection: {}", type, e.getMessage()); Line 143: log.error("Exception", e); http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/RefreshHostDevicesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/RefreshHostDevicesCommand.java: Line 18: import java.util.HashMap; Line 19: import java.util.List; Line 20: import java.util.Map; Line 21: Line 22: @NonTransactiveCommandAttribute(forceCompensation = true) > why is compensation needed? If I understand correctly the transaction at line #65 needs compensation since it alters database state after it issues the VDS command. Line 23: public class RefreshHostDevicesCommand<T extends VdsActionParameters> extends RefreshHostInfoCommandBase<T> { Line 24: Line 25: @Inject Line 26: private ResourceManager resourceManager; Line 46: Line 47: for (Map.Entry<String, HostDevice> entry : fetchedMap.entrySet()) { Line 48: HostDevice device = entry.getValue(); Line 49: if (oldMap.containsKey(entry.getKey())) { Line 50: if (!oldMap.get(entry.getKey()).equals(device)) { > shouldn't it be !oldMap.get(entry.getValue()).equals(device)? `entry` iterates over new map and device = entry.getValue() by definition :-) Line 51: changedDevices.add(device); Line 52: } Line 53: } else { Line 54: newDevices.add(device); Line 52: } Line 53: } else { Line 54: newDevices.add(device); Line 55: } Line 56: } > how about extracting all this piece of code to method like 'deviceIntersect Not sure how do you want this to look like, but consider that this "device" list can contain thousands of items (its not just devices in regular sense but whole tree of any little piece of sensor attached to PCI, you can look at fixtures.xml data that was generated from real world tree of a fairly small host). Line 57: Line 58: final List<HostDevice> removedDevices = new ArrayList<>(); Line 59: for (Map.Entry<String, HostDevice> entry : oldMap.entrySet()) { Line 60: if (!fetchedMap.containsKey(entry.getKey())) { http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/HostDevice.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/HostDevice.java: Line 4: import org.ovirt.engine.core.compat.Guid; Line 5: Line 6: public class HostDevice implements BusinessEntity<HostDeviceId> { Line 7: Line 8: public static final Integer NO_IOMMU_GROUP = null; > please add a comment that explains why this field is 'public static' and wh Remnant of old code, removed. Line 9: Line 10: private Guid hostId; Line 11: private String deviceName; Line 12: private String parentDeviceName; http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/HostDeviceParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/HostDeviceParameters.java: Line 1: package org.ovirt.engine.core.common.queries; Line 2: Line 3: import org.ovirt.engine.core.compat.Guid; Line 4: Line 5: public class HostDeviceParameters extends VdcQueryParametersBase { > how about rename it to GetHostDeviceByHostIdAndDeviceNameParameters if it i The tuple (host_id, device_name) represents primary key of the HostDevice (see HostDeviceId) thus all commands manipulating HostDevice (update/attach/remove/hotplug) will need this information and thus derive from this base parameters. Line 6: Line 7: private Guid hostId; Line 8: private String deviceName; Line 9: http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsDao.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsDao.java: Line 41: * @param ids Line 42: */ Line 43: void removeAll(Collection<ID> ids); Line 44: Line 45: // TODO: batch remove all should probably use only IDs (and id parameter source) as its non-batch counterpart > please put it inside the the method's documentation comment Done Line 46: /** Line 47: * Calls a remove stored procedure multiple times in a batch Line 48: * Line 49: * @param entities Line 62: * Calls an insert stored procedure multiple times in a batch Line 63: * Line 64: * @param entities Line 65: */ Line 66: void saveAll(Collection<T> entities); > the comment is wrong, should be for the method below Done Line 67: Line 68: /** Line 69: * Saves the given entities using a more efficient method to save all of them at once, rather than each at a time. Line 70: * The procedure name to be used is "InsertEntityName" where EntityName stands for the name of the entity http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java: Line 1855: List<HostDevice> devices = new ArrayList<>(); Line 1856: Line 1857: for (Entry<String, Map<String, Object>> entry : deviceList.entrySet()) { Line 1858: Line 1859: Map<String, Object> params = (Map<String, Object>) entry.getValue().get(VdsProperties.PARAMS); > can't we define deviceList as: Done Line 1860: String deviceName = entry.getKey(); Line 1861: Line 1862: HostDevice device = new HostDevice(); Line 1863: device.setDeviceName(entry.getKey()); http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsProperties.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsProperties.java: Line 385: public static final String QOS_PEAK = "peak"; Line 386: public static final String QOS_BURST = "burst"; Line 387: Line 388: // host devices Line 389: public static final String ROOT_DEVICE = "computer"; > how about rename to ROOT_HOST_DEVICE or something like that to make its nam Done Line 390: public static final String DEVICE_LIST = "deviceList"; Line 391: public static final String PARAMS = "params"; Line 392: public static final String CAPABILITY = "capability"; Line 393: public static final String IOMMU_GROUP = "iommu_group"; http://gerrit.ovirt.org/#/c/35892/12/packaging/dbscripts/host_device_sp.sql File packaging/dbscripts/host_device_sp.sql: Line 112: WHERE host_id = v_host_id AND device_name = v_device_name; Line 113: END; $procedure$ Line 114: LANGUAGE plpgsql; Line 115: Line 116: CREATE OR REPLACE FUNCTION GetAllFromDevices() > +1 Done Line 117: RETURNS SETOF host_device STABLE Line 118: AS $procedure$ Line 119: BEGIN Line 120: RETURN QUERY -- To view, visit http://gerrit.ovirt.org/35892 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia5575c0db797d7d04339c4b309bb4325e853ffed Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
