launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/master-nodegroup into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/master-nodegroup/+merge/115503

Actually what this amounts to is configuring DHCP.  It must be done at least once for every MAAS installation, but the command can be used in 3 ways:

1. --clear: Clear all DHCP settings for the master node group.

Use this when MAAS is being configured such that it won't serve DHCP from the main MAAS server.

2. --ensure: Initialize if that hasn't been done already, but provide no DHCP settings.

Use this to initialize things, when you're not sure if maybe MAAS has already been set up to serve DHCP and there's no change in whatever configuration was chosen.  If no configuration had been set yet, this will do the same as --clear.

3. With a complete set of DHCP settings: configure DHCP.

This is where you provide DHCP settings from the shell, for configuring the master worker's DHCP server.

When you specify --clear or --ensure, any DHCP settings are ignored.  This was done to give scripts freedom in how they put together the options for this command.  It's not an error to pass a nonsensical or even an empty subnet mask, for example, if DHCP is not going to be configured anyway.

On the other hand, --clear and --ensure do conflict with one another.

Underneath it all, a call on the NodeGroup manager transparently produces (in either sense: deliver or create) the master node group.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/master-nodegroup/+merge/115503
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/master-nodegroup into lp:maas.

=== added file 'src/maasserver/management/commands/config_master_dhcp.py'
--- src/maasserver/management/commands/config_master_dhcp.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/management/commands/config_master_dhcp.py	2012-07-18 10:03:25 +0000
@@ -0,0 +1,96 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Django command: Configure master DHCP.
+
+The master DHCP settings apply to the DHCP server running on the MAAS server
+itself.  They can be either disabled (if you don't want MAAS to manage DHCP)
+or fully configured using this command.
+"""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'Command',
+    ]
+
+from optparse import (
+    make_option,
+    OptionValueError,
+    )
+
+from django.core.management.base import BaseCommand
+from maasserver.models import NodeGroup
+
+
+dhcp_items = {
+    '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.",
+    'ip_range_low': "Lowest IP address to assign to clients.",
+    'ip_range_high': "Highest IP address to assign to clients.",
+    }
+
+
+# DHCP settings when disabled.
+clear_settings = {item: None for item in dhcp_items}
+
+
+def get_settings(options):
+    """Get the DHCP settings from `options`, as a dict.
+
+    Checks validity of the settings.
+    """
+    settings = {
+        item: options.get(item)
+        for item in dhcp_items}
+    if not all(settings.values()):
+        raise OptionValueError(
+            "Specify all DHCP settings: %s" % ', '.join(dhcp_items))
+    return settings
+
+
+def name_option(dhcp_setting):
+    """Formulate the option name corresponding to a DHCP setting name."""
+    return '--%s' % dhcp_setting.replace('_', '-')
+
+
+class Command(BaseCommand):
+
+    option_list = BaseCommand.option_list + (
+        make_option(
+            '--clear', dest='clear', action='store_true', default=False,
+            help=(
+                "Clear settings.  Do only when MAAS DHCP is disabled.  "
+                "If given, any DHCP parameters are ignored.")),
+        make_option(
+            '--ensure', dest='ensure', action='store_true', default=False,
+            help=(
+                "Ensure that the master node group is configured, "
+                "but if it was already set up, don't change its settings.  "
+                "If given, any DHCP parameters are ignored.")),
+        ) + tuple(
+            make_option(
+                name_option(item), dest=item, default=None,
+                help="DHCP parameter: %s" % help)
+            for item, help in dhcp_items.items())
+    help = "Initialize master DHCP settings."
+
+    def handle(self, *args, **options):
+        if options.get('ensure') and options.get('clear'):
+            raise OptionValueError(
+                "The --ensure option conflicts with --clear.")
+        master_nodegroup = NodeGroup.objects.ensure_master()
+        if not options.get('ensure'):
+            if options.get('clear'):
+                settings = clear_settings
+            else:
+                settings = get_settings(options)
+            for item, value in settings.items():
+                setattr(master_nodegroup, item, value)
+            master_nodegroup.save()

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2012-07-17 10:18:42 +0000
+++ src/maasserver/models/nodegroup.py	2012-07-18 10:03:25 +0000
@@ -69,6 +69,13 @@
         nodegroup.save()
         return nodegroup
 
+    def ensure_master(self):
+        """Obtain the master node group, creating it first if needed."""
+        try:
+            return self.get(name='master')
+        except NodeGroup.DoesNotExist:
+            return self.new('master', '127.0.0.1')
+
 
 class NodeGroup(TimestampedModel):
 

=== added file 'src/maasserver/tests/test_commands_config_master_dhcp.py'
--- src/maasserver/tests/test_commands_config_master_dhcp.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_commands_config_master_dhcp.py	2012-07-18 10:03:25 +0000
@@ -0,0 +1,119 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the config_master_dhcp command."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from optparse import OptionValueError
+
+from django.core.management import call_command
+from maasserver.management.commands.config_master_dhcp import name_option
+from maasserver.models import NodeGroup
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestCase
+from testtools.matchers import MatchesStructure
+
+
+def make_master_constants():
+    """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 {
+        'subnet_mask': '255.255.0.0',
+        'broadcast_ip': '10.111.255.255',
+        'router_ip': factory.getRandomIPAddress(),
+        'ip_range_low': '10.111.123.9',
+        'ip_range_high': '10.111.244.18',
+    }
+
+
+def make_cleared_dhcp_settings():
+    """Return dict of cleared DHCP settings."""
+    return {
+        setting: None
+        for setting in make_dhcp_settings().keys()}
+
+
+class TestConfigMasterDHCP(TestCase):
+
+    def test_configures_dhcp_for_master_nodegroup(self):
+        settings = make_dhcp_settings()
+        call_command('config_master_dhcp', **settings)
+        master = NodeGroup.objects.get(name='master')
+        self.assertThat(
+            master,
+            MatchesStructure.fromExample(make_master_constants()))
+        self.assertThat(master, MatchesStructure.fromExample(settings))
+
+    def test_clears_dhcp_settings(self):
+        master = NodeGroup.objects.ensure_master()
+        for attribute, value in make_dhcp_settings().items():
+            setattr(master, attribute, value)
+        master.save()
+        call_command('config_master_dhcp', clear=True)
+        self.assertThat(
+            master,
+            MatchesStructure.fromExample(make_master_constants()))
+        self.assertThat(
+            master,
+            MatchesStructure.fromExample(make_cleared_dhcp_settings()))
+
+    def test_does_not_accept_partial_dhcp_settings(self):
+        settings = make_dhcp_settings()
+        del settings['subnet_mask']
+        self.assertRaises(
+            OptionValueError,
+            call_command, 'config_master_dhcp', **settings)
+
+    def test_ignores_nonsense_settings_when_clear_is_passed(self):
+        settings = make_dhcp_settings()
+        call_command('config_master_dhcp', **settings)
+        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()))
+
+    def test_clear_conflicts_with_ensure(self):
+        self.assertRaises(
+            OptionValueError,
+            call_command, 'config_master_dhcp', clear=True, ensure=True)
+
+    def test_ensure_creates_master_nodegroup_without_dhcp_settings(self):
+        call_command('config_master_dhcp', ensure=True)
+        self.assertThat(
+            NodeGroup.objects.get(name='master'),
+            MatchesStructure.fromExample(make_cleared_dhcp_settings()))
+
+    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()))
+
+    def test_ensure_leaves_dhcp_settings_intact(self):
+        settings = make_dhcp_settings()
+        call_command('config_master_dhcp', **settings)
+        call_command('config_master_dhcp', ensure=True)
+        self.assertThat(
+            NodeGroup.objects.get(name='master'),
+            MatchesStructure.fromExample(settings))
+
+    def test_name_option_turns_dhcp_setting_name_into_option(self):
+        self.assertEqual('--subnet-mask', name_option('subnet_mask'))

=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py	2012-07-18 04:01:06 +0000
+++ src/maasserver/tests/test_nodegroup.py	2012-07-18 10:03:25 +0000
@@ -74,3 +74,33 @@
         self.assertIsNotNone(nodegroup.api_key)
         self.assertEqual(get_worker_user(), nodegroup.api_token.user)
         self.assertEqual(nodegroup.api_key, nodegroup.api_token.key)
+
+    def test_ensure_master_creates_minimal_master_nodegroup(self):
+        self.assertThat(
+            NodeGroup.objects.ensure_master(),
+            MatchesStructure.fromExample({
+            'name': 'master',
+            'worker_ip': '127.0.0.1',
+            'subnet_mask': None,
+            'broadcast_ip': None,
+            'router_ip': None,
+            'ip_range_low': None,
+            'ip_range_high': None,
+        }))
+
+    def test_writes_master_nodegroup_to_database(self):
+        master = NodeGroup.objects.ensure_master()
+        self.assertEqual(
+            master.id, NodeGroup.objects.get(name=master.name).id)
+
+    def test_ensure_master_returns_same_nodegroup_every_time(self):
+        self.assertEqual(
+            NodeGroup.objects.ensure_master().id,
+            NodeGroup.objects.ensure_master().id)
+
+    def test_ensure_master_preserves_existing_attributes(self):
+        master = NodeGroup.objects.ensure_master()
+        ip = factory.getRandomIPAddress()
+        master.worker_ip = ip
+        master.save()
+        self.assertEqual(ip, NodeGroup.objects.ensure_master().worker_ip)