Antoni Segura Puimedon has posted comments on this change.
Change subject: ethtool_opts: Add hook for applying network custom property ......................................................................
Patch Set 1:
(8 comments)
http://gerrit.ovirt.org/#/c/26694/1/vdsm_hooks/ethtool_options/ethtool_optio... 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 stric
Done 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 c
Done 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
Done 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()
Done 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()??
Done 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.
Done 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
Done 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.
Done 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: