Merge ~mpontillo/maas:default-domain-selection into maas:master

[Mike Pontillo]

Add GlobalDefault model.

 * Use GlobalDefault model for get_default_domain().

The first step in allowing the default domain to be changed is creating somewhere in MAAS that tracks "global default" objects.

This branch creates a model for that and switches the Domain object over to its usage.

We could also use this concept for (at least) fabrics and availability zones.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b default-domain-selection lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2327/console
COMMIT: a1a270141ee6c56a3c4c3ba6e454b0c89b609bde

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b default-domain-selection lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2328/console
COMMIT: 7ddca341507677b43c9fadb478a396e2dacacfb2

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b default-domain-selection lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: cd947d8785cc97c533b7bb2be2f43c679bbd258b

review: Approve

[Alberto Donato (ack)]

Why do we need an extra model (which ends up having a single instance) just to track which is the default domain?
Can't we add a "default" flag to the domain instead (and have a contraint that only allow to have exactly one entry with default=True)?
We might have a similar case with default user groups and resource pools, if we'll want to changing the default. Having to add an extra model seems cumbersome

review: Needs Information

[Andres Rodriguez (andreserl)]

I agree!

On Fri, Apr 6, 2018 at 6:04 AM Alberto Donato <email address hidden>
wrote:

> Review: Needs Information
>
> Why do we need an extra model (which ends up having a single instance)
> just to track which is the default domain?
> Can't we add a "default" flag to the domain instead (and have a contraint
> that only allow to have exactly one entry with default=True)?
> We might have a similar case with default user groups and resource pools,
> if we'll want to changing the default. Having to add an extra model seems
> cumbersome
> --
> https://code.launchpad.net/~mpontillo/maas/+git/maas/+merge/342768
> Your team MAAS Committers is subscribed to branch maas:master.
>
> Launchpad-Message-Rationale: Subscriber @maas-committers
> Launchpad-Message-For: maas-committers
> Launchpad-Notification-Type: code-review
> Launchpad-Branch: ~maas-committers/maas/+git/maas:master
> Launchpad-Project: maas
>
--
Andres Rodriguez (RoAkSoAx)
Ubuntu Server Developer
MSc. Telecom & Networking
Systems Engineer

[Mike Pontillo (mpontillo)]

We discussed this at standup (well, mainly Blake and myself) and agreed on this option for a few reasons:

(1) We didn't like how changing one row in the domain table would require changing multiple rows (to set the default) to ensure referential integrity. We don't have a good existing pattern to do this with maas-obj-form or similar, so there's little benefit in adding an extraneous (and somewhat strangely modeled) row to the domain table.

(2) We wanted "can't delete the default record" to be able to be enforced by database constraints. The foreign key approach is a natural way to do this, and a clean way to model it.

(3) In the future we think it would be useful if different "groups" in MAAS (such as tenants - if we ever do multitenancy, or multitenancy "light", LDAP groups, etc) can specify their own default domain (or other default objects). Modeling global defaults in their own table allows us to, in the future, specify different defaults for different situations.

[Blake Rouse (blake-rouse)]

Looks good.

Sucks to have a table with just 1 row, but it does add a proper database constraint and makes setting the default a single action.

review: Approve

[Alberto Donato (ack)]

fair enough

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b default-domain-selection lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2338/consoleText

[Mike Pontillo (mpontillo)]

Dang randomly-failing full page reload tests again. Retrying.

diff --git a/src/maasserver/migrations/builtin/maasserver/0023_add_ttl_field.py b/src/maasserver/migrations/builtin/maasserver/0023_add_ttl_field.py
index 9442f8d..fa542b3 100644
--- a/src/maasserver/migrations/builtin/maasserver/0023_add_ttl_field.py
+++ b/src/maasserver/migrations/builtin/maasserver/0023_add_ttl_field.py
@@ -44,11 +44,11 @@ class Migration(migrations.Migration):
         migrations.AlterField(
             model_name='dnsresource',
             name='domain',
-            field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, default=maasserver.models.dnsresource.get_default_domain, to='maasserver.Domain'),
+            field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='maasserver.Domain'),
         ),
         migrations.AlterField(
             model_name='node',
             name='domain',
-            field=models.ForeignKey(default=maasserver.models.node.get_default_domain, null=False, to='maasserver.Domain', on_delete=django.db.models.deletion.PROTECT),
+            field=models.ForeignKey(null=False, to='maasserver.Domain', on_delete=django.db.models.deletion.PROTECT),
         ),
     ]
diff --git a/src/maasserver/migrations/builtin/maasserver/0097_node_chassis_storage_hints.py b/src/maasserver/migrations/builtin/maasserver/0097_node_chassis_storage_hints.py
index a2cfb27..08abeab 100644
--- a/src/maasserver/migrations/builtin/maasserver/0097_node_chassis_storage_hints.py
+++ b/src/maasserver/migrations/builtin/maasserver/0097_node_chassis_storage_hints.py
@@ -58,7 +58,7 @@ class Migration(migrations.Migration):
         migrations.AlterField(
             model_name='node',
             name='domain',
-            field=models.ForeignKey(to='maasserver.Domain', null=True, default=maasserver.models.node.get_default_domain, blank=True, on_delete=django.db.models.deletion.PROTECT),
+            field=models.ForeignKey(to='maasserver.Domain', null=True, blank=True, on_delete=django.db.models.deletion.PROTECT),
         ),
         migrations.AlterField(
             model_name='node',
diff --git a/src/maasserver/migrations/builtin/maasserver/0155_add_globaldefaults_model.py b/src/maasserver/migrations/builtin/maasserver/0155_add_globaldefaults_model.py
new file mode 100644
index 0000000..85a2ca8
--- /dev/null
+++ b/src/maasserver/migrations/builtin/maasserver/0155_add_globaldefaults_model.py
@@ -0,0 +1,45 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.11 on 2018-04-06 00:26
+from __future__ import unicode_literals
+
+from django.db import (
+    migrations,
+    models,
+)
+import django.db.models.deletion
+import maasserver.models.cleansave
+import maasserver.models.dnsresource
+import maasserver.models.node
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('maasserver', '0154_link_usergroup_role'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='GlobalDefault',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('created', models.DateTimeField(editable=False)),
+                ('updated', models.DateTimeField(editable=False)),
+                ('domain', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='maasserver.Domain')),
+            ],
+            options={
+                'abstract': False,
+            },
+            bases=(maasserver.models.cleansave.CleanSave, models.Model, object),
+        ),
+        migrations.AlterField(
+            model_name='dnsresource',
+            name='domain',
+            field=models.ForeignKey(default=maasserver.models.dnsresource.get_default_domain, on_delete=django.db.models.deletion.PROTECT, to='maasserver.Domain'),
+        ),
+        migrations.AlterField(
+            model_name='node',
+            name='domain',
+            field=models.ForeignKey(blank=True, default=maasserver.models.node.get_default_domain, null=True, on_delete=django.db.models.deletion.PROTECT, to='maasserver.Domain'),
+        ),
+    ]
diff --git a/src/maasserver/models/__init__.py b/src/maasserver/models/__init__.py
index 339a08c..75d2f34 100644
--- a/src/maasserver/models/__init__.py
+++ b/src/maasserver/models/__init__.py
@@ -34,6 +34,7 @@ __all__ = [
     'FileStorage',
     'Filesystem',
     'FilesystemGroup',
+    'GlobalDefault',
     'Interface',
     'IPRange',
     'ISCSIBlockDevice',
@@ -134,6 +135,7 @@ from maasserver.models.filesystemgroup import (
     RAID,
     VolumeGroup,
 )
+from maasserver.models.globaldefault import GlobalDefault
 from maasserver.models.interface import (
     BondInterface,
     BridgeInterface,
diff --git a/src/maasserver/models/domain.py b/src/maasserver/models/domain.py
index 480d8b8..653c033 100644
--- a/src/maasserver/models/domain.py
+++ b/src/maasserver/models/domain.py
@@ -95,6 +95,11 @@ class DomainManager(Manager, DomainQueriesMixin):
         return queryset
 
     def get_default_domain(self):
+        # Circular imports.
+        from maasserver.models.globaldefault import GlobalDefault
+        return GlobalDefault.objects.instance().domain
+
+    def get_or_create_default_domain(self):
         """Return the default domain."""
         now = datetime.datetime.now()
         domain, _ = self.get_or_create(
diff --git a/src/maasserver/models/globaldefault.py b/src/maasserver/models/globaldefault.py
new file mode 100644
index 0000000..7b37052
--- /dev/null
+++ b/src/maasserver/models/globaldefault.py
@@ -0,0 +1,48 @@
+# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Global default objects."""
+
+__all__ = [
+    "GlobalDefault",
+    ]
+
+from datetime import datetime
+
+from django.db.models import (
+    ForeignKey,
+    Manager,
+    PROTECT,
+)
+from maasserver.models.cleansave import CleanSave
+from maasserver.models.domain import Domain
+from maasserver.models.timestampedmodel import TimestampedModel
+from provisioningserver.logger import get_maas_logger
+
+
+maaslog = get_maas_logger("default")
+
+
+class GlobalDefaultManager(Manager):
+
+    def instance(self):
+        now = datetime.now()
+        instance, _ = self.get_or_create(
+            id=0,
+            defaults=dict(
+                id=0,
+                domain=Domain.objects.get_or_create_default_domain(),
+                created=now,
+                updated=now
+            )
+        )
+        return instance
+
+
+class GlobalDefault(CleanSave, TimestampedModel):
+    """Represents global default objects in MAAS."""
+
+    objects = GlobalDefaultManager()
+
+    domain = ForeignKey(
+        Domain, null=False, blank=False, editable=True, on_delete=PROTECT)
diff --git a/src/maasserver/models/tests/test_globaldefault.py b/src/maasserver/models/tests/test_globaldefault.py
new file mode 100644
index 0000000..2545d82
--- /dev/null
+++ b/src/maasserver/models/tests/test_globaldefault.py
@@ -0,0 +1,37 @@
+# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test GlobalDefault objects."""
+
+from maasserver.models import (
+    Domain,
+    GlobalDefault,
+)
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import MAASServerTestCase
+from testtools.matchers import Equals
+
+
+class TestGlobalDefault(MAASServerTestCase):
+    """Tests for :class:`GlobalDefault`."""
+
+    def test_get_instance_creates_instance_id_0_if_none_exits(self):
+        instance = GlobalDefault.objects.instance()
+        self.assertThat(instance.id, Equals(0))
+
+    def test_get_instance_returns_existing_id_0(self):
+        GlobalDefault.objects.instance()
+        instance = GlobalDefault.objects.instance()
+        self.assertThat(instance.id, Equals(0))
+
+    def test_returns_default_domain(self):
+        instance = GlobalDefault.objects.instance()
+        self.assertThat(instance.domain, Equals(
+            Domain.objects.get_default_domain()))
+
+    def test_default_domain_changes_take_effect(self):
+        instance = GlobalDefault.objects.instance()
+        instance.domain = factory.make_Domain()
+        instance.save()
+        self.assertThat(instance.domain, Equals(
+            Domain.objects.get_default_domain()))