Edward Haas has posted comments on this change. Change subject: net: introduce acquire module ......................................................................
Patch Set 16: (4 comments) https://gerrit.ovirt.org/#/c/60974/16/lib/vdsm/network/ifacquire.py File lib/vdsm/network/ifacquire.py: PS16, Line 59: netinfo_nets): : used_ports = frozenset(itertools.chain.from_iterable( : [attrs['ports'] for attrs in six.itervalues(netinfo_nets)])) Can be moved to init. used_ports can be called owned_ports. 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: > it cannot be part of the init, take a look at https://gerrit.ovirt.org/#/c/ I find 'prepare' and 'turn_down' hard to understand, it does not describe well what it does in relation to the acquire. How about something like this: with ifacquire.Transaction(netinfo) as a: do_somthing() a.acquire(iface) do_more() Regarding the 'disable_onboot': What will happen if setSafeNetworkConfig command will not arrive? I guess this means that we need to restore the ifcfg file to a 'release' state. Ok so how about applying the disable onboot and the context exit point (if no exceptions are detected)? 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): > "So I would recommend mocking only utils.atomic_write, checking that it is OK, I think I understand now... this with with made me dizzy. How about moving the mocks to the caller func, it will make it cleaner without the need of the double 'with' blocks. 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: Line 133: Line 134: @mock.patch.object(ifacquire.ifcfg, 'ifdown', return_value=None) Line 135: @mock.patch.object(ifacquire.os.path, 'isfile', return_value=True) Line 136: def test_rollback_acquired_ifcfg_nic(self, mock_isfile, mock_ifdown): Line 137: with mock_ifacquire_opens() as (open_handle, atomic_write_handle): I guess here it is even easier to get rid of the context. Line 138: open_handle.readlines.return_value = NIC_IFCFG Line 139: atomic_write_handle.readlines.return_value = NIC_IFCFG Line 140: Line 141: with self.assertRaises(TestException): -- 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 <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Edward Haas <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
