Merge lp:~mskalka/juju-ci-tools/remove-constraints-from-bootstrap into lp:juju-ci-tools

[Michael Skalka]

Removes forced constraints from Jujupy 2.x bootstraps with the exception of MaaS and Joyent environments where minimal constraints are needed. This forces Juju to determine its own instance type on bootstrap.

[Curtis Hovey (sinzui)]

This is fine to merge, but you must also be prepare to revert. While mem=2G is being ignored by Juju 2 because it does not satisfy juju controller minimums, the value is still used in models. We may find some tests fail in some substrates because our tests do not set a minimum requirement.

If add_basic_testing_arguments() provided --constraints, could tune tests for substrates. This will be a requirement if you need to revert.

review: Approve (code)

[Michael Skalka (mskalka)]

Going back to the bug Aaron submitted, if we have a substrate that Juju is pulling invalid instance-types for then that needs to be fixed in Juju, not our tests. If that means our tests break until they fix it then that's what happens. I agree with his statement that Juju should work 'out of the box' in all 2.0 cases.

Regarding being used in models, is that in our own code? I'm not sure what you mean there.

I'm going to revert the restore_backup change, but I would like to hear your thoughts before I merge this.

[lp:~mskalka/juju-ci-tools/remove-constraints-from-bootstrap]

1972. By Michael Skalka on 2017-04-06

reverted restore backup change

Unmerged revisions

1972. By Michael Skalka on 2017-04-06

reverted restore backup change

1971. By Michael Skalka on 2017-04-03

removed constraints from 2.x juju client bootstraps

=== modified file 'jujupy/client.py'
--- jujupy/client.py	2017-04-05 23:07:51 +0000
+++ jujupy/client.py	2017-04-06 14:39:27 +0000
@@ -1858,11 +1858,13 @@
         cloud_region = self.get_cloud_region(self.env.get_cloud(),
                                              self.env.get_region())
         # Note cloud_region before controller name
-        args = ['--constraints', constraints,
-                cloud_region,
-                self.env.environment,
-                '--config', config_filename,
-                '--default-model', self.env.environment]
+        args = []
+        if constraints:
+            args.extend(['--constraints', constraints])
+        args.extend([cloud_region,
+                     self.env.environment,
+                     '--config', config_filename,
+                     '--default-model', self.env.environment])
         if upload_tools:
             if agent_version is not None:
                 raise ValueError(
@@ -2338,18 +2340,17 @@
     def _get_substrate_constraints(self):
         if self.env.joyent:
             # Only accept kvm packages by requiring >1 cpu core, see lp:1446264
-            return 'mem=2G cpu-cores=1'
+            return 'cpu-cores=1'
         elif self.env.maas and self._maas_spaces_enabled():
             # For now only maas support spaces in a meaningful way.
-            return 'mem=2G spaces={}'.format(','.join(
+            return 'spaces={}'.format(','.join(
                 '^' + space for space in sorted(self.excluded_spaces)))
         else:
-            return 'mem=2G'
+            return None
 
     def quickstart(self, bundle_template, upload_tools=False):
         bundle = self.format_bundle(bundle_template)
-        constraints = 'mem=2G'
-        args = ('--constraints', constraints)
+        args = ()
         if upload_tools:
             args = ('--upload-tools',) + args
         args = args + ('--no-browser', bundle,)
@@ -2715,8 +2716,7 @@
             ('-b', '--constraints', 'mem=2G', '--file', backup_file))
 
     def restore_backup_async(self, backup_file):
-        return self.juju_async('restore-backup', ('-b', '--constraints',
-                               'mem=2G', '--file', backup_file))
+        return self.juju_async('restore-backup', ('-b', '--file', backup_file))
 
     def enable_ha(self):
         self.juju(

=== modified file 'jujupy/tests/test_client.py'
--- jujupy/tests/test_client.py	2017-03-28 00:51:36 +0000
+++ jujupy/tests/test_client.py	2017-04-06 14:39:27 +0000
@@ -668,7 +668,7 @@
                     client.bootstrap()
             mock.assert_called_with(
                 'bootstrap', (
-                    '--constraints', 'mem=2G spaces=^endpoint-bindings-data,'
+                    '--constraints', 'spaces=^endpoint-bindings-data,'
                     '^endpoint-bindings-public',
                     'foo/asdf', 'maas',
                     '--config', config_file.name, '--default-model', 'maas',
@@ -686,7 +686,6 @@
                     client.bootstrap()
             mock.assert_called_with(
                 'bootstrap', (
-                    '--constraints', 'mem=2G',
                     'foo/asdf', 'maas',
                     '--config', config_file.name, '--default-model', 'maas',
                     '--agent-version', '2.0'),
@@ -701,8 +700,13 @@
                 with observable_temp_file() as config_file:
                     client.bootstrap()
             mock.assert_called_once_with(
+<<<<<<< TREE
                 'bootstrap', (
                     '--constraints', 'mem=2G cpu-cores=1',
+=======
+                client, 'bootstrap', (
+                    '--constraints', 'cpu-cores=1',
+>>>>>>> MERGE-SOURCE
                     'joyent/foo', 'joyent',
                     '--config', config_file.name,
                     '--default-model', 'joyent', '--agent-version', '2.0',
@@ -715,8 +719,7 @@
                 client = ModelClient(env, '2.0-zeta1', None)
                 client.bootstrap()
                 mock.assert_called_with(
-                    'bootstrap', ('--constraints', 'mem=2G',
-                                  'bar/baz', 'foo',
+                    'bootstrap', ('bar/baz', 'foo',
                                   '--config', config_file.name,
                                   '--default-model', 'foo',
                                   '--agent-version', '2.0'), include_e=False)
@@ -732,7 +735,7 @@
                 client.bootstrap(upload_tools=True)
         mock.assert_called_with(
             'bootstrap', (
-                '--upload-tools', '--constraints', 'mem=2G',
+                '--upload-tools',
                 'foo/baz', 'foo',
                 '--config', config_file.name,
                 '--default-model', 'foo'), include_e=False)
@@ -745,7 +748,6 @@
                 client.bootstrap(credential='credential_name')
         mock.assert_called_with(
             'bootstrap', (
-                '--constraints', 'mem=2G',
                 'foo/baz', 'foo',
                 '--config', config_file.name,
                 '--default-model', 'foo', '--agent-version', '2.0',
@@ -759,7 +761,6 @@
                 client.bootstrap(bootstrap_series='angsty')
         mock.assert_called_with(
             'bootstrap', (
-                '--constraints', 'mem=2G',
                 'bar/baz', 'foo',
                 '--config', config_file.name, '--default-model', 'foo',
                 '--agent-version', '2.0',
@@ -773,7 +774,6 @@
                 client.bootstrap(auto_upgrade=True)
         mock.assert_called_with(
             'bootstrap', (
-                '--constraints', 'mem=2G',
                 'bar/baz', 'foo',
                 '--config', config_file.name, '--default-model', 'foo',
                 '--agent-version', '2.0', '--auto-upgrade'), include_e=False)
@@ -786,7 +786,6 @@
                 client.bootstrap(no_gui=True)
         mock.assert_called_with(
             'bootstrap', (
-                '--constraints', 'mem=2G',
                 'bar/baz', 'foo',
                 '--config', config_file.name, '--default-model', 'foo',
                 '--agent-version', '2.0', '--no-gui'), include_e=False)
@@ -799,7 +798,6 @@
                 client.bootstrap(metadata_source='/var/test-source')
         mock.assert_called_with(
             'bootstrap', (
-                '--constraints', 'mem=2G',
                 'bar/baz', 'foo',
                 '--config', config_file.name, '--default-model', 'foo',
                 '--agent-version', '2.0',
@@ -812,8 +810,7 @@
         args = client.get_bootstrap_args(
             upload_tools=False, config_filename='config')
         self.assertEqual(
-            ('--constraints', 'mem=2G', 'bar/baz', 'foo',
-             '--config', 'config', '--default-model', 'foo',
+            ('bar/baz', 'foo', '--config', 'config', '--default-model', 'foo',
              '--agent-version', '2.0', '--to', 'zone=fnord'),
             args)
 
@@ -826,7 +823,6 @@
                 with client.bootstrap_async():
                     mock.assert_called_once_with(
                         client, 'bootstrap', (
-                            '--constraints', 'mem=2G',
                             'bar/baz', 'foo',
                             '--config', config_file.name,
                             '--default-model', 'foo',
@@ -840,7 +836,7 @@
                 with client.bootstrap_async(upload_tools=True):
                     mock.assert_called_with(
                         client, 'bootstrap', (
-                            '--upload-tools', '--constraints', 'mem=2G',
+                            '--upload-tools',
                             'bar/baz', 'foo',
                             '--config', config_file.name,
                             '--default-model', 'foo',
@@ -854,7 +850,7 @@
                                          config_filename='config',
                                          bootstrap_series='angsty')
         self.assertEqual(args, (
-            '--upload-tools', '--constraints', 'mem=2G',
+            '--upload-tools',
             'bar/baz', 'foo',
             '--config', 'config', '--default-model', 'foo',
             '--bootstrap-series', 'angsty'))
@@ -865,8 +861,7 @@
         args = client.get_bootstrap_args(upload_tools=False,
                                          config_filename='config',
                                          agent_version='2.0-lambda1')
-        self.assertEqual(('--constraints', 'mem=2G',
-                          'bar/baz', 'foo',
+        self.assertEqual(('bar/baz', 'foo',
                           '--config', 'config', '--default-model', 'foo',
                           '--agent-version', '2.0-lambda1'), args)
 
@@ -2988,7 +2983,7 @@
         with patch.object(client, 'juju_async') as gjo_mock:
             result = client.restore_backup_async('quxx')
         gjo_mock.assert_called_once_with('restore-backup', (
-            '-b', '--constraints', 'mem=2G', '--file', 'quxx'))
+            '-b', '--file', 'quxx'))
         self.assertIs(gjo_mock.return_value, result)
 
     def test_enable_ha(self):
@@ -3123,7 +3118,7 @@
         with patch.object(ModelClient, 'juju') as mock:
             client.quickstart('bundle:~juju-qa/some-bundle')
         mock.assert_called_with(
-            'quickstart', ('--constraints', 'mem=2G', '--no-browser',
+            'quickstart', ('--no-browser',
                            'bundle:~juju-qa/some-bundle'),
             extra_env={'JUJU': '/juju'})
 
@@ -3133,8 +3128,7 @@
         with patch.object(ModelClient, 'juju') as mock:
             client.quickstart('bundle:~juju-qa/some-bundle')
         mock.assert_called_with(
-            'quickstart', ('--constraints', 'mem=2G', '--no-browser',
-                           'bundle:~juju-qa/some-bundle'),
+            'quickstart', ('--no-browser', 'bundle:~juju-qa/some-bundle'),
             extra_env={'JUJU': '/juju'})
 
     def test_quickstart_template(self):
@@ -3143,8 +3137,7 @@
         with patch.object(ModelClient, 'juju') as mock:
             client.quickstart('bundle:~juju-qa/some-{container}-bundle')
         mock.assert_called_with(
-            'quickstart', ('--constraints', 'mem=2G', '--no-browser',
-                           'bundle:~juju-qa/some-lxd-bundle'),
+            'quickstart', ('--no-browser', 'bundle:~juju-qa/some-lxd-bundle'),
             extra_env={'JUJU': '/juju'})
 
     def test_action_do(self):

=== modified file 'jujupy/version_client.py'
--- jujupy/version_client.py	2017-04-05 23:07:51 +0000
+++ jujupy/version_client.py	2017-04-06 14:39:27 +0000
@@ -432,6 +432,15 @@
         else:
             return 'mem=2G'
 
+    def quickstart(self, bundle_template, upload_tools=False):
+        bundle = self.format_bundle(bundle_template)
+        constraints = 'mem=2G'
+        args = ('--constraints', constraints)
+        if upload_tools:
+            args = ('--upload-tools',) + args
+        args = args + ('--no-browser', bundle,)
+        self.juju('quickstart', args, extra_env={'JUJU': self.full_path})
+
     def get_bootstrap_args(self, upload_tools, bootstrap_series=None,
                            credential=None):
         """Return the bootstrap arguments for the substrate."""

=== modified file 'tests/test_assess_cloud.py'
--- tests/test_assess_cloud.py	2017-03-02 16:11:04 +0000
+++ tests/test_assess_cloud.py	2017-04-06 14:39:27 +0000
@@ -79,7 +79,7 @@
         test_case.assertEqual([
             backend_call(
                 client, 'bootstrap', (
-                    '--constraints', 'mem=2G', 'foo/bar', 'foo', '--config',
+                    'foo/bar', 'foo', '--config',
                     config_file.name, '--default-model', 'foo',
                     '--agent-version', client.version)),
             backend_call(client, 'deploy', 'ubuntu', 'foo:foo'),
@@ -107,7 +107,7 @@
         test_case.assertEqual([
             backend_call(
                 client, 'bootstrap', (
-                    '--constraints', 'mem=2G', 'foo/bar', 'foo', '--config',
+                    'foo/bar', 'foo', '--config',
                     config_file.name, '--default-model', 'foo',
                     '--agent-version', client.version)),
             backend_call(
@@ -144,7 +144,7 @@
         expected = [
             backend_call(
                 client, 'bootstrap', (
-                    '--constraints', 'mem=2G', 'foo/bar', 'foo', '--config',
+                    'foo/bar', 'foo', '--config',
                     config_file.name, '--default-model', 'foo',
                     '--agent-version', client.version)),
             ] + series_calls + [

=== modified file 'tests/test_deploy_stack.py'
--- tests/test_deploy_stack.py	2017-04-05 23:07:51 +0000
+++ tests/test_deploy_stack.py	2017-04-06 14:39:27 +0000
@@ -2342,8 +2342,8 @@
                                   upload_tools=False):
                     pass
         assert_juju_call(self, cc_mock, client, (
-            'path', '--show-log', 'bootstrap', '--constraints',
-            'mem=2G', 'paas/qux', 'bar', '--config', config_file.name,
+            'path', '--show-log', 'bootstrap', 'paas/qux', 'bar',
+            '--config', config_file.name,
             '--default-model', 'bar', '--agent-version', '1.23'), 0)
         assert_juju_call(self, cc_mock, client, (
             'path', '--show-log', 'list-controllers'), 1)
@@ -2381,8 +2381,8 @@
                                   None, keep_env=True, upload_tools=False):
                     pass
         assert_juju_call(self, cc_mock, client, (
-            'path', '--show-log', 'bootstrap', '--constraints',
-            'mem=2G', 'paas/qux', 'bar', '--config', config_file.name,
+            'path', '--show-log', 'bootstrap', 'paas/qux',
+            'bar', '--config', config_file.name,
             '--default-model', 'bar', '--agent-version', '1.23'), 0)
         assert_juju_call(self, cc_mock, client, (
             'path', '--show-log', 'list-controllers'), 1)
@@ -2421,7 +2421,7 @@
                     pass
         assert_juju_call(self, cc_mock, client, (
             'path', '--show-log', 'bootstrap', '--upload-tools',
-            '--constraints', 'mem=2G', 'paas/qux', 'bar', '--config',
+            'paas/qux', 'bar', '--config',
             config_file.name, '--default-model', 'bar'), 0)
 
     def test_upload_tools_non_jes(self):
@@ -2433,8 +2433,8 @@
                               keep_env=False, upload_tools=True):
                 pass
         assert_juju_call(self, cc_mock, client, (
-            'path', '--show-log', 'bootstrap', '-e', 'bar', '--upload-tools',
-            '--constraints', 'mem=2G'), 0)
+            'path', '--show-log', 'bootstrap', '-e', 'bar',
+            '--upload-tools', '--constraints', 'mem=2G'), 0)
 
     def test_calls_update_env_2(self):
         cc_mock = self.addContext(patch('subprocess.check_call'))
@@ -2452,7 +2452,7 @@
             client.env, 'bar', agent_url='url', agent_stream='devel',
             series='wacky', bootstrap_host=None, region=None)
         assert_juju_call(self, cc_mock, client, (
-            'path', '--show-log', 'bootstrap', '--constraints', 'mem=2G',
+            'path', '--show-log', 'bootstrap',
             'paas/qux', 'bar', '--config', config_file.name,
             '--default-model', 'bar', '--agent-version', '1.23',
             '--bootstrap-series', 'wacky'), 0)
@@ -2616,9 +2616,9 @@
                         pass
                 self.assertIs(ctx.exception, error)
         assert_juju_call(self, cc_mock, client, (
-            'path', '--show-log', 'bootstrap', '--constraints',
-            'mem=2G', 'paas/qux', 'bar', '--config', config_file.name,
-            '--default-model', 'bar', '--agent-version', '1.23'), 0)
+            'path', '--show-log', 'bootstrap', 'paas/qux', 'bar',
+            '--config', config_file.name, '--default-model', 'bar',
+            '--agent-version', '1.23'), 0)
         assert_juju_call(self, cc_mock, client, (
             'path', '--show-log', 'list-controllers'), 1)
         assert_juju_call(self, cc_mock, client, (

=== modified file 'tests/test_industrial_test.py'
--- tests/test_industrial_test.py	2017-04-04 04:30:31 +0000
+++ tests/test_industrial_test.py	2017-04-06 14:39:27 +0000
@@ -1212,7 +1212,7 @@
             with patch('subprocess.Popen') as popen_mock:
                 self.assertEqual(boot_iter.next(), {'test_id': 'bootstrap'})
             assert_juju_call(self, popen_mock, client, (
-                'juju', '--show-log', 'bootstrap', '--constraints', 'mem=2G',
+                'juju', '--show-log', 'bootstrap',
                 'fake/regionx', 'steve', '--config', config_file.name,
                 '--default-model', 'steve', '--agent-version', '1.2'))
             statuses = [
@@ -1967,7 +1967,7 @@
                 self.assertEqual({'test_id': 'prepare-upgrade-juju'},
                                  puj_iterator.next())
             assert_juju_call(self, po_mock, future_client, (
-                'juju', '--show-log', 'bootstrap', '--constraints', 'mem=2G',
+                'juju', '--show-log', 'bootstrap',
                 'fake/regionx', 'steve', '--config', config_file.name,
                 '--default-model', 'steve', '--agent-version', '2.0.0'))
             po_mock.return_value.wait.return_value = 0