launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06643
[Merge] lp:~jtv/maas/select-profile into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/select-profile into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/select-profile/+merge/96974
As far as we can see now, a node's profile will always be <Ubuntu release>-<architecture> as per convention. The Ubuntu release, for now, is hard-coded to Precise.
The provisioning API requires a profile when creating a node, and the profile must exist in Cobbler. Therefore, barring further-reaching changes, the architecture just became mandatory. We can't select architectures in the UI yet, so I used a dirty but quick trick to make it “default” (not really, but close enough for now) to i386. See the comments.
There's another change. Previously, the code that created a system in Cobbler (in a post-save trigger on Node, which seems iffy) would check if the node already had a profile and if so, keep it. That's no good: it means we can't choose to boot a node into a different profile without a separate call to modify the profile, e.g. if its architecture is ever changed from i386 to amd64. It also means that we're relying on Cobbler to store our data for us. I made node creation overwrite any previous profile in accordance with our architecture.
Jeroen
--
https://code.launchpad.net/~jtv/maas/select-profile/+merge/96974
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/select-profile into lp:maas.
=== modified file 'src/maasserver/fixtures/dev_fixture.yaml'
--- src/maasserver/fixtures/dev_fixture.yaml 2012-03-08 08:50:46 +0000
+++ src/maasserver/fixtures/dev_fixture.yaml 2012-03-12 07:58:18 +0000
@@ -1,20 +1,35 @@
-- fields: {created: 2012-01-24, hostname: sun, owner: null, status: 0, system_id: node-2666dd64-4671-11e1-93b8-00225f89f211,
+- fields: {
+ created: 2012-01-24, hostname: sun, architecture: i386,
+ owner: null, status: 0,
+ system_id: node-2666dd64-4671-11e1-93b8-00225f89f211,
updated: !!timestamp '2012-01-24 10:52:38.735954'}
model: maasserver.node
pk: 15
-- fields: {created: 2012-01-24, hostname: moon, owner: 106, status: 2, system_id: node-29d7ad70-4671-11e1-93b8-00225f89f211,
+- fields: {
+ created: 2012-01-24, hostname: moon, architecture: i386,
+ owner: 106, status: 2,
+ system_id: node-29d7ad70-4671-11e1-93b8-00225f89f211,
updated: !!timestamp '2012-01-24 10:52:44.507777'}
model: maasserver.node
pk: 16
-- fields: {created: 2012-01-24, hostname: mars, owner: 106, status: 3, system_id: node-2d424b28-4671-11e1-93b8-00225f89f211,
+- fields: {
+ created: 2012-01-24, hostname: mars, architecture: i386,
+ owner: 106, status: 3,
+ system_id: node-2d424b28-4671-11e1-93b8-00225f89f211,
updated: !!timestamp '2012-01-24 10:52:50.239704'}
model: maasserver.node
pk: 17
-- fields: {created: 2012-01-24, hostname: jupiter, owner: null, status: 0, system_id: node-2fb4fec8-4671-11e1-93b8-00225f89f211,
+- fields: {
+ created: 2012-01-24, hostname: jupiter, architecture: i386,
+ owner: null, status: 0,
+ system_id: node-2fb4fec8-4671-11e1-93b8-00225f89f211,
updated: !!timestamp '2012-01-24 10:52:54.346832'}
model: maasserver.node
pk: 18
-- fields: {created: 2012-01-24, hostname: saturn, owner: 101, status: 3, system_id: node-33b55e28-4671-11e1-93b8-00225f89f211,
+- fields: {
+ created: 2012-01-24, hostname: saturn, architecture: i386,
+ owner: 101, status: 3,
+ system_id: node-33b55e28-4671-11e1-93b8-00225f89f211,
updated: !!timestamp '2012-01-24 10:53:01.060176'}
model: maasserver.node
pk: 19
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-03-07 08:32:33 +0000
+++ src/maasserver/forms.py 2012-03-12 07:58:18 +0000
@@ -33,6 +33,7 @@
)
from maasserver.fields import MACAddressFormField
from maasserver.models import (
+ ARCHITECTURE,
ARCHITECTURE_CHOICES,
Config,
MACAddress,
@@ -55,8 +56,13 @@
after_commissioning_action = forms.TypedChoiceField(
choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES, required=False,
empty_value=NODE_AFTER_COMMISSIONING_ACTION.DEFAULT)
- architecture = forms.ChoiceField(
+ # The architecture field is not visible yet, but requires a choice.
+ # Faking it by setting 'i386' as the representation for "none
+ # selected." Once this field becomes meaningful, it may simply have
+ # to become mandatory.
+ architecture = forms.TypedChoiceField(
choices=ARCHITECTURE_CHOICES, required=False,
+ empty_value=ARCHITECTURE.i386,
error_messages={'invalid_choice': INVALID_ARCHITECTURE_MESSAGE})
class Meta:
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-03-06 08:48:58 +0000
+++ src/maasserver/models.py 2012-03-12 07:58:18 +0000
@@ -363,8 +363,9 @@
choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
default=NODE_AFTER_COMMISSIONING_ACTION.DEFAULT)
- architecture = models.CharField(max_length=10,
- choices=ARCHITECTURE_CHOICES, blank=True)
+ architecture = models.CharField(
+ max_length=10, choices=ARCHITECTURE_CHOICES, blank=False,
+ default=ARCHITECTURE.i386)
objects = NodeManager()
=== modified file 'src/maasserver/provisioning.py'
--- src/maasserver/provisioning.py 2012-03-07 18:25:48 +0000
+++ src/maasserver/provisioning.py 2012-03-12 07:58:18 +0000
@@ -81,20 +81,17 @@
}
+def select_profile_for_node(node, papi):
+ """Select which profile a node should be configured for."""
+ assert node.architecture, "Node's architecture is not known."
+ return "%s-%s" % ("precise", node.architecture)
+
+
@receiver(post_save, sender=Node)
def provision_post_save_Node(sender, instance, created, **kwargs):
"""Create or update nodes in the provisioning server."""
papi = get_provisioning_api_proxy()
- nodes = papi.get_nodes_by_name([instance.system_id])
- if instance.system_id in nodes:
- profile = nodes[instance.system_id]["profile"]
- else:
- # TODO: Choose a sensible profile.
- profiles = papi.get_profiles()
- assert len(profiles) >= 1, (
- "No profiles defined in Cobbler; has "
- "cobbler-ubuntu-import been run?")
- profile = sorted(profiles)[0]
+ profile = select_profile_for_node(instance, papi)
papi.add_node(instance.system_id, profile, compose_metadata(instance))
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py 2012-03-07 08:32:33 +0000
+++ src/maasserver/testing/factory.py 2012-03-12 07:58:18 +0000
@@ -19,6 +19,7 @@
from django.contrib.auth.models import User
from maasserver.models import (
+ ARCHITECTURE,
FileStorage,
MACAddress,
Node,
@@ -61,13 +62,15 @@
return random.choice(choices)[0]
def make_node(self, hostname='', set_hostname=False, status=None,
- **kwargs):
+ architecture=ARCHITECTURE.i386, **kwargs):
# hostname=None is a valid value, hence the set_hostname trick.
if hostname is '' and set_hostname:
hostname = self.getRandomString(255)
if status is None:
status = NODE_STATUS.DEFAULT_STATUS
- node = Node(hostname=hostname, status=status, **kwargs)
+ node = Node(
+ hostname=hostname, status=status, architecture=architecture,
+ **kwargs)
node.save()
return node
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-03-06 08:22:51 +0000
+++ src/maasserver/tests/test_api.py 2012-03-12 07:58:18 +0000
@@ -149,7 +149,6 @@
self.assertEqual(httplib.BAD_REQUEST, response.status_code)
self.assertIn('application/json', response['Content-Type'])
- self.assertItemsEqual(['mac_addresses'], parsed_result)
self.assertEqual(
["One or more MAC Addresses is invalid."],
parsed_result['mac_addresses'])
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-03-07 08:32:33 +0000
+++ src/maasserver/tests/test_forms.py 2012-03-12 07:58:18 +0000
@@ -22,6 +22,7 @@
validate_hostname,
)
from maasserver.models import (
+ ARCHITECTURE,
Config,
DEFAULT_CONFIG,
)
@@ -45,8 +46,10 @@
def test_NodeWithMACAddressesForm_valid(self):
form = NodeWithMACAddressesForm(
- self.get_QueryDict(
- {'mac_addresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f']}))
+ self.get_QueryDict({
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
+ 'architecture': ARCHITECTURE.i386,
+ }))
self.assertTrue(form.is_valid())
self.assertEqual(
@@ -58,8 +61,10 @@
# the error message in form.errors['mac_addresses'] is the
# message from the field's validation error.
form = NodeWithMACAddressesForm(
- self.get_QueryDict(
- {'mac_addresses': ['invalid']}))
+ self.get_QueryDict({
+ 'mac_addresses': ['invalid'],
+ 'architecture': ARCHITECTURE.i386,
+ }))
self.assertFalse(form.is_valid())
self.assertEqual(['mac_addresses'], list(form.errors))
@@ -72,8 +77,10 @@
# if one or more fields are invalid, a single error message is
# present in form.errors['mac_addresses'] after validation.
form = NodeWithMACAddressesForm(
- self.get_QueryDict(
- {'mac_addresses': ['invalid_1', 'invalid_2']}))
+ self.get_QueryDict({
+ 'mac_addresses': ['invalid_1', 'invalid_2'],
+ 'architecture': ARCHITECTURE.i386,
+ }))
self.assertFalse(form.is_valid())
self.assertEqual(['mac_addresses'], list(form.errors))
@@ -84,15 +91,19 @@
def test_NodeWithMACAddressesForm_empty(self):
# Empty values in the list of MAC Addresses are simply ignored.
form = NodeWithMACAddressesForm(
- self.get_QueryDict(
- {'mac_addresses': ['aa:bb:cc:dd:ee:ff', '']}))
+ self.get_QueryDict({
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', ''],
+ 'architecture': ARCHITECTURE.i386,
+ }))
self.assertTrue(form.is_valid())
def test_NodeWithMACAddressesForm_save(self):
form = NodeWithMACAddressesForm(
- self.get_QueryDict(
- {'mac_addresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f']}))
+ self.get_QueryDict({
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
+ 'architecture': ARCHITECTURE.i386,
+ }))
node = form.save()
self.assertIsNotNone(node.id) # The node is persisted.
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-03-06 08:48:58 +0000
+++ src/maasserver/tests/test_models.py 2012-03-12 07:58:18 +0000
@@ -313,8 +313,7 @@
def make_MAC(self, address):
"""Create a MAC address."""
- node = Node()
- node.save()
+ node = factory.make_node()
return MACAddress(mac_address=address, node=node)
def test_stores_to_database(self):
=== modified file 'src/maasserver/tests/test_provisioning.py'
--- src/maasserver/tests/test_provisioning.py 2012-03-09 11:49:28 +0000
+++ src/maasserver/tests/test_provisioning.py 2012-03-12 07:58:18 +0000
@@ -15,14 +15,19 @@
from maasserver import provisioning
from maasserver.models import (
+ ARCHITECTURE,
Config,
Node,
+ NODE_AFTER_COMMISSIONING_ACTION,
+ NODE_STATUS,
)
from maasserver.provisioning import (
compose_metadata,
get_metadata_server_url,
+ select_profile_for_node,
)
from maasserver.testing import TestCase
+from maasserver.testing.enum import map_enum
from maasserver.testing.factory import factory
from metadataserver.models import NodeKey
@@ -33,22 +38,56 @@
# Must be defined in concrete subclasses.
papi = None
+ def make_node_without_saving(self, arch=ARCHITECTURE.i386):
+ """Create a Node, but don't save it to the database."""
+ system_id = "node-%s" % factory.getRandomString()
+ return Node(
+ system_id=system_id, hostname=factory.getRandomString(),
+ status=NODE_STATUS.DEFAULT_STATUS, owner=factory.make_user(),
+ after_commissioning_action=(
+ NODE_AFTER_COMMISSIONING_ACTION.DEFAULT),
+ architecture=arch)
+
+ def make_papi_profile(self):
+ """Create a new profile on the provisioning API."""
+ # Kernel and initrd are irrelevant here, but must be real files
+ # for Cobbler's benefit. Cobbler may not be running locally, so
+ # just feed it some filenames that it's sure to have (and won't
+ # be allowed accidentally to overwrite).
+ shared_name = factory.getRandomString()
+ distro_name = 'distro-%s' % shared_name
+ fake_initrd = '/etc/cobbler/settings'
+ fake_kernel = '/etc/cobbler/version'
+ distro = self.papi.add_distro(distro_name, fake_initrd, fake_kernel)
+ profile_name = 'profile-%s' % shared_name
+ return self.papi.add_profile(profile_name, distro)
+
+ def test_select_profile_for_node_ignores_previously_chosen_profile(self):
+ node = factory.make_node(architecture='i386')
+ self.papi.modify_nodes(
+ {node.system_id: {'profile': self.make_papi_profile()}})
+ self.assertEqual(
+ 'precise-i386', select_profile_for_node(node, self.papi))
+
+ def test_select_profile_for_node_selects_Precise_and_right_arch(self):
+ nodes = {
+ arch: self.make_node_without_saving(arch=arch)
+ for arch in map_enum(ARCHITECTURE).values()}
+ self.assertItemsEqual(
+ ['precise-%s' % arch for arch in nodes.keys()],
+ [
+ select_profile_for_node(node, self.papi)
+ for node in nodes.values()])
+
def test_provision_post_save_Node_create(self):
- # Creating and saving a node automatically creates a dummy distro and
- # profile too, and associates it with the new node.
- node_model = factory.make_node(system_id="frank")
+ # The handler for Node's post-save signal registers the node in
+ # its current state with the provisioning server.
+ node = factory.make_node(architecture=ARCHITECTURE.i386)
provisioning.provision_post_save_Node(
- sender=Node, instance=node_model, created=True)
- nodes = self.papi.get_nodes_by_name(["frank"])
- self.assertEqual(["frank"], sorted(nodes))
- node = nodes["frank"]
- profile_name = node["profile"]
- profiles = self.papi.get_profiles_by_name([profile_name])
- self.assertEqual([profile_name], sorted(profiles))
- profile = profiles[profile_name]
- distro_name = profile["distro"]
- distros = self.papi.get_distros_by_name([distro_name])
- self.assertEqual([distro_name], sorted(distros))
+ sender=Node, instance=node, created=True)
+ system_id = node.system_id
+ pserv_node = self.papi.get_nodes_by_name([system_id])[system_id]
+ self.assertEqual("precise-i386", pserv_node["profile"])
def test_provision_post_save_MACAddress_create(self):
# Creating and saving a MACAddress updates the Node with which it's
