Ido Barkan has posted comments on this change. Change subject: network: restore sriov devices number of vfs ......................................................................
Patch Set 6: (7 comments) Martin, about your concern. SRIOV is currently a network feature, both in engine and in VDSM. Since this is not a lot of code and since restoration is also currently a network related issue, lets sacrifice generality for simplicity. It will not be hard to move things around in the future, when we need to support non-network SRIOV devices. https://gerrit.ovirt.org/#/c/40088/6/vdsm/network/api.py File vdsm/network/api.py: Line 557: Since calling this verb is currently blocked by engine if this device is Line 558: already in use, the logic does not attempt to do anything further, such as Line 559: restoring possibly lost networks. Line 560: The persistence is stored in a place which is writable also in ovirt-node. Line 561: Garbage is will be collected during restoration. > type: just will Done Line 562: """ Line 563: logger = logging.getLogger("changeNumvfs") Line 564: original_value = _get_num_vfs(device_name) Line 565: logger.info("changing number of vfs on device %s to %s.", Line 562: """ Line 563: logger = logging.getLogger("changeNumvfs") Line 564: original_value = _get_num_vfs(device_name) Line 565: logger.info("changing number of vfs on device %s to %s.", Line 566: device_name, num_vfs) > Is there a reason not to use format here when it is used in later in _get_n Not sure I understand. Do you mean using _SYSFS_SRIOV_NUMVFS.format(device_name) in the log message? Line 567: _update_num_vfs(device_name, num_vfs) Line 568: if not clientSeen(CONNECTIVITY_TIMEOUT_DEFAULT): Line 569: _update_num_vfs(device_name, original_value) Line 570: logger.error("changing number of vfs on device %s failed during " https://gerrit.ovirt.org/#/c/40088/6/vdsm/vdsm-restore-net-config File vdsm/vdsm-restore-net-config: Line 50: if 'totalvfs' in device['params']] Line 51: Line 52: Line 53: def _get_persisted_numvfs(existing_sriov_devices): Line 54: num_vfs_by_device = {} > minor: I'd rather see numvfs as elsewhere Done Line 55: missing_devices = [] # devices that no longer exist Line 56: Line 57: if not os.path.exists(_VIRTUAL_FUNCTIONS_PATH): Line 58: os.makedirs(_VIRTUAL_FUNCTIONS_PATH) Line 54: num_vfs_by_device = {} Line 55: missing_devices = [] # devices that no longer exist Line 56: Line 57: if not os.path.exists(_VIRTUAL_FUNCTIONS_PATH): Line 58: os.makedirs(_VIRTUAL_FUNCTIONS_PATH) > Return here would save branch that is guaranteed to not find anything well, the for will iterate nothing but +1 for readability (and saving a syscall :-) ). Line 59: Line 60: for file_name in os.listdir(_VIRTUAL_FUNCTIONS_PATH): Line 61: if file_name not in existing_sriov_devices: Line 62: missing_devices.append(file_name) Line 76: Line 77: Line 78: def _restore_sriov_num_vfs(): Line 79: sriov_devices = _get_sriov_devices() Line 80: persisted_num_vfs, missing_devices = _get_persisted_numvfs(sriov_devices) > Same minor issue with numvfs and numvfs (very visible inconsistence here) Done Line 81: Line 82: for device in sriov_devices: Line 83: desired_num_vfs = persisted_num_vfs.get(device) Line 84: if desired_num_vfs is not None: Line 79: sriov_devices = _get_sriov_devices() Line 80: persisted_num_vfs, missing_devices = _get_persisted_numvfs(sriov_devices) Line 81: Line 82: for device in sriov_devices: Line 83: desired_num_vfs = persisted_num_vfs.get(device) > As above Done Line 84: if desired_num_vfs is not None: Line 85: logging.info('Changing number of virtual functions for device %s ' Line 86: 'to %s', device, desired_num_vfs) Line 87: else: Line 84: if desired_num_vfs is not None: Line 85: logging.info('Changing number of virtual functions for device %s ' Line 86: 'to %s', device, desired_num_vfs) Line 87: else: Line 88: logging.info('an SRIOV network device which is not persisted found' > typo: an -> a, additional question: is the else branch logging required? I believe so. Boot time part of VDSM is infamous of low predictability, especially when restoration takes place, and I would like us (and administrators) to be able to analyze what happened easily. Line 89: 'at: %s.', device) Line 90: continue Line 91: Line 92: changeNumvfs(device, desired_num_vfs) -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ido Barkan <ibar...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Ido Barkan <ibar...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Ondřej Svoboda <osvob...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
