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

pzhukov at redhat.com pzhukov at redhat.com
Tue May 10 14:02:40 UTC 2016


Pavel Zhukov has posted comments on this change.

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


Patch Set 20:

(9 comments)

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 8: 
Line 9: 
Line 10: import os
Line 11: import traceback
Line 12: from shutil import copyfile
> Dont import functions from module, it make it hard to understand what is th
ok
Line 13: 
Line 14: import six
Line 15: 
Line 16: import hooking


Line 20: 
Line 21: FCOE_CONFIG_DIR = '/etc/fcoe/'
Line 22: # TODO this is default for EL7, Fedora.
Line 23: # Debian, EL6 etc should be checked
Line 24: FCOE_DEFAULT_CONFIG = 'cfg-ethx'
> +1
ack
Line 25: 
Line 26: 
Line 27: def _has_fcoe_param(net_attr):
Line 28:     """ Check if fcoe parameter was specified as custom network property """


Line 26: 
Line 27: def _has_fcoe_param(net_attr):
Line 28:     """ Check if fcoe parameter was specified as custom network property """
Line 29:     return (net_attr.get('custom')
Line 30:             and hooking.tobool(net_attr['custom'].get('fcoe')))
> You can simplify this to:
Ack. thanks
Line 31: 
Line 32: 
Line 33: def _configure(interface):
Line 34:     """ Enable FCoE on specified interface by coping default configuration """


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
> if bond network is removed (for example).
moved checks level up
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 33: def _configure(interface):
Line 34:     """ Enable FCoE on specified interface by coping default configuration """
Line 35:     if interface is None:
Line 36:         return
Line 37:     filename = os.path.join(FCOE_CONFIG_DIR, 'cfg-%s' % interface)
> We repeat this code everywhere, lets make a helper to return the path:
Two times only but ok
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 35:     if interface is None:
Line 36:         return
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),
> If you use my suggestion about  FCOE_DEFAULT_CONFIG, we can do now:
ack
Line 40:                  filename)
Line 41:         utils.persist(filename)
Line 42: 
Line 43: 


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 41:         utils.persist(filename)
> What if the file exists but the contents are incorrect? for example, the co
Nope. We have pending task to change content of the file. it will be needed for some FCoE adapters. We cannot protect file from being overwritten by user anyway.
Line 42: 
Line 43: 
Line 44: def _unconfigure(interface):
Line 45:     """ Remove config file for specified interface """


Line 63: 
Line 64:     config = RunningConfig()
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')
> +1
ack. copy=paste mistake
Line 68:             if nic is None:
Line 69:                 hooking.exit_hook("Failed to configure fcoe \
Line 70:                     on %s with no physical nic" % (net))
Line 71:             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 86:         if net in configured:
Line 87:             _unconfigure(config.networks[net].get('nic'))
> You man it can be None, and we still need to unconfigure it?
We have to unconfigure "existing" network which has name listed in "removed" while nic is none in "removed" json. I don't know why it works like that
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)


-- 
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: Fabian Deutsch <fabiand 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