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

Mark Wu has uploaded a new change for review.

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

netconf: Improve unified persistence's rollback in memory.

This patch removes the dependency on ifcfg's code for the unified
persistence's rollback in memory. It also move transaction management
implementaion from ifcfg to its base class Configurator. With this
change, the rollback function be reused for the iproute2 configuration.
This patch also adds a functional test for live rollback. It passes with
ifcfg persistence and unified persistence.

Change-Id: I7436976d8bacbfaa1c4b059c71c4abb46192f99a
Signed-off-by: Mark Wu 
---
M lib/vdsm/constants.py.in
M tests/functional/networkTests.py
M vdsm/netconf/__init__.py
M vdsm/netconf/ifcfg.py
M vdsm/vdsm-restore-net-config
5 files changed, 95 insertions(+), 19 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/32/20032/1

diff --git a/lib/vdsm/constants.py.in b/lib/vdsm/constants.py.in
index 1fb729b..a1a0dd6 100644
--- a/lib/vdsm/constants.py.in
+++ b/lib/vdsm/constants.py.in
@@ -139,6 +139,7 @@
 EXT_UNPERSIST = '@UNPERSIST_PATH@'

EXT_VDSM_STORE_NET_CONFIG = '@VDSMDIR@/vdsm-store-net-config'
+EXT_VDSM_RESTORE_NET_CONFIG = '@VDSMDIR@/vdsm-restore-net-config'

EXT_WGET = '@WGET_PATH@'

diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py
index b6c747f..d1f2329 100644
--- a/tests/functional/networkTests.py
+++ b/tests/functional/networkTests.py
@@ -1425,3 +1425,36 @@
                 status, msg = self.vdsm_net.setupNetworks(delete_networks,
                                                           {}, {})
                 self.assertEqual(status, SUCCESS, msg)
+
+    @cleanupNet
+    @permutations([[True], [False]])
+    @RequireDummyMod
+    @ValidateRunningAsRoot
+    def testSetupNetworksRollback(self, bridged):
+        with dummyIf(1) as nics:
+            with self.vdsm_net.pinger():
+                nic, = nics
+                tags = (10, 11, 12000)
+                networks = [NETWORK_NAME + str(tag) for tag in tags]
+                attrs = [dict(vlan=tag, nic=nic, bridged=bridged)
+                         for tag in tags]
+                status, msg = self.vdsm_net.setupNetworks({networks[0]:
+                                                           attrs[0]}, {}, {})
+
+                self.assertEqual(status, SUCCESS, msg)
+                self.assertTrue(self.vdsm_net.networkExists(networks[0]))
+
+                status, msg = self.vdsm_net.setupNetworks({networks[0]:
+                                                           dict(remove=True),
+                                                           networks[1]:
+                                                           attrs[1],
+                                                           networks[2]:
+                                                           attrs[2]}, {}, {})
+                self.assertFalse(self.vdsm_net.networkExists(networks[1]))
+                self.assertFalse(self.vdsm_net.networkExists(networks[2]))
+                self.assertTrue(self.vdsm_net.networkExists(networks[0]))
+
+                status, msg = self.vdsm_net.setupNetworks({networks[0]:
+                                                           dict(remove=True)},
+                                                          {}, {})
+                self.assertEqual(status, SUCCESS, msg)
diff --git a/vdsm/netconf/__init__.py b/vdsm/netconf/__init__.py
index c0e3ee9..9dc0f70 100644
--- a/vdsm/netconf/__init__.py
+++ b/vdsm/netconf/__init__.py
@@ -18,13 +18,16 @@
 #

import logging
+import pickle

from netmodels import Bond, Bridge
 from sourceRoute import DynamicSourceRoute
 from sourceRoute import StaticSourceRoute
+from vdsm import constants
 from vdsm import netinfo
 from vdsm.config import config
 from vdsm.netconfpersistence import RunningConfig
+from vdsm.utils import execCmd

class Configurator(object):
@@ -46,6 +49,28 @@
         else:
             self.rollback()

+    def begin(self, configApplierClass):
+        if self.configApplier is None:
+            self.configApplier = configApplierClass()
+        if self.unifiedPersistence and self.runningConfig is None:
+            self.runningConfig = RunningConfig()
+
+    def rollback(self):
+        if self.unifiedPersistence:
+            liveConfig = pickle.dumps(self.runningConfig)
+            execCmd([constants.EXT_VDSM_RESTORE_NET_CONFIG, '--memory',
+                     liveConfig])
+            self.runningConfig = None
+        else:
+            self.configApplier.restoreBackups()
+        self.configApplier = None
+
+    def commit(self):
+        self.configApplier = None
+        if self.unifiedPersistence:
+            self.runningConfig.save()
+            self.runningConfig = None
+
     def configureBridge(self, bridge, **opts):
         raise NotImplementedError

diff --git a/vdsm/netconf/ifcfg.py b/vdsm/netconf/ifcfg.py
index 0747b04..e0705d7 100644
--- a/vdsm/netconf/ifcfg.py
+++ b/vdsm/netconf/ifcfg.py
@@ -36,7 +36,6 @@
 from vdsm import constants
 from vdsm import netinfo
 from vdsm import utils
-from vdsm.netconfpersistence import RunningConfig
 import dsaversion
 import libvirtCfg
 import neterrors as ne
@@ -48,22 +47,7 @@
         super(Ifcfg, self).__init__(ConfigWriter())

def begin(self):
-        if self.configApplier is None:
-            self.configApplier = ConfigWriter()
-        if self.unifiedPersistence and self.runningConfig is None:
-            self.runningConfig = RunningConfig()
-
-    def rollback(self):
-        self.configApplier.restoreBackups()
-        self.configApplier = None
-        if self.unifiedPersistence:
-            self.runningConfig = None
-
-    def commit(self):
-        self.configApplier = None
-        if self.unifiedPersistence:
-            self.runningConfig.save()
-            self.runningConfig = None
+        super(Ifcfg, self).begin(ConfigWriter)

def configureBridge(self, bridge, **opts):
         self.configApplier.addBridge(bridge, **opts)
@@ -286,8 +270,8 @@
                       "(until next 'set safe config')", backup)

def _networkBackup(self, network):
-        self._atomicNetworkBackup(network)
         if config.get('vars', 'persistence') != 'unified':
+            self._atomicNetworkBackup(network)
             self._persistentNetworkBackup(network)

def _atomicNetworkBackup(self, network):
@@ -328,8 +312,8 @@
             logging.info('Restored %s', network)

def _backup(self, filename):
-        self._atomicBackup(filename)
         if config.get('vars', 'persistence') != 'unified':
+            self._atomicBackup(filename)
             self._persistentBackup(filename)

def _atomicBackup(self, filename):
diff --git a/vdsm/vdsm-restore-net-config b/vdsm/vdsm-restore-net-config
index 374387f..451d560 100755
--- a/vdsm/vdsm-restore-net-config
+++ b/vdsm/vdsm-restore-net-config
@@ -19,6 +19,9 @@
 # Refer to the README and COPYING files for full details of the license
 #

+import getopt
+import pickle
+import sys
 import logging
 import logging.config

@@ -64,6 +67,28 @@
         ifcfg_restoration()

+def restore_from_memory(liveConfig):
+    assert config.get('vars', 'persistence') == 'unified'
+    lastConfig = RunningConfig()  # snapshot of running config on disk
+
+    def get_restore_configs(liveConfig, lastConfig):
+        restoreConfigs = {}
+        for name in liveConfig:
+            if name not in lastConfig:
+                restoreConfigs[name] = {'remove': True}
+
+        for name, attr in lastConfig.iteritems():
+            if name not in liveConfig or attr != lastConfig[name]:
+                restoreConfigs[name] = lastConfig[name]
+        return restoreConfigs
+
+    nets = get_restore_configs(liveConfig.networks, lastConfig.networks)
+    bonds = get_restore_configs(liveConfig.bonds, lastConfig.bonds)
+    logging.debug("Calling setupNetworks with networks: %s, bondings: %s" %
+                  (nets, bonds))
+    setupNetworks(nets, bonds, connectivityCheck=False)
+
+
 if __name__ == '__main__':
     try:
         logging.config.fileConfig("/etc/vdsm/svdsm.logger.conf")
@@ -72,4 +97,12 @@
                             level=logging.DEBUG)
         logging.error("Could not init proper logging", exc_info=True)

+    opts, args = getopt.getopt(sys.argv[1:], "m:", ["memory="])
+    for opt, arg in opts:
+        if opt in ('-m', '--memory'):
+            restore_from_memory(pickle.loads(arg))
+            sys.exit(0)
+        else:
+            assert False, "Unhandled option"
+
     restore()

Mark Wu has posted comments on this change.

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

Patch Set 1: Verified+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/663/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4798/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4874/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3989/ : SUCCESS

Mark Wu has posted comments on this change.

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

Patch Set 2: Verified+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 2: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5130/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4326/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5204/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/754/ : FAILURE

Mark Wu has posted comments on this change.

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

Patch Set 3: Verified+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4360/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5164/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/759/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5240/ : SUCCESS

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:

Mark Wu has posted comments on this change.

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

Patch Set 3:

(4 comments)

Thanks a lot for your insightful review!  I will post a new patch in a few days.

....................................................
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.
Done
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()
Done
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
Done
Line 72: 
Line 73:     def get_restore_configs(liveConfig, lastConfig):
Line 74:         restoreConfigs = {}
Line 75:         for name in liveConfig:

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)
My thought of solving this problem is adding a new arg to  setupNetworks to indicate it's called in a normal api flow or the context of except handling,  and do the final restore like you suggest in Configurator.__exit__.  Maybe the new arg is not necessary,  we can tell the exception depth by exploring its stack frames.  I am going to look into it.
Line 89: 
Line 90: 
Line 91: if __name__ == '__main__':
Line 92:     try:

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 4: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4588/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5388/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5467/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/797/ : FAILURE

Mark Wu has posted comments on this change.

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

Patch Set 6: Verified+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 5:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4589/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5389/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5468/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/798/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 6:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4590/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5390/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5469/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/799/ : SUCCESS

Antoni Segura Puimedon has posted comments on this change.

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

Patch Set 6: Code-Review+1

Looking forward to this being up and running on my servers.

Assaf Muller has posted comments on this change.

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

Patch Set 6: Code-Review-1

(1 comment)

....................................................
File vdsm/netconf/__init__.py
Line 65:         if self.unifiedPersistence and self.runningConfig is None:
Line 66:             self.runningConfig = RunningConfig()
Line 67: 
Line 68:     def rollback(self):
Line 69:         if self.unifiedPersistence:
Currently vdsm_restore_net_config is used for on-boot restoration, and in-memory restoration code resides in ifcfg.py. I think vdsm_restore_net_config should not have in-memory restoration code, especially if it means that we use imp.load_source to get access to that code. Instead you can put the in memory restore code in the RunningConfig class.
Line 70:             restore_net = imp.load_source('restore_net',
Line 71:                                           EXT_VDSM_RESTORE_NET_CONFIG)
Line 72:             restore_net.restore_from_memory(self.runningConfig)
Line 73:             self.runningConfig = None

Mark Wu has posted comments on this change.

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

Patch Set 6:

(1 comment)

....................................................
File vdsm/netconf/__init__.py
Line 65:         if self.unifiedPersistence and self.runningConfig is None:
Line 66:             self.runningConfig = RunningConfig()
Line 67: 
Line 68:     def rollback(self):
Line 69:         if self.unifiedPersistence:
I am find to move  the memory restore code  to RunningConfig class. but we still need use imp.load to avoid circular importing issue.  Can you accept that?
Line 70:             restore_net = imp.load_source('restore_net',
Line 71:                                           EXT_VDSM_RESTORE_NET_CONFIG)
Line 72:             restore_net.restore_from_memory(self.runningConfig)
Line 73:             self.runningConfig = None

Assaf Muller has posted comments on this change.

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

Patch Set 6:

(1 comment)

....................................................
File vdsm/netconf/__init__.py
Line 65:         if self.unifiedPersistence and self.runningConfig is None:
Line 66:             self.runningConfig = RunningConfig()
Line 67: 
Line 68:     def rollback(self):
Line 69:         if self.unifiedPersistence:
netconf/__init__.py already imports vdsm.netconfpersistence prior to this patch, I might be missing something obvious, but where is the circular import?
Line 70:             restore_net = imp.load_source('restore_net',
Line 71:                                           EXT_VDSM_RESTORE_NET_CONFIG)
Line 72:             restore_net.restore_from_memory(self.runningConfig)
Line 73:             self.runningConfig = None

Mark Wu has posted comments on this change.

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

Patch Set 6:

(1 comment)

....................................................
File vdsm/netconf/__init__.py
Line 65:         if self.unifiedPersistence and self.runningConfig is None:
Line 66:             self.runningConfig = RunningConfig()
Line 67: 
Line 68:     def rollback(self):
Line 69:         if self.unifiedPersistence:
sorry for the unclear comments.  I mean the memory restore code also need call 'setupNetworks',  if we move it to RunningConfig,  then we're are making a dependency of configNetwork.py for vdsm.netconfpersistence
Line 70:             restore_net = imp.load_source('restore_net',
Line 71:                                           EXT_VDSM_RESTORE_NET_CONFIG)
Line 72:             restore_net.restore_from_memory(self.runningConfig)
Line 73:             self.runningConfig = None

Mark Wu has abandoned this change.

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

Abandoned

Resolved in Toni's series.