Merge lp:~andreserl/maas/distro_series_support into lp:maas/trunk

[Andres Rodriguez]

Adds the UI changes for the Ubuntu Release, as well as the necessary support to deploy machine using the specified release.

[John A Meinel (jameinel)]

maybe if the variable was 'release_series' it might be more obvious how it matches the 'release=' parameter? (In general, the fact that it is sometimes called distro_series, sometimes series, sometimes release is a bit confusing.)

def get_preseed_filenames(node, prefix='', release='', default=False):

Would it be better as 'release=None' ?

Or is that to handle: src/maasserver/templates/maasserver/snippets.html wanting to have a string there.

Otherwise it looks good to me.

review: Approve

[Andres Rodriguez (andreserl)]

Hi John,

Thanks for the review.

1. release parameter: I will address this in a different branch (I had planned to do it either way).

2. release='': It needs to be '' instead of None. We had agreed to default it that way as it requires a string eventually. (as you mention).

[MAAS Lander (maas-lander)]

Attempt to merge into lp:maas failed due to conflicts:

text conflict in src/maasserver/models/config.py

[lp:~andreserl/maas/distro_series_support]

1025. By Andres Rodriguez on 2012-09-19

Resolve conflict on src/maasserver/models/config.py

[Gavin Panella (allenap)]

Looks good, thank you for taking this on. I have a lot of comments,
but they're all fairly minor. However, I think [1] and [8] must be
addressed before landing (well, everything ought to be, but those are
especially important) so Needs Fixing.

[1]

In src/maasserver/api.py:

+    if node is None or node.status == NODE_STATUS.COMMISSIONING:
+        series = Config.objects.get_config('commissioning_distro_series')
+    else:
+        series = node.get_distro_series()

In src/maasserver/models/node.py:

+    def get_distro_series(self):
+        """Return the distro series to install that node."""
+        if not self.distro_series or self.distro_series == DISTRO_SERIES.default:
+            return Config.objects.get_config('default_distro_series')
+        else:
+            return self.distro_series

There's two levels of overriding here: get_distro_series() can return
something other than node.distro_series, and then the code in the API
view can return something else again.

I suggest putting the logic that's in the API view into
get_distro_series, and renaming the method:

    def get_effective_distro_series(self):
        """Return the distro series to install that node."""
        if node is None or node.status == NODE_STATUS.COMMISSIONING:
            return Config.objects.get_config('commissioning_distro_series')
        elif not self.distro_series or self.distro_series == DISTRO_SERIES.default:
            return Config.objects.get_config('default_distro_series')
        else:
            return self.distro_series

[2]

     params = KernelParameters(
-        arch=arch, subarch=subarch, release=release, purpose=purpose,
+        arch=arch, subarch=subarch, release=series, purpose=purpose,

That release argument ought to be renamed. File a bug if it's too much
to do here.

[3]

Before landing, please run `make lint` and fix all warnings.

[4]

Also before landing, please run:

  utilities/format-new-and-modified-imports -r submit:

and commit the changes it makes.

[5]

+    distro_series = CharField(
+        max_length=10, choices=DISTRO_SERIES_CHOICES, null=True,
+        blank=True, default=None)

To avoid mix-ups between None and "", I think this should be defined
as:

    distro_series = CharField(
        max_length=10, choices=DISTRO_SERIES_CHOICES, null=False,
        blank=True, default="")

Also, a maximum length of 10 seems tight. As Django should be
selecting a VARCHAR type in PostgreSQL here, the storage will be the
same:

  The storage requirement for a short string (up to 126 bytes) is 1
  byte plus the actual string, ...

  http://www.postgresql.org/docs/9.1/interactive/datatype-character.html

Make it at least 20, because it's not going to make any difference to
storage or performance, and because having to change it later will be
a much bigger PITA.

[6]

+    <label for="id_distro_series">Release</label>
+    {{ node_form.distro_series }}

s/Release/Series/ ?

Also, how about showing the effective series?

[7]

+    distro_series = forms.ChoiceField(
+        choices=DISTRO_SERIES_CHOICES, required=False,
+        initial=DISTRO_SERIES.default,
+        label="Release",
+        error_messages={'invalid_choi...

review: Needs Fixing

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-09-18 16:00:45 +0000
+++ src/maasserver/api.py	2012-09-19 14:12:24 +0000
@@ -1353,14 +1353,17 @@
         preseed_url = compose_preseed_url(node)
         hostname = node.hostname
 
-    # XXX JeroenVermeulen 2012-08-06 bug=1013146: Stop hard-coding this.
-    release = 'precise'
+    if node is None or node.status == NODE_STATUS.COMMISSIONING:
+        series = Config.objects.get_config('commissioning_distro_series')
+    else:
+        series = node.get_distro_series()
+
     purpose = get_boot_purpose(node)
     domain = 'local.lan'  # TODO: This is probably not enough!
     server_address = get_maas_facing_server_address()
 
     params = KernelParameters(
-        arch=arch, subarch=subarch, release=release, purpose=purpose,
+        arch=arch, subarch=subarch, release=series, purpose=purpose,
         hostname=hostname, domain=domain, preseed_url=preseed_url,
         log_host=server_address, fs_host=server_address)
 

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-09-18 16:36:51 +0000
+++ src/maasserver/forms.py	2012-09-19 14:12:24 +0000
@@ -59,6 +59,8 @@
     NODEGROUP_STATUS,
     NODEGROUPINTERFACE_MANAGEMENT,
     NODEGROUPINTERFACE_MANAGEMENT_CHOICES,
+    DISTRO_SERIES,
+    DISTRO_SERIES_CHOICES,
     )
 from maasserver.fields import MACAddressFormField
 from maasserver.models import (
@@ -97,6 +99,9 @@
 INVALID_ARCHITECTURE_MESSAGE = compose_invalid_choice_text(
     'architecture', ARCHITECTURE_CHOICES)
 
+INVALID_DISTRO_SERIES_MESSAGE = compose_invalid_choice_text(
+    'distro_series', DISTRO_SERIES_CHOICES)
+
 
 class NodeForm(ModelForm):
     after_commissioning_action = forms.TypedChoiceField(
@@ -104,6 +109,12 @@
         choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES, required=False,
         empty_value=NODE_AFTER_COMMISSIONING_ACTION.DEFAULT)
 
+    distro_series = forms.ChoiceField(
+        choices=DISTRO_SERIES_CHOICES, required=False,
+        initial=DISTRO_SERIES.default,
+        label="Release",
+        error_messages={'invalid_choice': INVALID_DISTRO_SERIES_MESSAGE})
+
     architecture = forms.ChoiceField(
         choices=ARCHITECTURE_CHOICES, required=True,
         initial=ARCHITECTURE.i386,
@@ -115,6 +126,7 @@
             'hostname',
             'after_commissioning_action',
             'architecture',
+            'distro_series',
             )
 
 
@@ -154,6 +166,7 @@
             'hostname',
             'after_commissioning_action',
             'architecture',
+            'distro_series',
             'power_type',
             'power_parameters',
             )

=== modified file 'src/maasserver/models/config.py'
--- src/maasserver/models/config.py	2012-09-18 16:36:51 +0000
+++ src/maasserver/models/config.py	2012-09-19 14:12:24 +0000
@@ -26,7 +26,10 @@
     )
 from django.db.models.signals import post_save
 from maasserver import DefaultMeta
-from maasserver.enum import NODE_AFTER_COMMISSIONING_ACTION
+from maasserver.enum import (
+    DISTRO_SERIES,
+    NODE_AFTER_COMMISSIONING_ACTION,
+    )
 from maasserver.fields import JSONObjectField
 from provisioningserver.enum import POWER_TYPE
 
@@ -49,6 +52,8 @@
         'maas_name': gethostname(),
         'enlistment_domain': b'local',
         ## /settings
+        'default_distro_series': DISTRO_SERIES.precise,
+        'commissioning_distro_series': DISTRO_SERIES.precise,
         }
 
 

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-09-18 17:58:14 +0000
+++ src/maasserver/models/node.py	2012-09-19 14:12:24 +0000
@@ -359,6 +359,10 @@
         max_length=10, choices=DISTRO_SERIES_CHOICES, null=True,
         blank=True, default='')
 
+    distro_series = CharField(
+        max_length=10, choices=DISTRO_SERIES_CHOICES, null=True,
+        blank=True, default=None)
+
     architecture = CharField(
         max_length=10, choices=ARCHITECTURE_CHOICES, blank=False,
         default=ARCHITECTURE.i386)
@@ -556,6 +560,13 @@
         else:
             return None
 
+    def get_distro_series(self):
+        """Return the distro series to install that node."""
+        if not self.distro_series or self.distro_series == DISTRO_SERIES.default:
+            return Config.objects.get_config('default_distro_series')
+        else:
+            return self.distro_series
+
     def get_effective_power_parameters(self):
         """Return effective power parameters, including any defaults."""
         if self.power_parameters:

=== modified file 'src/maasserver/preseed.py'
--- src/maasserver/preseed.py	2012-08-21 20:27:47 +0000
+++ src/maasserver/preseed.py	2012-09-19 14:12:24 +0000
@@ -30,6 +30,7 @@
     )
 from maasserver.server_address import get_maas_facing_server_host
 from maasserver.utils import absolute_reverse
+from maasserver.models import Config
 import tempita
 
 
@@ -54,28 +55,26 @@
     return render_preseed(None, PRESEED_TYPE.ENLIST_USERDATA)
 
 
-# XXX: rvb 2012-06-21 bug=1013146:  'precise' is hardcoded here.
-def get_preseed(node, release="precise"):
+def get_preseed(node):
     """Return the preseed for a given node.  Depending on the node's status
     this will be a commissioning preseed (if the node is commissioning) or the
     standard preseed (normal installation preseed).
 
     :param node: The node to return preseed for.
     :type node: :class:`maasserver.models.Node`
-    :param release: The Ubuntu release to be used.
-    :type release: basestring
     :return: The rendered preseed string.
     :rtype: basestring.
     """
     if node.status == NODE_STATUS.COMMISSIONING:
         return render_preseed(
-            node, PRESEED_TYPE.COMMISSIONING, release=release)
+            node, PRESEED_TYPE.COMMISSIONING, 
+            release=Config.objects.get_config('commissioning_distro_series'))
     else:
-        return render_preseed(node, PRESEED_TYPE.DEFAULT, release=release)
-
-
-# XXX: rvb 2012-06-14 bug=1013146:  'precise' is hardcoded here.
-def get_preseed_filenames(node, prefix='', release='precise', default=False):
+        return render_preseed(node, PRESEED_TYPE.DEFAULT,
+            release=node.get_distro_series())
+
+
+def get_preseed_filenames(node, prefix='', release='', default=False):
     """List possible preseed template filenames for the given node.
 
     :param node: The node to return template preseed filenames for.
@@ -174,8 +173,7 @@
         self.name = name
 
 
-# XXX: rvb 2012-06-18 bug=1013146:  'precise' is hardcoded here.
-def load_preseed_template(node, prefix, release="precise"):
+def load_preseed_template(node, prefix, release=''):
     """Find and load a `PreseedTemplate` for the given node.
 
     :param node: See `get_preseed_filenames`.
@@ -201,8 +199,7 @@
     return get_template(prefix, None, default=True)
 
 
-# XXX: rvb 2012-06-19 bug=1013146:  'precise' is hardcoded here.
-def get_preseed_context(node, release="precise"):
+def get_preseed_context(node, release=''):
     """Return the context dictionary to be used to render preseed templates
     for this node.
 
@@ -236,8 +233,7 @@
     return context
 
 
-# XXX: rvb 2012-06-19 bug=1013146:  'precise' is hardcoded here.
-def render_preseed(node, prefix, release="precise"):
+def render_preseed(node, prefix, release=''):
     """Find and load a `PreseedTemplate` for the given node.
 
     :param node: See `get_preseed_filenames`.

=== modified file 'src/maasserver/templates/maasserver/snippets.html'
--- src/maasserver/templates/maasserver/snippets.html	2012-06-11 12:25:37 +0000
+++ src/maasserver/templates/maasserver/snippets.html	2012-09-19 14:12:24 +0000
@@ -20,6 +20,10 @@
     {{ node_form.after_commissioning_action }}
   </p>
   <p>
+    <label for="id_distro_series">Release</label>
+    {{ node_form.distro_series }}
+  </p>
+  <p>
     <label for="id_architecture">Architecture</label>
     {{ node_form.architecture }}
   </p>

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-09-18 07:51:29 +0000
+++ src/maasserver/tests/test_forms.py	2012-09-19 14:12:24 +0000
@@ -226,6 +226,7 @@
                 'hostname',
                 'after_commissioning_action',
                 'architecture',
+                'distro_series',
             ], list(form.fields))
 
     def test_NodeForm_changes_node(self):
@@ -256,6 +257,7 @@
                 'hostname',
                 'after_commissioning_action',
                 'architecture',
+                'distro_series',
                 'power_type',
                 'power_parameters',
             ],