Petr Horáček has posted comments on this change. Change subject: net: introduce acquire module ......................................................................
Patch Set 16: (2 comments) https://gerrit.ovirt.org/#/c/60974/16/tests/network/ifacquire_test.py File tests/network/ifacquire_test.py: Line 69: @mock.patch.object(ifacquire.ifcfg, 'ifdown') Line 70: @mock.patch.object(ifacquire, 'open', create=True) Line 71: def test_do_not_acquire_owned_nic(self, mock_open, mock_ifdown, mock_kill, Line 72: mock_flush): Line 73: with ifacquire.Transaction() as a: > I'm not clear of why it is split between prepare and turn_down. Couldn't th it cannot be part of the init, take a look at https://gerrit.ovirt.org/#/c/60404/28/lib/vdsm/network/netswitch.py. we must open Setup inside ifacquire.Transaction (if we want to call disable_onboot from that), but setup.acquired_ifaces are not available back then. prepare can be part of turn_down, but then we must say, that turn_down must be called before disable_onboot. i will change it if you prefer it. i like separate prepare as 'postponed __init__'. i think it fits here well. this is the last point of network setup (except post_setup_hook which must not fail) before we finish setupNetworks. if it fail now, we are still able to rollback preexisting networking configuration. if we move it outside it it won't save our networking after reboot -- nic would be attached to OVS and even with IP on top it it wouldn't be able to reach outside network. Line 74: a.prepare(ifaces=[NIC_NAME], netinfo_nets=NETINFO_NETS) Line 75: a.turn_down() Line 76: a.disable_onboot() Line 77: Line 112: def _test_acquire_ifcfg_nic(self, mock_isfile, mock_ifdown, Line 113: original_ifcfg, Line 114: ifcfg_after_turn_down, Line 115: ifcfg_after_disable_onboot): Line 116: with mock_ifacquire_opens() as (open_handle, atomic_write_handle): > I do not understand why you are testing the atomic write. "So I would recommend mocking only utils.atomic_write, checking that it is called with." i think that is what i do now, but i have to mock open as well because of it is used to backup original ifcfg. i mock open and atomic_write (in contextmanager, since i wanted to extract double with statement and handles) and assert if we called expected methods. Line 117: open_handle.readlines.return_value = original_ifcfg Line 118: atomic_write_handle.readlines.return_value = original_ifcfg Line 119: Line 120: with ifacquire.Transaction() as a: -- To view, visit https://gerrit.ovirt.org/60974 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I180cfd7d69c0ae0a24188bc3d909b9d3d7c12145 Gerrit-PatchSet: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <phora...@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
