Edward Haas has posted comments on this change.
Change subject: net: Link setup module - includes bond setup logic.
......................................................................
Patch Set 17:
(8 comments)
https://gerrit.ovirt.org/#/c/62831/17//COMMIT_MSG
Commit Message:
PS17, Line 16: clenup
cleanup
Done
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 any
I don't think so, but perhaps you have a proposal for all this mess
here.
First, I think I should write a good explanation for this method as it is not trivial at
all and uses some tricks and has some limitations.
Let me know if it is more clear after this explanation:
Editing bonds requires special treatment due to the required steps to perform editation on
a bulk of bonds.
To support scenarios of moving slaves between bonds, the following algorithm has been
chosen: Go over all bonds twice, once to remove slaves and secondly to add slaves.
When we split the bond editing like that, we break a single bond transaction into two
parts.
If we assume that the removal always succeeds, then we can do this:
- Create a Bond object to preserve initial state.
- Process bonds slaves removal, but not through the initial bond object created in the
previous step (to preserve the initial state intact).
- Process the bonds slaves adding using step1 bond object, so if it will fail, it can
revert to the initial state.
- For each bond in the second process, on success, save it in running config.
So the limitation we are left with is in a case of removal failure, all previous handled
bonds will not be reverted as well (only the current failing one).
Perhaps they can be handled on failure using the initial bond objects, I am not sure.
I'm not even clear if failing on slave removal is something that can happen, but that
is not a good argument.
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.
This is a limitation, see
response above.
If we save here and fail on adding slaves, the running config will not represent the
correct config and we cannot even recover from a case where only a partial editing of a
bond has occurred.
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
Done
PS17, Line 58: bond
in the previous iteration is is called init_bond
Indeed, but
just because I needed the name of the bond. The bond object used was still
'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.
Done
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
Done
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, d
This is a tricky as it is dependent on the Bond driver functionality
of reverting to init state if its transaction fails. Such a test will be a bit more than a
unit tests and a bit less than an integration test.
I think we do have it covered in the functional tests.
To summarize, if you think we should cover the messy edit logic in a unit test, we will
need to depend on the specific bond driver, mock its file access and check the integration
between SetupBonds and BondSysFS.
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:
--
To view, visit
https://gerrit.ovirt.org/62831
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f01a401cb1b96e357bc462e528a2a547c59c24
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes