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(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes