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