Merge lp:~free.ekanayaka/landscape-client/juju-machine-registration into lp:~landscape/landscape-client/trunk

[Free Ekanayaka]

This branch:

- Bumps the server API to 3.3: this will let us introduce a new schema
  for the juju-info registration field, since we have released clients
  out there that could potentially send the old juju-info schema (although
  they'd need to be deployed with an older version of the client charm,
  so it's more a theoretical situation than one that can happen in practice).
  A server branch has been pushed to bump the server to API 3.3. as well.

- Include the machine ID information in the registration message if the server
  is capable of handling it.

This branch:

- Bumps the server API to 3.3: this will let us introduce a new schema for the juju-info registration field, since we have released clients out there that could potentially send the old juju-info schema (although they'd need to be deployed with an older version of the client charm, so it's more a theoretical situation than one that can happen in practice). A server branch has been pushed to bump the server to API 3.3. as well. See:

https://code.launchpad.net/~free.ekanayaka/landscape/handle-machine-id-registration/+merge/232535

- Include the machine ID information in the registration message if the server is capable of handling it.

Just to be clear, after the server-side branch lands too what happen will be that new clients will send a 3.3 registration message with the new juju info schema, which the server knows how to handle. In case old clients send the old juju-info field, the server will just ignore it.

[Alberto Donato (ack)]

Looks good, +1

A few comments inline.

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Thanks Albert, all fixed.

[lp:~free.ekanayaka/landscape-client/juju-machine-registration]

788. By Free Ekanayaka on 2014-08-29

Address review comments

[Adam Collard (adam-collard)]

Lint issue:

landscape/monitor/tests/test_activeprocessinfo.py:13: 'SERVER_API' imported but unused

[Adam Collard (adam-collard)]

Needs Fixing because there are several changes needed and I'd like to give it another pass.

7 inline comments for your perusal below!

review: Needs Fixing

[Free Ekanayaka (free.ekanayaka)]

Thanks Adam, should be all fixed/replied.

[lp:~free.ekanayaka/landscape-client/juju-machine-registration]

789. By Free Ekanayaka on 2014-09-01

Address review comments

[Adam Collard (adam-collard)]

LGTM! +1

review: Approve

=== modified file 'landscape/__init__.py'
--- landscape/__init__.py	2014-07-16 17:15:13 +0000
+++ landscape/__init__.py	2014-09-01 13:16:19 +0000
@@ -13,10 +13,12 @@
 #
 # Changelog:
 #
+# 3.3:
+#  * Add new schema for the "registration" message, providing Juju information
 # 3.2:
 #  * Add new "eucalyptus-info" and "eucalyptus-info-error" messages.
 #
-SERVER_API = "3.2"
+SERVER_API = "3.3"
 
 # XXX This is needed for backward compatibility in the server code importing
 # the API variable. We should eventually replace it in the server code.

=== modified file 'landscape/broker/registration.py'
--- landscape/broker/registration.py	2014-08-22 14:00:17 +0000
+++ landscape/broker/registration.py	2014-09-01 13:16:19 +0000
@@ -16,6 +16,7 @@
 from landscape.lib.tag import is_valid_tag_list
 from landscape.lib.network import get_fqdn
 from landscape.lib.vm_info import get_vm_info, get_container_info
+from landscape.lib.versioning import is_version_higher
 
 
 class InvalidCredentialsError(Exception):
@@ -126,10 +127,10 @@
 
     def _get_juju_data(self):
         """Load Juju information."""
-        juju_info = get_juju_info(self._config)[0]
+        juju_info = get_juju_info(self._config)
         if juju_info is None:
             return None
-        self._juju_data = juju_info  # A list of dicts
+        self._juju_data = juju_info  # (list of dicts, new juju info struct)
 
     def _handle_exchange_done(self):
         """Registered handler for the C{"exchange-done"} event.
@@ -188,11 +189,19 @@
         if group:
             message["access_group"] = group
 
-        if self._juju_data is not None:
-            # For backwards compatibility, set the juju-info to be one of
-            # the juju infos (it used not to be a list).
-            message["juju-info"] = self._juju_data[0]
-            message["juju-info-list"] = self._juju_data
+        server_api = self._message_store.get_server_api()
+        # If we have juju data to send and if the server is recent enough to
+        # know how to handle juju data, then we include it in the registration
+        # message. We want to trigger the 3.3 server handler because client
+        # version 14.01 has a different format for the juju-info field,
+        # so this makes sure that the correct schema is used by the server
+        # when validating our message.
+        if self._juju_data and is_version_higher(server_api, "3.3"):
+            juju_info_list, juju_info = self._juju_data
+            message["juju-info"] = juju_info
+            # XXX the juju-info-list field will be dropped once the migration
+            # to computer-machine association is complete
+            message["juju-info-list"] = juju_info_list
 
         # The computer is a normal computer, possibly a container.
         with_word = "with" if bool(registration_key) else "without"

=== modified file 'landscape/broker/tests/test_exchange.py'
--- landscape/broker/tests/test_exchange.py	2014-08-22 13:53:51 +0000
+++ landscape/broker/tests/test_exchange.py	2014-09-01 13:16:19 +0000
@@ -1,4 +1,4 @@
-from landscape import SERVER_API, CLIENT_API
+from landscape import CLIENT_API
 from landscape.lib.persist import Persist
 from landscape.lib.hashlib import md5
 from landscape.schema import Message, Int
@@ -111,7 +111,7 @@
         messages = self.transport.payloads[0]["messages"]
         self.assertEqual(messages, [{"type": "empty",
                                      "timestamp": 0,
-                                     "api": SERVER_API}])
+                                     "api": "3.2"}])
 
     def test_send_urgent(self):
         """
@@ -528,8 +528,8 @@
         payload = self.transport.payloads[-1]
         self.assertMessages(payload["messages"], [])
         self.assertEqual(payload.get("client-api"), CLIENT_API)
-        self.assertEqual(payload.get("server-api"), SERVER_API)
-        self.assertEqual(self.transport.message_api, SERVER_API)
+        self.assertEqual(payload.get("server-api"), "3.2")
+        self.assertEqual(self.transport.message_api, "3.2")
 
         self.mstore.add({"type": "a", "api": "1.0"})
         self.mstore.add({"type": "b", "api": "1.0"})

=== modified file 'landscape/broker/tests/test_registration.py'
--- landscape/broker/tests/test_registration.py	2014-08-28 07:36:58 +0000
+++ landscape/broker/tests/test_registration.py	2014-09-01 13:16:19 +0000
@@ -498,15 +498,36 @@
 class JujuRegistrationHandlerTest(RegistrationHandlerTestBase):
 
     juju_contents = json.dumps({"environment-uuid": "DEAD-BEEF",
+                                "machine-id": "1",
                                 "unit-name": "service/0",
                                 "api-addresses": "10.0.3.1:17070"})
 
-    def test_juju_information_added_when_present(self):
+    def test_juju_info_added_when_present(self):
+        """
+        When information about the Juju environment is found in
+        the $data_dir/juju-info.d/ directory, it's included in
+        the registration message.
+        """
+        self.mstore.set_accepted_types(["register"])
+        self.mstore.set_server_api("3.3")
+        self.config.account_name = "account_name"
+        self.reactor.fire("run")
+        self.reactor.fire("pre-exchange")
+
+        messages = self.mstore.get_pending_messages()
+        self.assertEqual(
+            {"environment-uuid": "DEAD-BEEF",
+             "machine-id": "1",
+             "api-addresses": ["10.0.3.1:17070"]},
+            messages[0]["juju-info"])
+
+    def test_juju_info_list_added_when_present(self):
         """
         When Juju information is found in $data_dir/juju-info.d/*.json,
         key parts of it are sent in the registration message.
         """
         self.mstore.set_accepted_types(["register"])
+        self.mstore.set_server_api("3.3")
         self.config.account_name = "account_name"
         self.reactor.fire("run")
         self.reactor.fire("pre-exchange")
@@ -517,23 +538,6 @@
                     "unit-name": "service/0"}
         self.assertEqual(expected, messages[0]["juju-info-list"][0])
 
-    def test_juju_info_compatibility_present(self):
-        """
-        When Juju information is found in $data_dir/juju-info.d/*.json,
-        the registration message also contains a "juju-info" key for
-        backwards compatibility with older servers.
-        """
-        self.mstore.set_accepted_types(["register"])
-        self.config.account_name = "account_name"
-        self.reactor.fire("run")
-        self.reactor.fire("pre-exchange")
-
-        messages = self.mstore.get_pending_messages()
-        expected = {"environment-uuid": "DEAD-BEEF",
-                    "api-addresses": ["10.0.3.1:17070"],
-                    "unit-name": "service/0"}
-        self.assertEqual(expected, messages[0]["juju-info"])
-
     def test_multiple_juju_information_added_when_present(self):
         """
         When Juju information is found in $data_dir/juju-info.json,
@@ -541,6 +545,7 @@
         """
         # Write a second file in the config directory
         contents = json.dumps({"environment-uuid": "DEAD-BEEF",
+                               "machine-id": "1",
                                "unit-name": "service-2/0",
                                "api-addresses": "10.0.3.2:17070",
                                "private-address": "127.0.0.1"})
@@ -549,6 +554,7 @@
             dirname=self.config.juju_directory, suffix=".json")
 
         self.mstore.set_accepted_types(["register"])
+        self.mstore.set_server_api("3.3")
         self.config.account_name = "account_name"
         self.reactor.fire("run")
         self.reactor.fire("pre-exchange")
@@ -567,3 +573,17 @@
                      "unit-name": "service-2/0",
                      "private-address": "127.0.0.1"}
         self.assertIn(expected2, juju_info)
+
+    def test_juju_info_skipped_with_old_server(self):
+        """
+        If a server doesn't speak at least 3.3, the juju-info field is
+        isn't included in the message.
+        """
+        self.mstore.set_accepted_types(["register"])
+        self.mstore.set_server_api("3.2")
+        self.config.account_name = "account_name"
+        self.reactor.fire("run")
+        self.reactor.fire("pre-exchange")
+
+        messages = self.mstore.get_pending_messages()
+        self.assertNotIn("juju-info", messages[0])

=== modified file 'landscape/broker/tests/test_store.py'
--- landscape/broker/tests/test_store.py	2014-08-21 09:43:26 +0000
+++ landscape/broker/tests/test_store.py	2014-09-01 13:16:19 +0000
@@ -8,7 +8,6 @@
 
 from landscape.tests.helpers import LandscapeTest
 from landscape.tests.mocker import ANY
-from landscape import SERVER_API
 
 
 class MessageStoreTest(LandscapeTest):
@@ -133,7 +132,7 @@
         self.assertMessages(messages,
                             [{"type": "data",
                               "data": "A thing",
-                              "api": SERVER_API}])
+                              "api": "3.2"}])
 
     def test_max_pending(self):
         for i in range(10):
@@ -271,7 +270,7 @@
         messages = self.store.get_pending_messages()
 
         self.assertEqual(messages, [{"type": "data", "data": "2",
-                                     "api": SERVER_API}])
+                                     "api": "3.2"}])
 
         self.store.set_pending_offset(len(messages))
 
@@ -302,7 +301,7 @@
         self.mocker.verify()
         self.mocker.reset()
         self.assertEqual(self.store.get_pending_messages(),
-                         [{"type": "data", "data": 1, "api": SERVER_API}])
+                         [{"type": "data", "data": 1, "api": "3.2"}])
 
     def test_get_server_api_default(self):
         """

=== modified file 'landscape/manager/tests/test_customgraph.py'
--- landscape/manager/tests/test_customgraph.py	2011-07-05 05:09:11 +0000
+++ landscape/manager/tests/test_customgraph.py	2014-09-01 13:16:19 +0000
@@ -5,8 +5,6 @@
 from twisted.internet.error import ProcessDone
 from twisted.python.failure import Failure
 
-from landscape import SERVER_API
-
 from landscape.manager.customgraph import CustomGraphPlugin
 from landscape.manager.store import ManagerStore
 
@@ -458,7 +456,7 @@
         self.graph_manager.exchange()
         self.assertMessages(
             self.broker_service.message_store.get_pending_messages(),
-            [{"api": SERVER_API,
+            [{"api": "3.2",
               "data": {123: {"error": u"",
                              "script-hash": "e00a2f44dbc7b6710ce32af2348aec9b",
                              "values": []}},
@@ -481,7 +479,7 @@
         self.graph_manager.exchange()
         self.assertMessages(
             self.broker_service.message_store.get_pending_messages(),
-            [{"api": SERVER_API,
+            [{"api": "3.2",
               "data": {},
               "timestamp": 0,
               "type": "custom-graph"}])
@@ -506,13 +504,13 @@
         self.graph_manager.exchange()
         self.assertMessages(
             self.broker_service.message_store.get_pending_messages(),
-            [{"api": SERVER_API,
+            [{"api": "3.2",
               "data": {123: {"error": u"",
                              "script-hash": "e00a2f44dbc7b6710ce32af2348aec9b",
                              "values": []}},
               "timestamp": 0,
               "type": "custom-graph"},
-             {"api": SERVER_API,
+             {"api": "3.2",
               "data": {123: {"error": u"",
                              "script-hash": "e00a2f44dbc7b6710ce32af2348aec9b",
                              "values": []}},
@@ -540,13 +538,13 @@
         self.graph_manager.exchange()
         self.assertMessages(
             self.broker_service.message_store.get_pending_messages(),
-            [{"api": SERVER_API,
+            [{"api": "3.2",
               "data": {123: {"error": u"",
                              "script-hash": "e00a2f44dbc7b6710ce32af2348aec9b",
                              "values": []}},
               "timestamp": 0,
               "type": "custom-graph"},
-             {"api": SERVER_API,
+             {"api": "3.2",
               "data": {123: {"error": u"",
                              "script-hash": "d483816dc0fbb51ede42502a709b0e2a",
                              "values": []}},
@@ -589,7 +587,7 @@
             self.graph_manager.exchange()
             self.assertMessages(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"api": SERVER_API,
+                [{"api": "3.2",
                   "data": {123: {"error": u"",
                                  "script-hash":
                                     "991e15a81929c79fe1d243b2afd99c62",
@@ -631,7 +629,7 @@
             self.graph_manager.exchange()
             self.assertMessages(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"api": SERVER_API, "data": {}, "timestamp": 0, "type":
+                [{"api": "3.2", "data": {}, "timestamp": 0, "type":
                   "custom-graph"}])
         return result.addCallback(check)
 

=== modified file 'landscape/manager/tests/test_shutdownmanager.py'
--- landscape/manager/tests/test_shutdownmanager.py	2013-03-21 14:02:06 +0000
+++ landscape/manager/tests/test_shutdownmanager.py	2014-09-01 13:16:19 +0000
@@ -1,7 +1,6 @@
 from twisted.python.failure import Failure
 from twisted.internet.error import ProcessTerminated, ProcessDone
 
-from landscape import SERVER_API
 from landscape.manager.plugin import SUCCEEDED, FAILED
 from landscape.manager.shutdownmanager import (
     ShutdownManager, ShutdownProcessProtocol)
@@ -44,7 +43,7 @@
             self.assertTrue(self.broker_service.exchanger.is_urgent())
             self.assertEqual(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"type": "operation-result", "api": SERVER_API,
+                [{"type": "operation-result", "api": "3.2",
                   "operation-id": 100, "timestamp": 10, "status": SUCCEEDED,
                   "result-text": u"Data may arrive in batches."}])
 
@@ -84,7 +83,7 @@
             self.assertTrue(self.broker_service.exchanger.is_urgent())
             self.assertEqual(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"type": "operation-result", "api": SERVER_API,
+                [{"type": "operation-result", "api": "3.2",
                   "operation-id": 100, "timestamp": 0, "status": FAILED,
                   "result-text": u"Failure text is reported."}])
 

=== modified file 'landscape/message_schemas.py'
--- landscape/message_schemas.py	2014-08-21 09:43:26 +0000
+++ landscape/message_schemas.py	2014-09-01 13:16:19 +0000
@@ -54,14 +54,6 @@
      "result-text": Unicode()},
     optional=["result-code", "result-text"])
 
-#ACTION_INFO is obsolete.
-ACTION_INFO = Message(
-    "action-info",
-    {"response-id": Int(),
-     "success": Bool(),
-     "kind": Bytes(),
-     "parameters": Bytes()})
-
 COMPUTER_INFO = Message(
     "computer-info",
     {"hostname": Unicode(),
@@ -112,11 +104,7 @@
              "unit-name": Unicode(),
              "private-address": Unicode()}
 
-# The copy is needed because Message mutates the dictionary
-JUJU_INFO = Message("juju-info",
-                    juju_data.copy(),
-                    optional=["private-address"])
-
+# The copy of juju_data is needed because Message mutates the dictionary
 JUJU_UNITS_INFO = Message("juju-units-info", {
     "juju-info-list": List(KeyDict(juju_data.copy(),
                                    optional=["private-address"]))
@@ -196,18 +184,37 @@
      "tags": Any(Unicode(), Constant(None)),
      "vm-info": Bytes(),
      "container-info": Unicode(),
-     "juju-info": KeyDict(juju_data, optional=["private-address"]),
-     # Because of backwards compatibility we need another member with the list
-     # of juju-info, so it can safely be ignored by old servers.
+     "access_group": Unicode()},
+    optional=["registration_password", "hostname", "tags", "vm-info",
+              "container-info", "access_group"])
+
+
+REGISTER_3_3 = Message(
+    "register",
+    # The term used in the UI is actually 'registration_key', but we keep
+    # the message schema field as 'registration_password' in case a new
+    # client contacts an older server.
+    {"registration_password": Any(Unicode(), Constant(None)),
+     "computer_title": Unicode(),
+     "hostname": Unicode(),
+     "account_name": Unicode(),
+     "tags": Any(Unicode(), Constant(None)),
+     "vm-info": Bytes(),
+     "container-info": Unicode(),
+     "juju-info": KeyDict({"environment-uuid": Unicode(),
+                           "api-addresses": List(Unicode()),
+                           "machine-id": Unicode()}),
+     # XXX temporary field with unit info, will be dropped when we complete
+     # the migration to machine info.
      "juju-info-list": List(KeyDict(juju_data, optional=["private-address"])),
      "access_group": Unicode()},
+    api="3.3",
     optional=["registration_password", "hostname", "tags", "vm-info",
-              "container-info", "juju-info", "juju-info-list", "unicode",
-              "access_group"])
+              "container-info", "access_group", "juju-info", "juju-info-list"])
 
 
 # XXX The register-provisioned-machine message is obsolete, it's kept around
-# just to not break older LDS releases that import it the last LDS release
+# just to not break older LDS releases that import it (the last LDS release
 # to have it is 14.07). Eventually it shall be dropped.
 REGISTER_PROVISIONED_MACHINE = Message(
     "register-provisioned-machine",
@@ -238,6 +245,7 @@
      "access_group": Unicode()},
     optional=["tags", "vm-info", "public_ipv4", "local_ipv4", "access_group"])
 
+
 TEMPERATURE = Message("temperature", {
     "thermal-zone": Unicode(),
     "temperatures": List(Tuple(Int(), Float())),
@@ -482,7 +490,7 @@
     OPERATION_RESULT, COMPUTER_INFO, DISTRIBUTION_INFO,
     HARDWARE_INVENTORY, HARDWARE_INFO, LOAD_AVERAGE, MEMORY_INFO,
     RESYNCHRONIZE, MOUNT_ACTIVITY, MOUNT_INFO, FREE_SPACE,
-    REGISTER,
+    REGISTER, REGISTER_3_3,
     TEMPERATURE, PROCESSOR_INFO, USERS, PACKAGES, PACKAGE_LOCKS,
     CHANGE_PACKAGES_RESULT, UNKNOWN_PACKAGE_HASHES,
     ADD_PACKAGES, PACKAGE_REPORTER_RESULT, TEXT_MESSAGE, TEST,
@@ -490,4 +498,4 @@
     NETWORK_DEVICE, NETWORK_ACTIVITY,
     REBOOT_REQUIRED_INFO, UPDATE_MANAGER_INFO, CPU_USAGE,
     CEPH_USAGE, SWIFT_USAGE, SWIFT_DEVICE_INFO, KEYSTONE_TOKEN,
-    CHANGE_HA_SERVICE, JUJU_INFO, JUJU_UNITS_INFO, CLOUD_METADATA)
+    CHANGE_HA_SERVICE, JUJU_UNITS_INFO, CLOUD_METADATA)

=== modified file 'landscape/monitor/tests/test_activeprocessinfo.py'
--- landscape/monitor/tests/test_activeprocessinfo.py	2013-07-17 07:26:31 +0000
+++ landscape/monitor/tests/test_activeprocessinfo.py	2014-09-01 13:16:19 +0000
@@ -10,7 +10,6 @@
 from landscape.tests.helpers import (LandscapeTest, MonitorHelper,
                                      ProcessDataBuilder)
 from landscape.tests.mocker import ANY
-from landscape import SERVER_API
 
 
 class ActiveProcessInfoTest(LandscapeTest):
@@ -577,7 +576,7 @@
         messages = self.mstore.get_pending_messages()
         self.assertEqual(len(messages), 2)
         self.assertMessages(messages, [{"timestamp": 0,
-                                        "api": SERVER_API,
+                                        "api": "3.2",
                                         "type": "active-process-info",
                                         "kill-all-processes": True,
                                         "add-processes": [{"start-time": 110,
@@ -589,7 +588,7 @@
                                                             "vm-size": 11676,
                                                             "uid": 0}]},
                                        {"timestamp": 0,
-                                        "api": SERVER_API,
+                                        "api": "3.2",
                                         "type": "active-process-info",
                                         "update-processes": [
                                             {"start-time": 110,