From b604d08463982362dcaed698c75bd592bba97a6f Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 12 Jun 2019 16:47:25 -0400 Subject: [PATCH 01/10] Add a test for the success case --- src/allmydata/client.py | 5 ++- src/allmydata/test/test_client.py | 59 +++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/src/allmydata/client.py b/src/allmydata/client.py index 2333a35b7..8f426b312 100644 --- a/src/allmydata/client.py +++ b/src/allmydata/client.py @@ -8,7 +8,6 @@ from twisted.internet import reactor, defer from twisted.application import service from twisted.application.internet import TimerService from twisted.python.filepath import FilePath -from twisted.python.failure import Failure from pycryptopp.publickey import rsa import allmydata @@ -207,7 +206,7 @@ def create_client(basedir=u".", _client_factory=None): _client_factory=_client_factory, ) except Exception: - return Failure() + return defer.fail() def create_client_from_config(config, _client_factory=None): @@ -261,7 +260,7 @@ def create_client_from_config(config, _client_factory=None): storage_broker.setServiceParent(client) return defer.succeed(client) except Exception: - return Failure() + return defer.fail() def _sequencer(config): diff --git a/src/allmydata/test/test_client.py b/src/allmydata/test/test_client.py index 1d0fa085c..0fd35353e 100644 --- a/src/allmydata/test/test_client.py +++ b/src/allmydata/test/test_client.py @@ -1,9 +1,22 @@ import os, sys import mock import twisted +from yaml import ( + safe_dump, +) from twisted.trial import unittest from twisted.application import service from twisted.internet import defer +from twisted.python.filepath import ( + FilePath, +) +from testtools.matchers import ( + Equals, + AfterPreprocessing, +) +from testtools.twistedsupport import ( + succeeded, +) import allmydata import allmydata.frontends.magic_folder @@ -20,6 +33,9 @@ from allmydata.interfaces import IFilesystemNode, IFileNode, \ IImmutableFileNode, IMutableFileNode, IDirectoryNode from foolscap.api import flushEventualQueue import allmydata.test.common_util as testutil +from allmydata.test.common import ( + SyncTestCase, +) BASECONFIG = ("[client]\n" @@ -666,6 +682,49 @@ class IntroducerClients(unittest.TestCase): ) +class StorageClients(SyncTestCase): + """ + Tests for storage-related behavior of ``_Client``. + """ + def test_static_servers(self): + """ + Storage servers defined in ``private/servers.yaml`` are loaded into the + storage broker. + """ + serverid = u"v0-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + announcement = { + u"nickname": 'some-storage-server', + u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum", + } + basedir = FilePath(self.mktemp()) + private = basedir.child(u"private") + private.makedirs() + servers = private.child(u"servers.yaml") + servers.setContent(safe_dump({ + u"storage": { + serverid: { + u"ann": announcement, + }, + }, + })) + def get_known_server_details(a_client): + return list( + (s.get_serverid(), s.get_announcement()) + for s + in a_client.storage_broker.get_known_servers() + ) + self.assertThat( + client.create_client(basedir.asTextMode().path), + succeeded( + AfterPreprocessing( + get_known_server_details, + Equals([(serverid, announcement)]), + ), + ), + ) + + + class Run(unittest.TestCase, testutil.StallMixin): def setUp(self): From 225aec912a0c87a856b68a51ab6d039bb1cd8847 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 12 Jun 2019 17:03:09 -0400 Subject: [PATCH 02/10] refactor the test to use a servers.yaml fixture --- src/allmydata/test/test_client.py | 66 +++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 16 deletions(-) diff --git a/src/allmydata/test/test_client.py b/src/allmydata/test/test_client.py index 0fd35353e..abe2b9068 100644 --- a/src/allmydata/test/test_client.py +++ b/src/allmydata/test/test_client.py @@ -4,6 +4,9 @@ import twisted from yaml import ( safe_dump, ) +from fixtures import ( + Fixture, +) from twisted.trial import unittest from twisted.application import service from twisted.internet import defer @@ -682,6 +685,47 @@ class IntroducerClients(unittest.TestCase): ) +def get_known_server_details(a_client): + """ + Get some details about known storage servers from a client. + + :param _Client a_client: The client to inspect. + + :return: A ``list`` of two-tuples. Each element of the list corresponds + to a "known server". The first element of each tuple is a server id. + The second is the server's announcement. + """ + return list( + (s.get_serverid(), s.get_announcement()) + for s + in a_client.storage_broker.get_known_servers() + ) + + +class StaticServers(Fixture): + """ + Create a ``servers.yaml`` file. + """ + def __init__(self, basedir, server_details): + super(StaticServers, self).__init__() + self._basedir = basedir + self._server_details = server_details + + def _setUp(self): + private = self._basedir.child(u"private") + private.makedirs() + servers = private.child(u"servers.yaml") + servers.setContent(safe_dump({ + u"storage": { + serverid: { + u"ann": announcement, + } + for (serverid, announcement) + in self._server_details + }, + })) + + class StorageClients(SyncTestCase): """ Tests for storage-related behavior of ``_Client``. @@ -697,22 +741,12 @@ class StorageClients(SyncTestCase): u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum", } basedir = FilePath(self.mktemp()) - private = basedir.child(u"private") - private.makedirs() - servers = private.child(u"servers.yaml") - servers.setContent(safe_dump({ - u"storage": { - serverid: { - u"ann": announcement, - }, - }, - })) - def get_known_server_details(a_client): - return list( - (s.get_serverid(), s.get_announcement()) - for s - in a_client.storage_broker.get_known_servers() - ) + static_servers = self.useFixture( + StaticServers( + basedir, + [(serverid, announcement)], + ), + ) self.assertThat( client.create_client(basedir.asTextMode().path), succeeded( From 92724449a07d8c87fab3f6b8c9e9334b0bd55062 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 12 Jun 2019 17:03:24 -0400 Subject: [PATCH 03/10] unicode! --- src/allmydata/test/test_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/test_client.py b/src/allmydata/test/test_client.py index abe2b9068..fbd591a3e 100644 --- a/src/allmydata/test/test_client.py +++ b/src/allmydata/test/test_client.py @@ -737,7 +737,7 @@ class StorageClients(SyncTestCase): """ serverid = u"v0-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" announcement = { - u"nickname": 'some-storage-server', + u"nickname": u"some-storage-server", u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum", } basedir = FilePath(self.mktemp()) From dd0cda8a41ae9b9edd420cbb955462a30ea378fc Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 12 Jun 2019 17:05:14 -0400 Subject: [PATCH 04/10] Add a test for a bogus announcement --- src/allmydata/test/test_client.py | 34 +++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/allmydata/test/test_client.py b/src/allmydata/test/test_client.py index fbd591a3e..de616c85a 100644 --- a/src/allmydata/test/test_client.py +++ b/src/allmydata/test/test_client.py @@ -757,6 +757,40 @@ class StorageClients(SyncTestCase): ), ) + def test_invalid_static_server(self): + """ + An invalid announcement for a static server does not prevent other static + servers from being loaded. + """ + # Some good details + serverid = u"v0-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + announcement = { + u"nickname": u"some-storage-server", + u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum", + } + basedir = FilePath(self.mktemp()) + static_servers = self.useFixture( + StaticServers( + basedir, + [(serverid, announcement), + # Alongside some bad details + (u"v0-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + {u"nickname": u"another-storage-server", + u"anonymous-storage-FURL": None, + }), + ], + ), + ) + self.assertThat( + client.create_client(basedir.asTextMode().path), + succeeded( + AfterPreprocessing( + get_known_server_details, + # It should have the good server details. + Equals([(serverid, announcement)]), + ), + ), + ) class Run(unittest.TestCase, testutil.StallMixin): From f6ad8fa56b09573266434a0684dc8704cd637eeb Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 13 Jun 2019 09:08:42 -0400 Subject: [PATCH 05/10] Make the new test pass by catching and logging --- src/allmydata/storage_client.py | 39 +++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 3c761032f..c319d3fa9 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -34,7 +34,9 @@ import attr from zope.interface import implementer from twisted.internet import defer from twisted.application import service - +from eliot import ( + log_call, +) from foolscap.api import eventually from allmydata.interfaces import ( IStorageBroker, @@ -90,18 +92,32 @@ class StorageFarmBroker(service.MultiService): self._threshold_listeners = [] # tuples of (threshold, Deferred) self._connected_high_water_mark = 0 + @log_call(action_type=u"storage-client:broker:set-static-servers") def set_static_servers(self, servers): for (server_id, server) in servers.items(): - assert isinstance(server_id, unicode) # from YAML - server_id = server_id.encode("ascii") - self._static_server_ids.add(server_id) - handler_overrides = server.get("connections", {}) - s = NativeStorageServer(server_id, server["ann"], - self._tub_maker, handler_overrides) - s.on_status_changed(lambda _: self._got_connection()) - s.setServiceParent(self) - self.servers[server_id] = s - s.start_connecting(self._trigger_connections) + try: + storage_server = self._make_storage_server(server_id, server) + except Exception: + pass + else: + self._static_server_ids.add(server_id) + self.servers[server_id] = storage_server + storage_server.setServiceParent(self) + storage_server.start_connecting(self._trigger_connections) + + @log_call( + action_type=u"storage-client:broker:make-storage-server", + include_args=["server_id"], + include_result=False, + ) + def _make_storage_server(self, server_id, server): + assert isinstance(server_id, unicode) # from YAML + server_id = server_id.encode("ascii") + handler_overrides = server.get("connections", {}) + s = NativeStorageServer(server_id, server["ann"], + self._tub_maker, handler_overrides) + s.on_status_changed(lambda _: self._got_connection()) + return s def when_connected_enough(self, threshold): """ @@ -254,6 +270,7 @@ class StubServer(object): def get_nickname(self): return "?" + @implementer(IServer) class NativeStorageServer(service.MultiService): """I hold information about a storage server that we want to connect to. From b040a22ca3a3ac2b178e5e2d0e71a26ad6b38090 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 13 Jun 2019 11:58:14 -0400 Subject: [PATCH 06/10] Use the TempDir fixture --- src/allmydata/test/test_client.py | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/allmydata/test/test_client.py b/src/allmydata/test/test_client.py index de616c85a..e03963c35 100644 --- a/src/allmydata/test/test_client.py +++ b/src/allmydata/test/test_client.py @@ -6,6 +6,11 @@ from yaml import ( ) from fixtures import ( Fixture, + TempDir, +) +from eliot.testing import ( + capture_logging, + assertHasAction, ) from twisted.trial import unittest from twisted.application import service @@ -730,6 +735,19 @@ class StorageClients(SyncTestCase): """ Tests for storage-related behavior of ``_Client``. """ + def setUp(self): + super(StorageClients, self).setUp() + # Some other tests create Nodes and Node mutates tempfile.tempdir and + # that screws us up because we're *not* making a Node. "Fix" it. See + # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3052 for the real fix, + # though. + import tempfile + tempfile.tempdir = None + + tempdir = TempDir() + self.useFixture(tempdir) + self.basedir = FilePath(tempdir.path) + def test_static_servers(self): """ Storage servers defined in ``private/servers.yaml`` are loaded into the @@ -740,15 +758,14 @@ class StorageClients(SyncTestCase): u"nickname": u"some-storage-server", u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum", } - basedir = FilePath(self.mktemp()) static_servers = self.useFixture( StaticServers( - basedir, + self.basedir, [(serverid, announcement)], ), ) self.assertThat( - client.create_client(basedir.asTextMode().path), + client.create_client(self.basedir.asTextMode().path), succeeded( AfterPreprocessing( get_known_server_details, @@ -768,10 +785,9 @@ class StorageClients(SyncTestCase): u"nickname": u"some-storage-server", u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum", } - basedir = FilePath(self.mktemp()) static_servers = self.useFixture( StaticServers( - basedir, + self.basedir, [(serverid, announcement), # Alongside some bad details (u"v0-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", @@ -782,7 +798,7 @@ class StorageClients(SyncTestCase): ), ) self.assertThat( - client.create_client(basedir.asTextMode().path), + client.create_client(self.basedir.asTextMode().path), succeeded( AfterPreprocessing( get_known_server_details, From bbb1ebdd26f32820da1c5ef7bc5a898babe1d25a Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 13 Jun 2019 12:23:41 -0400 Subject: [PATCH 07/10] Make some assertions about the logging --- src/allmydata/test/test_client.py | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/allmydata/test/test_client.py b/src/allmydata/test/test_client.py index e03963c35..e113860b1 100644 --- a/src/allmydata/test/test_client.py +++ b/src/allmydata/test/test_client.py @@ -748,7 +748,15 @@ class StorageClients(SyncTestCase): self.useFixture(tempdir) self.basedir = FilePath(tempdir.path) - def test_static_servers(self): + @capture_logging( + lambda case, logger: assertHasAction( + case, + logger, + actionType=u"storage-client:broker:set-static-servers", + succeeded=True, + ), + ) + def test_static_servers(self, logger): """ Storage servers defined in ``private/servers.yaml`` are loaded into the storage broker. @@ -774,13 +782,21 @@ class StorageClients(SyncTestCase): ), ) - def test_invalid_static_server(self): + @capture_logging( + lambda case, logger: assertHasAction( + case, + logger, + actionType=u"storage-client:broker:make-storage-server", + succeeded=False, + ), + ) + def test_invalid_static_server(self, logger): """ An invalid announcement for a static server does not prevent other static servers from being loaded. """ # Some good details - serverid = u"v0-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + serverid = u"v1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" announcement = { u"nickname": u"some-storage-server", u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum", @@ -789,7 +805,9 @@ class StorageClients(SyncTestCase): StaticServers( self.basedir, [(serverid, announcement), - # Alongside some bad details + # Along with a "bad" server announcement. Order in this list + # doesn't matter, yaml serializer and Python dicts are going + # to shuffle everything around kind of randomly. (u"v0-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", {u"nickname": u"another-storage-server", u"anonymous-storage-FURL": None, From 053b4c861bdff03e519c5f16c820e177fd11c87a Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 13 Jun 2019 12:54:04 -0400 Subject: [PATCH 08/10] news fragment --- newsfragments/3051.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/3051.feature diff --git a/newsfragments/3051.feature b/newsfragments/3051.feature new file mode 100644 index 000000000..8f4f6f377 --- /dev/null +++ b/newsfragments/3051.feature @@ -0,0 +1 @@ +Static storage server "announcements" in ``private/servers.yaml`` are now individually logged and ignored if they cannot be interpreted. From f5fc38e8a71c130cb07356766eb09200c8d8294b Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 21 Jun 2019 08:17:53 -0400 Subject: [PATCH 09/10] remove unused locals --- src/allmydata/test/test_client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_client.py b/src/allmydata/test/test_client.py index e113860b1..3d9ed479e 100644 --- a/src/allmydata/test/test_client.py +++ b/src/allmydata/test/test_client.py @@ -766,7 +766,7 @@ class StorageClients(SyncTestCase): u"nickname": u"some-storage-server", u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum", } - static_servers = self.useFixture( + self.useFixture( StaticServers( self.basedir, [(serverid, announcement)], @@ -801,7 +801,7 @@ class StorageClients(SyncTestCase): u"nickname": u"some-storage-server", u"anonymous-storage-FURL": u"pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@tcp:storage.example:100/swissnum", } - static_servers = self.useFixture( + self.useFixture( StaticServers( self.basedir, [(serverid, announcement), From e0a31aebf58019fd457bced7e7cc1224b8a9bd7a Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 21 Jun 2019 08:38:57 -0400 Subject: [PATCH 10/10] Sort the static storage servers for deterministic tests --- src/allmydata/storage_client.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index c319d3fa9..ba9a02191 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -94,7 +94,11 @@ class StorageFarmBroker(service.MultiService): @log_call(action_type=u"storage-client:broker:set-static-servers") def set_static_servers(self, servers): - for (server_id, server) in servers.items(): + # Sorting the items gives us a deterministic processing order. This + # doesn't really matter but it makes the logging behavior more + # predictable and easier to test (and at least one test does depend on + # this sorted order). + for (server_id, server) in sorted(servers.items()): try: storage_server = self._make_storage_server(server_id, server) except Exception: