Merge lp:~sseman/juju-ci-tools/resource-attach-2 into lp:juju-ci-tools

[Seman]

This is the second Resources branch.

- Added wait_for_resource function to wait until a resource has been downloaded to a unit.
- Added a CI test: update a resource file, attach and make sure the update is propagated to the unit.
- Added large tests by creating dummy large file and use it as a resource. An average, it takes about an hour to download 200 MB file from the controller to the unit. To me, this is slow. Users may have to wait for about half an hour to download Java runtime.

This completes adding CI tests for Resources. The next branch will add test for Resource Charmstore.

First branch:
https://code.launchpad.net/~sseman/juju-ci-tools/resource-attach/+merge/295141

[Curtis Hovey (sinzui)]

Hi seman. I like this branch's feature, but am not excited by the passing of args to so many methods. It isn't clear which args are used. I had to search the diff to learn that a deep function needs agent_timeout and resource_timeout. The function's required are ambiguous. There are a lot of args to out scripts and I don't want to lookup/make everything to setup timeouts.

I prefer to agent_timeout and resource_timeout as args. If passing timeouts is a problem for many scripts, maybe we want to create a timeout object that has default timeouts for everything and we can override it in scripts and tests.

review: Needs Information (code)

[Curtis Hovey (sinzui)]

We agreed on IRC to replace args with agent_timeout and resource_timeout.

review: Approve (code)

[lp:~sseman/juju-ci-tools/resource-attach-2]

1419. By Seman on 2016-05-24

Replaced args with agent_timeout and resource_timeout.

[Seman (sseman)]

Replaced args with agent_timeout and resource_timeout.

=== modified file 'assess_resources.py'
--- assess_resources.py	2016-05-19 03:28:29 +0000
+++ assess_resources.py	2016-05-24 22:43:31 +0000
@@ -6,6 +6,7 @@
 import logging
 import os
 import sys
+from tempfile import NamedTemporaryFile
 
 from deploy_stack import (
     BootstrapManager,
@@ -56,38 +57,95 @@
         raise JujuAssertionError('Resource id not found.')
 
 
-def push_resource(client, resource_name, finger_print, size, deploy=True):
+def push_resource(client, resource_name, finger_print, size, agent_timeout,
+                  resource_timeout, deploy=True, resource_file=None):
     charm_name = 'dummy-resource'
     charm_path = local_charm_path(charm=charm_name, juju_ver=client.version)
-    resource_file = os.path.join(charm_path, '{}.txt'.format(resource_name))
+    if resource_file is None:
+        resource_file = os.path.join(
+            charm_path, '{}.txt'.format(resource_name))
+    else:
+        resource_file = os.path.join(charm_path, resource_file)
     resource_arg = '{}={}'.format(resource_name, resource_file)
-    log.info("Deploy charm with resource {}".format(resource_name))
+    log.info("Deploy charm with resource {} Size: {} File: {}".format(
+        resource_name, size, resource_file))
     if deploy:
         client.deploy(charm_path, resource=resource_arg)
     else:
         client.attach(charm_name, resource=resource_arg)
-        # TODO: maybe need to add a wait until resource get is executed.
-    client.wait_for_started()
+    client.wait_for_started(timeout=agent_timeout)
+    resource_id = '{}/{}'.format(charm_name, resource_name)
+    client.wait_for_resource(
+        resource_id, charm_name, timeout=resource_timeout)
     status = client.list_resources(charm_name)
-    resource_id = '{}/{}'.format(charm_name, resource_name)
     verify_status(status, resource_id, resource_name, finger_print, size)
-
-
-def assess_resources(client):
+    client.show_status()
+
+
+def fill_dummy_file(file_path, size):
+    with open(file_path, "wb") as f:
+        f.seek(size - 1)
+        f.write('\0')
+
+
+def large_assess(client, agent_timeout, resource_timeout):
+    tests = [
+        {"size": 1024 * 1024 * 10,
+         "finger_print": ('d7c014629d74ae132cc9f88e3ec2f31652f40a7a1fcc52c54b'
+                          '04d6c0d089169bcd55958d1277b4cdf6262f21c712d0a7')},
+        {"size": 1024 * 1024 * 100,
+         "finger_print": ('c11e93892b66de781e4d0883efe10482f8d1642f3b6574ba2e'
+                          'e0da6f8db03f53c0eadfb5e5e0463574c113024ded369e')},
+        {"size": 1024 * 1024 * 200,
+         "finger_print": ('77db39eca74c6205e31a7701e488a1df4b9b38a527a6084bd'
+                          'bb6843fd430a0b51047378ee0255e633b32c0dda3cf43ab')}]
+    for test in tests:
+        with NamedTemporaryFile(suffix=".txt") as temp_file:
+            fill_dummy_file(temp_file.name, size=test['size'])
+            push_resource(
+                client, 'bar', test['finger_print'], test['size'],
+                agent_timeout, resource_timeout, deploy=False,
+                resource_file=temp_file.name)
+
+
+def assess_resources(client, args):
     finger_print = ('4ddc48627c6404e538bb0957632ef68618c0839649d9ad9e41ad94472'
                     'c1589f4b7f9d830df6c4b209d7eb1b4b5522c4d')
     size = 27
-    push_resource(client, 'foo', finger_print, size)
+    push_resource(client, 'foo', finger_print, size, args.agent_timeout,
+                  args.resource_timeout)
     finger_print = ('ffbf43d68a6960de63908bb05c14a026abeda136119d3797431bdd7b'
                     '469c1f027e57a28aeec0df01a792e9e70aad2d6b')
     size = 17
-    push_resource(client, 'bar', finger_print, size, deploy=False)
+    push_resource(client, 'bar', finger_print, size, args.agent_timeout,
+                  args.resource_timeout, deploy=False)
+    finger_print = ('2a3821585efcccff1562efea4514dd860cd536441954e182a764991'
+                    '0e21f6a179a015677a68a351a11d3d2f277e551e4')
+    size = 27
+    push_resource(client, 'bar', finger_print, size, args.agent_timeout,
+                  args.resource_timeout, deploy=False, resource_file='baz.txt')
+    with NamedTemporaryFile(suffix=".txt") as temp_file:
+        size = 1024 * 1024
+        finger_print = ('3164673a8ac27576ab5fc06b9adc4ce0aca5bd3025384b1cf2128'
+                        'a8795e747c431e882785a0bf8dc70b42995db388575')
+        fill_dummy_file(temp_file.name, size=size)
+        push_resource(client, 'bar', finger_print, size, args.agent_timeout,
+                      args.resource_timeout, deploy=False,
+                      resource_file=temp_file.name)
+    if args.large_test_enabled:
+        large_assess(client, args.agent_timeout, args.resource_timeout)
 
 
 def parse_args(argv):
     """Parse all arguments."""
     parser = argparse.ArgumentParser(description="Assess resources")
     add_basic_testing_arguments(parser)
+    parser.add_argument('--large-test-enabled', action='store_true',
+                        help="Uses large file for testing.")
+    parser.add_argument('--agent-timeout', type=int, default=1800,
+                        help='The time to wait for agents to start')
+    parser.add_argument('--resource-timeout', type=int, default=1800,
+                        help='The time to wait for agents to start')
     return parser.parse_args(argv)
 
 
@@ -96,7 +154,7 @@
     configure_logging(args.verbose)
     bs_manager = BootstrapManager.from_args(args)
     with bs_manager.booted_context(args.upload_tools):
-        assess_resources(bs_manager.client)
+        assess_resources(bs_manager.client, args)
     return 0
 
 

=== modified file 'jujupy.py'
--- jujupy.py	2016-05-23 01:34:59 +0000
+++ jujupy.py	2016-05-24 22:43:31 +0000
@@ -36,6 +36,7 @@
     ensure_deleted,
     ensure_dir,
     is_ipv6_address,
+    JujuResourceTimeout,
     pause,
     quote,
     scoped_environ,
@@ -910,6 +911,21 @@
             args = args + ('--details',)
         return yaml_loads(self.get_juju_output('list-resources', *args))
 
+    def wait_for_resource(self, resource_id, service_or_unit, timeout=60):
+        log.info('Waiting for resource. Resource id:{}'.format(resource_id))
+        for _ in until_timeout(timeout):
+            resources = self.list_resources(service_or_unit)['resources']
+            for resource in resources:
+                if resource['expected']['resourceid'] == resource_id:
+                    if (resource['expected']['fingerprint'] ==
+                            resource['unit']['fingerprint']):
+                        return
+            time.sleep(.1)
+        raise JujuResourceTimeout(
+            'Timeout waiting for a resource to be downloaded. '
+            'ResourceId: {} Service or Unit: {} Timeout: {}'.format(
+                    resource_id, service_or_unit, timeout))
+
     def upgrade_charm(self, service, charm_path=None):
         args = (service,)
         if charm_path is not None:

=== modified file 'tests/test_assess_resources.py'
--- tests/test_assess_resources.py	2016-05-19 03:28:29 +0000
+++ tests/test_assess_resources.py	2016-05-24 22:43:31 +0000
@@ -1,10 +1,11 @@
-import copy
 import logging
+from argparse import Namespace
 from mock import Mock, patch, call
 import StringIO
 
 from assess_resources import (
     assess_resources,
+    large_assess,
     parse_args,
     push_resource,
     main,
@@ -57,36 +58,24 @@
         mock_e.assert_called_once_with("an-env")
         mock_c.assert_called_once_with(env, "/bin/juju", debug=False)
         self.assertEqual(mock_bc.call_count, 1)
-        mock_assess.assert_called_once_with(client)
-
-
-class TestAssess(TestCase):
-
-    status = {'resources': [{
-        'expected': {
-            'origin': 'upload', 'used': True, 'description': 'foo resource.',
-            'username': 'admin@local', 'resourceid': 'dummy-resource/foo',
-            'name': 'foo', 'serviceid': 'dummy-resource', 'path': 'foo.txt',
-            'fingerprint': '1234', 'type': 'file', 'size': 27},
-        'unit': {
-            'origin': 'upload', 'username': 'admin@local', 'used': True,
-            'name': 'foo', 'resourceid': 'dummy-resource/foo',
-            'serviceid': 'dummy-resource', 'fingerprint': '1234',
-            'path': 'foo.txt', 'size': 27, 'type': 'file',
-            'description': 'foo resource.'}}]}
+        mock_assess.assert_called_once_with(client, make_args())
+
+
+class TestAssessResources(TestCase):
 
     def test_verify_status(self):
-        verify_status(self.status, 'dummy-resource/foo', 'foo', '1234', 27)
+        verify_status(
+            make_resource_list(), 'dummy-resource/foo', 'foo', '1234', 27)
 
     def test_verify_status_exception(self):
-        status = copy.deepcopy(self.status)
+        status = make_resource_list()
         status['resources'][0]['expected']['origin'] = 'charmstore'
         with self.assertRaisesRegexp(
                 JujuAssertionError, 'Unexpected resource list values'):
             verify_status(status, 'dummy-resource/foo', 'foo', '1234', 27)
 
     def test_verify_status_unit_exception(self):
-        status = copy.deepcopy(self.status)
+        status = make_resource_list()
         status['resources'][0]['unit']['origin'] = 'charmstore'
         with self.assertRaisesRegexp(
                 JujuAssertionError, 'Unexpected unit resource list values'):
@@ -95,38 +84,149 @@
     def test_verify_status_no_resoruce_id_exception(self):
         with self.assertRaisesRegexp(
                 JujuAssertionError, 'Resource id not found.'):
-            verify_status(self.status, 'NO_ID', 'foo', '1234', 27)
+            verify_status(make_resource_list(), 'NO_ID', 'foo', '1234', 27)
 
     def test_push_resource(self):
         mock_client = Mock(
-            spec=["deploy", "wait_for_started", "list_resources"])
+            spec=["deploy", "wait_for_started", "list_resources",
+                  "wait_for_resource", "show_status"])
         mock_client.version = '2.0.0'
-        mock_client.list_resources.return_value = self.status
-        push_resource(mock_client, 'foo', '1234', 27)
+        mock_client.list_resources.return_value = make_resource_list()
+        push_resource(mock_client, 'foo', '1234', 27, 1800, 1800)
         mock_client.deploy.assert_called_once_with(
             'dummy-resource', resource='foo=dummy-resource/foo.txt')
-        mock_client.wait_for_started.assert_called_once_with()
+        mock_client.wait_for_started.assert_called_once_with(timeout=1800)
+        mock_client.show_status.assert_called_once_with()
 
     def test_push_resource_attach(self):
         mock_client = Mock(
-            spec=["attach", "wait_for_started", "list_resources"])
+            spec=["attach", "wait_for_started", "list_resources",
+                  "wait_for_resource", "show_status"])
         mock_client.version = '2.0.0'
-        mock_client.list_resources.return_value = self.status
-        push_resource(mock_client, 'foo', '1234', 27, deploy=False)
+        mock_client.list_resources.return_value = make_resource_list()
+        push_resource(mock_client, 'foo', '1234', 27, 1800, 1800, deploy=False)
         mock_client.attach.assert_called_once_with(
             'dummy-resource', resource='foo=dummy-resource/foo.txt')
-        mock_client.wait_for_started.assert_called_once_with()
+        mock_client.wait_for_started.assert_called_once_with(timeout=1800)
 
     def test_assess_resources(self):
-        with patch("assess_resources.push_resource", autospec=True) as mock_p:
-            assess_resources(None)
-        calls = [
-            call(
-                None, 'foo',
-                '4ddc48627c6404e538bb0957632ef68618c0839649d9ad9e41ad94472c158'
-                '9f4b7f9d830df6c4b209d7eb1b4b5522c4d', 27),
-            call(
-                None, 'bar',
-                'ffbf43d68a6960de63908bb05c14a026abeda136119d3797431bdd7b469c1'
-                'f027e57a28aeec0df01a792e9e70aad2d6b', 17, deploy=False)]
-        self.assertEqual(mock_p.mock_calls, calls)
+        fake_file = FakeFile()
+        args = make_args()
+        with patch("assess_resources.push_resource", autospec=True) as mock_p:
+            with patch('assess_resources.NamedTemporaryFile',
+                       autospec=True) as mock_ntf:
+                mock_ntf.return_value.__enter__.return_value = fake_file
+                assess_resources(None, args)
+        calls = [
+            call(
+                None, 'foo',
+                '4ddc48627c6404e538bb0957632ef68618c0839649d9ad9e41ad94472c158'
+                '9f4b7f9d830df6c4b209d7eb1b4b5522c4d', 27, 1800, 1800),
+            call(
+                None, 'bar',
+                'ffbf43d68a6960de63908bb05c14a026abeda136119d3797431bdd7b469c1'
+                'f027e57a28aeec0df01a792e9e70aad2d6b', 17, 1800, 1800,
+                deploy=False),
+            call(
+                None, 'bar',
+                '2a3821585efcccff1562efea4514dd860cd536441954e182a7649910e21f6'
+                'a179a015677a68a351a11d3d2f277e551e4', 27, 1800, 1800,
+                deploy=False, resource_file='baz.txt'),
+            call(
+                None, 'bar',
+                '3164673a8ac27576ab5fc06b9adc4ce0aca5bd3025384b1cf2128a8795e74'
+                '7c431e882785a0bf8dc70b42995db388575', 1024 * 1024, 1800, 1800,
+                deploy=False, resource_file='/tmp/fake'),
+        ]
+        self.assertEqual(mock_p.mock_calls, calls)
+
+    def test_assess_resources_large_test(self):
+        fake_file = FakeFile()
+        args = make_args()
+        args.large_test_enabled = True
+        with patch("assess_resources.push_resource", autospec=True) as mock_p:
+            with patch('assess_resources.fill_dummy_file',
+                       autospec=True) as mock_fdf:
+                with patch('assess_resources.NamedTemporaryFile',
+                           autospec=True) as mock_ntf:
+                    mock_ntf.return_value.__enter__.return_value = fake_file
+                    with patch('assess_resources.large_assess',
+                               autospec=True) as mock_lt:
+                        assess_resources(None, args)
+        calls = [
+            call(
+                None, 'foo',
+                '4ddc48627c6404e538bb0957632ef68618c0839649d9ad9e41ad94472c158'
+                '9f4b7f9d830df6c4b209d7eb1b4b5522c4d', 27, 1800, 1800),
+            call(
+                None, 'bar',
+                'ffbf43d68a6960de63908bb05c14a026abeda136119d3797431bdd7b469c1'
+                'f027e57a28aeec0df01a792e9e70aad2d6b', 17, 1800, 1800,
+                deploy=False),
+            call(
+                None, 'bar',
+                '2a3821585efcccff1562efea4514dd860cd536441954e182a7649910e21f6'
+                'a179a015677a68a351a11d3d2f277e551e4', 27, 1800, 1800,
+                deploy=False, resource_file='baz.txt'),
+            call(None, 'bar',
+                 '3164673a8ac27576ab5fc06b9adc4ce0aca5bd3025384b1cf2128a8795e7'
+                 '47c431e882785a0bf8dc70b42995db388575', 1024 * 1024, 1800,
+                 1800, deploy=False, resource_file='/tmp/fake')]
+        self.assertEqual(mock_p.mock_calls, calls)
+        mock_fdf.assert_called_once_with('/tmp/fake', 1024 * 1024)
+        mock_lt.assert_called_once_with(None, 1800, 1800)
+
+    def test_large_tests(self):
+        fake_file = FakeFile()
+        with patch("assess_resources.push_resource", autospec=True) as mock_pr:
+            with patch('assess_resources.NamedTemporaryFile',
+                       autospec=True) as mock_ntf:
+                mock_ntf.return_value.__enter__.return_value = fake_file
+                with patch('assess_resources.fill_dummy_file',
+                           autospec=True):
+                    large_assess(None, 1800, 1800)
+        calls = [
+            call(
+                None, 'bar',
+                'd7c014629d74ae132cc9f88e3ec2f31652f40a7a1fcc52c54b04d6c0d0891'
+                '69bcd55958d1277b4cdf6262f21c712d0a7', 1024 * 1024 * 10, 1800,
+                1800, deploy=False, resource_file='/tmp/fake'),
+            call(
+                None, 'bar',
+                'c11e93892b66de781e4d0883efe10482f8d1642f3b6574ba2ee0da6f8db03'
+                'f53c0eadfb5e5e0463574c113024ded369e', 1024 * 1024 * 100, 1800,
+                1800, deploy=False, resource_file='/tmp/fake'),
+            call(
+                None, 'bar',
+                '77db39eca74c6205e31a7701e488a1df4b9b38a527a6084bdbb6843fd430a'
+                '0b51047378ee0255e633b32c0dda3cf43ab', 1024 * 1024 * 200, 1800,
+                1800, deploy=False, resource_file='/tmp/fake')]
+        self.assertEqual(mock_pr.mock_calls, calls)
+
+
+def make_args():
+    return Namespace(
+        agent_stream=None, agent_timeout=1800, agent_url=None,
+        bootstrap_host=None, debug=False, env='an-env', juju_bin='/bin/juju',
+        keep_env=False, large_test_enabled=False, logs='/tmp/logs', machine=[],
+        region=None, resource_timeout=1800, series=None,
+        temp_env_name='an-env-mod', upload_tools=False, verbose=10)
+
+
+def make_resource_list():
+    return {'resources': [{
+        'expected': {
+            'origin': 'upload', 'used': True, 'description': 'foo resource.',
+            'username': 'admin@local', 'resourceid': 'dummy-resource/foo',
+            'name': 'foo', 'serviceid': 'dummy-resource', 'path': 'foo.txt',
+            'fingerprint': '1234', 'type': 'file', 'size': 27},
+        'unit': {
+            'origin': 'upload', 'username': 'admin@local', 'used': True,
+            'name': 'foo', 'resourceid': 'dummy-resource/foo',
+            'serviceid': 'dummy-resource', 'fingerprint': '1234',
+            'path': 'foo.txt', 'size': 27, 'type': 'file',
+            'description': 'foo resource.'}}]}
+
+
+class FakeFile:
+    name = '/tmp/fake'

=== modified file 'tests/test_jujupy.py'
--- tests/test_jujupy.py	2016-05-23 03:38:07 +0000
+++ tests/test_jujupy.py	2016-05-24 22:43:31 +0000
@@ -79,7 +79,9 @@
     TestCase,
     FakeHomeTestCase,
 )
+from tests.test_assess_resources import make_resource_list
 from utility import (
+    JujuResourceTimeout,
     scoped_environ,
     temp_dir,
 )
@@ -1609,6 +1611,33 @@
         mock_gjo.assert_called_with(
             'list-resources', '--format', 'yaml', 'foo', '--details')
 
+    def test_wait_for_resource(self):
+        client = EnvJujuClient(JujuData('local'), None, None)
+        with patch.object(
+                client, 'list_resources',
+                return_value=make_resource_list()) as mock_lr:
+            client.wait_for_resource('dummy-resource/foo', 'foo')
+        mock_lr.assert_called_once_with('foo')
+
+    def test_wait_for_resource_timeout(self):
+        client = EnvJujuClient(JujuData('local'), None, None)
+        resource_list = make_resource_list()
+        resource_list['resources'][0]['expected']['resourceid'] = 'bad_id'
+        with patch.object(
+                client, 'list_resources',
+                return_value=resource_list) as mock_lr:
+            with patch('jujupy.until_timeout', autospec=True,
+                       return_value=[0, 1]) as mock_ju:
+                with patch('time.sleep', autospec=True) as mock_ts:
+                    with self.assertRaisesRegexp(
+                            JujuResourceTimeout,
+                            'Timeout waiting for a resource to be downloaded'):
+                        client.wait_for_resource('dummy-resource/foo', 'foo')
+        calls = [call('foo'), call('foo')]
+        self.assertEqual(mock_lr.mock_calls, calls)
+        self.assertEqual(mock_ts.mock_calls, [call(.1), call(.1)])
+        self.assertEqual(mock_ju.mock_calls, [call(60)])
+
     def test_deploy_bundle_2x(self):
         client = EnvJujuClient(JujuData('an_env', None),
                                '1.23-series-arch', None)

=== modified file 'utility.py'
--- utility.py	2016-05-19 03:28:29 +0000
+++ utility.py	2016-05-24 22:43:31 +0000
@@ -97,6 +97,10 @@
     """Exception for juju assertion failures."""
 
 
+class JujuResourceTimeout(Exception):
+    """A timeout exception for a resource not being downloaded into a unit."""
+
+
 class enforce_juju_path(argparse.Action):
     """Enforces that a path ending with juju is given."""
     def __call__(self, parser, namespace, values, option_string=None):