Merge lp:~soren/nova/network-fixes into lp:~hudson-openstack/nova/trunk

[Soren Hansen]

Fixes to address the following issues:

 * nova-network no longer refrains from configuring bridges if it finds them already created. This solves a race condition with nova-network and nova-compute running on the same box. If nova-compute happened to do its network config first, nova-network would find the bridge already created and assume it was correctly setup. If nova-network got there first, it would apply the correct IP configuration to the interface. The assumption is that no harm will be done by reapplying the correct IP configuration, so we no longer make this check before doing so.

 * nova-network does not use persistent network configuration (like putting stuff in /etc/network/interfaces), so it needs to check at boot time whether it's already set as a network's designated network node and do the appropriate configuration.

 * If installed, dnsmasq wouldn't find nova-dhcpbridge properly (it would look for it in /usr/lib/python2.6/blahblahblha). Add a flag to specify its location (and make use of it in the packaging).

 * dnsmasq needs to be able to read the dhcp config written by nova. dnsmasq runs as user "nobody", so chmod 644 the config file so that it can be read.

 * dnsmasq needs to be sent a SIGHUP to reread its config. Wrap this call in sudo, because we may be running as non-root.

[Vish Ishaya (vishvananda)]

This looks good. One issue:

We have a couple different solutions in the works for applying network configuration at service boot. In the branch I am working on lp:~vishvananda/nova/fix-iptables which fixes a number of isssues in devcamcar's version that is under review, we have added an explicitly called init_host() which is called when the manager is loaded from service.py. I don't want init to happen in two places, so we should move one or the other. Preference on whether we do it in init or an explicit method?

On Sep 27, 2010, at 4:57 AM, Soren Hansen wrote:

> Soren Hansen has proposed merging lp:~soren/nova/network-fixes into lp:nova.
>
> Requested reviews:
> Nova Core (nova-core)
>
>
> Fixes to address the following issues:
>
> * nova-network no longer refrains from configuring bridges if it finds them already created. This solves a race condition with nova-network and nova-compute running on the same box. If nova-compute happened to do its network config first, nova-network would find the bridge already created and assume it was correctly setup. If nova-network got there first, it would apply the correct IP configuration to the interface. The assumption is that no harm will be done by reapplying the correct IP configuration, so we no longer make this check before doing so.
>
> * nova-network does not use persistent network configuration (like putting stuff in /etc/network/interfaces), so it needs to check at boot time whether it's already set as a network's designated network node and do the appropriate configuration.
>
> * If installed, dnsmasq wouldn't find nova-dhcpbridge properly (it would look for it in /usr/lib/python2.6/blahblahblha). Add a flag to specify its location (and make use of it in the packaging).
>
> * dnsmasq needs to be able to read the dhcp config written by nova. dnsmasq runs as user "nobody", so chmod 644 the config file so that it can be read.
> --
> https://code.launchpad.net/~soren/nova/network-fixes/+merge/36700
> Your team Nova Core is requested to review the proposed merge of lp:~soren/nova/network-fixes into lp:nova.
> === modified file 'nova/db/api.py'
> --- nova/db/api.py 2010-09-23 19:31:17 +0000
> +++ nova/db/api.py 2010-09-27 11:57:40 +0000
> @@ -554,3 +554,15 @@
>
> """
> return IMPL.volume_update(context, volume_id, values)
> +
> +
> +###################
> +
> +
> +def host_get_networks(context, host):
> + """Return all networks for which the given host is the designated
> + network host
> + """
> + return IMPL.host_get_networks(context, host)
> +
> +
>
> === modified file 'nova/db/sqlalchemy/api.py'
> --- nova/db/sqlalchemy/api.py 2010-09-23 19:31:17 +0000
> +++ nova/db/sqlalchemy/api.py 2010-09-27 11:57:40 +0000
> @@ -848,3 +848,15 @@
> for (key, value) in values.iteritems():
> volume_ref[key] = value
> volume_ref.save(session=session)
> +
> +
> +###################
> +
> +
> +def host_get_networks(context, host):
> + session = get_session()
> + with session.begin():
> + return session.query(models.Network
> + ).filter_by(deleted=False
> + ).filter_by(host=host
> + ...

[Soren Hansen (soren)]

Your approach sounds great. If your stuff lands first, I'll rework this branch to use init_host. If it doesn't, I'll do it once your stuff has landed and file a new merge proposal. Sounds good?

[Vish Ishaya (vishvananda)]

lgtm.

review: Approve

[Vish Ishaya (vishvananda)]

I am using this and it works fine for me. Someone else want to approve it?

[Todd Willey (xtoddx)]

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~soren/nova/network-fixes into lp:nova failed.Below is the output from the failed tests.

nova.tests.access_unittest
  AccessTestCase
    test_001_allow_all ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_002_allow_none ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_003_allow_project_manager ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_004_allow_sys_and_net ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
nova.tests.api_unittest
  ApiEc2TestCase
    test_describe_instances ... /var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
                                           [OK]
    test_get_all_key_pairs ... /var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
                                            [OK]
nova.tests.auth_unittest
  AuthManagerTestCase
    test_004_signature_is_valid ... [OK]
    test_005_can_get_credentials ... [OK]
    test_add_user_role_doesnt_infect_project_roles ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_adding_role_to_project_is_ignored_unless_added_to_user ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_add_and_remove_user_role ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_add_remove_user_with_role ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_add_user_to_project ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_create_and_get_project ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_create_and_get_project_with_attributes ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_create_project_with_manager ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchem...

[Vish Ishaya (vishvananda)]

merge failed because there is no networks directory in the core repo. This is one possible fix:

=== modified file 'nova/network/manager.py'
--- nova/network/manager.py 2010-10-01 16:20:34 +0000
+++ nova/network/manager.py 2010-10-01 16:52:35 +0000
@@ -378,7 +378,8 @@
         self.driver.ensure_vlan_bridge(network_ref['vlan'],
                                        network_ref['bridge'],
                                        network_ref)
- self.driver.update_dhcp(context, network_id)
+ if not FLAGS.fake_network:
+ self.driver.update_dhcp(context, network_id)

     @property
     def _bottom_reserved_ips(self):

[Soren Hansen (soren)]

There. That should do it.

[Soren Hansen (soren)]

Todd/Vish: If either of you could re-review and approve, that would be great. Thanks.

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~soren/nova/network-fixes into lp:nova failed.Below is the output from the failed tests.

nova.tests.access_unittest
  AccessTestCase
    test_001_allow_all ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_002_allow_none ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_003_allow_project_manager ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_004_allow_sys_and_net ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
nova.tests.api_unittest
  ApiEc2TestCase
    test_describe_instances ... /var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
                                           [OK]
    test_get_all_key_pairs ... /var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
                                            [OK]
nova.tests.auth_unittest
  AuthManagerTestCase
    test_004_signature_is_valid ... [OK]
    test_005_can_get_credentials ... [OK]
    test_add_user_role_doesnt_infect_project_roles ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_adding_role_to_project_is_ignored_unless_added_to_user ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_add_and_remove_user_role ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_add_remove_user_with_role ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_add_user_to_project ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_create_and_get_project ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_create_and_get_project_with_attributes ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchemy/api.py:42: DeprecationWarning: Use of empty request context is deprecated
    test_can_create_project_with_manager ... [OK]
/var/lib/hudson/src/nova/hudson/nova/db/sqlalchem...

[lp:~soren/nova/network-fixes]

304. By Soren Hansen on 2010-10-03

s/APIRequestContext/get_admin_context/ <-- sudo for request contexts.

=== modified file 'nova/db/api.py'
--- nova/db/api.py	2010-10-01 09:28:31 +0000
+++ nova/db/api.py	2010-10-03 18:24:43 +0000
@@ -565,3 +565,15 @@
 
     """
     return IMPL.volume_update(context, volume_id, values)
+
+
+###################
+
+
+def host_get_networks(context, host):
+    """Return all networks for which the given host is the designated
+    network host
+    """
+    return IMPL.host_get_networks(context, host)
+
+

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2010-10-01 09:28:31 +0000
+++ nova/db/sqlalchemy/api.py	2010-10-03 18:24:43 +0000
@@ -1273,3 +1273,15 @@
         for (key, value) in values.iteritems():
             volume_ref[key] = value
         volume_ref.save(session=session)
+
+
+###################
+
+
+def host_get_networks(context, host):
+    session = get_session()
+    with session.begin():
+        return session.query(models.Network
+                     ).filter_by(deleted=False
+                     ).filter_by(host=host
+                     ).all()

=== modified file 'nova/network/linux_net.py'
--- nova/network/linux_net.py	2010-09-29 03:28:20 +0000
+++ nova/network/linux_net.py	2010-10-03 18:24:43 +0000
@@ -28,6 +28,11 @@
 from nova import utils
 
 
+def _bin_file(script):
+    """Return the absolute path to scipt in the bin directory"""
+    return os.path.abspath(os.path.join(__file__, "../../../bin", script))
+
+
 FLAGS = flags.FLAGS
 flags.DEFINE_string('dhcpbridge_flagfile',
                     '/etc/nova/nova-dhcpbridge.conf',
@@ -38,7 +43,9 @@
 flags.DEFINE_string('public_interface', 'vlan1',
                     'Interface for public IP addresses')
 flags.DEFINE_string('bridge_dev', 'eth0',
-                    'network device for bridges')
+                        'network device for bridges')
+flags.DEFINE_string('dhcpbridge', _bin_file('nova-dhcpbridge'),
+                        'location of nova-dhcpbridge')
 flags.DEFINE_string('routing_source_ip', '127.0.0.1',
                     'Public IP of network host')
 flags.DEFINE_bool('use_nova_chains', False,
@@ -139,16 +146,16 @@
         # _execute("sudo brctl setageing %s 10" % bridge)
         _execute("sudo brctl stp %s off" % bridge)
         _execute("sudo brctl addif %s %s" % (bridge, interface))
-        if net_attrs:
-            _execute("sudo ifconfig %s %s broadcast %s netmask %s up" % \
-                    (bridge,
-                     net_attrs['gateway'],
-                     net_attrs['broadcast'],
-                     net_attrs['netmask']))
-        else:
-            _execute("sudo ifconfig %s up" % bridge)
-        _confirm_rule("FORWARD", "--in-interface %s -j ACCEPT" % bridge)
-        _confirm_rule("FORWARD", "--out-interface %s -j ACCEPT" % bridge)
+    if net_attrs:
+        _execute("sudo ifconfig %s %s broadcast %s netmask %s up" % \
+                (bridge,
+                 net_attrs['gateway'],
+                 net_attrs['broadcast'],
+                 net_attrs['netmask']))
+    else:
+        _execute("sudo ifconfig %s up" % bridge)
+    _confirm_rule("FORWARD", "--in-interface %s -j ACCEPT" % bridge)
+    _confirm_rule("FORWARD", "--out-interface %s -j ACCEPT" % bridge)
 
 
 def get_dhcp_hosts(context, network_id):
@@ -172,9 +179,14 @@
     signal causing it to reload, otherwise spawn a new instance
     """
     network_ref = db.network_get(context, network_id)
-    with open(_dhcp_file(network_ref['vlan'], 'conf'), 'w') as f:
+
+    conffile = _dhcp_file(network_ref['vlan'], 'conf')
+    with open(conffile, 'w') as f:
         f.write(get_dhcp_hosts(context, network_id))
 
+    # Make sure dnsmasq can actually read it (it setuid()s to "nobody")
+    os.chmod(conffile, 0644)
+
     pid = _dnsmasq_pid_for(network_ref['vlan'])
 
     # if dnsmasq is already running, then tell it to reload
@@ -182,7 +194,7 @@
         # TODO(ja): use "/proc/%d/cmdline" % (pid) to determine if pid refers
         #           correct dnsmasq process
         try:
-            os.kill(pid, signal.SIGHUP)
+            _execute('sudo kill -HUP %d' % pid)
             return
         except Exception as exc:  # pylint: disable-msg=W0703
             logging.debug("Hupping dnsmasq threw %s", exc)
@@ -243,7 +255,7 @@
            ' --except-interface=lo',
            ' --dhcp-range=%s,static,120s' % net['dhcp_start'],
            ' --dhcp-hostsfile=%s' % _dhcp_file(net['vlan'], 'conf'),
-           ' --dhcp-script=%s' % _bin_file('nova-dhcpbridge'),
+           ' --dhcp-script=%s' % FLAGS.dhcpbridge,
            ' --leasefile-ro']
     return ''.join(cmd)
 
@@ -254,7 +266,7 @@
 
     if pid:
         try:
-            os.kill(pid, signal.SIGTERM)
+            _execute('sudo kill -TERM %d' % pid)
         except Exception as exc:  # pylint: disable-msg=W0703
             logging.debug("Killing dnsmasq threw %s", exc)
 
@@ -262,14 +274,12 @@
 def _dhcp_file(vlan, kind):
     """Return path to a pid, leases or conf file for a vlan"""
 
+    if not os.path.exists(FLAGS.networks_path):
+        os.makedirs(FLAGS.networks_path)
+
     return os.path.abspath("%s/nova-%s.%s" % (FLAGS.networks_path, vlan, kind))
 
 
-def _bin_file(script):
-    """Return the absolute path to scipt in the bin directory"""
-    return os.path.abspath(os.path.join(__file__, "../../../bin", script))
-
-
 def _dnsmasq_pid_for(vlan):
     """Returns he pid for prior dnsmasq instance for a vlan
 

=== modified file 'nova/network/manager.py'
--- nova/network/manager.py	2010-10-01 09:28:31 +0000
+++ nova/network/manager.py	2010-10-03 18:24:43 +0000
@@ -85,6 +85,12 @@
         self.driver = utils.import_object(network_driver)
         super(NetworkManager, self).__init__(*args, **kwargs)
 
+    def init_host(self):
+        # Set up networking for the projects for which we're already
+        # the designated network host.
+        for network in self.db.host_get_networks(None, self.host):
+            self._on_set_network_host(None, network['id'])
+
     def set_network_host(self, context, project_id):
         """Safely sets the host of the projects network"""
         logging.debug("setting network host")
@@ -240,6 +246,7 @@
         """Do any initialization that needs to be run if this is a
            standalone service.
         """
+        super(VlanManager, self).init_host()
         self.driver.init_host()
 
     def allocate_fixed_ip(self, context, instance_id, *args, **kwargs):
@@ -367,6 +374,7 @@
         self.driver.ensure_vlan_bridge(network_ref['vlan'],
                                        network_ref['bridge'],
                                        network_ref)
+        self.driver.update_dhcp(context, network_id)
 
     @property
     def _bottom_reserved_ips(self):

=== modified file 'nova/tests/network_unittest.py'
--- nova/tests/network_unittest.py	2010-10-01 09:28:31 +0000
+++ nova/tests/network_unittest.py	2010-10-03 18:24:43 +0000
@@ -56,8 +56,8 @@
                                                              'netuser',
                                                              name))
             # create the necessary network data for the project
-            user_context = context.APIRequestContext(project=self.projects[i],
-                                                     user=self.user)
+            user_context = context.get_admin_context(user=self.user)
+
             self.network.set_network_host(user_context, self.projects[i].id)
         instance_ref = self._create_instance(0)
         self.instance_id = instance_ref['id']