Dan Kenigsberg has posted comments on this change. Change subject: network: persist SR-IOV devices' number of VFs ......................................................................
Patch Set 24: Code-Review-1 (9 comments) few style comments and one blunt mistake (or so I believe) https://gerrit.ovirt.org/#/c/40088/24//COMMIT_MSG Commit Message: Line 13: collect garbage. > What is the nature of the garbage, why is it important to mention it? Is is I believe that the GC was dropped in a former review https://gerrit.ovirt.org/#/c/40088/24/vdsm/API.py File vdsm/API.py: Line 1305: self._cif._netConfigDirty = True > Why is the config being marked as dirty? It is a means to notify Engine that the host now has yet-unpersisted configuration. If it is not persisted by setSafeNetworkConfig verb, it would not show up on the next boot. https://gerrit.ovirt.org/#/c/40088/24/vdsm/vdsm-restore-net-config File vdsm/vdsm-restore-net-config: Line 55: def _get_persisted_numvfs(existing_sriov_devices): Line 56: numvfs_by_device = {} Line 57: Line 58: if not os.path.exists(_VIRTUAL_FUNCTIONS_PATH): Line 59: os.makedirs(_VIRTUAL_FUNCTIONS_PATH) why do we need to create the directory here? it is awkward for a _get*() function to do so. Line 60: return Line 61: Line 62: for file_name in os.listdir(_VIRTUAL_FUNCTIONS_PATH): Line 63: if file_name not in existing_sriov_devices: Line 56: numvfs_by_device = {} Line 57: Line 58: if not os.path.exists(_VIRTUAL_FUNCTIONS_PATH): Line 59: os.makedirs(_VIRTUAL_FUNCTIONS_PATH) Line 60: return return None? I suspect the caller expect a dictionary always. Line 61: Line 62: for file_name in os.listdir(_VIRTUAL_FUNCTIONS_PATH): Line 63: if file_name not in existing_sriov_devices: Line 64: logging.error('Physical device in %s no longer exists. Skipping ' Line 64: in I don't think that "in" makes sense grammatically - maybe I don't understand what you tried to say. Line 76: persisted_numvfs = _get_persisted_numvfs(sriov_devices) Line 77: Line 78: for device in sriov_devices: Line 79: desired_numvfs = persisted_numvfs.get(device) Line 80: if desired_numvfs is not None: please reverse the condition. Positive logic is more readable. Line 81: logging.info('Changing number of virtual functions for device %s ' Line 82: '-> %s', device, desired_numvfs) Line 83: else: Line 84: logging.info('SRIOV network device which is not persisted found ' Line 80: if desired_numvfs is not None: Line 81: logging.info('Changing number of virtual functions for device %s ' Line 82: '-> %s', device, desired_numvfs) Line 83: else: Line 84: logging.info('SRIOV network device which is not persisted found ' Do we really want to get an INFO for every PF that has no persistence? why? Iterating the items of persisted_numvfs seems like the natural action. Line 85: 'at: %s.', device) Line 86: continue Line 87: try: Line 88: change_numvfs(device, desired_numvfs) Line 82: '-> %s', device, desired_numvfs) Line 83: else: Line 84: logging.info('SRIOV network device which is not persisted found ' Line 85: 'at: %s.', device) Line 86: continue I like short-circuiting with continue, but in this case, I think that condition would be simpler if change_numvfs() is called on the relevant branch. Line 87: try: Line 88: change_numvfs(device, desired_numvfs) Line 89: except: Line 90: logging.exception('Restoring VF configuration for device %s ' Line 250: def restore(args): Line 251: if not args.force and _nets_already_restored(_NETS_RESTORED_MARK): Line 252: logging.info('networks already restored. doing nothing.') Line 253: return Line 254: _restore_sriov_numvfs() can be placed here, only once. Its usage is unrelated to ifcfg/unified persistence. Line 255: unified = config.get('vars', 'net_persistence') == 'unified' Line 256: logging.info('starting network restoration.') Line 257: try: Line 258: if unified: -- To view, visit https://gerrit.ovirt.org/40088 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I76b898019840ffe65939ffad4a1e98829ad3c887 Gerrit-PatchSet: 24 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ido Barkan <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
