Merge ~zioproto/ubuntu/+source/neutron:stable/liberty into ~ubuntu-server-dev/ubuntu/+source/neutron:stable/liberty

[Saverio Proto]

Fixes bug https://bugs.launchpad.net/neutron/+bug/1573073

[Corey Bryant (corey.bryant)]

Hi Saverio,

Thanks for the patch.

Do you know how this is fixed, or why it doesn't exist, in Mitaka and above?

I reviewed your latest upstream patch proposal at https://review.openstack.org/#/c/309050/24 and it seemed like a reasonable approach. Can you explain why you've gone with the approach in this merge proposal vs what you had in that upstream review?

Thanks,
Corey

[Saverio Proto (zioproto)]

Hello,
I believe the bug is there also in master, but I cannot test it
because I dont have a production environment newer than Liberty. Maybe
you guys at Canonical can help sorting this out ?

The patch I proposed to merge on Ubuntu packages is what I really run
in production and I consider stable.

The patch based in review 309050 patchset 24 improves the previous
patch with a try catch block, to fix a race condition in case the
cronjob is running exactly when the code is running. It is really a
corner case of the corner case, I did not try that patchset in
production.

Both patches are valuable to improve the stability of Neutron.

I would really like the Liberty packages patched with this merge
request, and somebody to help me to make this patch upstream and
backported at least to Newton.

Thanks for your help.

Saverio

2016-10-31 18:06 GMT+01:00 Corey Bryant <email address hidden>:
> Hi Saverio,
>
> Thanks for the patch.
>
> Do you know how this is fixed, or why it doesn't exist, in Mitaka and above?
>
> I reviewed your latest upstream patch proposal at https://review.openstack.org/#/c/309050/24 and it seemed like a reasonable approach. Can you explain why you've gone with the approach in this merge proposal vs what you had in that upstream review?
>
> Thanks,
> Corey
>
>
> --
> https://code.launchpad.net/~zioproto/ubuntu/+source/neutron/+git/neutron/+merge/309457
> You are the owner of ~zioproto/ubuntu/+source/neutron:stable/liberty.

[Corey Bryant (corey.bryant)]

Saverio, Can you try to get your latest patches landed upstream in master via https://review.openstack.org/#/c/309050?

Once it lands in master, we can get them backported to upstream stable/newton and stable/mitaka branches. Then we (canonical folks) can help sort the Ubuntu/cloud-archive backports. But we need to get this fixed upstream and on the latest releases before we can fix it in Liberty.

Thanks,
Corey

[Saverio Proto (zioproto)]

Hello,
the upstream patch is merged. I just pushed a refreshed commit in my tree, and this merge request got automatically updated.
thank you

Unmerged commits

0986932... by Saverio Proto <email address hidden> on 2016-10-27

Fix router namespace cleanup (LP: #1573073)

* Fix router namespace cleanup (LP: #1573073)
* d/p/ns-exists-before-get-devices.patch

diff --git a/debian/changelog b/debian/changelog
index fe6d05e..0f2211e 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+neutron (2:7.2.0-0ubuntu1~cloud0ubuntu1) UNRELEASED; urgency=medium
+
+  * Fix router namespace cleanup (LP: #1573073)
+  * d/p/ns-exists-before-get-devices.patch
+
+ -- Saverio Proto <[e-mail]>  Thu, 27 Oct 2016 09:33:25 +0000
+
 neutron (2:7.2.0-0ubuntu1~cloud0) trusty-liberty; urgency=medium
 
   * New upstream stable point release for OpenStack Liberty (LP: #1619452).
diff --git a/debian/patches/ns-exists-before-get-devices.patch b/debian/patches/ns-exists-before-get-devices.patch
new file mode 100644
index 0000000..58951ae
--- /dev/null
+++ b/debian/patches/ns-exists-before-get-devices.patch
@@ -0,0 +1,82 @@
+commit 83c6ace58571f97b4da560c545800eb4d144308b
+Author: Saverio Proto <[e-mail]>
+Date:   Thu Apr 21 16:50:07 2016 +0200
+
+    Check if namespace exists before getting devices
+    
+    If a neutron router has no ports defined, its namespace is deleted
+    by the cronjob /etc/cron.d/neutron-l3-agent-netns-cleanup.
+    It is required to check if the namespace exists before calling
+    iproute2 commands against the namespace.
+    It is not enough for the string of the namespace uuid to be defined,
+    the namespace must really exist on the network node.
+    This patch checks if the namespace exists when calling get_devices().
+    Otherwise the agent log file will be flooded with messages like
+    Cannot open network namespace "qrouter-<uuid>": No such file or directory
+    Failed to process compatible router '<uuid>'
+    
+    Related-bug: 1573073
+    
+    Change-Id: I744ef11529f9da5cbfdb812de0b35b95f9d078bb
+
+diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py
+index 115686f..a4bb9a2 100644
+--- a/neutron/agent/linux/ip_lib.py
++++ b/neutron/agent/linux/ip_lib.py
+@@ -118,12 +118,21 @@ class IPWrapper(SubProcessBase):
+             # we call out manually because in order to avoid screen scraping
+             # iproute2 we use find to see what is in the sysfs directory, as
+             # suggested by Stephen Hemminger (iproute2 dev).
+-            output = utils.execute(['ip', 'netns', 'exec', self.namespace,
+-                                    'find', SYS_NET_PATH, '-maxdepth', '1',
+-                                    '-type', 'l', '-printf', '%f '],
+-                                   run_as_root=True,
+-                                   log_fail_as_error=self.log_fail_as_error
+-                                   ).split()
++            try:
++                cmd = ['ip', 'netns', 'exec', self.namespace,
++                       'find', SYS_NET_PATH, '-maxdepth', '1',
++                       '-type', 'l', '-printf', '%f ']
++                output = utils.execute(
++                    cmd,
++                    run_as_root=True,
++                    log_fail_as_error=self.log_fail_as_error).split()
++            except RuntimeError:
++                # We could be racing with a cron job deleting namespaces.
++                # Just return a empty list if the namespace is deleted.
++                with excutils.save_and_reraise_exception() as ctx:
++                    if not self.netns.exists(self.namespace):
++                        ctx.reraise = False
++                        return []
+         else:
+             output = (
+                 i for i in os.listdir(SYS_NET_PATH)
+diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py
+index a33f36e..e462962 100644
+--- a/neutron/tests/unit/agent/linux/test_ip_lib.py
++++ b/neutron/tests/unit/agent/linux/test_ip_lib.py
+@@ -281,6 +281,24 @@ class TestIpWrapper(base.BaseTestCase):
+         self.assertTrue(fake_str.split.called)
+         self.assertEqual(retval, [ip_lib.IPDevice('lo', namespace='foo')])
+ 
++    @mock.patch('neutron.agent.common.utils.execute')
++    def test_get_devices_namespaces_ns_not_exists(self, mocked_execute):
++        mocked_execute.side_effect = RuntimeError(
++            "Cannot open network namespace")
++        with mock.patch.object(ip_lib.IpNetnsCommand, 'exists',
++                               return_value=False):
++            retval = ip_lib.IPWrapper(namespace='foo').get_devices()
++            self.assertEqual([], retval)
++
++    @mock.patch('neutron.agent.common.utils.execute')
++    def test_get_devices_namespaces_ns_exists(self, mocked_execute):
++        mocked_execute.side_effect = RuntimeError(
++            "Cannot open network namespace")
++        with mock.patch.object(ip_lib.IpNetnsCommand, 'exists',
++                               return_value=True):
++            self.assertRaises(RuntimeError,
++                              ip_lib.IPWrapper(namespace='foo').get_devices)
++
+     def test_get_namespaces(self):
+         self.execute.return_value = '\n'.join(NETNS_SAMPLE)
+         retval = ip_lib.IPWrapper.get_namespaces()
diff --git a/debian/patches/series b/debian/patches/series
index 2afa859..3ab5e9c 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -2,3 +2,4 @@ fix-test-treat-devices-added-updated-no-local-interface.patch
 fix-neutron-configuration.patch
 skip-iptest.patch
 drop-ryu-dep.patch
+ns-exists-before-get-devices.patch