Change in vdsm[master]: net: native ovs [5]: rollback trigger
edwardh at redhat.com
edwardh at redhat.com
Sun Apr 10 15:13:16 UTC 2016
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 <phoracek at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list