Merge lp:~dpb/juju-deployer/api-endpoints into lp:juju-deployer

[David Britton]

Consult juju api-endpoints if using gojuju

Consult juju api-endpoints if using gojuju. Fall back to using the old mechanism of looking at juju status for node 0.

I created a daily build PPA for this as well, so we can make forward progress on testing:

https://code.launchpad.net/~davidpbritton/+archive/ubuntu/juju-deployer-devonly

[Kapil Thangavelu (hazmat)]

Thanks for the patch.. I have some mixed feelings about using api-endpoints. All api-endpoints is giving us is the addresses. The information we already want is in the the jenv file (credentials, state-servers, and critically the ca-cert to validate the server). There's a class method on the jujuclient Environment to enable connecting using the jenv file given an environment name .connect("foobar"). I'd like to push the logic for trying servers there, and just have deployer call out to it as ideally this should be abstracted for folks at the client library level.

[Kapil Thangavelu (hazmat)]

actually i'm curious if there are places where the first state server from api-endpoints or that's listed in the .jenv file under state-servers isn't the appropriate one.

[David Britton (dpb)]

On Sat, Aug 23, 2014 at 04:57:33PM -0000, Kapil Thangavelu wrote:
> actually i'm curious if there are places where the first state server
> from api-endpoints or that's listed in the .jenv file under
> state-servers isn't the appropriate one.

Good question... IME, Yes there are:

- In maas there can often be multiple network interfaces, each of which
  gets an IP address... The name is supposed to point to the primary
  IP, but it depends on maas managing the DNS, and also my client having
  access to do queries on MAAS (depending on the network config, this
  can be a problem). In these cases, I get 3 or 4 lines of addresses to
  check. :)

- In openstack, often the primary ip is an internal address and the
  floating IP is an external address. Juju exposes both as
  api-endpoints, often (maybe always), the first is the primary.

> --
> https://code.launchpad.net/~davidpbritton/juju-deployer/api-endpoints/+merge/230190
> You are the owner of lp:~davidpbritton/juju-deployer/api-endpoints.

--
David Britton <email address hidden>

[David Britton (dpb)]

On Sat, Aug 23, 2014 at 04:50:07PM -0000, Kapil Thangavelu wrote:
> there, and just have deployer call out to it as ideally this should be
> abstracted for folks at the client library level.

Makes sense... let me eval what is necessary there. I'll set to WIP
until then.

--
David Britton <email address hidden>

[Kapil Thangavelu (hazmat)]

i went ahead and reimplemented this against the jujuclient over the weekend
fwiw, i'm keen on the other patch for placement. let's chat on irc.

On Mon, Aug 25, 2014 at 4:19 PM, David Britton <email address hidden>
wrote:

> The proposal to merge lp:~davidpbritton/juju-deployer/api-endpoints into
> lp:juju-deployer has been updated.
>
> Status: Needs review => Work in progress
>
> For more details, see:
>
> https://code.launchpad.net/~davidpbritton/juju-deployer/api-endpoints/+merge/230190
> --
>
> https://code.launchpad.net/~davidpbritton/juju-deployer/api-endpoints/+merge/230190
> Your team juju-deployers is requested to review the proposed merge of
> lp:~davidpbritton/juju-deployer/api-endpoints into lp:juju-deployer.
>

=== modified file 'deployer/env/go.py'
--- deployer/env/go.py	2014-03-05 23:18:36 +0000
+++ deployer/env/go.py	2014-08-08 21:50:04 +0000
@@ -4,7 +4,7 @@
 import time
 
 from .base import BaseEnvironment
-from ..utils import ErrorExit, _get_juju_home, yaml_load
+from ..utils import ErrorExit, _get_juju_home, yaml_load, is_port_up
 
 from jujuclient import (
     EnvError,
@@ -64,9 +64,14 @@
 
     def connect(self):
         if not self.api_endpoint:
-            # Should really have a cheaper/faster way of getting the endpoint
-            # ala juju status 0 for a get_endpoint method.
-            self.get_cli_status()
+            endpoint = self._get_cli_endpoint()
+            if endpoint is None:
+                # Fallback to older methods
+                self.log.info(
+                    "Could not get endpoints directly, will infer from status")
+                status = self.get_cli_status()
+                endpoint = (status["machines"]["0"]["dns-name"], 17070)
+            self.api_endpoint = "wss://%s:%s/" % endpoint
         while True:
             try:
                 self.client = EnvironmentClient(self.api_endpoint)
@@ -92,6 +97,21 @@
                 return {}
             raise
 
+    def _get_cli_endpoint(self):
+        """ Consult juju for api-endpoints, returns None if not found """
+        params = self._named_env(["juju", "api-endpoints"])
+        try:
+            result = self._check_call(
+                params, self.log, "Error getting endpoints!")
+        except ErrorExit:
+            return None
+        endpoints = result.rstrip().split()
+        for endpoint in endpoints:
+            (host, port) = endpoint.split(":")
+            if is_port_up(host, port):
+                return (host, port)
+        return None
+
     def get_cli_status(self):
         status = super(GoEnvironment, self).get_cli_status()
         # Opportunistic, see connect method comment.

=== modified file 'deployer/tests/test_goenv.py'
--- deployer/tests/test_goenv.py	2014-02-22 23:11:02 +0000
+++ deployer/tests/test_goenv.py	2014-08-08 21:50:04 +0000
@@ -1,14 +1,99 @@
 import logging
+from mock import (
+    MagicMock,
+    patch,
+)
 import os
 import time
 import sys
 import unittest
 
 from deployer.env.go import GoEnvironment
+from ..utils import ErrorExit
 
 from .base import Base
 
 
+class GoEnvTests(Base):
+    def setUp(self):
+        self.env = GoEnvironment("unit-tests-go")
+
+    @patch("deployer.env.go.is_port_up")
+    @patch("deployer.env.go.GoEnvironment._check_call")
+    def test__get_cli_endpoint_single_endpoint(self, call, isup):
+        """Single api-endpoint."""
+        isup.return_value = True
+        call.return_value = "one.two:12\n"
+        endpoint = self.env._get_cli_endpoint()
+        self.assertEqual(endpoint, ("one.two", "12"))
+
+    @patch("deployer.env.go.is_port_up")
+    @patch("deployer.env.go.GoEnvironment._check_call")
+    def test__get_cli_endpoint_multiple_endpoints(self, call, isup):
+        """
+        Multiple api-endpoints.  Since list is traversed in order
+        make sure we reutrn something interesting, like the last one
+        symbolizing the first two not being active api endpoints
+        """
+        def _get_endpoint(host, value):
+            if host == "one.four":
+                return True
+            else:
+                return False
+        isup.side_effect = _get_endpoint
+        call.return_value = "one.two:12\none.three:13\none.four:14\n"
+        endpoint = self.env._get_cli_endpoint()
+        self.assertEqual(endpoint, ("one.four", "14"))
+
+    @patch("deployer.env.go.is_port_up")
+    @patch("deployer.env.go.GoEnvironment._check_call")
+    def test__get_cli_endpoint_multiple_endpoints_exhausted(self, call, isup):
+        """
+        Multiple api-endpoints.  Check when list is exhausted that we
+        return None appropriately.
+        """
+        isup.return_value = False
+        call.return_value = "one.two:12\none.three:13\none.four:14\n"
+        self.assertIsNone(self.env._get_cli_endpoint())
+
+    @patch("deployer.env.go.GoEnvironment._check_call")
+    def test__get_cli_endpoint_empty(self, call):
+        """Empty api-endpoints list."""
+        call.return_value = "\n"
+        self.assertIsNone(self.env._get_cli_endpoint())
+
+    @patch("deployer.env.go.GoEnvironment._check_call")
+    def test__get_cli_endpoint_error(self, call):
+        """Juju api-endpoints error should return None."""
+        call.side_effect = ErrorExit
+        self.assertIsNone(self.env._get_cli_endpoint())
+
+    @patch("deployer.env.go.GoEnvironment.log")
+    @patch("deployer.env.go.GoEnvironment._get_token")
+    @patch("deployer.env.go.EnvironmentClient")
+    @patch("deployer.env.go.GoEnvironment._get_cli_endpoint")
+    def test_connect_no_api_endpoint(self, getendpoint, *args):
+        """Test connect happy path."""
+        getendpoint.return_value = ("one.two", "12")
+        self.env.connect()
+        self.assertEqual(self.env.api_endpoint, "wss://one.two:12/")
+
+    @patch("deployer.env.go.GoEnvironment._get_token")
+    @patch("deployer.env.go.EnvironmentClient")
+    @patch("deployer.env.go.GoEnvironment.log")
+    @patch("deployer.env.go.GoEnvironment.get_cli_status")
+    @patch("deployer.env.go.GoEnvironment._get_cli_endpoint")
+    def test_connect_no_api_endpoint_command(
+            self, get_endpoint, get_clistat, log, *args):
+        """Test connect happy path on older juju w/o api-endpoints."""
+        get_endpoint.return_value = None
+        get_clistat.return_value = {"machines": {"0": {"dns-name": "foo"}}}
+        self.env.connect()
+        log.info.assert_called_with(
+            "Could not get endpoints directly, will infer from status")
+        self.assertEqual(self.env.api_endpoint, "wss://foo:17070/")
+
+
 # Takes roughly about 6m on core2 + ssd, mostly cloudinit time
 @unittest.skipIf(
     (not bool(os.environ.get("TEST_ENDPOINT"))),

=== modified file 'deployer/tests/test_utils.py'
--- deployer/tests/test_utils.py	2014-02-22 23:11:02 +0000
+++ deployer/tests/test_utils.py	2014-08-08 21:50:04 +0000
@@ -1,4 +1,6 @@
+import errno
 import os
+import socket
 from StringIO import StringIO
 from subprocess import CalledProcessError
 
@@ -18,6 +20,7 @@
     HTTPError,
     mkdir,
     URLError,
+    is_port_up,
 )
 
 
@@ -68,6 +71,25 @@
         # 1 failure + 3 retries
         self.assertEquals(len(check_output.call_args_list), 3)
 
+    @patch('socket.create_connection')
+    def test_is_port_up_valid(self, create_connection):
+        """Happy path through the is_port_up test"""
+        self.assertTrue(is_port_up("foo.example.com", "123"))
+
+    @patch('socket.create_connection')
+    def test_is_port_up_unknown_error(self, create_connection):
+        """Unknown error should re-raise"""
+        create_connection.side_effect = Exception
+        self.assertRaises(Exception, is_port_up, ("foo.example.com", "123"))
+
+    @patch('socket.create_connection')
+    def test_is_port_up_known_error(self, create_connection ):
+        """Known error should return false"""
+        e = socket.error()
+        e.errno = errno.ETIMEDOUT
+        create_connection.side_effect = e
+        self.assertFalse(is_port_up("foo.example.com", "123"))
+
 
 class TestMkdir(Base):
 

=== modified file 'deployer/utils.py'
--- deployer/utils.py	2014-02-22 23:17:02 +0000
+++ deployer/utils.py	2014-08-08 21:50:04 +0000
@@ -16,6 +16,7 @@
     exists as path_exists,
 )
 
+import socket
 import stat
 import subprocess
 import time
@@ -372,3 +373,16 @@
         if not 'default' in conf:
             raise ValueError("No Environment specified")
         return conf['default']
+
+
+def is_port_up(address, port):
+    """ Given address/port, return true/false if it's up """
+    try:
+        socket.create_connection((address, port)) 
+        return True
+    except socket.error as err:
+        if err.errno in (
+            errno.ETIMEDOUT, errno.ECONNREFUSED, errno.ECONNRESET):
+            return False
+        else:
+            raise