Change in vdsm[master]: netconf: Improve unified persistence's rollback in memory.

asegurap at redhat.com asegurap at redhat.com
Mon Nov 4 09:23:02 UTC 2013


Antoni Segura Puimedon has posted comments on this change.

Change subject: netconf: Improve unified persistence's rollback in memory.
......................................................................


Patch Set 3: Code-Review-1

(5 comments)

I like the patch a lot. There's some things that should be in a separate patch and we should protect us from entering a recursion that would blow up the stack.

....................................................
File tests/functional/networkTests.py
Line 1514:                 self.assertNetworkExists(networks[0])
Line 1515: 
Line 1516:                 # Submit a transaction composed of removing the created network
Line 1517:                 # above, creating two new networks with good params and bad
Line 1518:                 # params separately.
It could help test readability to mention that the bad param comes from the 12000 tag number.
Line 1519:                 status, msg = self.vdsm_net.setupNetworks({networks[0]:
Line 1520:                                                            dict(remove=True),
Line 1521:                                                            networks[1]:
Line 1522:                                                            attrs[1],


....................................................
File vdsm/netconf/__init__.py
Line 71:             self.runningConfig.save()
Line 72:             self.runningConfig = None
Line 73: 
Line 74:     def flush(self):
Line 75:         libvirtCfg.flush()
This change of flushing libvirtCfg in the base class and calling the super on the derived classes should be on a separate patch.
Line 76: 
Line 77:     def configureBridge(self, bridge, **opts):
Line 78:         raise NotImplementedError
Line 79: 


....................................................
File vdsm/vdsm-restore-net-config
Line 67: 
Line 68: 
Line 69: def restore_from_memory(liveConfig):
Line 70:     assert config.get('vars', 'persistence') == 'unified'
Line 71:     lastConfig = RunningConfig()  # snapshot of running config on disk
this would be better named lastGoodConfig.
Line 72: 
Line 73:     def get_restore_configs(liveConfig, lastConfig):
Line 74:         restoreConfigs = {}
Line 75:         for name in liveConfig:


Line 69: def restore_from_memory(liveConfig):
Line 70:     assert config.get('vars', 'persistence') == 'unified'
Line 71:     lastConfig = RunningConfig()  # snapshot of running config on disk
Line 72: 
Line 73:     def get_restore_configs(liveConfig, lastConfig):
Personally I'd probably like this method to work for at the Config level inside the Config class. But we can leave that for a future patch ;-)
Line 74:         restoreConfigs = {}
Line 75:         for name in liveConfig:
Line 76:             if name not in lastConfig:
Line 77:                 restoreConfigs[name] = {'remove': True}


Line 84:     nets = get_restore_configs(liveConfig.networks, lastConfig.networks)
Line 85:     bonds = get_restore_configs(liveConfig.bonds, lastConfig.bonds)
Line 86:     logging.debug("Calling setupNetworks with networks: %s, bondings: %s" %
Line 87:                   (nets, bonds))
Line 88:     setupNetworks(nets, bonds, connectivityCheck=False)
There is danger of failing this setupNetworks (like dhcp giving us troubles), which would make the rollback execute again. We might want to put a limit to the recursion where we do something like configurator.flush() and then a final setupNetworks.
Line 89: 
Line 90: 
Line 91: if __name__ == '__main__':
Line 92:     try:


-- 
To view, visit http://gerrit.ovirt.org/20032
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7436976d8bacbfaa1c4b059c71c4abb46192f99a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list