launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/config_master_dhcp-fix into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~rvb/maas/config_master_dhcp-fix/+merge/124376

This branch fixes the 'config_master_dhcp' which has been broken by the creating of the nodegroupinterface model.  That problem broke the upgrade (http://paste.ubuntu.com/1203878/). The reason we didn't notice the problem is that MatchesStructure.fromExample seems to be utterly broken or, at least, misused (http://paste.ubuntu.com/1204412/).  I'll see if I can understand better why this is so broken but I'd like to get the fix in first.

This branch changes the 'config_master_dhcp' so that it uses the new nodegroupinterface object to store DHCP information.

= Pre-imp =

This is a rather urgent fix and the fix itself is pretty straightforward (no real architectural decision involved) so no real pre-imp for this but I'll welcome any suggestion made by the reviewer for this of course.

= Notes =

- I added the new required parameter 'ip' to the command.
-- 
https://code.launchpad.net/~rvb/maas/config_master_dhcp-fix/+merge/124376
Your team MAAS Maintainers is requested to review the proposed merge of lp:~rvb/maas/config_master_dhcp-fix into lp:maas.

=== modified file 'src/maasserver/management/commands/config_master_dhcp.py'
--- src/maasserver/management/commands/config_master_dhcp.py	2012-09-12 09:46:16 +0000
+++ src/maasserver/management/commands/config_master_dhcp.py	2012-09-14 10:19:21 +0000
@@ -25,15 +25,20 @@
     )
 
 from django.core.management.base import BaseCommand
-from maasserver.enum import DNS_DHCP_MANAGEMENT
+from maasserver.enum import (
+    DNS_DHCP_MANAGEMENT,
+    NODEGROUPINTERFACE_MANAGEMENT,
+    )
 from maasserver.models import (
     Config,
     NodeGroup,
+    NodeGroupInterface,
     )
 
 
 dhcp_items = {
     'interface': "The network interface that should service DHCP requests.",
+    'ip': "The IP address associated with that network interface.",
     'subnet_mask': "Subnet mask, e.g. 255.0.0.0.",
     'broadcast_ip': "Broadcast address for this subnet, e.g. 10.255.255.255.",
     'router_ip': "Address of default gateway.",
@@ -42,14 +47,6 @@
     }
 
 
-# DHCP settings when disabled.
-clear_settings = {item: None for item in dhcp_items}
-
-# Django is weird with null strings.  Not all settings use None as the
-# not-set value.
-clear_settings['interface'] = ''
-
-
 def get_settings(options):
     """Get the DHCP settings from `options`, as a dict.
 
@@ -98,12 +95,20 @@
         master_nodegroup = NodeGroup.objects.ensure_master()
         if not options.get('ensure'):
             if options.get('clear'):
-                settings = clear_settings
-            else:
-                settings = get_settings(options)
+                master_nodegroup.nodegroupinterface_set.all().delete()
+                return
+
+            settings = get_settings(options)
+            interface = master_nodegroup.get_managed_interface()
+            if interface is None:
+                interface = NodeGroupInterface(
+                    nodegroup=master_nodegroup,
+                    management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+            # TODO: that kind of manipulation should really be done via
+            # a form rather than with 'setattr'.
             for item, value in settings.items():
-                setattr(master_nodegroup, item, value)
-            master_nodegroup.save()
+                setattr(interface, item, value)
+            interface.save()
 
             # Enable DHCP management if it was previously disabled.
             dns_dhcp_management = Config.objects.get_config(

=== modified file 'src/maasserver/tests/test_commands_config_master_dhcp.py'
--- src/maasserver/tests/test_commands_config_master_dhcp.py	2012-09-12 09:46:16 +0000
+++ src/maasserver/tests/test_commands_config_master_dhcp.py	2012-09-14 10:19:21 +0000
@@ -15,7 +15,7 @@
 from optparse import OptionValueError
 
 from django.core.management import call_command
-from maasserver.enum import DNS_DHCP_MANAGEMENT
+from maasserver.enum import DNS_DHCP_MANAGEMENT, NODEGROUPINTERFACE_MANAGEMENT
 from maasserver.management.commands.config_master_dhcp import name_option
 from maasserver.models import (
     Config,
@@ -31,13 +31,13 @@
     """Return the standard, unchanging config for the master nodegroup."""
     return {
         'name': 'master',
-        'worker_ip': '127.0.0.1',
     }
 
 
 def make_dhcp_settings():
     """Return an arbitrary dict of DHCP settings."""
     return {
+        'ip': '10.111.123.10',
         'interface': factory.make_name('interface'),
         'subnet_mask': '255.255.0.0',
         'broadcast_ip': '10.111.255.255',
@@ -65,10 +65,24 @@
         settings = make_dhcp_settings()
         call_command('config_master_dhcp', **settings)
         master = NodeGroup.objects.get(name='master')
+        interface = master.get_managed_interface()
         self.assertThat(
             master,
             MatchesStructure.fromExample(make_master_constants()))
-        self.assertThat(master, MatchesStructure.fromExample(settings))
+        self.assertThat(interface, MatchesStructure.byEquality(**settings))
+        self.assertEqual(
+            NODEGROUPINTERFACE_MANAGEMENT.DHCP, interface.management)
+
+    def test_re_configures_dhcp_for_master_nodegroup(self):
+        management = NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS
+        master = factory.make_node_group(name='master', management=management)
+        settings = make_dhcp_settings()
+        call_command('config_master_dhcp', **settings)
+        master = NodeGroup.objects.get(name='master')
+        interface = master.get_managed_interface()
+        self.assertThat(interface, MatchesStructure.byEquality(**settings))
+        # master's management has been preserved.
+        self.assertEqual(management, interface.management)
 
     def test_clears_dhcp_settings(self):
         master = NodeGroup.objects.ensure_master()
@@ -78,10 +92,8 @@
         call_command('config_master_dhcp', clear=True)
         self.assertThat(
             master,
-            MatchesStructure.fromExample(make_master_constants()))
-        self.assertThat(
-            master,
-            MatchesStructure.fromExample(make_cleared_dhcp_settings()))
+            MatchesStructure.byEquality(**make_master_constants()))
+        self.assertIsNone(master.get_managed_interface())
 
     def test_does_not_accept_partial_dhcp_settings(self):
         settings = make_dhcp_settings()
@@ -96,9 +108,8 @@
         settings['subnet_mask'] = '@%$^&'
         settings['broadcast_ip'] = ''
         call_command('config_master_dhcp', clear=True, **settings)
-        self.assertThat(
-            NodeGroup.objects.get(name='master'),
-            MatchesStructure.fromExample(make_cleared_dhcp_settings()))
+        master = NodeGroup.objects.get(name='master')
+        self.assertIsNone(master.get_managed_interface())
 
     def test_clear_conflicts_with_ensure(self):
         self.assertRaises(
@@ -114,9 +125,8 @@
     def test_ensure_leaves_cleared_settings_cleared(self):
         call_command('config_master_dhcp', clear=True)
         call_command('config_master_dhcp', ensure=True)
-        self.assertThat(
-            NodeGroup.objects.get(name='master'),
-            MatchesStructure.fromExample(make_cleared_dhcp_settings()))
+        master = NodeGroup.objects.get(name='master')
+        self.assertIsNone(master.get_managed_interface())
 
     def test_ensure_leaves_dhcp_settings_intact(self):
         settings = make_dhcp_settings()