Change in vdsm[master]: hooks: Add fcoe hook

pzhukov at redhat.com pzhukov at redhat.com
Mon Apr 11 12:02:19 UTC 2016


Pavel Zhukov has posted comments on this change.

Change subject: hooks: Add fcoe hook
......................................................................


Patch Set 20:

(11 comments)

https://gerrit.ovirt.org/#/c/55029/18/vdsm_hooks/fcoe/fcoe_before_network_setup.py
File vdsm_hooks/fcoe/fcoe_before_network_setup.py:

Line 38:     if not os.path.exists(filename):
Line 39:         copyfile(os.path.join(FCOE_CONFIG_DIR, FCOE_DEFAULT_CONFIG),
Line 40:                  filename)
Line 41:         utils.persist(filename)
Line 42: 
> We like to see nicely wrapped text in comments/docstrings.
Nir,
Thank you for explanation.
I know about gq. 
I prefer to have lines warped using "logic" rules not "counters". 

For example: 
# IOError can be raised here is FCOE_DEFAULT_CONFIG doesn't exist or we're not
# able to copy file

Doesn't look good, do it? But if it's so important I can close my eyes and use automatic tool.
Line 43: 
Line 44: def _unconfigure(interface):
Line 45:     """ Remove config file for specified interface """
Line 46:     if interface is None:


PS18, Line 90:         if net in configured:
> Since master. On 3.6, we need to use tobool.
I'd prefer to keep it just because it's more safe. Design mistake (not used value) doesn't guarantee it will not used in the future. In case if behavior will be changed you will silently break the hook with ("remove" : false) and fcoe will be removed for all interfaces with a lot of fun for users.


https://gerrit.ovirt.org/#/c/55029/20/vdsm_hooks/fcoe/fcoe_before_network_setup.py
File vdsm_hooks/fcoe/fcoe_before_network_setup.py:

Line 14: import six
Line 15: 
Line 16: import hooking
Line 17: from vdsm import utils
Line 18: from vdsm.netconfpersistence import RunningConfig
> Same, import modules instead of names from the modules.
Well. I'm not good programmer but I can use grep and I checked how more experience colleagues write the code:
14 modules use "from vdsm.netconfpersistence import RunningConfig" and only 2 use "import netconfpersistence" one of them has name "legacy_switch.py". 

The reason of importing entire module in these modules is very simple:
        return netconfpersistence
Line 19: 
Line 20: 
Line 21: FCOE_CONFIG_DIR = '/etc/fcoe/'
Line 22: # TODO this is default for EL7, Fedora.


Line 32: 
Line 33: def _configure(interface):
Line 34:     """ Enable FCoE on specified interface by coping default configuration """
Line 35:     if interface is None:
Line 36:         return
> Why interface can be None? This kind of code hides error in the calling cod
if bond network is removed (for example).
Line 37:     filename = os.path.join(FCOE_CONFIG_DIR, 'cfg-%s' % interface)
Line 38:     if not os.path.exists(filename):
Line 39:         copyfile(os.path.join(FCOE_CONFIG_DIR, FCOE_DEFAULT_CONFIG),
Line 40:                  filename)


Line 43: 
Line 44: def _unconfigure(interface):
Line 45:     """ Remove config file for specified interface """
Line 46:     if interface is None:
Line 47:         return
> Same
See above
Line 48:     filename = os.path.join(FCOE_CONFIG_DIR, 'cfg-%s' % (interface))
Line 49:     if os.path.exists(filename):
Line 50:         utils.rmFile(filename)
Line 51:         utils.unpersist(filename)


Line 65:     for net, net_attr in six.iteritems(config.networks):
Line 66:         if _has_fcoe_param(net_attr):
Line 67:             nic = configured[net].get('nic')
Line 68:             if nic is None:
Line 69:                 hooking.exit_hook("Failed to configure fcoe \
> your string still has redundant spaces.
Ack. I spent too much time with this long string :(
Line 70:                     on %s with no physical nic" % (net))
Line 71:             else:
Line 72:                 configured[net] = nic
Line 73: 


Line 75:     changed_networks = setup_nets_config['request']['networks']
Line 76: 
Line 77:     for net, net_attr in changed_networks:
Line 78:         if _has_fcoe_param(config.networks[net]):
Line 79:             changed_fcoe[net] = net_attr.get('nic')
> This is the root cause of having to handle None in _configure and _unconfig
We have to handle None anyway.
For example removed network can have None
Line 80:         elif hooking.tobool(net_attr.get('remove')):
Line 81:             removed_networks[net] = config.networks[net].get('nic')
Line 82:         else:
Line 83:             changed_non_fcoe[net] = net_attr.get('nic')


Line 77:     for net, net_attr in changed_networks:
Line 78:         if _has_fcoe_param(config.networks[net]):
Line 79:             changed_fcoe[net] = net_attr.get('nic')
Line 80:         elif hooking.tobool(net_attr.get('remove')):
Line 81:             removed_networks[net] = config.networks[net].get('nic')
> Is it possible to have config.networks[net] without a 'nic'?
It's possible. It's even possible to have two different 'nic' for same network in config.networks[net] and changed_networks[net]. As well as it's possible to have one of them None and second one not None.
Line 82:         else:
Line 83:             changed_non_fcoe[net] = net_attr.get('nic')
Line 84: 
Line 85:     for net, net_nic in six.iteritems(removed_networks):


Line 83:             changed_non_fcoe[net] = net_attr.get('nic')
Line 84: 
Line 85:     for net, net_nic in six.iteritems(removed_networks):
Line 86:         if net in configured:
Line 87:             _unconfigure(config.networks[net].get('nic'))
> Use net_nic
Net_nic is none here
Line 88: 
Line 89:     for net, net_nic in six.iteritems(changed_non_fcoe):
Line 90:         if net in configured:
Line 91:             _unconfigure(net_nic)


Line 98:     # TODO If services are failed to start restore previous configuration
Line 99:     # and notify user
Line 100:     ret, _, err = hooking.execCmd(['/bin/systemctl', 'restart', 'lldpad'])
Line 101:     if ret:
Line 102:         hooking.log('Failed to restart lldpad service. err = %s' % (err))
> Can we reload the service instead?
Configuration guide says we should use restart.
Line 103: 
Line 104:     ret, _, err = hooking.execCmd(['/bin/systemctl', 'restart', 'fcoe'])
Line 105:     if ret:
Line 106:         hooking.log('Failed to restart fcoe service. err = %s' % (err))


Line 102:         hooking.log('Failed to restart lldpad service. err = %s' % (err))
Line 103: 
Line 104:     ret, _, err = hooking.execCmd(['/bin/systemctl', 'restart', 'fcoe'])
Line 105:     if ret:
Line 106:         hooking.log('Failed to restart fcoe service. err = %s' % (err))
> Can we reload the service instead?
same
Line 107: 
Line 108: if __name__ == '__main__':
Line 109:     try:
Line 110:         main()


-- 
To view, visit https://gerrit.ovirt.org/55029
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad2faed7205ca08801132df072b469dbe781318c
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Pavel Zhukov <pzhukov at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Pavel Zhukov <pzhukov at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list