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