I am new to a lot of this part of MAAS and as a result, this review is probably more of the trees than of the forest. Over all it looks good to me, but I do have some questions and comments. [1] Needs make format/make lint. [2] @@ -403,7 +407,7 @@ available to the nodes through the metadata service. :type user_data: base64-encoded unicode :param distro_series: If present, this parameter specifies the - Ubuntu Release the node will use. + os relase the node will use. Typo 'relase'. Maybe s/os/OS too. [3] -def get_boot_purpose(node): +def get_boot_purpose(node, osystem, arch, subarch, series, label): """Return a suitable "purpose" for this boot, e.g. "install".""" # XXX: allenap bug=1031406 2012-07-31: The boot purpose is still in # flux. It may be that there will just be an "ephemeral" environment and @@ -2364,7 +2368,15 @@ if node.should_use_traditional_installer(): return "install" else: - return "xinstall" + # Check that the booting operating system, actually supports + # fast-path installation. If it does not then, we need to + # return normal install. + osystem_obj = OperatingSystemRegistry[osystem] + purposes = osystem_obj.get_boot_image_purposes( + arch, subarch, series, label) + if BOOT_IMAGE_PURPOSE.XINSTALL in purposes: + return "xinstall" + return "install" Why isn't this error state handled when we configure the node? It is confusing to have a node configured for xinstall, but to have it really use install behind the scenes. [4] + # Get the purpose, checking that if node is using xinstall, the operating + # system actaully supports that mode. We pass None for the label, here + # because it is unknown which label will be used yet. + purpose = get_boot_purpose(node, osystem, arch, subarch, series, None) Same comment as [3]. Also, don't need the comma between 'label' and 'here'. [5] label = get_optional_param(request.GET, 'label', latest_label) + # Now that we have the correct label, lets check the boot purpose again + # to make sure that the boot image with that label, is the correct purpose. + purpose = get_boot_purpose(node, osystem, arch, subarch, series, label) Same comment as [3] again - didn't need to grab purpose more than once with the previous implementation. Also, s/lets/let's and drop comma between 'label' and 'is'. === modified file 'src/maasserver/forms.py' --- src/maasserver/forms.py 2014-05-14 07:12:10 +0000 +++ src/maasserver/forms.py 2014-05-14 14:40:00 +0000 @@ -71,9 +71,6 @@ ) from maasserver.config_forms import SKIP_CHECK_NAME from maasserver.enum import ( - COMMISSIONING_DISTRO_SERIES_CHOICES, - DISTRO_SERIES, - DISTRO_SERIES_CHOICES, NODE_STATUS, NODEGROUPINTERFACE_MANAGEMENT, NODEGROUPINTERFACE_MANAGEMENT_CHOICES, @@ -86,7 +83,7 @@ from maasserver.forms_settings import ( CONFIG_ITEMS_KEYS, get_config_field, - INVALID_DISTRO_SERIES_MESSAGE, + list_commisioning_choices, INVALID_SETTING_MSG_TEMPLATE, ) from maasserver.models import ( @@ -113,6 +110,7 @@ from maasserver.utils.network import make_network from metadataserver.fields import Bin from metadataserver.models import CommissioningScript +from provisioningserver.driver import OperatingSystemRegistry # A reusable null-option for choice fields. BLANK_CHOICE = ('', '-------') @@ -202,6 +200,81 @@ return all_architectures[0] [6] +def clean_distro_series_field(form, field, os_field): + # distro_series field can be supplied the value os/release, that is the + # way the web UI provides the value. + new_distro_series = form.cleaned_data.get(field) + if new_distro_series is not None and '/' in new_distro_series: + os, release = new_distro_series.split('/', 1) + if 'os' in form.cleaned_data: + new_os = form.cleaned_data[os_field] + if os != new_os: + raise ValidationError( + "%s in distro_series does not match with " + "operating system %" % (field, os)) + return release + return new_distro_series I'm confused here - why does this method need to handle two forms of input? This code may be clearer like this: if new_distro_series is None or '/' not in new_distro_series: return new_distro_series os, release = ... You could even split the second part out into a second function to make it easier to test. [7] +def validate_osystem(name): + """Validator for operating system. + + :raise ValidationError: If invalid operating system selected. + """ + if name is not None and name != '': + osystem = OperatingSystemRegistry.get_item(name) + if osystem is None: + raise ValidationError( + "Value u'%s' is not a valid choice." % name) In general it's nice to return early if possible, to prevent code from scrolling to the right and to make it clear nothing else happens in that case: if name is None or name == '': return osystem = ... [8] +def get_all_releases(): + """Gets all the releases that is supported.""" s/is/are [9] class NodeManager(Manager): """A utility to manage the collection of Nodes.""" @@ -478,11 +517,12 @@ User, default=None, blank=True, null=True, editable=False) osystem = CharField( - max_length=20, null=True, blank=True, default='') + max_length=20, null=True, blank=True, default='', + validators=[validate_osystem]) distro_series = CharField( - max_length=20, choices=DISTRO_SERIES_CHOICES, null=True, - blank=True, default='') + max_length=20, null=True, blank=True, default='', + validators=[validate_distro_series]) There is a merge conflict with trunk here. architecture = CharField(max_length=31, blank=False) @@ -777,18 +817,38 @@ """The name of the queue for tasks specific to this node.""" return self.nodegroup.work_queue [10] + def get_osystem(self): + """Return the operating system to install that node.""" + use_default_osystem = (self.osystem is None or self.osystem == '') + if use_default_osystem: + return Config.objects.get_config('default_osystem') + else: + return self.osystem We're often checking for both None and '' - where does the need to do that originate? Why not one or the other? === modified file 'src/maasserver/models/tests/test_node.py' --- src/maasserver/models/tests/test_node.py 2014-05-09 06:04:54 +0000 +++ src/maasserver/models/tests/test_node.py 2014-05-14 14:40:00 +0000 @@ -20,7 +20,6 @@ from django.core.exceptions import ValidationError from maasserver.clusterrpc.power_parameters import get_power_types from maasserver.enum import ( - DISTRO_SERIES, NODE_PERMISSION, NODE_STATUS, NODE_STATUS_CHOICES, @@ -270,17 +269,31 @@ offset += timedelta(1) self.assertEqual(macs[0], node.get_primary_mac().mac_address) [11] - def test_get_distro_series_returns_default_series(self): + def test_get_osystem_returns_default_osystem_and_series(self): node = factory.make_node() - series = Config.objects.get_config('commissioning_distro_series') + osystem = Config.objects.get_config('default_osystem') + series = Config.objects.get_config('default_distro_series') + self.assertEqual(osystem, node.get_osystem()) self.assertEqual(series, node.get_distro_series()) You're testing two different config bits here - why not two tests? === modified file 'src/maasserver/tests/test_api_node.py' --- src/maasserver/tests/test_api_node.py 2014-04-25 18:03:38 +0000 +++ src/maasserver/tests/test_api_node.py 2014-05-14 14:40:00 +0000 @@ -23,7 +23,6 @@ import bson from django.core.urlresolvers import reverse from maasserver.enum import ( - DISTRO_SERIES, NODE_STATUS, NODE_STATUS_CHOICES_DICT, ) @@ -221,18 +220,23 @@ self.assertEqual( node.system_id, json.loads(response.content)['system_id']) [12] - def test_POST_start_sets_distro_series(self): + def test_POST_start_sets_osystem_and_distro_series(self): node = factory.make_node( owner=self.logged_in_user, mac=True, power_type='ether_wake') - distro_series = factory.getRandomEnum(DISTRO_SERIES) + osystem = factory.getRandomOS() + distro_series = factory.getRandomRelease(osystem) response = self.client.post( - self.get_node_uri(node), - {'op': 'start', 'distro_series': distro_series}) + self.get_node_uri(node), { + 'op': 'start', + 'distro_series': distro_series + }) self.assertEqual( (httplib.OK, node.system_id), (response.status_code, json.loads(response.content)['system_id'])) self.assertEqual( + osystem.name, reload_object(node).osystem) + self.assertEqual( distro_series, reload_object(node).distro_series) def test_POST_start_validates_distro_series(self): @@ -300,18 +304,23 @@ self.client.post(self.get_node_uri(node), {'op': 'release'}) self.assertTrue(reload_object(node).netboot) Does this test anything beyond what test_set_distro_series_returns_osystem_series already tests? Could this fail without test_set_distro_series_returns_osystem_series failing? [13] - def test_POST_release_resets_distro_series(self): + def test_POST_release_resets_osystem_and_distro_series(self): Same question as [12]. [14] + def test_list_all_usable_releases_combines_nodegroups(self): + expected = {} + osystems = [] + os_names = [factory.make_name('os') for _ in range(3)] I've seen the make_names os/release list comprehension a few times, maybe factor it out to 'make_name_list(prefix, count)? There is also factory.make_names(['os'] * 3). [15] + for name in os_names: + releases = [factory.make_name('release') for _ in range(3)] + for release in releases: + self.make_usable_boot_images(osystem=name, release=release) + osystems.append( + make_usable_osystem(self, name, releases=releases)) + expected[name] = releases + self.assertItemsEqual(expected, list_all_usable_releases(osystems)) + + def test_list_all_usable_releases_sorts_output(self): + expected = {} + osystems = [] + os_names = [factory.make_name('os') for _ in range(3)] + for name in os_names: + releases = [factory.make_name('release') for _ in range(3)] + for release in releases: + self.make_usable_boot_images(osystem=name, release=release) + osystems.append( + make_usable_osystem(self, name, releases=releases)) + expected[name] = sorted(releases) + self.assertEqual(expected, list_all_usable_releases(osystems)) These last two tests are mostly duplicate code and could be refactored to not be. [16] Additional test cases I think there are some areas that could use additional test cases. maasserver.api.get_boot_purpose(): No test for the return 'install' case. I've been trying to cover each new method with a set of explicit tests, since it's easier to reason about tests for small, isolated chunks of code than it is for big chunks with deep call stacks. These new methods have no excplit tests and aren't trivial, so they seem like they'd be unlikely to get implicit coverage: NodeForm.set_up_osystem_and_distro_series_fields() maasserver.forms.clean_distro_series_field() maasserver.models.node.validate_osystem() maasserver.models.node.validate_distro_series() These new methods have no explicit tests but are fairly simple and could be getting decent coverage implicitly: maasserver.forms.list_release_choices() maasserver.forms.list_osystem_choices() NodeForm.clean_distro_series() maasserver.form_settings.list_osystem_choices() maasserver.form_settings.list_release_choices() maasserver.form_settings.list_commissioning_choices() maasserver.models.node.get_all_releases() maasserver.modells.node.get_osystem_from_release() DeployForm has no tests in maasserver.tests.test_forms. I think it gets covered via def test_settings_deploy_POST() though.