Merge lp:~ajkavanagh/charm-helpers/memcache-context-ipv4-support into lp:charm-helpers

[Alex Kavanagh]

Add ipv4 support to the class MemcacheContext

[lp:~ajkavanagh/charm-helpers/memcache-context-ipv4-support]

735. By Alex Kavanagh on 2017-04-19

Revised version of ipv4 support in the MemcacheContext

This version defaults to ipv6 unless ipv6 is explicitly disabled or
not available. This retains the existing behaviour of defaulting to
ipv6 regardless of the 'prefer-ipv6' setting.

[James Page (james-page)]

review: Needs Fixing

[lp:~ajkavanagh/charm-helpers/memcache-context-ipv4-support]

736. By Alex Kavanagh on 2017-04-19

Remove spurious extra line missed during refactor

=== modified file 'charmhelpers/contrib/network/ip.py'
--- charmhelpers/contrib/network/ip.py	2017-03-31 12:58:23 +0000
+++ charmhelpers/contrib/network/ip.py	2017-04-19 14:35:25 +0000
@@ -239,6 +239,16 @@
     return None
 
 
+def is_ipv6_disabled():
+    try:
+        result = subprocess.check_output(
+            ['sysctl', 'net.ipv6.conf.all.disable_ipv6'],
+            stderr=subprocess.STDOUT)
+        return "net.ipv6.conf.all.disable_ipv6 = 1" in result
+    except subprocess.CalledProcessError:
+        return True
+
+
 def get_iface_addr(iface='eth0', inet_type='AF_INET', inc_aliases=False,
                    fatal=True, exc_list=None):
     """Return the assigned IP address for a given interface, if any.

=== modified file 'charmhelpers/contrib/openstack/context.py'
--- charmhelpers/contrib/openstack/context.py	2017-03-30 08:38:44 +0000
+++ charmhelpers/contrib/openstack/context.py	2017-04-19 14:35:25 +0000
@@ -89,6 +89,7 @@
     format_ipv6_addr,
     is_address_in_network,
     is_bridge_member,
+    is_ipv6_disabled,
 )
 from charmhelpers.contrib.openstack.utils import (
     config_flags_parser,
@@ -1602,7 +1603,7 @@
     """Memcache context
 
     This context provides options for configuring a local memcache client and
-    server
+    server for both IPv4 and IPv6
     """
 
     def __init__(self, package=None):
@@ -1620,13 +1621,24 @@
             # Trusty version of memcached does not support ::1 as a listen
             # address so use host file entry instead
             release = lsb_release()['DISTRIB_CODENAME'].lower()
-            if CompareHostReleases(release) > 'trusty':
-                ctxt['memcache_server'] = '::1'
+            if is_ipv6_disabled():
+                if CompareHostReleases(release) > 'trusty':
+                    ctxt['memcache_server'] = '127.0.0.1'
+                else:
+                    ctxt['memcache_server'] = 'localhost'
+                ctxt['memcache_server_formatted'] = '127.0.0.1'
+                ctxt['memcache_port'] = '11211'
+                ctxt['memcache_url'] = '{}:{}'.format(
+                    ctxt['memcache_server_formatted'],
+                    ctxt['memcache_port'])
             else:
-                ctxt['memcache_server'] = 'ip6-localhost'
-            ctxt['memcache_server_formatted'] = '[::1]'
-            ctxt['memcache_port'] = '11211'
-            ctxt['memcache_url'] = 'inet6:{}:{}'.format(
-                ctxt['memcache_server_formatted'],
-                ctxt['memcache_port'])
+                if CompareHostReleases(release) > 'trusty':
+                    ctxt['memcache_server'] = '::1'
+                else:
+                    ctxt['memcache_server'] = 'ip6-localhost'
+                ctxt['memcache_server_formatted'] = '[::1]'
+                ctxt['memcache_port'] = '11211'
+                ctxt['memcache_url'] = 'inet6:{}:{}'.format(
+                    ctxt['memcache_server_formatted'],
+                    ctxt['memcache_port'])
         return ctxt

=== modified file 'tests/contrib/network/test_ip.py'
--- tests/contrib/network/test_ip.py	2017-03-31 14:35:43 +0000
+++ tests/contrib/network/test_ip.py	2017-04-19 14:35:25 +0000
@@ -292,7 +292,8 @@
             '255.255.0.0')
         self.assertEquals(net_ip.get_netmask_for_address('172.4.5.5'), None)
         self.assertEquals(
-            net_ip.get_netmask_for_address('2a01:348:2f4:0:685e:5748:ae62:210f'),
+            net_ip.get_netmask_for_address(
+                '2a01:348:2f4:0:685e:5748:ae62:210f'),
             '64'
         )
         self.assertEquals(
@@ -379,6 +380,28 @@
         _interfaces.return_value = DUMMY_ADDRESSES.keys()
         self.assertRaises(Exception, net_ip.get_ipv6_addr, 'eth1')
 
+    @patch('charmhelpers.contrib.network.ip.subprocess.check_output')
+    def test_is_ipv6_disabled(self, mock_check_output):
+        # verify that the function does look for the right thing
+        mock_check_output.return_value = """
+        Some lines before
+        net.ipv6.conf.all.disable_ipv6 = 1
+        Some lines afterward
+        """
+        self.assertTrue(net_ip.is_ipv6_disabled())
+        mock_check_output.assert_called_once_with(
+            ['sysctl', 'net.ipv6.conf.all.disable_ipv6'],
+            stderr=subprocess.STDOUT)
+        # if it isn't there, it must return false
+        mock_check_output.return_value = ""
+        self.assertFalse(net_ip.is_ipv6_disabled())
+        # If the syscall returns an error, then return True
+
+        def fake_check_call(*args, **kwargs):
+            raise subprocess.CalledProcessError(['called'], 1)
+        mock_check_output.side_effect = fake_check_call
+        self.assertTrue(net_ip.is_ipv6_disabled())
+
     @patch.object(netifaces, 'ifaddresses')
     @patch.object(netifaces, 'interfaces')
     def test_get_iface_addr(self, _interfaces, _ifaddresses):
@@ -561,9 +584,8 @@
 
     @patch('charmhelpers.contrib.network.ip.subprocess.check_output')
     @patch('charmhelpers.contrib.network.ip.get_iface_addr')
-    def test_get_ipv6_global_dynamic_address_invalid_address(self,
-                                                             mock_get_iface_addr,
-                                                             mock_check_out):
+    def test_get_ipv6_global_dynamic_address_invalid_address(
+            self, mock_get_iface_addr, mock_check_out):
         mock_get_iface_addr.return_value = []
         with nose.tools.assert_raises(Exception):
             net_ip.get_ipv6_addr()

=== modified file 'tests/contrib/openstack/test_os_contexts.py'
--- tests/contrib/openstack/test_os_contexts.py	2017-03-30 08:38:44 +0000
+++ tests/contrib/openstack/test_os_contexts.py	2017-04-19 14:35:25 +0000
@@ -228,7 +228,9 @@
 }
 
 AMQP_OSLO_CONFIG = {
-    'oslo-messaging-flags': "rabbit_max_retries=1,rabbit_retry_backoff=1,rabbit_retry_interval=1"
+    'oslo-messaging-flags': ("rabbit_max_retries=1"
+                             ",rabbit_retry_backoff=1"
+                             ",rabbit_retry_interval=1")
 }
 
 AMQP_NOVA_CONFIG = {
@@ -493,13 +495,15 @@
     'cinder-subordinate:0': {
         'cinder-subordinate/0': {
             'private-address': 'cinder_node1',
-            'subordinate_configuration': json.dumps(yaml.load(CINDER_SUB_CONFIG1)),
+            'subordinate_configuration': json.dumps(
+                yaml.load(CINDER_SUB_CONFIG1)),
         },
     },
     'cinder-subordinate:1': {
         'cinder-subordinate/1': {
             'private-address': 'cinder_node1',
-            'subordinate_configuration': json.dumps(yaml.load(CINDER_SUB_CONFIG2)),
+            'subordinate_configuration': json.dumps(
+                yaml.load(CINDER_SUB_CONFIG2)),
         },
     },
 }
@@ -508,19 +512,22 @@
     'nova-ceilometer:6': {
         'ceilometer-agent/0': {
             'private-address': 'nova_node1',
-            'subordinate_configuration': json.dumps(yaml.load(NOVA_SUB_CONFIG1)),
+            'subordinate_configuration': json.dumps(
+                yaml.load(NOVA_SUB_CONFIG1)),
         },
     },
     'neutron-plugin:3': {
         'neutron-ovs-plugin/0': {
             'private-address': 'nova_node1',
-            'subordinate_configuration': json.dumps(yaml.load(NOVA_SUB_CONFIG2)),
+            'subordinate_configuration': json.dumps(
+                yaml.load(NOVA_SUB_CONFIG2)),
         },
     },
     'neutron-plugin:4': {
         'neutron-other-plugin/0': {
             'private-address': 'nova_node1',
-            'subordinate_configuration': json.dumps(yaml.load(NOVA_SUB_CONFIG3)),
+            'subordinate_configuration': json.dumps(
+                yaml.load(NOVA_SUB_CONFIG3)),
         },
     }
 }
@@ -672,7 +679,8 @@
         self.assertEquals(result, expected)
 
     def test_shared_db_context_with_data_and_access_net_mismatch(self):
-        '''Mismatch between hostname and hostname for access net - defers execution'''
+        """Mismatch between hostname and hostname for access net - defers
+        execution"""
         relation = FakeRelation(
             relation_data=SHARED_DB_RELATION_ACCESS_NETWORK)
         self.relation_get.side_effect = relation.get
@@ -686,7 +694,7 @@
                 'hostname': '10.5.5.1'})
 
     def test_shared_db_context_with_data_and_access_net_match(self):
-        '''Correctly set hostname for access net returns complete context'''
+        """Correctly set hostname for access net returns complete context"""
         relation = FakeRelation(
             relation_data=SHARED_DB_RELATION_ACCESS_NETWORK)
         self.relation_get.side_effect = relation.get
@@ -952,7 +960,8 @@
 
     def test_identity_service_context_with_data_versioned(self):
         '''Test shared-db context with api version supplied from keystone'''
-        relation = FakeRelation(relation_data=IDENTITY_SERVICE_RELATION_VERSIONED)
+        relation = FakeRelation(
+            relation_data=IDENTITY_SERVICE_RELATION_VERSIONED)
         self.relation_get.side_effect = relation.get
         identity_service = context.IdentityServiceContext()
         result = identity_service()
@@ -1117,7 +1126,8 @@
             'rabbitmq_user': 'adam',
             'rabbitmq_virtual_host': 'foo',
             'rabbitmq_hosts': 'rabbithost1,rabbithost2',
-            'transport_url': 'rabbit://adam:foobar@rabbithost1:5672,adam:foobar@rabbithost2:5672/foo'
+            'transport_url': ('rabbit://adam:foobar@rabbithost1:5672'
+                              ',adam:foobar@rabbithost2:5672/foo')
         }
         self.assertEquals(result, expected)
 
@@ -1160,7 +1170,8 @@
             'rabbitmq_user': 'adam',
             'rabbitmq_virtual_host': 'foo',
             'rabbitmq_hosts': '[2001:db8:1::1],[2001:db8:1::1]',
-            'transport_url': 'rabbit://adam:foobar@[2001:db8:1::1]:5672,adam:foobar@[2001:db8:1::1]:5672/foo'
+            'transport_url': ('rabbit://adam:foobar@[2001:db8:1::1]:5672'
+                              ',adam:foobar@[2001:db8:1::1]:5672/foo')
         }
         self.assertEquals(result, expected)
 
@@ -1829,7 +1840,8 @@
 
     @patch('charmhelpers.contrib.openstack.context.unit_get')
     @patch('charmhelpers.contrib.openstack.context.local_unit')
-    def test_haproxy_context_with_no_peers_singlemode(self, local_unit, unit_get):
+    def test_haproxy_context_with_no_peers_singlemode(
+            self, local_unit, unit_get):
         '''Test haproxy context with single unit'''
         # peer relations always show at least one peer relation, even
         # if unit is alone. should be an incomplete context.
@@ -3186,7 +3198,8 @@
         rel = FakeRelation(QUANTUM_NETWORK_SERVICE_RELATION_VERSIONED)
         self.relation_ids.side_effect = rel.relation_ids
         self.related_units.side_effect = rel.relation_units
-        relation = FakeRelation(relation_data=QUANTUM_NETWORK_SERVICE_RELATION_VERSIONED)
+        relation = FakeRelation(
+            relation_data=QUANTUM_NETWORK_SERVICE_RELATION_VERSIONED)
         self.relation_get.side_effect = relation.get
         self.assertEquals(context.NetworkServiceContext()(), data_result)
 
@@ -3274,10 +3287,14 @@
         AA.manually_disable_aa_profile.assert_called_with()
 
     @patch.object(context, 'enable_memcache')
-    def test_memcache_context(self, _enable_memcache):
+    @patch.object(context, 'is_ipv6_disabled')
+    def test_memcache_context_ipv6(self, _is_ipv6_disabled, _enable_memcache):
         self.lsb_release.return_value = {'DISTRIB_CODENAME': 'xenial'}
         _enable_memcache.return_value = True
-        config = {'openstack-origin': 'distro'}
+        _is_ipv6_disabled.return_value = False
+        config = {
+            'openstack-origin': 'distro',
+        }
         self.config.side_effect = fake_config(config)
         ctxt = context.MemcacheContext()
         self.assertTrue(ctxt()['use_memcache'])
@@ -3294,6 +3311,30 @@
         self.assertEqual(ctxt(), expect)
 
     @patch.object(context, 'enable_memcache')
+    @patch.object(context, 'is_ipv6_disabled')
+    def test_memcache_context_ipv4(self, _is_ipv6_disabled, _enable_memcache):
+        self.lsb_release.return_value = {'DISTRIB_CODENAME': 'xenial'}
+        _enable_memcache.return_value = True
+        _is_ipv6_disabled.return_value = True
+        config = {
+            'openstack-origin': 'distro',
+        }
+        self.config.side_effect = fake_config(config)
+        ctxt = context.MemcacheContext()
+        self.assertTrue(ctxt()['use_memcache'])
+        expect = {
+            'memcache_port': '11211',
+            'memcache_server': '127.0.0.1',
+            'memcache_server_formatted': '127.0.0.1',
+            'memcache_url': '127.0.0.1:11211',
+            'use_memcache': True}
+        self.assertEqual(ctxt(), expect)
+        self.lsb_release.return_value = {'DISTRIB_CODENAME': 'trusty'}
+        expect['memcache_server'] = 'localhost'
+        ctxt = context.MemcacheContext()
+        self.assertEqual(ctxt(), expect)
+
+    @patch.object(context, 'enable_memcache')
     def test_memcache_off_context(self, _enable_memcache):
         _enable_memcache.return_value = False
         config = {'openstack-origin': 'distro'}