Change in vdsm[master]: network: persist SR-IOV devices' number of VFs

ibarkan at redhat.com ibarkan at redhat.com
Mon Jun 1 06:40:03 UTC 2015


Ido Barkan has posted comments on this change.

Change subject: network: persist SR-IOV devices' number of VFs
......................................................................


Patch Set 24:

(8 comments)

https://gerrit.ovirt.org/#/c/40088/24//COMMIT_MSG
Commit Message:

Line 13: collect garbage.
> I believe that the GC was dropped in a former review
rewarded commit msg


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*() fu
I agree. But, this code usually runs first on a server without the directory structure in place. Alternatives are:
 1. fail silently here and count on the creation in network/api.py
 2. create the directory during rpm installation (yak).
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.
Done
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 understan
well, file_name is a pci address so 'in' made sense to me as 'device in address' but I guess I could drop the in as well.


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.
Done
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?
Personally, I prefer too much logging than littler less during restoration. This area is hairy and has an infamous history of being mysterious and hard to reproduce bug producer.
Or in other words, let me be paranoid here.
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 condi
done
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 unrelat
Done
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 <ibarkan at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list