Merge lp:~ltrager/maas/lp1592246 into lp:~maas-committers/maas/trunk

[Lee Trager]

Remove /var/lib/maas_id on rack register and cache contents with get_maas_id

This stops maas-rackd, removes /var/lib/maas_id, and restarts it after the secret key and config has been written. This ensures that a rack doesn't register using a stale system_id. Deleting a rack also stops the service and disables it. By starting and enabling the service users now have an easy way to reregister a rack that was deleted.

get_maas_id now caches /var/lib/maas_id so deleting it during registration has no effect on a region running on the same machine.

[Andres Rodriguez (andreserl)]

Hi Lee,

I thought we said we were not going to do this, and decided that the random system id's would be the way to go?

That said, I have a few questions:

1. So if we are caching the maas_id, what happens if for whatever reason, maas-rack deletes /var/lib/maas/maas_id and the rack controller is restarted?
2. This also means that there's a period of time where maas_id will not be present on the disk, and we agreed not to do that.
3. Have you thought about any corner cases? What happens if you don't delete a rack from the MAAS UI, and then try to re-register the rack? For what we discussed, this would mean that a new rack gets added resulting in two racks being present, when it should have been just one?

review: Needs Information

[Gavin Panella (allenap)]

I agree with Andres about the random system_id thing. The likelihood of
a collision is very small, almost certainly less risk than removing the
maas_id file unconditionally here.

review: Needs Fixing

[Lee Trager (ltrager)]

> Hi Lee,
>
> I thought we said we were not going to do this, and decided that the random
> system id's would be the way to go?
I had this branch mostly done before the standup yesterday so I figured I'd post it. Besides having a fix for lp:1592246 it caches maas_id so its only read once per process instead of multiple times and resolves lp:1576357.

>
> That said, I have a few questions:
>
> 1. So if we are caching the maas_id, what happens if for whatever reason,
> maas-rack deletes /var/lib/maas/maas_id and the rack controller is restarted?
Both rackd and regiond look for a node object by system_id, hostname, or MAC address. If /var/lib/maas/maas_id is missing it will fall back on the looking the host up by hostname then by MAC address. We only have /var/lib/maas/maas_id in case the user renames their hostname on a hardware platform that doesn't have unique MAC address(SystemZ)

> 2. This also means that there's a period of time where maas_id will not be
> present on the disk, and we agreed not to do that.
By caching /var/lib/maas/maas_id it doesn't matter. I tested this by starting up MAAS deleting /var/lib/maas/maas_id and running a deployment.

> 3. Have you thought about any corner cases? What happens if you don't delete a
> rack from the MAAS UI, and then try to re-register the rack? For what we
> discussed, this would mean that a new rack gets added resulting in two racks
> being present, when it should have been just one?
If you run maas-rack register on an already registered rack MAAS will find the existing node table entry from the hostname or MAC address and return the same system_id. The same goes for the region. The only way to get duplicate racks/regions is by changing the hostname and MAC addresses of an already registered host. Since this restarts maas-rackd the worst that will happen in this case is you'll have an extra rack entry which would need to be manually deleted.

[Blake Rouse (blake-rouse)]

Looks good. You need to fix the two tests I commented on. Other than that this looks good.

Make sure you have tested this on an installed version of MAAS as well.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~ltrager/maas/lp1592246 into lp:maas failed. Below is the output from the failed tests.

Get:1 http://security.ubuntu.com/ubuntu xenial-security InRelease [94.5 kB]
Hit:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [94.5 kB]
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Get:5 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe amd64 Packages [97.1 kB]
Fetched 286 kB in 0s (584 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu3).
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
build-essential is already the newest version (12.1ubuntu2).
bzr is already the newest version (2.7.0-2ubuntu1).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
isc-dhcp-common is already the newest version (4.3.3-5ubuntu12).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
...

[Gavin Panella (allenap)]

Minor comment.

[Gavin Panella (allenap)]

I don't understand why the read_cache parameter is necessary, plus a few other things still need fixing.

review: Needs Fixing

[Lee Trager (ltrager)]

I landed this on Friday to solve a couple of bugs for RC1 that we wanted in. The read_cache parameter is used to force get_maas_id to read from the disk even if something is cached. The main use case is for start_up. Right now the region object is created on the master process while the other processes wait for the region object to be created. If the user has a stale value(e.g from a previous install) in /var/lib/maas/maas_id it will be read and cached. The other processes will never come up because they'll have the cached a stale value. By using read_cache=False we keep rereading /var/lib/maas/maas_id until we get a value that has a corresponding row in the database.

I've posted a separate review for the corrections you've suggested at lp:~ltrager/maas/cleanup_lp1592246

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2016-06-16 17:20:19 +0000
+++ src/maasserver/models/node.py	2016-06-18 01:12:20 +0000
@@ -551,8 +551,8 @@
             NODE_TYPE.REGION_AND_RACK_CONTROLLER,
             ]}
 
-    def get_running_controller(self):
-        return self.get(system_id=get_maas_id())
+    def get_running_controller(self, read_cache=True):
+        return self.get(system_id=get_maas_id(read_cache))
 
 
 class RackControllerManager(ControllerManager):

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2016-06-14 04:22:21 +0000
+++ src/maasserver/models/tests/test_node.py	2016-06-18 01:12:20 +0000
@@ -6,6 +6,7 @@
 __all__ = []
 
 from datetime import datetime
+import os
 import random
 from unittest.mock import (
     ANY,
@@ -53,6 +54,7 @@
     BootResource,
     BridgeInterface,
     Config,
+    Controller,
     Device,
     Domain,
     Fabric,
@@ -136,6 +138,7 @@
     EVENT_DETAILS,
     EVENT_TYPES,
 )
+from provisioningserver.path import get_path
 from provisioningserver.power import QUERY_POWER_TYPES
 from provisioningserver.power.schema import JSON_POWER_TYPE_PARAMETERS
 from provisioningserver.rpc.cluster import (
@@ -156,7 +159,12 @@
     map_enum,
     map_enum_reverse,
 )
+from provisioningserver.utils.env import (
+    get_maas_id,
+    set_maas_id,
+)
 from provisioningserver.utils.fs import NamedLock
+from provisioningserver.utils.testing import MAASIDFixture
 from testtools import ExpectedException
 from testtools.matchers import (
     AfterPreprocessing,
@@ -301,7 +309,13 @@
             list(Machine.objects.get_available_machines_for_acquisition(user)))
 
 
-class TestControllerManager(MAASServerTestCase):
+class TestControllerManaer(MAASServerTestCase):
+
+    def tearDown(self):
+        super().tearDown()
+        # Make sure the maas_id is cleared when done
+        set_maas_id(None)
+
     def test_controller_lists_node_type_rack_and_region(self):
         racks_and_regions = set()
         for _ in range(3):
@@ -311,13 +325,23 @@
                     NODE_TYPE.RACK_CONTROLLER, NODE_TYPE.REGION_CONTROLLER,
                     NODE_TYPE.REGION_AND_RACK_CONTROLLER):
                 racks_and_regions.add(factory.make_Node(node_type=node_type))
-        self.assertItemsEqual(racks_and_regions, Node.controllers.all())
+        self.assertItemsEqual(racks_and_regions, Controller.objects.all())
 
     def test_get_running_controller(self):
         rack = factory.make_RackController()
-        get_maas_id = self.patch(node_module, 'get_maas_id')
-        get_maas_id.return_value = rack.system_id
-        self.assertEquals(rack, Node.controllers.get_running_controller())
+        self.useFixture(MAASIDFixture(rack.system_id))
+        self.assertEquals(rack, Controller.objects.get_running_controller())
+
+    def test_get_running_controller_can_ignore_cache(self):
+        set_maas_id(factory.make_string())
+        rack = factory.make_RackController()
+        maas_id_path = get_path('/var/lib/maas/maas_id')
+        os.unlink(maas_id_path)
+        with open(maas_id_path, 'w') as fd:
+            fd.write(rack.system_id)
+        self.assertEquals(
+            rack, Controller.objects.get_running_controller(read_cache=False))
+        self.assertEquals(rack.system_id, get_maas_id())
 
 
 class TestRackControllerManager(MAASServerTestCase):
@@ -418,8 +442,7 @@
         mock_getaddr_info = self.patch(node_module.socket, "getaddrinfo")
         ip = random.choice(['127.0.0.1', '::1'])
         mock_getaddr_info.return_value = (('', '', '', '', (ip,)),)
-        get_maas_id = self.patch(node_module, 'get_maas_id')
-        get_maas_id.return_value = rack.system_id
+        self.useFixture(MAASIDFixture(rack.system_id))
         self.assertEquals(
             [rack, ],
             RackController.objects.filter_by_url_accessible(ip, False))

=== modified file 'src/maasserver/rpc/regionservice.py'
--- src/maasserver/rpc/regionservice.py	2016-06-14 03:04:05 +0000
+++ src/maasserver/rpc/regionservice.py	2016-06-18 01:12:20 +0000
@@ -1171,7 +1171,8 @@
 
         :return: An instance of `RegionAdvertising`.
         """
-        region_obj = RegionController.objects.get_running_controller()
+        region_obj = RegionController.objects.get_running_controller(
+            read_cache=False)
         # Create the process for this region. This process object will be
         # updated by calls to `update`, which is the responsibility of the
         # rpc-advertise service. Calls to `update` also remove old process

=== modified file 'src/maasserver/start_up.py'
--- src/maasserver/start_up.py	2016-06-16 01:30:23 +0000
+++ src/maasserver/start_up.py	2016-06-18 01:12:20 +0000
@@ -140,7 +140,7 @@
 
 
 def create_region_obj():
-    region_id = get_maas_id()
+    region_id = get_maas_id(read_cache=False)
     hostname = gethostname()
     update_fields = []
     region_filter = Q() if region_id is None else Q(system_id=region_id)

=== modified file 'src/maasserver/tests/test_start_up.py'
--- src/maasserver/tests/test_start_up.py	2016-06-16 01:30:23 +0000
+++ src/maasserver/tests/test_start_up.py	2016-06-18 01:12:20 +0000
@@ -5,6 +5,7 @@
 
 __all__ = []
 
+import os
 from unittest.mock import call
 
 from maasserver import (
@@ -29,6 +30,8 @@
     MockCallsMatch,
     MockNotCalled,
 )
+from provisioningserver.path import get_path
+from provisioningserver.utils.env import set_maas_id
 from provisioningserver.utils.testing import MAASIDFixture
 
 
@@ -61,6 +64,7 @@
         super(TestStartUp, self).tearDown()
         # start_up starts the Twisted event loop, so we need to stop it.
         eventloop.reset().wait(5)
+        set_maas_id(None)
 
     def test_inner_start_up_runs_in_exclusion(self):
         # Disable boot source cache signals.
@@ -112,6 +116,11 @@
         self.addCleanup(bootsources.signals.enable)
         bootsources.signals.disable()
 
+    def tearDown(self):
+        super().tearDown()
+        # Clear maas_id cache
+        set_maas_id(None)
+
     def test__calls_dns_kms_setting_changed_if_master(self):
         self.patch(start_up, "is_master_process").return_value = True
         self.patch(start_up, "post_commit_do")
@@ -162,12 +171,25 @@
 
     """Tests for the actual work done in `create_region_obj`."""
 
+    def tearDown(self):
+        super().tearDown()
+        # Clear maas_id cache
+        set_maas_id(None)
+
     def test__creates_obj(self):
         region = start_up.create_region_obj()
         self.assertIsNotNone(region)
         self.assertIsNotNone(
             RegionController.objects.get(system_id=region.system_id))
 
+    def test__doesnt_read_maas_id_from_cache(self):
+        set_maas_id(factory.make_string())
+        os.unlink(get_path('/var/lib/maas/maas_id'))
+        region = start_up.create_region_obj()
+        self.assertIsNotNone(region)
+        self.assertIsNotNone(
+            RegionController.objects.get(system_id=region.system_id))
+
     def test__finds_region_by_maas_id(self):
         region = factory.make_RegionController()
         self.useFixture(MAASIDFixture(region.system_id))

=== modified file 'src/provisioningserver/register_command.py'
--- src/provisioningserver/register_command.py	2016-06-03 22:32:14 +0000
+++ src/provisioningserver/register_command.py	2016-06-18 01:12:20 +0000
@@ -30,6 +30,11 @@
     set_shared_secret_on_filesystem,
     to_bin,
 )
+from provisioningserver.utils.env import set_maas_id
+from provisioningserver.utils.shell import (
+    call_and_check,
+    ExternalProcessError,
+)
 
 
 all_arguments = (
@@ -103,6 +108,14 @@
             "MAAS region controller URL must be given when supplying the "
             "shared secret via stdin with a non-interactive shell.")
         raise SystemExit(1)
+    try:
+        call_and_check(['systemctl', 'stop', 'maas-rackd'])
+    except ExternalProcessError as e:
+        print("Unable to stop maas-rackd service.")
+        print("Failed with error %s." % e.output_as_unicode)
+        return
+    # maas_id could be stale so remove it
+    set_maas_id(None)
     if args.url is not None:
         with ClusterConfiguration.open_for_update() as config:
             config.maas_url = args.url
@@ -122,3 +135,9 @@
         set_shared_secret_on_filesystem(to_bin(args.secret))
     else:
         InstallSharedSecretScript.run(args)
+    try:
+        call_and_check(['systemctl', 'enable', 'maas-rackd'])
+        call_and_check(['systemctl', 'start', 'maas-rackd'])
+    except ExternalProcessError as e:
+        print("Unable to enable and start the maas-rackd service")
+        print("Failed with error %s." % e.output_as_unicode)

=== modified file 'src/provisioningserver/rpc/clusterservice.py'
--- src/provisioningserver/rpc/clusterservice.py	2016-06-14 04:39:10 +0000
+++ src/provisioningserver/rpc/clusterservice.py	2016-06-18 01:12:20 +0000
@@ -531,7 +531,7 @@
             cluster_uuid = config.cluster_uuid
 
         # Grab the set system_id if already set for this controller.
-        system_id = get_maas_id()
+        system_id = get_maas_id(read_cache=False)
         if system_id is None:
             # Cannot send None over RPC when the system_id is not set.
             system_id = ''

=== modified file 'src/provisioningserver/rpc/tests/test_clusterservice.py'
--- src/provisioningserver/rpc/tests/test_clusterservice.py	2016-06-14 04:39:10 +0000
+++ src/provisioningserver/rpc/tests/test_clusterservice.py	2016-06-18 01:12:20 +0000
@@ -61,6 +61,7 @@
     power_drivers_by_name,
     PowerError,
 )
+from provisioningserver.path import get_path
 from provisioningserver.power import QUERY_POWER_TYPES
 from provisioningserver.power.schema import JSON_POWER_TYPE_PARAMETERS
 from provisioningserver.rpc import (
@@ -95,6 +96,7 @@
 from provisioningserver.security import set_shared_secret_on_filesystem
 from provisioningserver.service_monitor import service_monitor
 from provisioningserver.testing.config import ClusterConfigurationFixture
+from provisioningserver.utils.env import set_maas_id
 from provisioningserver.utils.fs import NamedLock
 from provisioningserver.utils.network import get_all_interfaces_definition
 from provisioningserver.utils.shell import ExternalProcessError
@@ -844,7 +846,7 @@
         self.set_maas_id = self.patch(clusterservice, "set_maas_id")
         self.set_maas_id.side_effect = set_maas_id
 
-        def get_maas_id():
+        def get_maas_id(read_cache=True):
             return self.maas_id
 
         self.get_maas_id = self.patch(clusterservice, "get_maas_id")
@@ -1247,6 +1249,28 @@
         self.assertThat(self.set_maas_id, MockCalledOnceWith(system_id))
 
     @inlineCallbacks
+    def test_registerRackWithRegion_doesnt_read_maas_id_from_cache(self):
+        set_maas_id(factory.make_string())
+        os.unlink(get_path('/var/lib/maas/maas_id'))
+
+        maas_url = factory.make_simple_http_url()
+        hostname = platform.node().split('.')[0]
+        interfaces = get_all_interfaces_definition()
+        self.useFixture(ClusterConfigurationFixture(
+            maas_url=maas_url))
+        fixture = self.useFixture(MockLiveClusterToRegionRPCFixture())
+        protocol, connecting = fixture.makeEventLoop()
+        self.addCleanup((yield connecting))
+        yield getRegionClient()
+        self.assertThat(
+            protocol.RegisterRackController, MockCalledOnceWith(
+                protocol, system_id='', hostname=hostname,
+                interfaces=interfaces, url=urlparse(maas_url),
+                nodegroup_uuid=None))
+        # Clear cache for the next test
+        set_maas_id(None)
+
+    @inlineCallbacks
     def test_registerRackWithRegion_returns_False_when_rejected(self):
         client = self.make_running_client()
 

=== modified file 'src/provisioningserver/tests/test_register_command.py'
--- src/provisioningserver/tests/test_register_command.py	2016-06-03 18:48:32 +0000
+++ src/provisioningserver/tests/test_register_command.py	2016-06-18 01:12:20 +0000
@@ -9,10 +9,16 @@
 import io
 from itertools import combinations
 import pprint
-from unittest.mock import Mock
+from unittest.mock import (
+    call,
+    Mock,
+)
 
 from maastesting.factory import factory
-from maastesting.matchers import MockCalledOnceWith
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockCallsMatch,
+)
 from maastesting.testcase import MAASTestCase
 from provisioningserver import register_command
 from provisioningserver.config import ClusterConfiguration
@@ -22,6 +28,8 @@
     to_hex,
 )
 from provisioningserver.testing.config import ClusterConfigurationFixture
+from provisioningserver.utils.env import get_maas_id
+from provisioningserver.utils.testing import MAASIDFixture
 from testtools.matchers import Equals
 
 
@@ -92,6 +100,8 @@
     def setUp(self):
         super(TestRegisterMAASRack, self).setUp()
         self.useFixture(ClusterConfigurationFixture())
+        self.mock_call_and_check = self.patch_autospec(
+            register_command, 'call_and_check')
 
     def make_args(self, **kwargs):
         args = Mock()
@@ -170,3 +180,20 @@
         input.side_effect = KeyboardInterrupt
         self.assertRaises(
             KeyboardInterrupt, register_command.run, args)
+
+    def test__restarts_maas_rackd_service(self):
+        url = factory.make_simple_http_url()
+        secret = factory.make_bytes()
+        register_command.run(self.make_args(url=url, secret=to_hex(secret)))
+        self.assertThat(self.mock_call_and_check, MockCallsMatch(
+            call(['systemctl', 'stop', 'maas-rackd']),
+            call(['systemctl', 'enable', 'maas-rackd']),
+            call(['systemctl', 'start', 'maas-rackd'])
+        ))
+
+    def test__deletes_maas_id_file(self):
+        self.useFixture(MAASIDFixture(factory.make_string()))
+        url = factory.make_simple_http_url()
+        secret = factory.make_bytes()
+        register_command.run(self.make_args(url=url, secret=to_hex(secret)))
+        self.assertIsNone(get_maas_id())

=== modified file 'src/provisioningserver/utils/env.py'
--- src/provisioningserver/utils/env.py	2016-06-07 11:34:46 +0000
+++ src/provisioningserver/utils/env.py	2016-06-18 01:12:20 +0000
@@ -35,10 +35,19 @@
         os.environ.update(prior_environ)
 
 
-def get_maas_id():
+# Cache the MAAS ID so we don't have to keep reading it from the filesystem.
+# This avoids errors when running maas-rack register while regiond is running.
+_maas_id = None
+
+
+def get_maas_id(read_cache=True):
     """Return the system_id for this rack/region controller that is created
     when either the rack or region first starts.
     """
+    global _maas_id
+    if read_cache and _maas_id is not None:
+        return _maas_id
+
     maas_id_path = get_path('/var/lib/maas/maas_id')
     try:
         with open(maas_id_path, "r", encoding="ascii") as fp:
@@ -46,19 +55,24 @@
     except FileNotFoundError:
         return None
     else:
-        return _normalise_maas_id(contents)
+        _maas_id = _normalise_maas_id(contents)
+        return _maas_id
 
 
 def set_maas_id(system_id):
     """Set the system_id for this rack/region permanently for MAAS."""
+    global _maas_id
     maas_id_path = get_path('/var/lib/maas/maas_id')
-    if _normalise_maas_id(system_id) is None:
+    maas_id = _normalise_maas_id(system_id)
+    if maas_id is None:
         try:
             atomic_delete(maas_id_path)
         except FileNotFoundError:
             pass  # Job done already.
+        _maas_id = None
     else:
-        atomic_write(system_id.encode("ascii"), maas_id_path)
+        atomic_write(maas_id.encode("ascii"), maas_id_path)
+        _maas_id = maas_id
 
 
 def _normalise_maas_id(system_id):

=== modified file 'src/provisioningserver/utils/tests/test_env.py'
--- src/provisioningserver/utils/tests/test_env.py	2016-06-07 11:34:46 +0000
+++ src/provisioningserver/utils/tests/test_env.py	2016-06-18 01:12:20 +0000
@@ -11,11 +11,8 @@
 from maastesting.factory import factory
 from maastesting.testcase import MAASTestCase
 from provisioningserver.path import get_path
-from provisioningserver.utils.env import (
-    environment_variables,
-    get_maas_id,
-    set_maas_id,
-)
+from provisioningserver.utils import env
+from testtools import ExpectedException
 from testtools.matchers import (
     Equals,
     FileContains,
@@ -23,50 +20,49 @@
     Is,
     Not,
 )
-from testtools.testcase import ExpectedException
 
 
 class TestEnvironmentVariables(MAASTestCase):
-    """Tests for `environment_variables`."""
+    """Tests for `env.environment_variables`."""
 
     def make_variable(self):
         return factory.make_name('testvar'), factory.make_name('value')
 
     def test__sets_variables(self):
         var, value = self.make_variable()
-        with environment_variables({var: value}):
-            env = os.environ.copy()
-        self.assertEqual(value, env[var])
+        with env.environment_variables({var: value}):
+            environment = os.environ.copy()
+        self.assertEqual(value, environment[var])
 
     def test__overrides_prior_values(self):
         var, prior_value = self.make_variable()
         temp_value = factory.make_name('temp-value')
-        with environment_variables({var: prior_value}):
-            with environment_variables({var: temp_value}):
-                env = os.environ.copy()
-        self.assertEqual(temp_value, env[var])
+        with env.environment_variables({var: prior_value}):
+            with env.environment_variables({var: temp_value}):
+                environment = os.environ.copy()
+        self.assertEqual(temp_value, environment[var])
 
     def test__leaves_other_variables_intact(self):
         untouched_var, untouched_value = self.make_variable()
         var, value = self.make_variable()
-        with environment_variables({untouched_var: untouched_value}):
-            with environment_variables({var: value}):
-                env = os.environ.copy()
-        self.assertEqual(untouched_value, env[untouched_var])
+        with env.environment_variables({untouched_var: untouched_value}):
+            with env.environment_variables({var: value}):
+                environment = os.environ.copy()
+        self.assertEqual(untouched_value, environment[untouched_var])
 
     def test__restores_variables_to_previous_values(self):
         var, prior_value = self.make_variable()
         temp_value = factory.make_name('temp-value')
-        with environment_variables({var: prior_value}):
-            with environment_variables({var: temp_value}):
+        with env.environment_variables({var: prior_value}):
+            with env.environment_variables({var: temp_value}):
                 pass
-            env = os.environ.copy()
-        self.assertEqual(prior_value, env[var])
+            environment = os.environ.copy()
+        self.assertEqual(prior_value, environment[var])
 
     def test__restores_previously_unset_variables_to_being_unset(self):
         var, value = self.make_variable()
         self.assertNotIn(var, os.environ)
-        with environment_variables({var: value}):
+        with env.environment_variables({var: value}):
             pass
         self.assertNotIn(var, os.environ)
 
@@ -78,7 +74,7 @@
             pass
 
         with ExpectedException(DeliberateException):
-            with environment_variables({var: value}):
+            with env.environment_variables({var: value}):
                 raise DeliberateException()
 
         self.assertNotIn(var, os.environ)
@@ -93,32 +89,36 @@
 
 
 class TestMAASID(MAASTestCase):
-    """Tests for `get_maas_id` and `set_maas_id`."""
+    """Tests for `env.get_maas_id` and `env.set_maas_id`."""
 
     def setUp(self):
         super(TestMAASID, self).setUp()
         self.maas_id_path = get_path('/var/lib/maas/maas_id')
         self.addCleanup(unlink_if_exists, self.maas_id_path)
 
+    def tearDown(self):
+        super(TestMAASID, self).tearDown()
+        env.set_maas_id(None)
+
     def test_get_returns_None_if_maas_id_file_does_not_exist(self):
         self.assertThat(self.maas_id_path, Not(FileExists()))
-        self.assertThat(get_maas_id(), Is(None))
+        self.assertThat(env.get_maas_id(), Is(None))
 
     def test_get_returns_None_if_maas_id_file_is_empty(self):
         with open(self.maas_id_path, "w"):
             pass  # Write nothing.
-        self.assertThat(get_maas_id(), Is(None))
+        self.assertThat(env.get_maas_id(), Is(None))
 
     def test_get_returns_None_if_maas_id_file_is_whitespace(self):
         with open(self.maas_id_path, "w") as fd:
             fd.write(string.whitespace)
-        self.assertThat(get_maas_id(), Is(None))
+        self.assertThat(env.get_maas_id(), Is(None))
 
     def test_get_returns_contents_if_maas_id_file_contains_something(self):
         contents = factory.make_name("contents")
         with open(self.maas_id_path, "w") as fd:
             fd.write(contents)
-        self.assertThat(get_maas_id(), Equals(contents))
+        self.assertThat(env.get_maas_id(), Equals(contents))
 
     def test_get_strips_contents_if_maas_id_file_contains_something(self):
         contents = factory.make_name("contents")
@@ -126,35 +126,76 @@
             fd.write(string.whitespace)
             fd.write(contents)
             fd.write(string.whitespace)
-        self.assertThat(get_maas_id(), Equals(contents))
+        self.assertThat(env.get_maas_id(), Equals(contents))
 
     def test_get_rejects_non_ASCII_content(self):
         contents = factory.make_unicode_non_ascii_string()
         with open(self.maas_id_path, "w") as fd:
             fd.write(contents)
-        self.assertRaises(UnicodeDecodeError, get_maas_id)
+        self.assertRaises(UnicodeDecodeError, env.get_maas_id)
+
+    def test_get_caches_result(self):
+        contents = factory.make_name("contents")
+        with open(self.maas_id_path, "w") as fd:
+            fd.write(contents)
+        self.assertEquals(contents, env.get_maas_id())
+        os.unlink(self.maas_id_path)
+        self.assertEquals(contents, env.get_maas_id())
+
+    def test_get_can_ignore_cache(self):
+        contents = factory.make_name("contents")
+        with open(self.maas_id_path, "w") as fd:
+            fd.write(contents)
+        self.assertEquals(contents, env.get_maas_id())
+        contents = factory.make_name("contents")
+        os.unlink(self.maas_id_path)
+        with open(self.maas_id_path, "w") as fd:
+            fd.write(contents)
+        self.assertEquals(contents, env.get_maas_id(read_cache=False))
 
     def test_set_writes_argument_to_maas_id_file(self):
         contents = factory.make_name("contents")
-        set_maas_id(contents)
+        env.set_maas_id(contents)
         self.assertThat(self.maas_id_path, FileContains(contents))
 
     def test_set_deletes_maas_id_file_if_argument_is_None(self):
         with open(self.maas_id_path, "w") as fd:
             fd.write("This file will be deleted.")
-        set_maas_id(None)
+        env.set_maas_id(None)
         self.assertThat(self.maas_id_path, Not(FileExists()))
+        self.assertIsNone(env.get_maas_id())
 
     def test_set_deletes_maas_id_file_if_argument_is_whitespace(self):
         with open(self.maas_id_path, "w") as fd:
             fd.write("This file will be deleted.")
-        set_maas_id(string.whitespace)
+        env.set_maas_id(string.whitespace)
         self.assertThat(self.maas_id_path, Not(FileExists()))
+        self.assertIsNone(env.get_maas_id())
 
-    def test_set_None_does_nothing_if_maas_id_file_idoes_not_exist(self):
+    def test_set_None_does_nothing_if_maas_id_file_does_not_exist(self):
         self.assertThat(self.maas_id_path, Not(FileExists()))
-        set_maas_id(None)
+        env.set_maas_id(None)
 
     def test_set_rejects_non_ASCII_content(self):
         contents = factory.make_unicode_non_ascii_string()
-        self.assertRaises(UnicodeEncodeError, set_maas_id, contents)
+        self.assertRaises(UnicodeEncodeError, env.set_maas_id, contents)
+
+    def test_set_caches(self):
+        contents = factory.make_name("contents")
+        env.set_maas_id(contents)
+        os.unlink(self.maas_id_path)
+        self.assertEquals(contents, env.get_maas_id())
+
+    def test_set_doesnt_cache_when_write_fails(self):
+        mock_atomic_write = self.patch_autospec(env, 'atomic_write')
+        exception = factory.make_exception()
+        mock_atomic_write.side_effect = exception
+        contents = factory.make_name("contents")
+        with ExpectedException(type(exception)):
+            env.set_maas_id(contents)
+        self.assertIsNone(env.get_maas_id())
+
+    def test_set_caches_to_normalized_value(self):
+        contents = "  %s  " % factory.make_name("contents")
+        env.set_maas_id(contents)
+        self.assertEquals(env._normalise_maas_id(contents), env.get_maas_id())