Edward Haas has posted comments on this change. Change subject: net: Link setup module - includes bond setup logic. ......................................................................
Patch Set 21: (7 comments) https://gerrit.ovirt.org/#/c/62831/22/lib/vdsm/network/link/setup.py File lib/vdsm/network/link/setup.py: PS22, Line 53: ume th > slaves Done PS22, Line 57: e previous step (to prese > is it needed with this approach? we should be in consistent state after eve It is preferred over depending on the running config update. When implemented, we won't need to update running config twice. Moreover, we could execute a save only in the transaction exit point, splitting that admin logic from the bonding parts. PS22, Line 67: > attrs contains the final state. we must save only currently attached slaves Done PS22, Line 80: frozenset(attrs['nics'])) : bond.del_slaves(slaves > please move this before final setBonding() so it is obvious that we save at Done PS22, Line 95: # TODO: Options : # attrs.get('options', '') > move this as well to be consistent. Done https://gerrit.ovirt.org/#/c/62831/21/tests/network/link_setup_test.py File tests/network/link_setup_test.py: PS21, Line 47: set > frozen It's just for the assert, I do not think we need to do this here. PS21, Line 49: set > frozen We expect set in the production code, as the slaves are editable (can be added and removed). -- 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: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Petr Horáček <phora...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
