Merge lp:~facundo/magicicada-client/connect-multi-server into lp:magicicada-client

[Facundo Batista]

Allow in the config multiple servers to connect.

These possible multiple servers travel in one simple object, not a miriad of parameters to ActionQueue, which will use them to try to connect. It first tries the first one, on connection error tries the second, and so on until the list ends, and starts again with the first one.

For a quick retry on different servers, I lowered the initial delay to reconnect to 200ms.

[lp:~facundo/magicicada-client/connect-multi-server]

1437. By Facundo Batista on 2017-02-22

Better explanation in the .conf

[Facundo Batista (facundo)]

Comments addressed

[Natalia Bidart (nataliabidart)]

Looks good, thanks!

review: Approve

=== modified file 'bin/ubuntuone-syncdaemon'
--- bin/ubuntuone-syncdaemon	2015-09-29 21:05:26 +0000
+++ bin/ubuntuone-syncdaemon	2017-02-22 16:49:18 +0000
@@ -1,6 +1,7 @@
 #!/usr/bin/python
 #
 # Copyright 2009-2015 Canonical Ltd.
+# Copyright 2017 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -218,9 +219,7 @@
     monitor_class = yield get_filemonitor_class(options.fs_monitor)
 
     main = Main(options.root_dir, options.shares_dir, options.data_dir,
-                partials_dir, host=options.host, port=int(options.port),
-                dns_srv=options.dns_srv, ssl=True,
-                disable_ssl_verify=options.disable_ssl_verify,
+                partials_dir, options.server,
                 mark_interval=options.mark_interval,
                 broadcast_events=options.send_events_over_dbus,
                 handshake_timeout=options.handshake_timeout,

=== modified file 'data/syncdaemon.conf'
--- data/syncdaemon.conf	2016-09-17 01:06:23 +0000
+++ data/syncdaemon.conf	2017-02-22 16:49:18 +0000
@@ -1,18 +1,9 @@
 [__main__]
-host.default = fs-1.one.ubuntu.com
-host.help = The server address
-
-dns_srv.default = _https._tcp.fs.one.ubuntu.com
-dns_srv.help = The DNS SRV record
-
-disable_ssl_verify.default = False
-disable_ssl_verify.action = store_true
-disable_ssl_verify.parser = bool
-disable_ssl_verify.help = Disable SSL certificate verification in a test environment.
-
-port.default = 443
-port.parser = int
-port.help = The port on which to connect to the server
+server.default = localhost:443
+server.parser = connection
+server.help = The info to connect to the server: a comma-separated 
+    list of HOST:PORT or HOST:PORT:SSL_MODE (SSL_MODE 
+    options: 'plain', 'ssl', 'ssl_noverify', defaults to 'ssl')
 
 files_sync_enabled.default = True
 files_sync_enabled.action = store_true

=== modified file 'ubuntuone/syncdaemon/action_queue.py'
--- ubuntuone/syncdaemon/action_queue.py	2017-01-07 14:01:39 +0000
+++ ubuntuone/syncdaemon/action_queue.py	2017-02-22 16:49:18 +0000
@@ -30,9 +30,9 @@
 """Queue and execute operations on the server."""
 
 import inspect
+import itertools
 import logging
 import os
-import random
 import tempfile
 import traceback
 import zlib
@@ -45,7 +45,6 @@
 from zope.interface import implements
 from twisted.internet import reactor, defer, task
 from twisted.internet import error as twisted_errors
-from twisted.names import client as dns_client
 from twisted.python.failure import Failure, DefaultException
 
 from ubuntuone import clientdefs
@@ -756,8 +755,7 @@
     implements(IActionQueue)
     protocol = ActionQueueProtocol
 
-    def __init__(self, event_queue, main, host, port, dns_srv,
-                 use_ssl=False, disable_ssl_verify=False,
+    def __init__(self, event_queue, main, connection_info,
                  read_limit=None, write_limit=None, throttling_enabled=False,
                  connection_timeout=30):
         ThrottlingStorageClientFactory.__init__(
@@ -765,11 +763,8 @@
             throttling_enabled=throttling_enabled)
         self.event_queue = event_queue
         self.main = main
-        self.host = host
-        self.port = port
-        self.dns_srv = dns_srv
-        self.use_ssl = use_ssl
-        self.disable_ssl_verify = disable_ssl_verify
+        self.connection_info = itertools.cycle(connection_info)
+
         self.connection_timeout = connection_timeout
         self.credentials = {}
 
@@ -787,7 +782,6 @@
         self.zip_queue = ZipQueue()
         self.conditions_locker = ConditionsLocker()
         self.disk_queue = offload_queue.OffloadQueue()
-        self.tunnel_runner = tunnel_runner.TunnelRunner(self.host, self.port)
 
         self.estimated_free_space = {}
         event_queue.subscribe(self)
@@ -862,53 +856,22 @@
         self.event_queue.push('SV_VOLUME_NEW_GENERATION',
                               volume_id=volume_id, generation=generation)
 
-    def _lookup_srv(self):
-        """Do the SRV lookup.
-
-        Return a deferred whose callback is going to be called with
-        (host, port). If we can't do the lookup, the default host, port
-        is used.
-
-        """
-
-        def on_lookup_ok(results):
-            """Get a random host from the SRV result."""
-            logger.debug('SRV lookup done, choosing a server.')
-            records, auth, add = results
-            if not records:
-                raise ValueError('No available records.')
-            # pick a random server
-            record = random.choice(records)
-            logger.debug('Using record: %r', record)
-            if record.payload:
-                return record.payload.target.name, record.payload.port
-            else:
-                logger.info('Empty SRV record, fallback to %r:%r',
-                            self.host, self.port)
-                return self.host, self.port
-
-        def on_lookup_error(failure):
-            """Return the default host/post on a DNS SRV lookup failure."""
-            logger.info("SRV lookup error, fallback to %r:%r \n%s",
-                        self.host, self.port, failure.getTraceback())
-            return self.host, self.port
-
-        if self.dns_srv:
-            # lookup the DNS SRV records
-            d = dns_client.lookupService(self.dns_srv, timeout=[3, 2])
-            d.addCallback(on_lookup_ok)
-            d.addErrback(on_lookup_error)
-            return d
-        else:
-            return defer.succeed((self.host, self.port))
+    def _get_tunnel_runner(self, host, port):
+        """Build the tunnel runner."""
+        return tunnel_runner.TunnelRunner(host, port)
 
     @defer.inlineCallbacks
-    def _make_connection(self, result):
+    def _make_connection(self):
         """Do the real connect call."""
-        host, port = result
-        ssl_context = get_ssl_context(self.disable_ssl_verify, host)
-        client = yield self.tunnel_runner.get_client()
-        if self.use_ssl:
+        connection_info = self.connection_info.next()
+        logger.info("Attempting connection to %s", connection_info)
+        host = connection_info['host']
+        port = connection_info['port']
+        tunnelrunner = self._get_tunnel_runner(host, port)
+        client = yield tunnelrunner.get_client()
+        if connection_info['use_ssl']:
+            ssl_context = get_ssl_context(
+                connection_info['disable_ssl_verify'], host)
             self.connector = client.connectSSL(
                 host, port, factory=self, contextFactory=ssl_context,
                 timeout=self.connection_timeout)
@@ -925,9 +888,7 @@
             return
 
         self.connect_in_progress = True
-        d = self._lookup_srv()
-        # DNS lookup always succeeds, proceed to actually connect
-        d.addCallback(self._make_connection)
+        self._make_connection()
 
     def buildProtocol(self, addr):
         """Build the client and store it. Connect callbacks."""

=== modified file 'ubuntuone/syncdaemon/config.py'
--- ubuntuone/syncdaemon/config.py	2016-09-17 01:06:23 +0000
+++ ubuntuone/syncdaemon/config.py	2017-02-22 16:49:18 +0000
@@ -1,6 +1,7 @@
 # ubuntuone.syncdaemon.config - SyncDaemon config utilities
 #
 # Copyright 2009-2012 Canonical Ltd.
+# Copyright 2017 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -128,6 +129,42 @@
     return result
 
 
+def server_connection_parser(value):
+    """Parser for the server connection info."""
+    results = []
+    for item in value.split(","):
+        ci_parts = item.split(':')
+        if len(ci_parts) == 2:
+            host, port = ci_parts
+            mode = 'ssl'  # default
+        elif len(ci_parts) == 3:
+            host, port, mode = ci_parts
+        else:
+            raise ValueError(
+                "--server info must be HOST:PORT or HOST:PORT:SSL_MODE")
+
+        if mode == 'plain':
+            use_ssl = False
+            disable_ssl_verify = False
+        elif mode == 'ssl':
+            use_ssl = True
+            disable_ssl_verify = False
+        elif mode == 'ssl_noverify':
+            use_ssl = True
+            disable_ssl_verify = True
+        else:
+            raise ValueError(
+                "--server form (HOST:PORT:SSL_MODE) accepts the following"
+                "SSL_MODE options only: 'plain', 'ssl', 'ssl_noverify'")
+        results.append({
+            'host': host,
+            'port': int(port),
+            'use_ssl': use_ssl,
+            'disable_ssl_verify': disable_ssl_verify,
+        })
+    return results
+
+
 def log_level_parser(value):
     """Parser for "logging" module log levels.
 
@@ -157,6 +194,7 @@
             ('xdg_cache', xdg_cache_dir_parser),
             ('xdg_data', xdg_data_dir_parser),
             ('log_level', log_level_parser),
+            ('connection', server_connection_parser),
             ('throttling_limit', throttling_limit_parser)]
 
 

=== modified file 'ubuntuone/syncdaemon/main.py'
--- ubuntuone/syncdaemon/main.py	2016-09-17 01:06:23 +0000
+++ ubuntuone/syncdaemon/main.py	2017-02-22 16:49:18 +0000
@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 #
 # Copyright 2009-2015 Canonical Ltd.
-# Copyright 2015-2016 Chicharreros (https://launchpad.net/~chicharreros)
+# Copyright 2015-2017 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -83,10 +83,8 @@
     """The one who executes the syncdaemon."""
 
     def __init__(self, root_dir, shares_dir, data_dir, partials_dir,
-                 host='fs-1.one.ubuntu.com', port=443, dns_srv=None, ssl=True,
-                 disable_ssl_verify=False,
-                 mark_interval=120, broadcast_events=False,
-                 handshake_timeout=30,
+                 connection_info, mark_interval=120,
+                 broadcast_events=False, handshake_timeout=30,
                  shares_symlink_name='Shared With Me',
                  read_limit=None, write_limit=None, throttling_enabled=False,
                  ignore_files=None, auth_credentials=None,
@@ -125,9 +123,7 @@
 
         # we don't have the auth tokens yet, we 'll get them later
         self.action_q = action_queue.ActionQueue(self.event_q, self,
-                                                 host, port,
-                                                 dns_srv, ssl,
-                                                 disable_ssl_verify,
+                                                 connection_info,
                                                  read_limit, write_limit,
                                                  throttling_enabled)
         self.hash_q = hash_queue.HashQueue(self.event_q)

=== modified file 'ubuntuone/syncdaemon/states.py'
--- ubuntuone/syncdaemon/states.py	2015-09-19 23:15:50 +0000
+++ ubuntuone/syncdaemon/states.py	2017-02-22 16:49:18 +0000
@@ -1,8 +1,5 @@
-# ubuntuone.syncdaemon.states.py - States!
-#
-# Author: Facundo Batista <[e-mail]>
-#
 # Copyright 2010-2012 Canonical Ltd.
+# Copyright 2017 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -63,7 +60,7 @@
         self.sm = state_manager
         self.eq = state_manager.main.event_q
         self.handshake_timeout = handshake_timeout
-        self.waiting_timeout = 1
+        self.waiting_timeout = .2
         self.state = self.NU_NN
         self.working = True
 

=== modified file 'ubuntuone/syncdaemon/tests/test_action_queue.py'
--- ubuntuone/syncdaemon/tests/test_action_queue.py	2017-01-07 14:01:39 +0000
+++ ubuntuone/syncdaemon/tests/test_action_queue.py	2017-02-22 16:49:18 +0000
@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 #
 # Copyright 2009-2015 Canonical Ltd.
-# Copyright 2016-2017 Chicharreros
+# Copyright 2016-2017 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -33,6 +33,7 @@
 
 import collections
 import inspect
+import itertools
 import logging
 import operator
 import os
@@ -237,9 +238,11 @@
 class SavingConnectionTunnelRunner(object):
     """A fake proxy.tunnel_client.TunnelRunner."""
 
-    def __init__(self, *args):
+    def __init__(self, host, port):
         """Fake a proxy tunnel."""
         self.client = FakeTunnelClient()
+        self.host = host
+        self.port = port
 
     def get_client(self):
         """Always return the reactor."""
@@ -287,7 +290,8 @@
 
         # set up FakeMain to use the testing AQ, the port will be decided later
         self.patch(FakeMain, '_fake_AQ_class', TestActionQueue)
-        self.patch(FakeMain, '_fake_AQ_params', ('127.0.0.1', 0, False))
+        connection_info = [dict(host='127.0.0.1', port=0, use_ssl=False)]
+        self.patch(FakeMain, '_fake_AQ_params', (connection_info,))
         self.patch(offload_queue, "OffloadQueue", FakeOffloadQueue)
 
         self.main = FakeMain(root_dir=self.root, shares_dir=self.shares,
@@ -1130,12 +1134,23 @@
 class FactoryBaseTestCase(BasicTestCase):
     """Helper for by-pass Twisted."""
 
+    def _patch_connection_info(self, **attrvalues):
+        """Helper to patch the ActionQueue connection_info.
+
+        This assumes there is only one item in the connection_info, which
+        is the case for the tests.
+        """
+        connection_info = self.action_queue.connection_info.next()
+        for attr, value in attrvalues.items():
+            connection_info[attr] = value
+        self.action_queue.connection_info = itertools.cycle([connection_info])
+
     def _start_sample_webserver(self):
         """Start a web server serving content at its root"""
         # start listening on `decide yourself` port, and fix AQ with it
         website = server.Site(None)
         webport = reactor.listenTCP(0, website)
-        self.action_queue.port = webport.getHost().port
+        self._patch_connection_info(port=webport.getHost().port)
 
         server_transport_deferred = defer.Deferred()
         transport_class = webport.transport
@@ -1356,8 +1371,9 @@
 
     def test_connection_started_logging(self):
         """Test that the connection started logs connector info, not AQ's."""
-        assert self.action_queue.host == '127.0.0.1'
-        assert self.action_queue.port == 0
+        connection_info = self.action_queue.connection_info.next()
+        assert connection_info['host'] == '127.0.0.1'
+        assert connection_info['port'] == 0
 
         class FakeConnector(object):
             """Fake connector."""
@@ -1368,26 +1384,70 @@
         self.assertTrue(self.handler.check_info("Connection started",
                                                 "host 1.2.3.4", "port 4321"))
 
+    @defer.inlineCallbacks
+    def test_connection_info_rotation(self):
+        """It tries to connect to different servers."""
+
+        multiple_conn = [
+            {'host': 'host1', 'port': 'port1', 'use_ssl': False},
+            {'host': 'host2', 'port': 'port2', 'use_ssl': False},
+        ]
+        self.action_queue.connection_info = itertools.cycle(multiple_conn)
+
+        self.tunnel_runner = None
+
+        def mitm(*args):
+            tunnel_runner = SavingConnectionTunnelRunner(*args)
+            self.tunnel_runner = tunnel_runner
+            return tunnel_runner
+
+        self.action_queue._get_tunnel_runner = mitm
+
+        yield self.action_queue._make_connection()
+        self.assertEqual(self.tunnel_runner.host, 'host1')
+        self.assertEqual(self.tunnel_runner.port, 'port1')
+
+        yield self.action_queue._make_connection()
+        self.assertEqual(self.tunnel_runner.host, 'host2')
+        self.assertEqual(self.tunnel_runner.port, 'port2')
+
+        yield self.action_queue._make_connection()
+        self.assertEqual(self.tunnel_runner.host, 'host1')
+        self.assertEqual(self.tunnel_runner.port, 'port1')
+
 
 class TunnelRunnerTestCase(FactoryBaseTestCase):
     """Tests for the tunnel runner."""
 
     tunnel_runner_class = SavingConnectionTunnelRunner
 
+    def setUp(self):
+        result = super(TunnelRunnerTestCase, self).setUp()
+        self.tunnel_runner = None
+        orig_get_tunnel_runner = self.action_queue._get_tunnel_runner
+
+        def mitm(*args):
+            tunnel_runner = orig_get_tunnel_runner(*args)
+            self.tunnel_runner = tunnel_runner
+            return tunnel_runner
+
+        self.action_queue._get_tunnel_runner = mitm
+        return result
+
     @defer.inlineCallbacks
     def test_make_connection_uses_tunnelrunner_non_ssl(self):
         """Check that _make_connection uses TunnelRunner."""
-        self.action_queue.use_ssl = False
-        yield self.action_queue._make_connection(("127.0.0.1", 1234))
-        self.assertTrue(self.action_queue.tunnel_runner.client.tcp_connected,
+        self._patch_connection_info(use_ssl=False)
+        yield self.action_queue._make_connection()
+        self.assertTrue(self.tunnel_runner.client.tcp_connected,
                         "connectTCP is called on the client.")
 
     @defer.inlineCallbacks
     def test_make_connection_uses_tunnelrunner_ssl(self):
         """Check that _make_connection uses TunnelRunner."""
-        self.action_queue.use_ssl = True
-        yield self.action_queue._make_connection(("127.0.0.1", 1234))
-        self.assertTrue(self.action_queue.tunnel_runner.client.ssl_connected,
+        self._patch_connection_info(use_ssl=True, disable_ssl_verify=False)
+        yield self.action_queue._make_connection()
+        self.assertTrue(self.tunnel_runner.client.ssl_connected,
                         "connectSSL is called on the client.")
 
 
@@ -1400,13 +1460,18 @@
     def test_context_request_passes_host(self):
         """The context is requested passing the host."""
         fake_host = "fake_host"
+        fake_disable_ssl_verify = False
 
         def fake_get_ssl_context(disable_ssl_verify, host):
             """The host is used to call get_ssl_context."""
+            self.assertEqual(disable_ssl_verify, fake_disable_ssl_verify)
             self.assertEqual(host, fake_host)
 
         self.patch(action_queue, "get_ssl_context", fake_get_ssl_context)
-        yield self.action_queue._make_connection((fake_host, 1234))
+        self._patch_connection_info(
+            host=fake_host, use_ssl=True,
+            disable_ssl_verify=fake_disable_ssl_verify)
+        yield self.action_queue._make_connection()
 
 
 class ConnectedBaseTestCase(FactoryBaseTestCase):

=== modified file 'ubuntuone/syncdaemon/tests/test_config.py'
--- ubuntuone/syncdaemon/tests/test_config.py	2016-09-17 01:06:23 +0000
+++ ubuntuone/syncdaemon/tests/test_config.py	2017-02-22 16:49:18 +0000
@@ -1,6 +1,7 @@
 # encoding: utf-8
 #
 # Copyright 2009-2012 Canonical Ltd.
+# Copyright 2017 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -494,6 +495,73 @@
         self.assertEqual(logging.DEBUG, parser(bad_value))
         self.assertEqual(logging.DEBUG, parser(invalid_value))
 
+    def test_serverconnection_simple_defaultmode(self):
+        results = config.server_connection_parser('test.host:666')
+        self.assertEqual(results, [{
+            'host': 'test.host',
+            'port': 666,
+            'use_ssl': True,
+            'disable_ssl_verify': False,
+        }])
+
+    def test_serverconnection_simple_plain(self):
+        results = config.server_connection_parser('test.host:666:plain')
+        self.assertEqual(results, [{
+            'host': 'test.host',
+            'port': 666,
+            'use_ssl': False,
+            'disable_ssl_verify': False,
+        }])
+
+    def test_serverconnection_simple_ssl(self):
+        results = config.server_connection_parser('test.host:666:ssl')
+        self.assertEqual(results, [{
+            'host': 'test.host',
+            'port': 666,
+            'use_ssl': True,
+            'disable_ssl_verify': False,
+        }])
+
+    def test_serverconnection_simple_noverify(self):
+        results = config.server_connection_parser('test.host:666:ssl_noverify')
+        self.assertEqual(results, [{
+            'host': 'test.host',
+            'port': 666,
+            'use_ssl': True,
+            'disable_ssl_verify': True,
+        }])
+
+    def test_serverconnection_simple_bad_mode(self):
+        self.assertRaises(
+            ValueError, config.server_connection_parser, 'host:666:badmode')
+
+    def test_serverconnection_simple_too_many_parts(self):
+        self.assertRaises(
+            ValueError, config.server_connection_parser, 'host:666:plain:what')
+
+    def test_serverconnection_simple_too_few_parts(self):
+        self.assertRaises(
+            ValueError, config.server_connection_parser, 'test.host')
+
+    def test_serverconnection_simple_port_not_numeric(self):
+        self.assertRaises(
+            ValueError, config.server_connection_parser, 'test.host:port')
+
+    def test_serverconnection_multiple(self):
+        results = config.server_connection_parser(
+            'test.host1:666:plain,host2.com:447')
+        self.assertEqual(results, [{
+            'host': 'test.host1',
+            'port': 666,
+            'use_ssl': False,
+            'disable_ssl_verify': False,
+        }, {
+            'host': 'host2.com',
+            'port': 447,
+            'use_ssl': True,
+            'disable_ssl_verify': False,
+        }])
+
 
 class XdgHomeParsersTests(BaseTwistedTestCase):
     """Tests for our custom xdg parsers."""

=== modified file 'ubuntuone/syncdaemon/tests/test_main.py'
--- ubuntuone/syncdaemon/tests/test_main.py	2016-09-17 14:29:53 +0000
+++ ubuntuone/syncdaemon/tests/test_main.py	2017-02-22 16:49:18 +0000
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 #
 # Copyright 2009-2015 Canonical Ltd.
+# Copyright 2016-2017 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -96,8 +97,7 @@
                     shares_dir=self.shares,
                     data_dir=self.data,
                     partials_dir=self.partials_dir,
-                    host='localhost', port=0,
-                    dns_srv=False, ssl=False,
+                    connection_info='localhost:0:plain',
                     mark_interval=60,
                     handshake_timeout=2,
                     auth_credentials=FAKED_CREDENTIALS,