Omer Frenkel has posted comments on this change. Change subject: backend: Add HostDev passthrough support #3 ......................................................................
Patch Set 20: (7 comments) do we need to consider the devices in the ovf? (import/export/snapshots) https://gerrit.ovirt.org/#/c/37619/20/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java: Line 1896: } Line 1897: } Line 1898: Line 1899: /** hook for subclasses that hold additional custom locks */ Line 1900: protected void freeUnmanagedLocks() { i would call it freeCustomLocks Line 1901: } Line 1902: Line 1903: protected LockManager getLockManager() { Line 1904: return LockManagerFactory.getLockManager(); https://gerrit.ovirt.org/#/c/37619/20/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmHostDeviceByVmIdAndDeviceNameQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmHostDeviceByVmIdAndDeviceNameQuery.java: Line 21: @Override Line 22: protected void executeQueryCommand() { Line 23: List<VmDevice> vmDevices = vmDeviceDAO.getVmDeviceByVmIdTypeAndDevice(getParameters().getId(), Line 24: VmDeviceGeneralType.HOSTDEV, getParameters().getDeviceName()); Line 25: if (vmDevices != null && vmDevices.size() > 0) { please use !isEmpty() Line 26: setReturnValue(new VmHostDevice(vmDevices.get(0))); Line 27: } Line 28: } https://gerrit.ovirt.org/#/c/37619/20/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java: Line 239: try { Line 240: if (connectLunDisks(getVdsId())) { Line 241: status = createVm(); Line 242: ExecutionHandler.setAsyncJob(getExecutionContext(), true); Line 243: markHostDevicesAsUsed(); in case createVm() failed with no exception, we will not free the device from the vm? Line 244: } Line 245: } catch(VdcBLLException e) { Line 246: // if the returned exception is such that shoudn't trigger the re-run process, Line 247: // re-throw it. otherwise, continue (the vm will be down and a re-run will be triggered) Line 314: public void rerun() { Line 315: setFlow(null); Line 316: // re-acquire the host device lock (if needed) as the canDoAction already expects this Line 317: // lock to be held (originally acquired in 'postConstruct' Line 318: acquireHostDevicesLock(); is it ok that the device already marked as used by this vm? Line 319: super.rerun(); Line 320: } Line 321: Line 322: private RunVmFlow setFlow(RunVmFlow flow) { https://gerrit.ovirt.org/#/c/37619/20/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/AbstractVmHostDevicesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/AbstractVmHostDevicesCommand.java: Line 99: } Line 100: Line 101: private Collection<HostDevice> getDeviceAtomicGroup(HostDevice hostDevice) { Line 102: // iommu group restriction only applicable to 'pci' devices Line 103: if (!"pci".equals(hostDevice.getCapability())) { please make this a constant Line 104: return Collections.singleton(hostDevice); Line 105: } Line 106: Line 107: return hostDeviceDao.getHostDevicesByHostIdAndIommuGroup(getVm().getDedicatedVmForVds(), https://gerrit.ovirt.org/#/c/37619/20/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/HostDeviceManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/HostDeviceManager.java: Line 81: public void releaseHostDevicesLock(Guid vdsId) { Line 82: lockManager.releaseLock(new EngineLock(getExclusiveLockForHostDevices(vdsId))); Line 83: } Line 84: Line 85: private static Map<String, Pair<String, String>> getExclusiveLockForHostDevices(Guid vdsId) { why static? Line 86: return Collections.singletonMap( Line 87: vdsId.toString(), Line 88: LockMessagesMatchUtil.makeLockingPair( Line 89: LockingGroup.HOST_DEVICES, Line 86: return Collections.singletonMap( Line 87: vdsId.toString(), Line 88: LockMessagesMatchUtil.makeLockingPair( Line 89: LockingGroup.HOST_DEVICES, Line 90: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); consider adding more descriptive message for the lock and not use the default one Line 91: } Line 92: Line 93: private static boolean supportsHostDevicePassthrough(VM vm) { Line 94: return FeatureSupported.hostDevicePassthrough(vm.getVdsGroupCompatibilityVersion()); -- To view, visit https://gerrit.ovirt.org/37619 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93c746cdda71678f7840d37683b890080a74341d Gerrit-PatchSet: 20 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Martin Betak <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
