Change in vdsm[master]: ethtool_opts: Add hook for applying network custom property

danken at redhat.com danken at redhat.com
Thu Apr 17 21:45:29 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: ethtool_opts: Add hook for applying network custom property
......................................................................


Patch Set 1: Code-Review-1

(8 comments)

Partial review. I am mostly worried about the need to specify the net's nic, and to update them manually if they change. Not that I have a better idea.

http://gerrit.ovirt.org/#/c/26694/1/vdsm_hooks/ethtool_options/ethtool_options.py
File vdsm_hooks/ethtool_options/ethtool_options.py:

Line 42:                  'custom': ethtool_opts,
Line 43:                  'bootproto': 'dhcp', 'STP': 'no', 'bridged': 'true'}
Line 44: 
Line 45:     _validate_dev_ownership(nics, 'test_net',
Line 46:                             net_attrs['custom']['ethtool_opts'].split(' '))
please treat multiple spaces as a single whitespace - it's hard to be strict about this on the Engine side.
Line 47: 
Line 48: 
Line 49: def test():
Line 50:     opts = {'ethtool_opts':


Line 63:     # Test with a subset of the nics
Line 64:     nics = ('em1',)
Line 65:     try:
Line 66:         _test_cmd_with_nics(nics, opts)
Line 67:     except Exception:
it's only a test, but please catch a more precise exception. Current test considers a SyntaxError in  _parse_into_subcommands() as success.
Line 68:         print('ethtool options hook: Correctly rejected input %s for fake '
Line 69:               'nics %s' % (opts['ethtool_opts'], nics))
Line 70:     else:
Line 71:         raise ValueError('ethtool options hook: Incorrectly accepted input %s '


Line 72:                          'for fake nics %s' % (opts['ethtool_opts'], nics))
Line 73: 
Line 74: 
Line 75: def main():
Line 76:     """Read bridge_options from the network 'custom' properties and apply them
stale copy-paste
Line 77:     to the network's bridge."""
Line 78:     with open(os.environ['_hook_json']) as data_file:
Line 79:         setup_nets_config = json.load(data_file)
Line 80: 


Line 74: 
Line 75: def main():
Line 76:     """Read bridge_options from the network 'custom' properties and apply them
Line 77:     to the network's bridge."""
Line 78:     with open(os.environ['_hook_json']) as data_file:
use the recent hooking.read_json()
Line 79:         setup_nets_config = json.load(data_file)
Line 80: 
Line 81:     for network, attrs in setup_nets_config['request']['networks'].items():
Line 82:         if 'remove' in attrs:


Line 89:     """Applies ethtool_options to the network if necessary"""
Line 90:     options = attrs['custom'].get('ethtool_opts')
Line 91:     if options is not None:
Line 92:         tokens = options.split(' ')
Line 93:         _validate_dev_ownership(_net_nics(), network, tokens)
no args to _net_nics()??
Line 94:         _set_ethtool_opts(network, tokens)
Line 95: 
Line 96: 
Line 97: def _net_nics(attrs):


Line 101:         return [attrs.pop('nic')] if 'nic' in attrs else ()
Line 102: 
Line 103: 
Line 104: def _validate_dev_ownership(nics, name, tokens):
Line 105:     """Generator that takes ethtool cmdline arguments and yields Subcommands if
mismatching docstring. This function yields nothing.
Line 106:     all of them are valid"""
Line 107:     if not nics:
Line 108:         raise RuntimeError('Network %s has no nics.' % name)
Line 109: 


Line 125: 
Line 126: 
Line 127: def _set_ethtool_opts(network, options):
Line 128:     """Takes an iterable of the tokenized ethtool command line arguments and
Line 129:     applies them to the bridge"""
bridge->nic I suppose
Line 130:     command = [ETHTOOL_BINARY.cmd] + options
Line 131:     rc, _, err = hooking.execCmd(command, sudo=True)
Line 132:     if rc != 0:
Line 133:         raise RuntimeError('Failed to set ethtool opts (%s) for network %s' %


Line 127: def _set_ethtool_opts(network, options):
Line 128:     """Takes an iterable of the tokenized ethtool command line arguments and
Line 129:     applies them to the bridge"""
Line 130:     command = [ETHTOOL_BINARY.cmd] + options
Line 131:     rc, _, err = hooking.execCmd(command, sudo=True)
Please add ETHTOOL_BINARY to a hook-specific sudoers.d drop-in.
Line 132:     if rc != 0:
Line 133:         raise RuntimeError('Failed to set ethtool opts (%s) for network %s' %
Line 134:                            (' '.join(options), network))
Line 135: 


-- 
To view, visit http://gerrit.ovirt.org/26694
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41d89e00002e2b011bc1dc2825276d14bc99f577
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Lior Vernia <lvernia at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list