cf-charmers team mailing list archive
-
cf-charmers team
-
Mailing list archive
-
Message #00577
Re: [Merge] lp:~johnsca/charms/trusty/cloudfoundry/trunk-webadmin into lp:~cf-charmers/charms/trusty/cloudfoundry/trunk
On Fri, Oct 31, 2014 at 12:48 PM, Benjamin Saller <
benjamin.saller@xxxxxxxxxxxxx> wrote:
> 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.
>
if its local data then sounds like sqlite might be just as useful.
>
> > + 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.
>
> --
> Mailing list: https://launchpad.net/~cf-charmers
> Post to : cf-charmers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~cf-charmers
> More help : https://help.launchpad.net/ListHelp
>
--
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.
References
