Petr Horáček has posted comments on this change.
Change subject: net: Link setup module - includes bond setup logic. ......................................................................
Patch Set 17: Code-Review-1
(8 comments)
https://gerrit.ovirt.org/#/c/62831/17//COMMIT_MSG Commit Message:
PS17, Line 16: clenup cleanup
https://gerrit.ovirt.org/#/c/62831/17/lib/vdsm/network/link/setup.py File lib/vdsm/network/link/setup.py:
PS17, Line 46: attrs can we create a to-be-configured bond object here and don't touch attrs anymore? it will require Bond.__init__ changes, so it won't import existing.
Line 52: # allow a proper rollback to occur in case of failure. Line 53: with Bond(init_bond.master) as bond: Line 54: slaves2remove = self._slaves2remove(bond.slaves, Line 55: frozenset(attrs['nics'])) Line 56: bond.del_slaves(slaves2remove) you need to update _config here. Line 57: Line 58: for (bond, attrs) in init_bond_pool: Line 59: with bond: Line 60: bond.refresh()
PS17, Line 58: (bond, attrs) parentheses are not needed
PS17, Line 58: bond in the previous iteration is is called init_bond
Line 61: requested_slaves = frozenset(attrs['nics']) Line 62: slaves2add = self._slaves2add(bond.slaves, requested_slaves) Line 63: bond.add_slaves(slaves2add) Line 64: Line 65: self._config.removeBonding(bond.master) we don't need to remove it if we set it. Line 66: self._config.setBonding(bond.master, attrs) Line 67: Line 68: bond.up() Line 69: _ip_flush(slaves2add)
https://gerrit.ovirt.org/#/c/62831/17/tests/network/link_setup_test.py File tests/network/link_setup_test.py:
Line 1: # remove this nasty extra # please Line 2: # Copyright 2016 Red Hat, Inc. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by
Line 85: setup_bonds.acquired_ifaces) Line 86: BondMock.assert_called_with(BOND1_NAME) Line 87: BondMock.return_value.add_slaves.assert_called_once_with(bond_slaves) Line 88: config_mock.removeBonding.assert_called_once_with(BOND1_NAME) Line 89: config_mock.setBonding.assert_called_once_with(BOND1_NAME, bond_attrs) can you please add a 'explode-in-the-middle' test here? setup some bonds, do some detaches during editations and then explode when trying to attach. test if _config is in consistent state (and therefore we are able to roll back). Line 90: Line 91: @staticmethod Line 92: def _assert_ip_flush_called(bond_slaves, dhclient_mock, address_mock): Line 93: for slave in bond_slaves: