Merge lp:~adam-collard/charms/trusty/landscape-client/landscape-client-multiple-juju-files into lp:charms/trusty/landscape-client

[Adam Collard]

Let the charm output several juju-info JSON files instead of a single file, since the landscape client charm can be deployed to several machines that are "hulk-smashed" together on the same physical machine.

This branch lets the charm output several juju-info JSON files instead of a single file, since the landscape client charm can be deployed to several machines that are "hulk-smashed" together on the same physical machine.

I forked it from lp:~tribaal/charms/trusty/landscape-client/landscape-client-multiple-juju-files because it's blocking my work on lp:1326261

For testing you can do something like this:

PYTHONPATH=~/src/landscape-client/13.07/ make test

[Adam Collard (adam-collard)]

Looks good! Inline comment below, but otherwise +1

review: Approve

[Adam Collard (adam-collard)]

Some typoes (inline)

[Adam Collard (adam-collard)]

temporary abstain until I get a chance to test it in anger

review: Abstain

[Adam Collard (adam-collard)]

As discussed in IRC, we need the charm to continue working with released landscape-client code (in precise/ and trusty/ not just in $series-updates).

review: Needs Fixing

[Adam Collard (adam-collard)]

review: Abstain

[Chris Glass (tribaal)]

FWIW Adam's changes look good - but I won't +1 here since it's a take over from my own branch

[Geoff Teale (tealeg)]

+1

Check the inline comments below.

review: Approve

[Adam Collard (adam-collard)]

Thanks for the review, I've opted not to address the concerns you raised. Replied inline :)

[Данило Шеган (danilo)]

Looks good, even the tests pass. Only one comment regarding "delete" as argument name.

review: Approve

[lp:~adam-collard/charms/trusty/landscape-client/landscape-client-multiple-juju-files]

46. By Adam Collard on 2014-06-13

Rename delete=False to try_to_register=True (danilo's review)

=== modified file 'hooks/common.py'
--- hooks/common.py	2013-11-19 12:39:44 +0000
+++ hooks/common.py	2014-06-13 15:44:07 +0000
@@ -1,6 +1,7 @@
 import os
 import sys
 import json
+from glob import glob
 from pwd import getpwnam
 from subprocess import check_output
 
@@ -52,13 +53,19 @@
     def __init__(self):
         self.config = Configuration()
 
-    def update_client_config(self, service_config):
+    def update_client_config(self, service_config, try_to_register=True):
         """Update the client config.
 
         @param service_config: A dict containing the config items to be
             updated. Dashes in the key values will be converted to
             underscore, to match the Landscape configuration.
 
+        @param try_to_register: Should we attempt to register after
+            updating the client configuration? (default True). No need
+            to show warnings if we are updating the configuration to
+            be deleted (as is the case when exiting a relation, for
+            example).
+
         An attempt to register the client will be made, if the client wasn't
         configured before.
         """
@@ -74,8 +81,12 @@
         is_configured_enough = all([
             self.config.get("account_name"),
             self.config.get("computer_title"),
-            os.path.exists(
-                os.path.join(self.config.data_path, "juju-info.json"))])
+            get_juju_info_filenames(self.juju_directory)])
+        if not try_to_register:
+            # We don't need to further check the configuration if we are
+            # deleting it.
+            return 0
+
         if is_configured_enough:
             return self.try_to_register()
         else:
@@ -83,6 +94,13 @@
                    " proceed, skipping")
             return 0
 
+    @property
+    def juju_directory(self):
+        try:
+            return self.config.juju_directory
+        except AttributeError:  # older landscape-client versions
+            return os.path.join(self.config.data_path, "juju-info.d")
+
     def clear_registration(self):
         """
         Do steps necessary to clear the client registration, so that next time
@@ -153,3 +171,7 @@
     """Dump content in JSON format to the specified file."""
     with open(filename, "w") as dumpfile:
         json.dump(content, dumpfile)
+
+
+def get_juju_info_filenames(juju_directory):
+    return glob("%s/*.json" % juju_directory)

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-05-30 05:59:47 +0000
+++ hooks/hooks.py	2014-06-13 15:44:07 +0000
@@ -14,7 +14,9 @@
 from shutil import rmtree
 from subprocess import CalledProcessError
 
-from common import write_json_file, JujuBroker, LandscapeBroker, chown
+from common import (
+    write_json_file, JujuBroker, LandscapeBroker, chown,
+    get_juju_info_filenames)
 from ceph import (
     get_ceph_client_path, write_ceph_client_keyring, write_ceph_client_config)
 
@@ -52,7 +54,7 @@
             data_path])
 
 
[e-mail]("registration-relation-joined","registration-relation-changed")
[e-mail]("registration-relation-joined", "registration-relation-changed")
 def registration_relation(juju_broker=JUJU_BROKER,
                           landscape_broker=LANDSCAPE_BROKER):
     data = juju_broker.get_relation_config()
@@ -95,15 +97,28 @@
                               landscape_broker=LANDSCAPE_BROKER):
     landscape_broker.config.reload()
     relation_conf = juju_broker.get_relation_config()
+    remote_unit_name = juju_broker.environment.get("JUJU_REMOTE_UNIT")
     # We use the remote unit for the unit name, since we want to associate this
     # client with the unit it's managing, not its own unit.
     juju_info = {
         "environment-uuid": juju_broker.environment.get("JUJU_ENV_UUID"),
-        "unit-name": juju_broker.environment.get("JUJU_REMOTE_UNIT"),
+        "unit-name": remote_unit_name,
         "api-addresses": juju_broker.environment.get("JUJU_API_ADDRESSES"),
         "private-address": relation_conf.get("private-address")}
-    juju_info_file = os.path.join(
-        landscape_broker.config.data_path, "juju-info.json")
+
+    # Let's not use any "/" in a filename.
+    unit_name = remote_unit_name.replace("/", "-")
+    juju_file = "%s.json" % unit_name
+
+    juju_info_dir = landscape_broker.juju_directory
+
+    try:
+        os.mkdir(juju_info_dir)
+    except OSError:
+        pass  # The file already exists.
+
+    juju_info_file = os.path.join(juju_info_dir, juju_file)
+
     write_json_file(juju_info_file, juju_info)
     exit_code = landscape_broker.update_client_config(
         {"computer-title": socket.gethostname()})
@@ -119,12 +134,14 @@
     computer title unset and the juju info file removed, so that we
     won't try to register again when the config-changed hook is fired.
     """
-    landscape_broker.clear_registration(),
-    juju_info_path = os.path.join(
-        landscape_broker.config.data_path, "juju-info.json")
-    if os.path.exists(juju_info_path):
-        os.remove(juju_info_path)
-    landscape_broker.update_client_config({"computer-title": ""})
+    landscape_broker.clear_registration()
+
+    # Delete all of the juju-info files.
+    files_to_delete = get_juju_info_filenames(landscape_broker.juju_directory)
+    for to_delete in files_to_delete:
+        os.remove(to_delete)
+    landscape_broker.update_client_config(
+        {"computer-title": ""}, try_to_register=False)
 
 
 @hooks.hook("ceph-client-relation-changed")

=== modified file 'hooks/test_hooks.py'
--- hooks/test_hooks.py	2014-05-30 05:59:47 +0000
+++ hooks/test_hooks.py	2014-06-13 15:44:07 +0000
@@ -5,6 +5,7 @@
 from tempfile import mkdtemp
 from unittest import TestCase
 from uuid import uuid4
+from glob import glob
 
 from landscape.deployment import Configuration
 from landscape.lib.fs import read_file
@@ -59,6 +60,13 @@
         self.landscape_broker.config.write()
         self.juju_broker = FakeJujuBroker()
 
+    def touch_example_juju_info_file(self):
+        if not os.path.exists(self.landscape_broker.juju_directory):
+            os.mkdir(self.landscape_broker.juju_directory)
+        filename = os.path.join(self.landscape_broker.juju_directory,
+                                "whatever.json")
+        write_json_file(filename=filename, content={})
+
     def mkdtemp(self):
         directory = mkdtemp(prefix=self.__class__.__name__)
         self.addCleanup(rmtree, directory)
@@ -165,8 +173,10 @@
             "private-address": "10.0.0.99"}
         hooks.container_relation_joined(juju_broker=self.juju_broker,
                                         landscape_broker=self.landscape_broker)
+
         juju_info_path = os.path.join(
-            self.landscape_broker.config.data_path, "juju-info.json")
+            self.landscape_broker.juju_directory, "unit-0.json")
+
         juju_info = json.loads(read_file(juju_info_path))
         self.assertEqual(
             {"environment-uuid": "uuid1",
@@ -184,9 +194,10 @@
         registration_attempts = []
 
         def record_registration_attempt():
-            juju_info_path = os.path.join(
-                self.landscape_broker.config.data_path, "juju-info.json")
-            registration_attempts.append(os.path.exists(juju_info_path))
+            juju_directory = self.landscape_broker.juju_directory
+            juju_file_list = glob("%s/*.json" % juju_directory)
+            for _ in juju_file_list:
+                registration_attempts.append(True)
 
         self.landscape_broker.try_to_register = record_registration_attempt
         hooks.container_relation_joined(juju_broker=self.juju_broker,
@@ -199,7 +210,8 @@
         The client adds the hostname as the "computer-title" field of the
         landscape broker config.
         """
-        self.hooks.run("container-relation-joined")
+        hooks.container_relation_joined(juju_broker=self.juju_broker,
+                                        landscape_broker=self.landscape_broker)
         result = self.landscape_broker.config.computer_title
         self.assertEqual(result, socket.gethostname())
 
@@ -234,7 +246,11 @@
         now gone).
         """
         juju_info_path = os.path.join(
-            self.landscape_broker.config.data_path, "juju-info.json")
+            self.landscape_broker.juju_directory, "unit-0.json")
+
+        if not os.path.exists(self.landscape_broker.juju_directory):
+            os.mkdir(self.landscape_broker.juju_directory)
+
         juju_info = {"environment-uuid": "uuid1",
                      "unit-name": "unit/0",
                      "api-addresses": "10.0.0.1:1234",
@@ -384,6 +400,9 @@
 
     def setUp(self):
         super(ConfigChangedTest, self).setUp()
+        self.landscape_broker.config.account_name = "account1"
+        self.landscape_broker.config.computer_title = "title"
+        self.touch_example_juju_info_file()
         self.addCleanup(setattr, hooks, "CERT_FILE", hooks.CERT_FILE)
         tmp_dir = self.mkdtemp()
         hooks.CERT_FILE = "%s/cert" % tmp_dir
@@ -410,6 +429,9 @@
 
     def setUp(self):
         super(RegistrationRelationJoinedTest, self).setUp()
+        self.landscape_broker.config.account_name = "account1"
+        self.landscape_broker.config.computer_title = "title"
+        self.touch_example_juju_info_file()
         self.addCleanup(setattr, hooks, "CERT_FILE", hooks.CERT_FILE)
         tmp_dir = self.mkdtemp()
         hooks.CERT_FILE = "%s/cert" % tmp_dir