Edward Haas has posted comments on this change. Change subject: net: native ovs [5]: rollback trigger ......................................................................
Patch Set 8: Code-Review-1 (7 comments) Note that there are comments that have not been answered from the last time. Some are still relevant. https://gerrit.ovirt.org/#/c/55312/6/lib/vdsm/network/ovs/switch.py File lib/vdsm/network/ovs/switch.py: Line 52: except: Line 53: type, value, traceback = sys.exc_info() Line 54: if in_rollback: Line 55: logging.error('Failed rollback transaction to last known good ' Line 56: 'network.', exc_info=(type, value, traceback)) > it cannot be. with migration support it won't be possible to save and creat As discussed, we can do it by using a kind of clone constructor. Even the docstring states this nicely. Line 57: raise Line 58: else: Line 59: config_diff = RunningConfig().diffFrom(running_config) Line 60: if config_diff: https://gerrit.ovirt.org/#/c/55312/6/tests/network/ovs_test.py File tests/network/ovs_test.py: Line 171 Line 172 Line 173 Line 174 Line 175 > it tests if it is also save()d in the system. What is saved is checked in the next assertion, here it checks the staging data (in memory). If you want to check something like this directly, I would extract it to a separated test: A test that checks that the change is set in staging and another test that checks it was actually applied. PS6, Line 189: > https://upload.wikimedia.org/wikipedia/en/b/b9/MagrittePipe.jpg :) Don't we have an exception for doing that? If not, at least lets create one here with a nice name: MiscFailure or something similar. PS6, Line 194: : > AFAIK assert raises doesn't allow us to assert contents of an exception, so If an exception type is not catch, it is propagated upward. It enters the 'else' clause when NO exception has been detected. As this is a test, we should not explicitly raise an exception anyway, if we expect one we should assert on it, if we do not expect it, we should let it propagate up. https://gerrit.ovirt.org/#/c/55312/8/tests/network/ovs_test.py File tests/network/ovs_test.py: PS8, Line 61: ['eth0', 'eth1'] can you use here the same pattern as in the net tests? fake_running_bonds PS8, Line 92: _initialize_fake_config Where is it defined? Line 91: def test_successful_setup(self): Line 92: self._initialize_fake_config({}) Line 93: Line 94: with ovs_switch.rollback_trigger(in_rollback=False) as running_config: Line 95: running_config.setNetwork('net1', {'nic': 'eth1'}) This will break soon, when you actually implement the setup parts. You need to fake an additional method so it will not really get implemented. Line 96: Line 97: self.assertEquals(running_config.networks['net1']['nic'], 'eth1') Line 98: self.assertEquals( Line 99: netconfpersistence.RunningConfig().networks['net1']['nic'], 'eth1') -- To view, visit https://gerrit.ovirt.org/55312 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5bceae0aada964c47aacfcfdbe93ca19eb8f3d15 Gerrit-PatchSet: 8 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: Ondřej Svoboda <[email protected]> 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/mailman/listinfo/vdsm-patches
