Merge lp:~johnsca/charms/trusty/cf-go-router/port-conflicts into lp:~cf-charmers/charms/trusty/cf-go-router/trunk

[Cory Johns]

Resolve port conflicts in cf-go-router

https://codereview.appspot.com/103270043/

[Cory Johns (johnsca)]

Reviewers: mp+222671_code.launchpad.net,

Message:
Please take a look.

Description:
Resolve port conflicts in cf-go-router

https://code.launchpad.net/~johnsca/charms/trusty/cf-go-router/port-conflicts/+merge/222671

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/103270043/

Affected files (+134, -75 lines):
   A [revision details]
   M config.yaml
   M hooks/charmhelpers/contrib/cloudfoundry/contexts.py
   M hooks/charmhelpers/core/services.py
   M hooks/config.py
   A hooks/logrouter-relation-changed
   M hooks/tests/test_hooks.py
   M metadata.yaml
   M templates/gorouter.yml

[lp:~johnsca/charms/trusty/cf-go-router/port-conflicts]

38. By Cory Johns on 2014-06-11

Removed varz_port option as it doesn't add much value to change

[Cory Johns (johnsca)]

Please take a look.

https://codereview.appspot.com/103270043/

[Benjamin Saller (bcsaller)]

Thanks, LGTM with changes

At min we should use the config['port'] the action for the rest might be
a card unless you disagree with the approach.

https://codereview.appspot.com/103270043/diff/20001/hooks/config.py
File hooks/config.py (right):

https://codereview.appspot.com/103270043/diff/20001/hooks/config.py#newcode39
hooks/config.py:39: 'ports': [80],
if this is a config option (and it can be because we expose this one)
then this can't be hard-coded here.

config['port']

The harder issue is that we then need to track the previous value if we
are to close the old port.

We (and it doesn't have to be now) could use a config context that
called callbacks here as callback(new_value, old_value=None) so we have
a chance to do something with the old saved context. seem reasonable?

https://codereview.appspot.com/103270043/

[lp:~johnsca/charms/trusty/cf-go-router/port-conflicts]

39. By Cory Johns on 2014-06-12

Removed hard-coded port and updated charm-helpers for port changing awareness logic

[Cory Johns (johnsca)]

Please take a look.

https://codereview.appspot.com/103270043/

[Cory Johns (johnsca)]

On 2014/06/11 22:23:56, benjamin.saller wrote:
> hooks/config.py:39: 'ports': [80],
> if this is a config option (and it can be because we expose this one)
then this
> can't be hard-coded here.

> The harder issue is that we then need to track the previous value if
we are to
> close the old port.

Thanks, good catches. I added the logic to close the old port when
switching to charm-helpers because it should always happen, and I feel
like it should be transparent to the charm author.

https://codereview.appspot.com/103270043/

[Benjamin Saller (bcsaller)]

LGTM

Nice fix for the port handling

https://codereview.appspot.com/103270043/diff/40001/hooks/charmhelpers/core/services.py
File hooks/charmhelpers/core/services.py (right):

https://codereview.appspot.com/103270043/diff/40001/hooks/charmhelpers/core/services.py#newcode348
hooks/charmhelpers/core/services.py:348: for port in new_ports:
Cool, thanks

https://codereview.appspot.com/103270043/diff/40001/hooks/config.py
File hooks/config.py (right):

https://codereview.appspot.com/103270043/diff/40001/hooks/config.py#newcode39
hooks/config.py:39: 'ports': [hookenv.config('port')],
funny that we use the hookenv.config()['domain'] above and this here. I
didn't recall that it even did per-key lookup :)

https://codereview.appspot.com/103270043/

[Cory Johns (johnsca)]

*** Submitted:

Resolve port conflicts in cf-go-router

R=benjamin.saller
CC=
https://codereview.appspot.com/103270043

https://codereview.appspot.com/103270043/

=== modified file 'config.yaml'
--- config.yaml	2014-05-27 21:50:52 +0000
+++ config.yaml	2014-06-12 14:01:13 +0000
@@ -25,4 +25,7 @@
     description: |
       Key ID to import to the apt keyring to support use with arbitary source
       configuration from outside of Launchpad archives or PPA's.
-
+  port:
+    type: int
+    default: 80
+    description: Port for the router to listen on.

=== modified file 'hooks/charmhelpers/contrib/cloudfoundry/contexts.py'
--- hooks/charmhelpers/contrib/cloudfoundry/contexts.py	2014-05-27 21:50:52 +0000
+++ hooks/charmhelpers/contrib/cloudfoundry/contexts.py	2014-06-12 14:01:13 +0000
@@ -33,7 +33,7 @@
 
 class NatsRelation(RelationContext):
     interface = 'nats'
-    required_keys = ['nats_port', 'nats_address', 'nats_user', 'nats_password']
+    required_keys = ['address', 'port', 'user', 'password']
 
 
 class MysqlRelation(RelationContext):
@@ -44,9 +44,10 @@
     def get_data(self):
         RelationContext.get_data(self)
         if self.is_ready():
-            if 'port' not in self['db']:
-                self['db']['port'] = '3306'
-            self['db']['dsn'] = self.dsn_template.format(**self['db'])
+            for unit in self['db']:
+                if 'port' not in unit:
+                    unit['port'] = '3306'
+                unit['dsn'] = self.dsn_template.format(**unit)
 
 
 class RouterRelation(RelationContext):
@@ -56,9 +57,19 @@
 
 class LogRouterRelation(RelationContext):
     interface = 'logrouter'
-    required_keys = ['shared-secret', 'logrouter-address']
+    required_keys = ['shared_secret', 'address', 'incoming_port', 'outgoing_port']
 
 
 class LoggregatorRelation(RelationContext):
     interface = 'loggregator'
-    required_keys = ['shared_secret', 'loggregator_address']
+    required_keys = ['address', 'incoming_port', 'outgoing_port']
+
+
+class EtcdRelation(RelationContext):
+    interface = 'etcd'
+    required_keys = ['hostname', 'port']
+
+
+class CloudControllerRelation(RelationContext):
+    interface = 'cc'
+    required_keys = ['hostname', 'port', 'user', 'password']

=== modified file 'hooks/charmhelpers/core/services.py'
--- hooks/charmhelpers/core/services.py	2014-05-29 17:27:32 +0000
+++ hooks/charmhelpers/core/services.py	2014-06-12 14:01:13 +0000
@@ -11,6 +11,14 @@
         """
         Register a list of services, given their definitions.
 
+        Traditional charm authoring is focused on implementing hooks.  That is,
+        the charm author is thinking in terms of "What hook am I handling; what
+        does this hook need to do?"  However, in most cases, the real question
+        should be "Do I have the information I need to configure and start this
+        piece of software and, if so, what are the steps for doing so."  The
+        ServiceManager framework tries to bring the focus to the data and the
+        setup tasks, in the most declarative way possible.
+
         Service definitions are dicts in the following formats (all keys except
         'service' are optional):
 
@@ -67,28 +75,28 @@
         a mongodb relation and which runs a custom `db_migrate` function prior to
         restarting the service, and a Runit serivce called spadesd.
 
-            >>> manager = services.ServiceManager([
-            ...     {
-            ...         'service': 'bingod',
-            ...         'ports': [80, 443],
-            ...         'required_data': [MongoRelation(), config()],
-            ...         'data_ready': [
-            ...             services.template(source='bingod.conf'),
-            ...             services.template(source='bingod.ini',
-            ...                               target='/etc/bingod.ini',
-            ...                               owner='bingo', perms=0400),
-            ...         ],
-            ...     },
-            ...     {
-            ...         'service': 'spadesd',
-            ...         'data_ready': services.template(source='spadesd_run.j2',
-            ...                                         target='/etc/sv/spadesd/run',
-            ...                                         perms=0555),
-            ...         'start': runit_start,
-            ...         'stop': runit_stop,
-            ...     },
-            ... ])
-            ... manager.manage()
+            manager = services.ServiceManager([
+                {
+                    'service': 'bingod',
+                    'ports': [80, 443],
+                    'required_data': [MongoRelation(), config(), {'my': 'data'}],
+                    'data_ready': [
+                        services.template(source='bingod.conf'),
+                        services.template(source='bingod.ini',
+                                          target='/etc/bingod.ini',
+                                          owner='bingo', perms=0400),
+                    ],
+                },
+                {
+                    'service': 'spadesd',
+                    'data_ready': services.template(source='spadesd_run.j2',
+                                                    target='/etc/sv/spadesd/run',
+                                                    perms=0555),
+                    'start': runit_start,
+                    'stop': runit_stop,
+                },
+            ])
+            manager.manage()
         """
         self.services = {}
         for service in services or []:
@@ -213,55 +221,73 @@
     interface = None
     required_keys = []
 
+    def __init__(self, *args, **kwargs):
+        super(RelationContext, self).__init__(*args, **kwargs)
+        self.get_data()
+
     def __bool__(self):
         """
-        Updates the data and returns True if all of the required_keys are available.
+        Returns True if all of the required_keys are available.
         """
-        self.get_data()
         return self.is_ready()
 
     __nonzero__ = __bool__
 
+    def __repr__(self):
+        return super(RelationContext, self).__repr__()
+
     def is_ready(self):
         """
-        Returns True if all of the required_keys are available.
-        """
-        return set(self.get(self.interface, {}).keys()).issuperset(set(self.required_keys))
+        Returns True if all of the `required_keys` are available from any units.
+        """
+        ready = len(self.get(self.interface, [])) > 0
+        if not ready:
+            hookenv.log('Incomplete relation: {}'.format(self.__class__.__name__), hookenv.DEBUG)
+        return ready
+
+    def _is_ready(self, unit_data):
+        """
+        Helper method that tests a set of relation data and returns True if
+        all of the `required_keys` are present.
+        """
+        return set(unit_data.keys()).issuperset(set(self.required_keys))
 
     def get_data(self):
         """
-        Retrieve the relation data and store it under `self[self.interface]`.
-
-        If there are more than one units related on the desired interface,
-        then each unit will have its data stored under `self[self.interface][unit_id]`
-        and one of the units with complete information will chosen at random
-        to fill the values at `self[self.interface]`.
-
-
-        For example:
-
-            {
-                'foo': 'bar',
-                'unit/0': {
-                    'foo': 'bar',
-                },
-                'unit/1': {
-                    'foo': 'baz',
-                },
-            }
+        Retrieve the relation data for each unit involved in a realtion and,
+        if complete, store it in a list under `self[self.interface]`.  This
+        is automatically called when the RelationContext is instantiated.
+
+        The units are sorted lexographically first by the service ID, then by
+        the unit ID.  Thus, if an interface has two other services, 'db:1'
+        and 'db:2', with 'db:1' having two units, 'wordpress/0' and 'wordpress/1',
+        and 'db:2' having one unit, 'mediawiki/0', all of which have a complete
+        set of data, the relation data for the units will be stored in the
+        order: 'wordpress/0', 'wordpress/1', 'mediawiki/0'.
+
+        If you only care about a single unit on the relation, you can just
+        access it as `{{ interface[0]['key'] }}`.  However, if you can at all
+        support multiple units on a relation, you should iterate over the list,
+        like:
+
+            {% for unit in interface -%}
+                {{ unit['key'] }}{% if not loop.last %},{% endif %}
+            {%- endfor %}
+
+        Note that since all sets of relation data from all related services and
+        units are in a single list, if you need to know which service or unit a
+        set of data came from, you'll need to extend this class to preserve
+        that information.
         """
         if not hookenv.relation_ids(self.interface):
             return
 
-        ns = self.setdefault(self.interface, {})
-        required = set(self.required_keys)
-        for rid in hookenv.relation_ids(self.interface):
-            for unit in hookenv.related_units(rid):
+        ns = self.setdefault(self.interface, [])
+        for rid in sorted(hookenv.relation_ids(self.interface)):
+            for unit in sorted(hookenv.related_units(rid)):
                 reldata = hookenv.relation_get(rid=rid, unit=unit)
-                unit_ns = ns.setdefault(unit, {})
-                unit_ns.update(reldata)
-                if set(reldata.keys()).issuperset(required):
-                    ns.update(reldata)
+                if self._is_ready(reldata):
+                    ns.append(reldata)
 
 
 class ManagerCallback(object):
@@ -308,7 +334,18 @@
     """
     def __call__(self, manager, service_name, event_name):
         service = manager.get_service(service_name)
-        for port in service.get('ports', []):
+        new_ports = service.get('ports', [])
+        port_file = os.path.join(hookenv.charm_dir(), '.{}.ports'.format(service_name))
+        if os.path.exists(port_file):
+            with open(port_file) as fp:
+                old_ports = fp.read().split(',')
+            for old_port in old_ports:
+                old_port = int(old_port)
+                if old_port not in new_ports:
+                    hookenv.close_port(old_port)
+        with open(port_file, 'w') as fp:
+            fp.write(','.join(str(port) for port in new_ports))
+        for port in new_ports:
             if event_name == 'start':
                 hookenv.open_port(port)
             elif event_name == 'stop':
@@ -316,6 +353,6 @@
 
 
 # Convenience aliases
-template = TemplateCallback
+render_template = template = TemplateCallback
 open_ports = PortManagerCallback()
 close_ports = PortManagerCallback()

=== modified file 'hooks/config.py'
--- hooks/config.py	2014-06-03 03:50:30 +0000
+++ hooks/config.py	2014-06-12 14:01:13 +0000
@@ -36,10 +36,11 @@
 SERVICES = [
     {
         'service': 'gorouter',
-        'ports': [80, 443],
+        'ports': [hookenv.config('port')],
         'required_data': [
-            {'domain': domain()},
+            {'domain': domain(), 'config': hookenv.config(), 'varz_port': 8081},
             contexts.NatsRelation(),
+            contexts.LogRouterRelation(),
         ],
         'data_ready': [
             services.template(source='gorouter.conf',

=== added file 'hooks/logrouter-relation-changed'
--- hooks/logrouter-relation-changed	1970-01-01 00:00:00 +0000
+++ hooks/logrouter-relation-changed	2014-06-12 14:01:13 +0000
@@ -0,0 +1,6 @@
+#!/usr/bin/env python
+from charmhelpers.core import services
+import config
+
+manager = services.ServiceManager(config.SERVICES)
+manager.manage()

=== modified file 'hooks/tests/test_hooks.py'
--- hooks/tests/test_hooks.py	2014-06-03 03:50:30 +0000
+++ hooks/tests/test_hooks.py	2014-06-12 14:01:13 +0000
@@ -14,10 +14,10 @@
     @mock.patch('hooks.hooks.host.write_file')
     def test_emit_router_config(self, write_file):
         emit_router_config({
-            'nats_address': 'addy',
-            'nats_port': 123,
-            'nats_password': 'password',
-            'nats_user': 'user'})
+            'address': 'addy',
+            'port': 123,
+            'password': 'password',
+            'user': 'user'})
 
         args, kw = write_file.call_args_list[0]
         self.assertEqual(

=== modified file 'metadata.yaml'
--- metadata.yaml	2014-05-12 07:19:21 +0000
+++ metadata.yaml	2014-06-12 14:01:13 +0000
@@ -13,3 +13,5 @@
 requires:
   nats:
     interface: nats
+  logrouter:
+    interface: logrouter

=== modified file 'templates/gorouter.yml'
--- templates/gorouter.yml	2014-04-17 15:12:52 +0000
+++ templates/gorouter.yml	2014-06-12 14:01:13 +0000
@@ -1,11 +1,11 @@
 nats:
-  - host: "{{ nats['nats_address'] }}"
-    port: {{ nats['nats_port'] }}
-    user: "{{ nats['nats_user'] }}"
-    pass: "{{ nats['nats_password'] }}"
+  - host: "{{ nats[0]['address'] }}"
+    port: {{ nats[0]['port'] }}
+    user: "{{ nats[0]['user'] }}"
+    pass: "{{ nats[0]['password'] }}"
 
 status:
-  port: 8081
+  port: {{ varz_port }}
   user: user
   pass: password
 
@@ -13,7 +13,11 @@
   file: /var/vcap/sys/log/gorouter/gorouter.log
   level: info
 
-port: 80
+loggregatorConfig:
+  url: {{ logrouter[0]['address'] }}:{{ logrouter[0]['incoming_port'] }}
+  shared_secret: {{ logrouter[0]['shared_secret'] }}
+
+port: {{ config['port'] }}
 index: 1
 pidfile: /var/vcap/sys/run/gorouter/gorouter.pid
 go_max_procs: 8
@@ -23,6 +27,6 @@
 publish_start_message_interval: 30
 prune_stale_droplets_interval: 30
 droplet_stale_threshold: 120
-publish_active_apps_interval: 0 
+publish_active_apps_interval: 0
 
 endpoint_timeout: 60