Merge lp:~gz/juju-ci-tools/substrate_maas2 into lp:juju-ci-tools

[Martin Packman]

Add MAAS 2.0 support in substrate

There are lots of different ways of doing this, the most elegant seemed a subclass for the 'old' api like we do with juju itself. The version selection is unfortunately a little ugly as I can't find a better way of doing it than trying login and swallowing the (unhelpful) failure and trying the other version.

This means I've broken the common pattern of having a usable manager_from_config classmethod, but MAAS was already special cased in practice and the alternatives seemed worse.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

[lp:~gz/juju-ci-tools/substrate_maas2]

1385. By Martin Packman on 2016-04-27

Correct check on MAASAccount types in test

=== modified file 'substrate.py'
--- substrate.py	2016-04-27 19:59:40 +0000
+++ substrate.py	2016-04-27 21:59:07 +0000
@@ -54,7 +54,7 @@
         environ.update(translate_to_env(env.config))
         command_args = ['nova', 'delete'] + instance_ids
     elif provider_type == 'maas':
-        with MAASAccount.manager_from_config(env.config) as substrate:
+        with maas_account_from_config(env.config) as substrate:
             substrate.terminate_instances(instance_ids)
         return
     elif provider_type == 'lxd':
@@ -362,23 +362,15 @@
 
 
 class MAASAccount:
-    """Represent a Mass account."""
+    """Represent a MAAS 2.0 account."""
+
+    _API_PATH = 'api/2.0/'
 
     def __init__(self, profile, url, oauth):
         self.profile = profile
-        self.url = urlparse.urljoin(url, 'api/1.0/')
+        self.url = urlparse.urljoin(url, self._API_PATH)
         self.oauth = oauth
 
-    @classmethod
-    @contextmanager
-    def manager_from_config(cls, config):
-        """Create a ContextManager for a MaasAccount."""
-        manager = cls(
-            config['name'], config['maas-server'], config['maas-oauth'])
-        manager.login()
-        yield manager
-        manager.logout()
-
     def login(self):
         """Login with the maas cli."""
         subprocess.check_call(
@@ -389,18 +381,22 @@
         subprocess.check_call(
             ['maas', 'logout', self.profile])
 
+    def _machine_release_args(self, machine_id):
+        return ['maas', self.profile, 'machine', 'release', machine_id]
+
     def terminate_instances(self, instance_ids):
         """Terminate the specified instances."""
         for instance in instance_ids:
             maas_system_id = instance.split('/')[5]
             log.info('Deleting %s.' % instance)
-            subprocess.check_call(
-                ['maas', self.profile, 'node', 'release', maas_system_id])
+            subprocess.check_call(self._machine_release_args(maas_system_id))
+
+    def _list_allocated_args(self):
+        return ['maas', self.profile, 'machines', 'list-allocated']
 
     def get_allocated_nodes(self):
         """Return a dict of allocated nodes with the hostname as keys."""
-        data = subprocess.check_output(
-            ['maas', self.profile, 'nodes', 'list-allocated'])
+        data = subprocess.check_output(self._list_allocated_args())
         nodes = json.loads(data)
         allocated = {node['hostname']: node for node in nodes}
         return allocated
@@ -417,6 +413,37 @@
         return ips
 
 
+class MAAS1Account(MAASAccount):
+    """Represent a MAAS 1.X account."""
+
+    _API_PATH = 'api/1.0/'
+
+    def _list_allocated_args(self):
+        return ['maas', self.profile, 'nodes', 'list-allocated']
+
+    def _machine_release_args(self, machine_id):
+        return ['maas', self.profile, 'node', 'release', machine_id]
+
+
+@contextmanager
+def maas_account_from_config(config):
+    """Create a ContextManager for either a MAASAccount or a MAAS1Account.
+
+    As it's not possible to tell from the maas config which version of the api
+    to use, try 2.0 and if that fails on login fallback to 1.0 instead.
+    """
+    args = (config['name'], config['maas-server'], config['maas-oauth'])
+    manager = MAASAccount(*args)
+    try:
+        manager.login()
+    except subprocess.CalledProcessError:
+        log.info("Could not login with MAAS 2.0 API, trying 1.0")
+        manager = MAAS1Account(*args)
+        manager.login()
+    yield manager
+    manager.logout()
+
+
 class LXDAccount:
     """Represent a LXD account."""
 
@@ -602,7 +629,7 @@
         # Only MAAS requires special handling at prsent.
         return
     # MAAS hostnames are not resolvable, but we can adapt them to IPs.
-    with MAASAccount.manager_from_config(env.config) as account:
+    with maas_account_from_config(env.config) as account:
         allocated_ips = account.get_allocated_ips()
     for remote in remote_machines:
         if remote.get_address() in allocated_ips:

=== modified file 'tests/test_substrate.py'
--- tests/test_substrate.py	2016-04-27 19:59:40 +0000
+++ tests/test_substrate.py	2016-04-27 21:59:07 +0000
@@ -31,6 +31,8 @@
     LXDAccount,
     make_substrate_manager,
     MAASAccount,
+    MAAS1Account,
+    maas_account_from_config,
     OpenStackAccount,
     parse_euca,
     run_instances,
@@ -159,11 +161,11 @@
         with patch('subprocess.check_call') as cc_mock:
             terminate_instances(env, ['/A/B/C/D/node-3d/'])
         expected = (
-            ['maas', 'login', 'mas', 'http://10.0.10.10/MAAS/api/1.0/',
+            ['maas', 'login', 'mas', 'http://10.0.10.10/MAAS/api/2.0/',
              'a:password:string'],
         )
         self.assertEqual(expected, cc_mock.call_args_list[0][0])
-        expected = (['maas', 'mas', 'node', 'release', 'node-3d'],)
+        expected = (['maas', 'mas', 'machine', 'release', 'node-3d'],)
         self.assertEqual(expected, cc_mock.call_args_list[1][0])
         expected = (['maas', 'logout', 'mas'],)
         self.assertEqual(expected, cc_mock.call_args_list[2][0])
@@ -674,25 +676,85 @@
         client.delete_hosted_service.assert_called_once_with('foo')
 
 
-class TestMAASAcount(TestCase):
-
-    @patch.object(MAASAccount, 'logout', autospec=True)
-    @patch.object(MAASAccount, 'login', autospec=True)
-    def test_manager_from_config(self, li_mock, lo_mock):
-        config = get_maas_env().config
-        with MAASAccount.manager_from_config(config) as account:
-            self.assertEqual(account.profile, 'mas')
-            self.assertEqual(account.url, 'http://10.0.10.10/MAAS/api/1.0/')
-            self.assertEqual(account.oauth, 'a:password:string')
-            # As the class object is patched, the mocked methods
-            # show that self is passed.
-            li_mock.assert_called_once_with(account)
-        lo_mock.assert_called_once_with(account)
-
-    @patch('subprocess.check_call', autospec=True)
-    def test_login(self, cc_mock):
-        config = get_maas_env().config
-        account = MAASAccount(
+class TestMAASAccount(TestCase):
+
+    @patch('subprocess.check_call', autospec=True)
+    def test_login(self, cc_mock):
+        config = get_maas_env().config
+        account = MAASAccount(
+            config['name'], config['maas-server'], config['maas-oauth'])
+        account.login()
+        cc_mock.assert_called_once_with([
+            'maas', 'login', 'mas', 'http://10.0.10.10/MAAS/api/2.0/',
+            'a:password:string'])
+
+    @patch('subprocess.check_call', autospec=True)
+    def test_logout(self, cc_mock):
+        config = get_maas_env().config
+        account = MAASAccount(
+            config['name'], config['maas-server'], config['maas-oauth'])
+        account.logout()
+        cc_mock.assert_called_once_with(['maas', 'logout', 'mas'])
+
+    @patch('subprocess.check_call', autospec=True)
+    def test_terminate_instances(self, cc_mock):
+        config = get_maas_env().config
+        account = MAASAccount(
+            config['name'], config['maas-server'], config['maas-oauth'])
+        instance_ids = ['/A/B/C/D/node-1d/', '/A/B/C/D/node-2d/']
+        account.terminate_instances(instance_ids)
+        cc_mock.assert_any_call(
+            ['maas', 'mas', 'machine', 'release', 'node-1d'])
+        cc_mock.assert_called_with(
+            ['maas', 'mas', 'machine', 'release', 'node-2d'])
+
+    def test_get_allocated_nodes(self):
+        config = get_maas_env().config
+        account = MAASAccount(
+            config['name'], config['maas-server'], config['maas-oauth'])
+        node = make_maas_node('maas-node-1.maas')
+        allocated_nodes_string = '[%s]' % json.dumps(node)
+        with patch('subprocess.check_output', autospec=True,
+                   return_value=allocated_nodes_string) as co_mock:
+            allocated = account.get_allocated_nodes()
+        co_mock.assert_called_once_with(
+            ['maas', 'mas', 'machines', 'list-allocated'])
+        self.assertEqual(node, allocated['maas-node-1.maas'])
+
+    def test_get_allocated_ips(self):
+        config = get_maas_env().config
+        account = MAASAccount(
+            config['name'], config['maas-server'], config['maas-oauth'])
+        node = make_maas_node('maas-node-1.maas')
+        allocated_nodes_string = '[%s]' % json.dumps(node)
+        with patch('subprocess.check_output', autospec=True,
+                   return_value=allocated_nodes_string) as co_mock:
+            ips = account.get_allocated_ips()
+        co_mock.assert_called_once_with(
+            ['maas', 'mas', 'machines', 'list-allocated'])
+        self.assertEqual('10.0.30.165', ips['maas-node-1.maas'])
+
+    def test_get_allocated_ips_empty(self):
+        config = get_maas_env().config
+        account = MAASAccount(
+            config['name'], config['maas-server'], config['maas-oauth'])
+        node = make_maas_node('maas-node-1.maas')
+        node['ip_addresses'] = []
+        allocated_nodes_string = '[%s]' % json.dumps(node)
+        with patch('subprocess.check_output', autospec=True,
+                   return_value=allocated_nodes_string) as co_mock:
+            ips = account.get_allocated_ips()
+        co_mock.assert_called_once_with(
+            ['maas', 'mas', 'machines', 'list-allocated'])
+        self.assertEqual({}, ips)
+
+
+class TestMAAS1Account(TestCase):
+
+    @patch('subprocess.check_call', autospec=True)
+    def test_login(self, cc_mock):
+        config = get_maas_env().config
+        account = MAAS1Account(
             config['name'], config['maas-server'], config['maas-oauth'])
         account.login()
         cc_mock.assert_called_once_with([
@@ -702,7 +764,7 @@
     @patch('subprocess.check_call', autospec=True)
     def test_logout(self, cc_mock):
         config = get_maas_env().config
-        account = MAASAccount(
+        account = MAAS1Account(
             config['name'], config['maas-server'], config['maas-oauth'])
         account.logout()
         cc_mock.assert_called_once_with(['maas', 'logout', 'mas'])
@@ -710,7 +772,7 @@
     @patch('subprocess.check_call', autospec=True)
     def test_terminate_instances(self, cc_mock):
         config = get_maas_env().config
-        account = MAASAccount(
+        account = MAAS1Account(
             config['name'], config['maas-server'], config['maas-oauth'])
         instance_ids = ['/A/B/C/D/node-1d/', '/A/B/C/D/node-2d/']
         account.terminate_instances(instance_ids)
@@ -719,10 +781,9 @@
         cc_mock.assert_called_with(
             ['maas', 'mas', 'node', 'release', 'node-2d'])
 
-    @patch('subprocess.check_call', autospec=True)
-    def test_get_allocated_nodes(self, cc_mock):
+    def test_get_allocated_nodes(self):
         config = get_maas_env().config
-        account = MAASAccount(
+        account = MAAS1Account(
             config['name'], config['maas-server'], config['maas-oauth'])
         node = make_maas_node('maas-node-1.maas')
         allocated_nodes_string = '[%s]' % json.dumps(node)
@@ -733,32 +794,100 @@
             ['maas', 'mas', 'nodes', 'list-allocated'])
         self.assertEqual(node, allocated['maas-node-1.maas'])
 
-    @patch('subprocess.check_call', autospec=True)
-    def test_get_allocated_ips(self, cc_mock):
+    def test_get_allocated_ips(self):
         config = get_maas_env().config
-        account = MAASAccount(
+        account = MAAS1Account(
             config['name'], config['maas-server'], config['maas-oauth'])
         node = make_maas_node('maas-node-1.maas')
         allocated_nodes_string = '[%s]' % json.dumps(node)
         with patch('subprocess.check_output', autospec=True,
-                   return_value=allocated_nodes_string):
+                   return_value=allocated_nodes_string) as co_mock:
             ips = account.get_allocated_ips()
+        co_mock.assert_called_once_with(
+            ['maas', 'mas', 'nodes', 'list-allocated'])
         self.assertEqual('10.0.30.165', ips['maas-node-1.maas'])
 
-    @patch('subprocess.check_call', autospec=True)
-    def test_get_allocated_ips_empty(self, cc_mock):
+    def test_get_allocated_ips_empty(self):
         config = get_maas_env().config
-        account = MAASAccount(
+        account = MAAS1Account(
             config['name'], config['maas-server'], config['maas-oauth'])
         node = make_maas_node('maas-node-1.maas')
         node['ip_addresses'] = []
         allocated_nodes_string = '[%s]' % json.dumps(node)
         with patch('subprocess.check_output', autospec=True,
-                   return_value=allocated_nodes_string):
+                   return_value=allocated_nodes_string) as co_mock:
             ips = account.get_allocated_ips()
+        co_mock.assert_called_once_with(
+            ['maas', 'mas', 'nodes', 'list-allocated'])
         self.assertEqual({}, ips)
 
 
+class TestMAASAccountFromConfig(TestCase):
+
+    def test_login_succeeds(self):
+        config = get_maas_env().config
+        with patch('subprocess.check_call', autospec=True) as cc_mock:
+            with maas_account_from_config(config) as maas:
+                self.assertIs(type(maas), MAASAccount)
+                self.assertEqual(maas.profile, 'mas')
+                self.assertEqual(maas.url, 'http://10.0.10.10/MAAS/api/2.0/')
+                self.assertEqual(maas.oauth, 'a:password:string')
+                # The login call has happened on context manager enter, reset
+                # the mock after to verify the logout call.
+                cc_mock.assert_called_once_with([
+                    'maas', 'login', 'mas', 'http://10.0.10.10/MAAS/api/2.0/',
+                    'a:password:string'])
+                cc_mock.reset_mock()
+        cc_mock.assert_called_once_with(['maas', 'logout', 'mas'])
+
+    def test_login_fallback(self):
+        config = get_maas_env().config
+        login_error = CalledProcessError(1, ['maas', 'login'])
+        with patch('subprocess.check_call', autospec=True,
+                   side_effect=[login_error, None, None]) as cc_mock:
+            with maas_account_from_config(config) as maas:
+                self.assertIs(type(maas), MAAS1Account)
+                self.assertEqual(maas.profile, 'mas')
+                self.assertEqual(maas.url, 'http://10.0.10.10/MAAS/api/1.0/')
+                self.assertEqual(maas.oauth, 'a:password:string')
+                # The first login attempt was with the 2.0 api, after which
+                # a 1.0 login succeeded.
+                self.assertEquals(cc_mock.call_args_list, [
+                    call(['maas', 'login', 'mas',
+                          'http://10.0.10.10/MAAS/api/2.0/',
+                          'a:password:string']),
+                    call(['maas', 'login', 'mas',
+                          'http://10.0.10.10/MAAS/api/1.0/',
+                          'a:password:string']),
+                ])
+                cc_mock.reset_mock()
+        cc_mock.assert_called_once_with(['maas', 'logout', 'mas'])
+        self.assertEqual(
+            self.log_stream.getvalue(),
+            'INFO Could not login with MAAS 2.0 API, trying 1.0\n')
+
+    def test_login_both_fail(self):
+        config = get_maas_env().config
+        login_error = CalledProcessError(1, ['maas', 'login'])
+        with patch('subprocess.check_call', autospec=True,
+                   side_effect=login_error) as cc_mock:
+            with self.assertRaises(CalledProcessError) as ctx:
+                with maas_account_from_config(config):
+                    self.fail('Should never get manager with failed login')
+        self.assertIs(ctx.exception, login_error)
+        self.assertEquals(cc_mock.call_args_list, [
+            call(['maas', 'login', 'mas',
+                  'http://10.0.10.10/MAAS/api/2.0/',
+                  'a:password:string']),
+            call(['maas', 'login', 'mas',
+                  'http://10.0.10.10/MAAS/api/1.0/',
+                  'a:password:string']),
+        ])
+        self.assertEqual(
+            self.log_stream.getvalue(),
+            'INFO Could not login with MAAS 2.0 API, trying 1.0\n')
+
+
 class TestMakeSubstrateManager(TestCase):
 
     def test_make_substrate_manager_aws(self):