Merge lp:~lamont/maas/bug-1372544 into lp:~maas-committers/maas/trunk

[LaMont Jones]

If there are no rack controllers connected, populate tags in the region controller.

If there are no rack controllers connected, populate tags in the region controller.

[Gavin Panella (allenap)]

I got a bit carried away reviewing this. I built on what you've done and came up with quite a large bunch of changes. I've made the evaluations more efficient and put everything into post-commit hooks. Take a look at lp:~allenap/maas/lamont--bug-1372544 (or the merge proposal at https://code.launchpad.net/~allenap/maas/lamont--bug-1372544/+merge/294167).

In time, a better solution might be to model a work queue, like we did for DNS zones recently. That way we can better manage the work in an HA environment, and better recover from failures. For example, we really don't want to be doing lots of tag evaluation work for more than one tag update at a time otherwise we run a real risk of DOSing the MAAS installation. There's no protection against that right now.

(We could add the DOS protection to this current implementation quite easily, but recovery would still need a queue, or some other additional support in the data model.)

Anyway, look at what I've done. If you like it, merge it into this, write some tests for `populate_tag_for_multiple_nodes` and it should be good to land.

[Blake Rouse (blake-rouse)]

Looks like you have a conflict. I also have one question.

review: Needs Information

[Gavin Panella (allenap)]

Responded to Blake's question.

[Gavin Panella (allenap)]

review: Abstain

[LaMont Jones (lamont)]

replied

[Blake Rouse (blake-rouse)]

Looks good. Thanks for the fix.

review: Approve

=== modified file 'src/maasserver/api/tests/test_tag.py'
--- src/maasserver/api/tests/test_tag.py	2016-05-06 11:41:05 +0000
+++ src/maasserver/api/tests/test_tag.py	2016-05-13 14:02:27 +0000
@@ -38,11 +38,6 @@
 from testtools.matchers import MatchesStructure
 
 
-def patch_populate_tags(test):
-    from maasserver import populate_tags
-    return test.patch_autospec(populate_tags, "populate_tags")
-
-
 class TestTagAPI(APITestCase):
     """Tests for /api/2.0/tags/<tagname>/."""
 
@@ -116,15 +111,16 @@
         self.assertTrue(Tag.objects.filter(name='new-tag-name').exists())
 
     def test_PUT_updates_node_associations(self):
-        populate_tags = patch_populate_tags(self)
-        tag = factory.make_Tag(definition='//node/foo')
-        self.expectThat(populate_tags, MockCalledOnceWith(tag))
+        populate_nodes = self.patch_autospec(Tag, "populate_nodes")
+        tag = Tag(name=factory.make_name("tag"), definition='//node/foo')
+        tag.save()
+        self.expectThat(populate_nodes, MockCalledOnceWith(tag))
         self.become_admin()
         response = self.client.put(
             self.get_tag_uri(tag),
             {'definition': '//node/bar'})
         self.assertEqual(http.client.OK, response.status_code)
-        self.expectThat(populate_tags, MockCallsMatch(call(tag), call(tag)))
+        self.expectThat(populate_nodes, MockCallsMatch(call(tag), call(tag)))
 
     def test_GET_nodes_with_no_nodes(self):
         tag = factory.make_Tag()
@@ -431,7 +427,7 @@
         node = factory.make_Node()
         client = make_worker_client(rack_controller)
         tag.definition = '//new/node/definition'
-        tag.save()
+        tag.save(populate=False)
         response = client.post(
             self.get_tag_uri(tag), {
                 'op': 'update_nodes',
@@ -444,16 +440,17 @@
         self.assertItemsEqual([], node.tags.all())
 
     def test_POST_rebuild_rebuilds_node_mapping(self):
-        populate_tags = patch_populate_tags(self)
-        tag = factory.make_Tag(definition='//foo/bar')
+        populate_nodes = self.patch_autospec(Tag, "populate_nodes")
+        tag = Tag(name=factory.make_name("tag"), definition='//foo/bar')
+        tag.save()
         self.become_admin()
-        self.assertThat(populate_tags, MockCalledOnceWith(tag))
+        self.assertThat(populate_nodes, MockCalledOnceWith(tag))
         response = self.client.post(self.get_tag_uri(tag), {'op': 'rebuild'})
         self.assertEqual(http.client.OK, response.status_code)
         parsed_result = json.loads(
             response.content.decode(settings.DEFAULT_CHARSET))
         self.assertEqual({'rebuilding': tag.name}, parsed_result)
-        self.assertThat(populate_tags, MockCallsMatch(call(tag), call(tag)))
+        self.assertThat(populate_nodes, MockCallsMatch(call(tag), call(tag)))
 
     def test_POST_rebuild_leaves_manual_tags(self):
         tag = factory.make_Tag(definition='')
@@ -589,7 +586,7 @@
             extra_kernel_opts, Tag.objects.filter(name=name)[0].kernel_opts)
 
     def test_POST_new_populates_nodes(self):
-        populate_tags = patch_populate_tags(self)
+        populate_nodes = self.patch_autospec(Tag, "populate_nodes")
         self.become_admin()
         name = factory.make_string()
         definition = '//node/child'
@@ -602,8 +599,8 @@
                 'definition': definition,
             })
         self.assertEqual(http.client.OK, response.status_code)
-        self.assertThat(populate_tags, MockCalledOnceWith(ANY))
-        # The tag passed to populate_tags() is the one created above.
-        [tag], _ = populate_tags.call_args
+        self.assertThat(populate_nodes, MockCalledOnceWith(ANY))
+        # The tag passed to populate_nodes() is the one created above.
+        [tag], _ = populate_nodes.call_args
         self.assertThat(tag, MatchesStructure.byEquality(
             name=name, comment=comment, definition=definition))

=== modified file 'src/maasserver/models/tag.py'
--- src/maasserver/models/tag.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/models/tag.py	2016-05-13 14:02:27 +0000
@@ -22,6 +22,9 @@
 from maasserver import DefaultMeta
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.timestampedmodel import TimestampedModel
+from maasserver.utils.orm import post_commit_do
+from maasserver.utils.threads import deferToDatabase
+from twisted.internet import reactor
 
 
 class TagManager(Manager):
@@ -90,25 +93,63 @@
     def __str__(self):
         return self.name
 
+    def _populate_nodes_later(self):
+        """Find all nodes that match this tag, and update them, later.
+
+        This schedules population to happen post-commit, without waiting for
+        its outcome.
+        """
+        # Avoid circular imports.
+        from maasserver.populate_tags import populate_tags
+
+        if self.is_defined:
+            # Schedule repopulate to happen after commit. This thread does not
+            # wait for it to complete.
+            post_commit_do(
+                reactor.callLater, 0, deferToDatabase,
+                populate_tags, self)
+
+    def _populate_nodes_now(self):
+        """Find all nodes that match this tag, and update them, now.
+
+        All computation will be done within the current transaction, within
+        the current thread. This could be costly.
+        """
+        # Avoid circular imports.
+        from maasserver.populate_tags import populate_tag_for_multiple_nodes
+        from maasserver.models.node import Node
+
+        if self.is_defined:
+            # Do the work here and now in this thread. This is probably a
+            # terrible mistake... unless you're testing.
+            populate_tag_for_multiple_nodes(self, Node.objects.all())
+
     def populate_nodes(self):
-        """Find all nodes that match this tag, and update them."""
-        if not self.is_defined:
-            return
-        # before we pass off any work, ensure the definition is valid XPATH
-        try:
-            etree.XPath(self.definition)
-        except etree.XPathSyntaxError as e:
-            msg = 'Invalid xpath expression: %s' % (e,)
-            raise ValidationError({'definition': [msg]})
-        # Now delete the existing tags
-        self.node_set.clear()
-        # Avoid circular imports.
-        from maasserver.populate_tags import populate_tags
-        populate_tags(self)
-
-    def save(self, *args, **kwargs):
+        """Find all nodes that match this tag, and update them.
+
+        By default, node population is deferred.
+        """
+        return self._populate_nodes_later()
+
+    def clean_definition(self):
+        if self.is_defined:
+            try:
+                etree.XPath(self.definition)
+            except etree.XPathSyntaxError as e:
+                msg = 'Invalid XPath expression: %s' % (e,)
+                raise ValidationError({'definition': [msg]})
+
+    def clean(self):
+        self.clean_definition()
+
+    def save(self, *args, populate=True, **kwargs):
+        """Save this tag.
+
+        :param populate: Whether or not to call `populate_nodes` if the
+            definition has changed.
+        """
         super(Tag, self).save(*args, **kwargs)
-        if self.definition != self._original_definition:
+        if populate and (self.definition != self._original_definition):
             self.populate_nodes()
         self._original_definition = self.definition
 

=== modified file 'src/maasserver/models/tests/test_tag.py'
--- src/maasserver/models/tests/test_tag.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/models/tests/test_tag.py	2016-05-13 14:02:27 +0000
@@ -5,12 +5,20 @@
 
 __all__ = []
 
+from unittest.mock import ANY
+
 from django.core.exceptions import ValidationError
-from maasserver import populate_tags as populate_tags_module
+from maasserver import populate_tags
+from maasserver.models import tag as tag_module
 from maasserver.models.tag import Tag
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from maastesting.matchers import MockCalledOnceWith
+from maasserver.utils.threads import deferToDatabase
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockNotCalled,
+)
+from twisted.internet import reactor
 
 
 class TagTest(MAASServerTestCase):
@@ -56,11 +64,17 @@
                         'too-long' * 33, '\xb5']:
             self.assertRaises(ValidationError, factory.make_Tag, name=invalid)
 
-    def test_applies_tags_to_nodes(self):
-        populate_tags = self.patch_autospec(
-            populate_tags_module, "populate_tags")
-        tag = factory.make_Tag(definition='//node/child')
-        self.assertThat(populate_tags, MockCalledOnceWith(tag))
+    def test_validate_traps_invalid_tag_definitions(self):
+        self.assertRaises(
+            ValidationError, factory.make_Tag,
+            definition="invalid::definition")
+
+    def test_applies_tags_to_nodes_on_save(self):
+        populate_nodes = self.patch_autospec(Tag, "populate_nodes")
+        tag = Tag(name=factory.make_name("tag"), definition='//node/child')
+        self.assertThat(populate_nodes, MockNotCalled())
+        tag.save()
+        self.assertThat(populate_nodes, MockCalledOnceWith(tag))
 
     def test_will_not_save_invalid_xpath(self):
         tag = factory.make_Tag(definition='//node/foo')
@@ -83,41 +97,86 @@
         self.assertIs(self.expected, tag.is_defined)
 
 
-class TestTagPopulateNodes(MAASServerTestCase):
+class TestTagPopulateNodesLater(MAASServerTestCase):
+
+    def test__populates_if_tag_is_defined(self):
+        post_commit_do = self.patch(tag_module, "post_commit_do")
+
+        tag = Tag(name=factory.make_name("tag"), definition="//foo")
+        tag.save(populate=False)
+
+        self.assertTrue(tag.is_defined)
+        self.assertThat(post_commit_do, MockNotCalled())
+        tag._populate_nodes_later()
+        self.assertThat(
+            post_commit_do, MockCalledOnceWith(
+                reactor.callLater, 0, deferToDatabase,
+                populate_tags.populate_tags, tag))
 
     def test__does_nothing_if_tag_is_not_defined(self):
-        tag = factory.make_Tag(definition="")
+        post_commit_do = self.patch(tag_module, "post_commit_do")
+
+        tag = Tag(name=factory.make_name("tag"), definition="")
+        tag.save(populate=False)
+
         self.assertFalse(tag.is_defined)
+        self.assertThat(post_commit_do, MockNotCalled())
+        tag._populate_nodes_later()
+        self.assertThat(post_commit_do, MockNotCalled())
+
+    def test__does_not_clear_node_set_before_populating(self):
+        post_commit_do = self.patch(tag_module, "post_commit_do")
+
+        tag = Tag(name=factory.make_name("tag"), definition="//foo")
+        tag.save(populate=False)
+
         nodes = [factory.make_Node() for _ in range(3)]
         tag.node_set.add(*nodes)
-        tag.populate_nodes()
-        # The set of related nodes is unchanged.
+        tag._populate_nodes_later()
         self.assertItemsEqual(nodes, tag.node_set.all())
-
-    def test__checks_definition_before_proceeding(self):
-        tag = factory.make_Tag(definition="")
-        tag.definition = "invalid::definition"
-        self.assertRaises(ValidationError, tag.populate_nodes)
-
-    def test__clears_node_set(self):
-        self.patch_autospec(populate_tags_module, "populate_tags")
-
-        tag = factory.make_Tag(definition="")
-        # Define the tag now but don't save because .save() calls
-        # populate_nodes(), but we want to do it explicitly here.
-        tag.definition = "//foo"
+        self.assertThat(post_commit_do, MockCalledOnceWith(
+            reactor.callLater, 0, deferToDatabase,
+            populate_tags.populate_tags, tag))
+
+    def test__later_is_the_default(self):
+        tag = Tag(name=factory.make_name("tag"))
+        self.patch(tag, "_populate_nodes_later")
+        self.assertThat(tag._populate_nodes_later, MockNotCalled())
+        tag.save()
+        self.assertThat(tag._populate_nodes_later, MockCalledOnceWith())
+
+
+class TestTagPopulateNodesNow(MAASServerTestCase):
+
+    def test__populates_if_tag_is_defined(self):
+        populate_multiple = self.patch_autospec(
+            populate_tags, "populate_tag_for_multiple_nodes")
+
+        tag = Tag(name=factory.make_name("tag"), definition="//foo")
+        tag.save(populate=False)
+
+        self.assertTrue(tag.is_defined)
+        self.assertThat(populate_multiple, MockNotCalled())
+        tag._populate_nodes_now()
+        self.assertThat(populate_multiple, MockCalledOnceWith(tag, ANY))
+
+    def test__does_nothing_if_tag_is_not_defined(self):
+        populate_multiple = self.patch_autospec(
+            populate_tags, "populate_tag_for_multiple_nodes")
+
+        tag = Tag(name=factory.make_name("tag"), definition="")
+        tag.save(populate=False)
+
+        self.assertFalse(tag.is_defined)
+        self.assertThat(populate_multiple, MockNotCalled())
+        tag._populate_nodes_now()
+        self.assertThat(populate_multiple, MockNotCalled())
+
+    def test__clears_node_set_before_populating(self):
+        tag = Tag(name=factory.make_name("tag"), definition="//foo")
+        tag.save(populate=False)
+
         nodes = [factory.make_Node() for _ in range(3)]
         tag.node_set.add(*nodes)
-        tag.populate_nodes()
+        tag._populate_nodes_now()
         self.assertItemsEqual([], tag.node_set.all())
-
-    def test__calls_populate_tags(self):
-        populate_tags = self.patch_autospec(
-            populate_tags_module, "populate_tags")
-
-        tag = factory.make_Tag(definition="")
-        # Define the tag now but don't save because .save() calls
-        # populate_nodes(), but we want to do it explicitly here.
-        tag.definition = "//foo"
-        tag.populate_nodes()
-        self.assertThat(populate_tags, MockCalledOnceWith(tag))

=== modified file 'src/maasserver/populate_tags.py'
--- src/maasserver/populate_tags.py	2016-04-27 20:52:15 +0000
+++ src/maasserver/populate_tags.py	2016-05-13 14:02:27 +0000
@@ -4,9 +4,10 @@
 """Populate what nodes are associated with a tag."""
 
 __all__ = [
+    'populate_tag_for_multiple_nodes',
     'populate_tags',
     'populate_tags_for_single_node',
-    ]
+]
 
 from functools import partial
 from math import ceil
@@ -19,6 +20,7 @@
     RackController,
 )
 from maasserver.models.nodeprobeddetails import (
+    get_probed_details,
     get_single_probed_details,
     script_output_nsmap,
 )
@@ -28,9 +30,14 @@
     get_creds_tuple,
 )
 from maasserver.rpc import getAllClients
+from maasserver.utils.orm import transactional
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.rpc.cluster import EvaluateTag
-from provisioningserver.tags import merge_details
+from provisioningserver.tags import (
+    DEFAULT_BATCH_SIZE,
+    gen_batches,
+    merge_details,
+)
 from provisioningserver.utils import classify
 from provisioningserver.utils.twisted import (
     asynchronous,
@@ -68,6 +75,7 @@
 
 
 @synchronous
+@transactional
 def populate_tags(tag):
     """Evaluate `tag` for all nodes.
 
@@ -85,45 +93,42 @@
     logger.debug('Evaluating the "%s" tag for all nodes.', tag.name)
     clients = getAllClients()
     if len(clients) == 0:
-        # XXX: allenap 2014-09-22 bug=1372544: No connected rack controllers.
-        # Nothing can be evaluated when this occurs.
-        return
-
-    # Split the work between the connected rack controllers. Building all the
-    # information that needs to be sent to them all.
-    node_ids = Node.objects.all().values_list("system_id", flat=True)
-    node_ids = [
-        {"system_id": node_id}
-        for node_id in node_ids
-    ]
-    chunked_node_ids = list(chunk_list(node_ids, len(clients)))
-    connected_racks = []
-    for idx, client in enumerate(clients):
-        rack = RackController.objects.get(system_id=client.ident)
-        tokens = list(get_auth_tokens(rack.owner))
-        if len(tokens) > 0:
-            # Use the latest token.
-            token = tokens[-1]
-        else:
-            token = create_auth_token(rack.owner)
-        creds = convert_tuple_to_string(get_creds_tuple(token))
-        if len(chunked_node_ids) > idx:
-            connected_racks.append({
-                "system_id": rack.system_id,
-                "hostname": rack.hostname,
-                "client": client,
-                "tag_name": tag.name,
-                "tag_definition": tag.definition,
-                "tag_nsmap": [
-                    {"prefix": prefix, "uri": uri}
-                    for prefix, uri in tag_nsmap.items()
-                ],
-                "credentials": creds,
-                "nodes": list(chunked_node_ids[idx]),
-            })
-
-    [d] = _do_populate_tags(connected_racks)
-    return d
+        # We have no clients so we need to do the work locally.
+        return populate_tag_for_multiple_nodes(tag, Node.objects.all())
+    else:
+        # Split the work between the connected rack controllers.
+        node_ids = Node.objects.all().values_list("system_id", flat=True)
+        node_ids = [{"system_id": node_id} for node_id in node_ids]
+        chunked_node_ids = list(chunk_list(node_ids, len(clients)))
+        connected_racks = []
+        for idx, client in enumerate(clients):
+            rack = RackController.objects.get(system_id=client.ident)
+            token = _get_or_create_auth_token(rack.owner)
+            creds = convert_tuple_to_string(get_creds_tuple(token))
+            if len(chunked_node_ids) > idx:
+                connected_racks.append({
+                    "system_id": rack.system_id,
+                    "hostname": rack.hostname,
+                    "client": client,
+                    "tag_name": tag.name,
+                    "tag_definition": tag.definition,
+                    "tag_nsmap": [
+                        {"prefix": prefix, "uri": uri}
+                        for prefix, uri in tag_nsmap.items()
+                    ],
+                    "credentials": creds,
+                    "nodes": list(chunked_node_ids[idx]),
+                })
+        [d] = _do_populate_tags(connected_racks)
+        return d
+
+
+def _get_or_create_auth_token(user):
+    """Get the most recent OAuth token for `user`, or create one."""
+    for token in reversed(get_auth_tokens(user)):
+        return token
+    else:
+        return create_auth_token(user)
 
 
 @asynchronous(timeout=FOREVER)
@@ -177,11 +182,15 @@
     return [d]
 
 
+@synchronous
 def populate_tags_for_single_node(tags, node):
     """Reevaluate all tags for a single node.
 
-    Presumably this node's details have recently changed. Use
-    `populate_tags` when many nodes need reevaluating.
+    Presumably this node's details have recently changed. Use `populate_tags`
+    when many nodes need reevaluating AND there are rack controllers available
+    to which to farm-out work. Use `populate_tag_for_multiple_nodes` when many
+    nodes need reevaluating locally, i.e. when there are no rack controllers
+    connected.
     """
     probed_details = get_single_probed_details(node.system_id)
     probed_details_doc = merge_details(probed_details)
@@ -192,3 +201,28 @@
     tags_matching, tags_nonmatching = classify(evaluator, tags_defined)
     node.tags.remove(*tags_nonmatching)
     node.tags.add(*tags_matching)
+
+
+@synchronous
+def populate_tag_for_multiple_nodes(tag, nodes, batch_size=DEFAULT_BATCH_SIZE):
+    """Reevaluate a single tag for a multiple nodes.
+
+    Presumably this tag's expression has recently changed. Use `populate_tags`
+    when many nodes need reevaluating AND there are rack controllers available
+    to which to farm-out work. Use this only when many nodes need reevaluating
+    locally, i.e. when there are no rack controllers connected.
+    """
+    # Same expression, multuple documents: compile expression with XPath.
+    xpath = etree.XPath(tag.definition, namespaces=tag_nsmap)
+    # The XML details documents can be large so work in batches.
+    for batch in gen_batches(nodes, batch_size):
+        probed_details = get_probed_details(node.system_id for node in batch)
+        probed_details_docs_by_node = {
+            node: merge_details(probed_details[node.system_id])
+            for node in batch
+        }
+        nodes_matching, nodes_nonmatching = classify(
+            partial(try_match_xpath, xpath, logger=maaslog),
+            probed_details_docs_by_node.items())
+        tag.node_set.remove(*nodes_nonmatching)
+        tag.node_set.add(*nodes_matching)

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2016-05-11 19:31:40 +0000
+++ src/maasserver/testing/factory.py	2016-05-13 14:02:27 +0000
@@ -1025,8 +1025,9 @@
         else:
             return self.make_ipv4_Subnet_with_IPRanges()
 
-    def make_Tag(self, name=None, definition=None, comment='',
-                 kernel_opts=None, created=None, updated=None):
+    def make_Tag(
+            self, name=None, definition=None, comment='', kernel_opts=None,
+            created=None, updated=None, populate=True):
         if name is None:
             name = self.make_name('tag')
         if definition is None:
@@ -1035,14 +1036,22 @@
         tag = Tag(
             name=name, definition=definition, comment=comment,
             kernel_opts=kernel_opts)
-        tag.save()
+        # Save without populating nodes.
+        tag.save(populate=False)
         # Update the 'updated'/'created' fields with a call to 'update'
         # preventing a call to save() from overriding the values.
         if updated is not None:
             Tag.objects.filter(id=tag.id).update(updated=updated)
         if created is not None:
             Tag.objects.filter(id=tag.id).update(created=created)
-        return reload_object(tag)
+        # Reload if we've changed the underlying record.
+        if updated is not None or created is not None:
+            tag = reload_object(tag)
+        # Populate nodes for this tag now that it's fully configured, if
+        # requested. This avoids dealing with the post-commit hook stuff.
+        if populate:
+            tag._populate_nodes_now()
+        return tag
 
     def make_user_with_keys(self, n_keys=2, user=None, **kwargs):
         """Create a user with n `SSHKey`.  If user is not None, use this user

=== modified file 'src/maasserver/testing/orm.py'
--- src/maasserver/testing/orm.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/testing/orm.py	2016-05-13 14:02:27 +0000
@@ -71,7 +71,7 @@
             self.expectThat(
                 post_commit_hooks.hooks, HasLength(0),
                 "One or more post-commit tasks were present at the end of "
-                "this test." + description_of_hooks)
+                "this test.\n" + description_of_hooks)
             super(PostCommitHooksTestMixin, self).tearDown()
         finally:
             # By this point we will have reported the leaked post-commit

=== modified file 'src/maasserver/tests/test_populate_tags.py'
--- src/maasserver/tests/test_populate_tags.py	2016-05-10 11:02:10 +0000
+++ src/maasserver/tests/test_populate_tags.py	2016-05-13 14:02:27 +0000
@@ -13,8 +13,15 @@
 
 from apiclient.creds import convert_tuple_to_string
 from fixtures import FakeLogger
-from maasserver import populate_tags as populate_tags_module
-from maasserver.models import Tag
+from maasserver import (
+    populate_tags as populate_tags_module,
+    rpc as rpc_module,
+)
+from maasserver.models import (
+    Node,
+    Tag,
+    tag as tag_module,
+)
 from maasserver.models.user import (
     create_auth_token,
     get_auth_tokens,
@@ -22,6 +29,7 @@
 )
 from maasserver.populate_tags import (
     _do_populate_tags,
+    populate_tag_for_multiple_nodes,
     populate_tags,
     populate_tags_for_single_node,
 )
@@ -31,7 +39,12 @@
     RunningEventLoopFixture,
 )
 from maasserver.testing.factory import factory
-from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.testing.testcase import (
+    MAASServerTestCase,
+    MAASTransactionServerTestCase,
+)
+from maasserver.utils.orm import post_commit_hooks
+from maasserver.utils.threads import deferToDatabase
 from maastesting.matchers import (
     MockCalledOnceWith,
     MockCallsMatch,
@@ -48,13 +61,16 @@
 from provisioningserver.rpc.cluster import EvaluateTag
 from provisioningserver.rpc.common import Client
 from provisioningserver.utils.twisted import asynchronous
-from testtools.monkey import MonkeyPatcher
-
-
-def make_Tag_without_populating():
-    # Create a tag but prevent evaluation when saving.
-    dont_populate = MonkeyPatcher((Tag, "populate_nodes", lambda self: None))
-    return dont_populate.run_with_patches(factory.make_Tag)
+from testtools.matchers import (
+    HasLength,
+    IsInstance,
+    MatchesAll,
+    MatchesStructure,
+)
+from twisted.internet import reactor
+from twisted.internet.base import DelayedCall
+from twisted.internet.task import Clock
+from twisted.internet.threads import blockingCallFromThread
 
 
 class TestDoPopulateTags(MAASServerTestCase):
@@ -186,6 +202,11 @@
 
 
 class TestPopulateTagsEndToNearlyEnd(MAASServerTestCase):
+    """Tests for populating tags on racks.
+
+    This happens when there are connected rack controllers able to carry out
+    the task.
+    """
 
     def prepare_live_rpc(self):
         self.useFixture(RegionEventLoopFixture("rpc"))
@@ -210,7 +231,7 @@
             protocol = rpc_fixture.makeCluster(rack, EvaluateTag)
             protocol.EvaluateTag.side_effect = always_succeed_with({})
             protocols.append(protocol)
-        tag = make_Tag_without_populating()
+        tag = factory.make_Tag(populate=False)
 
         d = populate_tags(tag)
 
@@ -227,6 +248,57 @@
                 tag_nsmap=ANY, credentials=creds, nodes=ANY))
 
 
+class TestPopulateTagsInRegion(MAASTransactionServerTestCase):
+    """Tests for populating tags in the region.
+
+    This happens when there are no rack controllers to carry out the task.
+    """
+
+    def test__saving_tag_schedules_node_population(self):
+        clock = self.patch(tag_module, "reactor", Clock())
+
+        with post_commit_hooks:
+            # Make a Tag by hand to trigger normal node population handling
+            # behaviour rather than the (generally more convenient) default
+            # behaviour in the factory.
+            tag = Tag(name=factory.make_name("tag"), definition='true()')
+            tag.save()
+
+        # A call has been scheduled to populate tags.
+        calls = clock.getDelayedCalls()
+        self.assertThat(calls, HasLength(1))
+        [call] = calls
+        self.assertThat(
+            call, MatchesAll(
+                IsInstance(DelayedCall),
+                MatchesStructure.byEquality(
+                    time=0, func=deferToDatabase,
+                    args=(populate_tags, tag), kw={}),
+                first_only=True,
+            ))
+
+    def test__populate_in_region_when_no_clients(self):
+        clock = self.patch(tag_module, "reactor", Clock())
+
+        # Ensure there are no clients.
+        self.patch(rpc_module, 'getAllClients').return_value = []
+
+        with post_commit_hooks:
+            node = factory.make_Node()
+            # Make a Tag by hand to trigger normal node population handling
+            # behaviour rather than the (generally more convenient) default
+            # behaviour in the factory.
+            tag = Tag(name=factory.make_name("tag"), definition='true()')
+            tag.save()
+
+        # A call has been scheduled to populate tags.
+        [call] = clock.getDelayedCalls()
+        # Execute the call ourselves in the real reactor.
+        blockingCallFromThread(reactor, call.func, *call.args, **call.kw)
+        # The tag's node set has been updated.
+        self.assertItemsEqual([node], tag.node_set.all())
+
+
 class TestPopulateTagsForSingleNode(MAASServerTestCase):
 
     def test_updates_node_with_all_applicable_tags(self):
@@ -236,9 +308,9 @@
         factory.make_NodeResult_for_commissioning(
             node, LLDP_OUTPUT_NAME, 0, b"<bar/>")
         tags = [
-            factory.make_Tag("foo", "/foo"),
-            factory.make_Tag("bar", "//lldp:bar"),
-            factory.make_Tag("baz", "/foo/bar"),
+            factory.make_Tag("foo", "/foo", populate=False),
+            factory.make_Tag("bar", "//lldp:bar", populate=False),
+            factory.make_Tag("baz", "/foo/bar", populate=False),
             ]
         populate_tags_for_single_node(tags, node)
         self.assertItemsEqual(
@@ -249,8 +321,8 @@
         factory.make_NodeResult_for_commissioning(
             node, LSHW_OUTPUT_NAME, 0, b"<foo/>")
         tags = [
-            factory.make_Tag("foo", "/foo"),
-            factory.make_Tag("lou", "//nge:bar"),
+            factory.make_Tag("foo", "/foo", populate=False),
+            factory.make_Tag("lou", "//nge:bar", populate=False),
             ]
         populate_tags_for_single_node(tags, node)  # Look mom, no exception!
         self.assertSequenceEqual(
@@ -261,10 +333,24 @@
         factory.make_NodeResult_for_commissioning(
             node, LSHW_OUTPUT_NAME, 0, b"<foo/>")
         tags = [
-            factory.make_Tag("foo", "/foo"),
+            factory.make_Tag("foo", "/foo", populate=False),
             Tag(name="empty", definition=""),
             Tag(name="null", definition=None),
             ]
         populate_tags_for_single_node(tags, node)  # Look mom, no exception!
         self.assertSequenceEqual(
             ["foo"], [tag.name for tag in node.tags.all()])
+
+
+class TestPopulateTagForMultipleNodes(MAASServerTestCase):
+
+    def test_updates_nodes_with_tag(self):
+        nodes = [factory.make_Node() for _ in range(5)]
+        for node in nodes[0:2]:
+            factory.make_NodeResult_for_commissioning(
+                node, LLDP_OUTPUT_NAME, 0, b"<bar/>")
+        tag = factory.make_Tag("bar", "//lldp:bar", populate=False)
+        populate_tag_for_multiple_nodes(tag, nodes)
+        self.assertItemsEqual(
+            [node.hostname for node in nodes[0:2]],
+            [node.hostname for node in Node.objects.filter(tags__name='bar')])