cf-charmers team mailing list archive

[Benjamin Saller <benjamin.saller@xxxxxxxxxxxxx>]

Review: Approve

Thanks for the additional changes. LGTM

There are some inline rambles but nothing that has to be changed now.

Diff comments:

> === modified file 'charmhelpers/core/services/base.py'
> --- charmhelpers/core/services/base.py	2014-09-18 19:34:19 +0000
> +++ charmhelpers/core/services/base.py	2014-10-31 16:12:00 +0000
> @@ -1,6 +1,7 @@
>  import os
> -import re
>  import json
> +import hashlib
> +import inspect
>  from collections import Iterable
>  
>  from charmhelpers.core import host
> @@ -105,6 +106,7 @@
>              manager.manage()
>          """
>          self._ready_file = os.path.join(hookenv.charm_dir(), 'READY-SERVICES.json')
> +        self._data_hash_file = os.path.join(hookenv.charm_dir(), 'DATA-HASHES.json')
>          self._ready = None
>          self.services = {}
>          for service in services or []:
> @@ -131,13 +133,16 @@
>              raise
>  
>      def provide_data(self):
> -        hook_name = hookenv.hook_name()
> -        for service in self.services.values():
> +        for service_name, service in self.services.items():
>              for provider in service.get('provided_data', []):
> -                if re.match(r'{}-relation-(joined|changed)'.format(provider.name), hook_name):
> -                    data = provider.provide_data()
> +                for rel_id in hookenv.relation_ids(provider.name):
> +                    pargs = inspect.getargspec(provider.provide_data).args

We do need to document this change. A card for that would be nice.

> +                    if len(pargs) > 1:
> +                        data = provider.provide_data(self, service_name, rel_id)
> +                    else:
> +                        data = provider.provide_data()
>                      if provider._is_ready(data):
> -                        hookenv.relation_set(None, data)
> +                        hookenv.relation_set(rel_id, data)
>  
>      def reconfigure_services(self, *service_names):
>          """
> @@ -148,10 +153,17 @@
>          """
>          for service_name in service_names or self.services.keys():
>              if self.is_ready(service_name):
> -                self.fire_event('data_ready', service_name)
> -                self.fire_event('start', service_name, default=[
> -                    service_restart,
> -                    manage_ports])
> +                was_ready = self.was_ready(service_name)
> +                data_changed = self.data_changed(service_name)
> +                if data_changed or not was_ready:
> +                    hookenv.log('Restarting service: %s' % service_name)
> +                    # only do work (restart) if the data says we're in a new state
> +                    self.fire_event('data_ready', service_name)
> +                    self.fire_event('start', service_name, default=[
> +                        service_restart,
> +                        manage_ports])
> +                else:
> +                    hookenv.log('Skipping service restart: %s' % service_name)
>                  self.save_ready(service_name)
>              else:
>                  if self.was_ready(service_name):
> @@ -246,6 +258,30 @@
>          self._load_ready_file()
>          return service_name in self._ready
>  
> +    def merged_data(self, service_name):
> +        service = self.get_service(service_name)
> +        context = {}
> +        for ctx in service.get('required_data', []):
> +            context.update(ctx)
> +        return context
> +
> +    def _get_data_hashes(self, service_name):
> +        data = self.merged_data(service_name)
> +        hashes = {}
> +        if os.path.exists(self._data_hash_file):
> +            with open(self._data_hash_file) as fp:
> +                hashes = json.load(fp)
> +        old_hash = hashes.get(service_name)
> +        new_hash = hashlib.md5(json.dumps(data, sort_keys=True)).hexdigest()
> +        hashes[service_name] = new_hash
> +        with open(self._data_hash_file, 'w') as fp:
> +            json.dump(hashes, fp)

we do leave a lot of these files around now. In the end it might make sense to revisit when/how we dump caches to the fs. even if its a centralized directory and naming convention, all the way up to an embedded db/kv store to using an etcd for scratch space.

> +        return old_hash, new_hash
> +
> +    def data_changed(self, service_name):
> +        old_hash, new_hash = self._get_data_hashes(service_name)
> +        return old_hash != new_hash
> +
>      def update_status_working(self):
>          if hookenv.juju_status()['status'] == 'blocked':
>              hookenv.juju_status('churning')
> 
> === modified file 'charmhelpers/core/services/helpers.py'
> --- charmhelpers/core/services/helpers.py	2014-08-21 17:34:34 +0000
> +++ charmhelpers/core/services/helpers.py	2014-10-31 16:12:00 +0000
> @@ -113,10 +113,7 @@
>          self.perms = perms
>  
>      def __call__(self, manager, service_name, event_name):
> -        service = manager.get_service(service_name)
> -        context = {}
> -        for ctx in service.get('required_data', []):
> -            context.update(ctx)
> +        context = manager.merged_data(service_name)
>          templating.render(self.source, self.target, context,
>                            self.owner, self.group, self.perms)
>  
> 
> === modified file 'cloudfoundry/contexts.py'
> --- cloudfoundry/contexts.py	2014-10-27 19:48:56 +0000
> +++ cloudfoundry/contexts.py	2014-10-31 16:12:00 +0000
> @@ -31,6 +31,13 @@
>                                                 rid=rid, unit=unit))
>          return list(units)
>  
> +    def get_first(self, key=None):
> +        data = self[self.name][0]
> +        return data[key] if key is not None else data
> +
> +    def get_all(self, key):
> +        return [u[key] for u in self[self.name]]
> +
>  
>  class StoredContext(dict):
>      """
> @@ -100,7 +107,7 @@
>      def get_data(self):
>          RelationContext.get_data(self)
>          if self.is_ready():
> -            for unit in self['db']:
> +            for unit in self[self.name]:
>                  if 'port' not in unit:
>                      unit['port'] = '3306'
>                  unit['dsn'] = self.dsn_template.format(**unit)
> @@ -114,7 +121,7 @@
>  
>  class UAARelation(RelationContext):
>      name = 'uaa'
> -    interface = 'http'
> +    interface = 'uaa'
>      required_keys = ['login_client_secret', 'admin_client_secret', 'cc_client_secret', 'cc_token_secret',
>                       'service_broker_client_secret', 'servicesmgmt_client_secret', 'port']
>      port = 8081
> @@ -168,6 +175,18 @@
>          }
>  
>  
> +class UAADBRelation(MysqlRelation):
> +    name = 'uaadb'
> +
> +    def provide_data(self, manager, service_name, rel_id):
> +        data = {}
> +        if manager.is_ready(service_name):
> +            # proxy data from the MysqlRelation to this relation

Thanks, the comment helps here.

> +            mysql = MysqlRelation()
> +            data = mysql.get_first()
> +        return data
> +
> +
>  class LoginRelation(RelationContext):
>      name = 'login'
>      interface = 'http'
> @@ -362,17 +381,16 @@
>          }
>  
>  
> -class CloudControllerDBRelation(RelationContext):
> -    name = 'cc-db'
> -    interface = 'controller-db'
> -    required_keys = MysqlRelation.required_keys
> +class CloudControllerDBRelation(MysqlRelation):
> +    name = 'ccdb'
>  
> -    @classmethod
> -    def send_data(cls, job_name):
> -        # using send_data instead of provide_data to delay it until data_ready
> -        data = MysqlRelation()['db'][0]
> -        for rid in hookenv.relation_ids(cls.name):
> -            hookenv.relation_set(rid, data)
> +    def provide_data(self, manager, service_name, rel_id):
> +        data = {}
> +        if manager.is_ready(service_name):
> +            # proxy data from the MysqlRelation to this relation
> +            mysql = MysqlRelation()
> +            data = mysql.get_first()
> +        return data
>  
>  
>  class RouterRelation(RelationContext):
> 
> === modified file 'cloudfoundry/mapper.py'
> --- cloudfoundry/mapper.py	2014-07-31 22:05:01 +0000
> +++ cloudfoundry/mapper.py	2014-10-31 16:12:00 +0000
> @@ -61,7 +61,7 @@
>  
>      #@@ This may need to be adjusted to scale / HA the databases.
>      """
> -    db = data.get('db', data.get('cc-db'))[0]
> +    db = data.get('db', data.get('ccdb'))[0]
>  
>      job_db = dict(tag='cc',
>                    name=db['database'])
> 
> === modified file 'cloudfoundry/services.py'
> --- cloudfoundry/services.py	2014-09-25 15:23:46 +0000
> +++ cloudfoundry/services.py	2014-10-31 16:12:00 +0000
> @@ -12,7 +12,7 @@
>          'description': '',
>          'jobs': [{
>              'job_name': 'cloud_controller_clock',
> -            'mapping': {'cc-db': mapper.ccdb},
> +            'mapping': {'ccdb': mapper.ccdb},
>              'provided_data': [],
>              'required_data': [contexts.NatsRelation,
>                                contexts.LTCRelation,
> @@ -30,7 +30,7 @@
>          'description': '',
>          'jobs': [
>              {'job_name': 'cloud_controller_clock',
> -             'mapping': {'cc-db': mapper.ccdb},
> +             'mapping': {'ccdb': mapper.ccdb},
>               'provided_data': [],
>               'required_data': [contexts.NatsRelation,
>                                 contexts.LTCRelation,
> @@ -62,9 +62,6 @@
>                                contexts.UAARelation,
>                                contexts.CloudControllerRelation.remote_view,
>                                ],
> -            'data_ready': [
> -                contexts.CloudControllerDBRelation.send_data,
> -            ],
>          }]
>      },
>  
> @@ -82,9 +79,7 @@
>                                 contexts.UAARelation,
>                                 contexts.CloudControllerRelation.remote_view,
>                                 ],
> -             'data_ready': [
> -                 contexts.CloudControllerDBRelation.send_data,
> -             ]},
> +             },
>              {'job_name': 'metron_agent',
>               'required_data': [contexts.LTCRelation,
>                                 contexts.NatsRelation,
> @@ -98,7 +93,7 @@
>          'description': '',
>          'jobs': [
>              {'job_name': 'cloud_controller_worker',
> -             'mapping': {'cc-db': mapper.ccdb},
> +             'mapping': {'ccdb': mapper.ccdb},
>               'provided_data': [],
>               'required_data': [contexts.NatsRelation,
>                                 contexts.LTCRelation,
> @@ -116,7 +111,7 @@
>          'description': '',
>          'jobs': [
>              {'job_name': 'cloud_controller_worker',
> -             'mapping': {'cc-db': mapper.ccdb},
> +             'mapping': {'ccdb': mapper.ccdb},
>               'provided_data': [],
>               'required_data': [contexts.NatsRelation,
>                                 contexts.LTCRelation,
> @@ -271,11 +266,11 @@
>          'jobs': [
>              {'job_name': 'uaa',
>               'mapping': {'db': mapper.uaadb},
> -             'provided_data': [contexts.UAARelation],
> +             'provided_data': [contexts.UAARelation,
> +                               contexts.UAADBRelation],
>               'required_data': [contexts.MysqlRelation,
>                                 contexts.NatsRelation,
> -                               contexts.UAARelation.remote_view]
> -             }
> +                               contexts.UAARelation.remote_view]},
>          ]
>      },
>  
> 
> === modified file 'cloudfoundry/utils.py'
> --- cloudfoundry/utils.py	2014-10-27 19:48:56 +0000
> +++ cloudfoundry/utils.py	2014-10-31 16:12:00 +0000
> @@ -10,7 +10,6 @@
>  from contextlib import contextmanager
>  from functools import partial, wraps
>  
> -import jujuclient
>  import requests
>  import yaml
>  
> @@ -232,6 +231,7 @@
>  
>  
>  def get_client():
> +    import jujuclient
>      global _client
>      if _client is None:
>          def env_connection():
> 
> === modified file 'tests/test_jobs.py'
> --- tests/test_jobs.py	2014-09-29 22:12:51 +0000
> +++ tests/test_jobs.py	2014-10-31 16:12:00 +0000
> @@ -73,8 +73,6 @@
>          self.assertIsInstance(services[0]['data_ready'][4],
>                                tasks.JobTemplates)
>          services = jobs.build_service_block('cloud-controller-v1')
> -        # Show that we include both default and additional handlers
> +        # Show that we include default handlers
>          self.assertIsInstance(services[0]['data_ready'][4],
>                                tasks.JobTemplates)
> -        self.assertEqual(services[0]['data_ready'][-1],
> -                         contexts.CloudControllerDBRelation.send_data)
> 


-- 
https://code.launchpad.net/~johnsca/charms/trusty/cloudfoundry/trunk-webadmin/+merge/240033
Your team Cloud Foundry Charmers is subscribed to branch lp:~cf-charmers/charms/trusty/cloudfoundry/trunk.