Edward Haas has posted comments on this change. Change subject: net: native ovs [5]: rollback trigger ......................................................................
Patch Set 6: Code-Review-1 (9 comments) https://gerrit.ovirt.org/#/c/55312/6/lib/vdsm/network/ovs/switch.py File lib/vdsm/network/ovs/switch.py: Line 52: validator.validate_bond_configuration(attrs, _netinfo) Line 53: Line 54: Line 55: @contextmanager Line 56: def rollback_trigger(in_rollback): Looking at this contextmanager here, it seems to me that it fits to be part of RunningConfig itself. All it does is reading, saving and comparing the configuration. with RunningConfig(in_rollback) rconfig: ... Line 57: """Keep running_config and in case of an exception, trigger rollback.""" Line 58: running_config = RunningConfig() Line 59: try: Line 60: yield running_config https://gerrit.ovirt.org/#/c/55312/6/tests/network/ovs_test.py File tests/network/ovs_test.py: PS6, Line 162: netconfpersistence I guess RunningConfig alone will be nicer. PS6, Line 168: fake_config = netconfpersistence.RunningConfig() : fake_config.networks = {} : fake_config.save() Please extract to a helper method Something like: initialize_config Line 171: Line 172: with ovs_switch.rollback_trigger(in_rollback=False) as running_config: Line 173: running_config.setNetwork('net1', {'nic': 'eth1'}) Line 174: Line 175: self.assertEquals(running_config.networks['net1']['nic'], 'eth1') Why this intermediate test is needed if the end result is the assert from the next line? Line 176: self.assertEquals( Line 177: netconfpersistence.RunningConfig().networks['net1']['nic'], 'eth1') Line 178: Line 179: def test_failed_setup(self): Line 173: running_config.setNetwork('net1', {'nic': 'eth1'}) Line 174: Line 175: self.assertEquals(running_config.networks['net1']['nic'], 'eth1') Line 176: self.assertEquals( Line 177: netconfpersistence.RunningConfig().networks['net1']['nic'], 'eth1') You could have checked something stronger here: That the setup dict equals what was read as a whole. Line 178: Line 179: def test_failed_setup(self): Line 180: fake_config = netconfpersistence.RunningConfig() Line 181: fake_config.networks = {'net1': {'nic': 'eth0'}} PS6, Line 189: raise Exception('This is not a test.') ? PS6, Line 192: self.assertEquals(diff.networks['net1']['nic'], 'eth0') : self.assertEquals(diff.networks['net2'], {'remove': True}) I would prefer to see the whole setup networks request, not a specific value in the dict. PS6, Line 194: else: : raise Why is this needed? Line 196: Line 197: self.assertEquals( Line 198: netconfpersistence.RunningConfig().networks['net1']['nic'], 'eth0') Line 199: Line 200: def test_failed_setup_in_rollback(self): The interesting part is missing from this test: Showing that if we fail in the middle, the configuration is NOT restored. Line 201: fake_config = netconfpersistence.RunningConfig() Line 202: fake_config.networks = {} Line 203: fake_config.save() Line 204: -- 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: 6 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
