Martin Betak has posted comments on this change. Change subject: backend: Add HostDev passthrough support #3 ......................................................................
Patch Set 20: (6 comments) 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 Done 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() Done 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 fr Isn't ProceedDownVmCommand called in such cases? All hostdev deallocation is done there. 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) 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 Done 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? It is just a helper that doesn't access instance fields. I was always of the opinion that these things should be static unless you need to override them for test purposes or the like. 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 defau Well since we are using this lock in "waiting" mode the message should never present itself. For this reason I consider the default to be appropriate - unless I can put null in there, but I'm not sure about that. 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: Moti Asayag <[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
