From: shihanzhang <shihanzhang@huawei.com>
Date: Fri, 18 Sep 2015 02:38:43 +0000 (+0800)
Subject: Execute ipset command using check_exit_code
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=fb40dbbce9ac17ee43d8921aa62c28f1ea723fba;p=openstack-build%2Fneutron-build.git

Execute ipset command using check_exit_code

When l2 agent execute ipset command, we should pass
parameter 'check_exit_code' to self.execute acording to
action.
The intention is to safely fail if we try to destroy a
non existing set, or delete a non existing member.
Otherwise the agent gets stuck in a loop if such situation
happens. Such kind of event would be logged as errors.

Change-Id: If67330523d114d6da13d0280851e7138a51d08f7
Closes-bug: #1497074
---

diff --git a/neutron/agent/linux/ipset_manager.py b/neutron/agent/linux/ipset_manager.py
index ebcc72dc1..a0fc70218 100644
--- a/neutron/agent/linux/ipset_manager.py
+++ b/neutron/agent/linux/ipset_manager.py
@@ -124,7 +124,7 @@ class IpsetManager(object):
 
     def _del_member_from_set(self, set_name, member_ip):
         cmd = ['ipset', 'del', set_name, member_ip]
-        self._apply(cmd)
+        self._apply(cmd, fail_on_errors=False)
         self.ipset_sets[set_name].remove(member_ip)
 
     def _create_set(self, set_name, ethertype):
@@ -133,13 +133,14 @@ class IpsetManager(object):
         self._apply(cmd)
         self.ipset_sets[set_name] = []
 
-    def _apply(self, cmd, input=None):
+    def _apply(self, cmd, input=None, fail_on_errors=True):
         input = '\n'.join(input) if input else None
         cmd_ns = []
         if self.namespace:
             cmd_ns.extend(['ip', 'netns', 'exec', self.namespace])
         cmd_ns.extend(cmd)
-        self.execute(cmd_ns, run_as_root=True, process_input=input)
+        self.execute(cmd_ns, run_as_root=True, process_input=input,
+                     check_exit_code=fail_on_errors)
 
     def _get_new_set_ips(self, set_name, expected_ips):
         new_member_ips = (set(expected_ips) -
@@ -175,5 +176,5 @@ class IpsetManager(object):
     def _destroy(self, set_name, forced=False):
         if set_name in self.ipset_sets or forced:
             cmd = ['ipset', 'destroy', set_name]
-            self._apply(cmd)
+            self._apply(cmd, fail_on_errors=False)
             self.ipset_sets.pop(set_name, None)
diff --git a/neutron/tests/functional/agent/linux/test_ipset.py b/neutron/tests/functional/agent/linux/test_ipset.py
index a575ad631..2a7794304 100644
--- a/neutron/tests/functional/agent/linux/test_ipset.py
+++ b/neutron/tests/functional/agent/linux/test_ipset.py
@@ -97,6 +97,5 @@ class IpsetManagerTestCase(IpsetBase):
         self.source.assert_ping(self.destination.ip)
 
     def test_destroy_ipset_set(self):
-        self.assertRaises(RuntimeError, self.ipset._destroy, self.ipset_name)
         self._remove_iptables_ipset_rules()
         self.ipset._destroy(self.ipset_name)
diff --git a/neutron/tests/unit/agent/linux/test_ipset_manager.py b/neutron/tests/unit/agent/linux/test_ipset_manager.py
index 2b5a58a6d..8b5b91cb8 100644
--- a/neutron/tests/unit/agent/linux/test_ipset_manager.py
+++ b/neutron/tests/unit/agent/linux/test_ipset_manager.py
@@ -67,19 +67,23 @@ class BaseIpsetManagerTest(base.BaseTestCase):
         self.expected_calls.extend([
             mock.call(['ipset', 'restore', '-exist'],
                       process_input=input,
-                      run_as_root=True),
+                      run_as_root=True,
+                      check_exit_code=True),
             mock.call(['ipset', 'swap', TEST_SET_NAME_NEW, TEST_SET_NAME],
                       process_input=None,
-                      run_as_root=True),
+                      run_as_root=True,
+                      check_exit_code=True),
             mock.call(['ipset', 'destroy', TEST_SET_NAME_NEW],
                       process_input=None,
-                      run_as_root=True)])
+                      run_as_root=True,
+                      check_exit_code=False)])
 
     def expect_add(self, addresses):
         self.expected_calls.extend(
             mock.call(['ipset', 'add', '-exist', TEST_SET_NAME, ip],
                       process_input=None,
-                      run_as_root=True)
+                      run_as_root=True,
+                      check_exit_code=True)
             for ip in self.ipset._sanitize_addresses(addresses))
 
     def expect_del(self, addresses):
@@ -87,7 +91,8 @@ class BaseIpsetManagerTest(base.BaseTestCase):
         self.expected_calls.extend(
             mock.call(['ipset', 'del', TEST_SET_NAME, ip],
                       process_input=None,
-                      run_as_root=True)
+                      run_as_root=True,
+                      check_exit_code=False)
             for ip in self.ipset._sanitize_addresses(addresses))
 
     def expect_create(self):
@@ -95,13 +100,15 @@ class BaseIpsetManagerTest(base.BaseTestCase):
             mock.call(['ipset', 'create', '-exist', TEST_SET_NAME,
                        'hash:net', 'family', 'inet'],
                       process_input=None,
-                      run_as_root=True))
+                      run_as_root=True,
+                      check_exit_code=True))
 
     def expect_destroy(self):
         self.expected_calls.append(
             mock.call(['ipset', 'destroy', TEST_SET_NAME],
                       process_input=None,
-                      run_as_root=True))
+                      run_as_root=True,
+                      check_exit_code=False))
 
     def add_first_ip(self):
         self.expect_set([FAKE_IPS[0]])