From 928e61bf224717aba58e4c340f82e0621f040609 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Wed, 18 May 2022 12:12:09 -0600 Subject: [PATCH 01/28] Log 'something' when we fail to instantiate a client --- newsfragments/3899.bugfix | 1 + src/allmydata/storage_client.py | 11 +++++++---- 2 files changed, 8 insertions(+), 4 deletions(-) create mode 100644 newsfragments/3899.bugfix diff --git a/newsfragments/3899.bugfix b/newsfragments/3899.bugfix new file mode 100644 index 000000000..a55239c38 --- /dev/null +++ b/newsfragments/3899.bugfix @@ -0,0 +1 @@ +Print a useful message when a storage-client cannot be matched to configuration diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 68164e697..9056b9e7a 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -667,6 +667,9 @@ def _storage_from_foolscap_plugin(node_config, config, announcement, get_rref): :param allmydata.node._Config node_config: The node configuration to pass to the plugin. + + :param dict announcement: The storage announcement for the storage + server we should build """ plugins = { plugin.name: plugin @@ -687,7 +690,8 @@ def _storage_from_foolscap_plugin(node_config, config, announcement, get_rref): option, get_rref, ) - raise AnnouncementNotMatched() + plugin_names = ", ".join(sorted(list(config.storage_plugins.keys()))) + raise AnnouncementNotMatched(plugin_names) @implementer(IServer) @@ -761,9 +765,8 @@ class NativeStorageServer(service.MultiService): # able to get the most up-to-date value. self.get_rref, ) - except AnnouncementNotMatched: - # Nope. - pass + except AnnouncementNotMatched as e: + print('No plugin for storage-server "{nickname}" from plugins: {plugins}'.format(nickname=ann.get("nickname", "<unknown>"), plugins=e.args[0])) else: return _FoolscapStorage.from_announcement( self._server_id, From 21112fd22bb780e8fb06478a1e5b43cbc41fecf1 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Wed, 18 May 2022 22:09:21 -0600 Subject: [PATCH 02/28] twisted new-logger, not print() --- src/allmydata/storage_client.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 9056b9e7a..3fc18a908 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -52,6 +52,7 @@ from zope.interface import ( ) from twisted.internet import defer from twisted.application import service +from twisted.logger import Logger from twisted.plugin import ( getPlugins, ) @@ -720,6 +721,7 @@ class NativeStorageServer(service.MultiService): }), "application-version": "unknown: no get_version()", }) + log = Logger() def __init__(self, server_id, ann, tub_maker, handler_overrides, node_config, config=StorageClientConfig()): service.MultiService.__init__(self) @@ -766,7 +768,11 @@ class NativeStorageServer(service.MultiService): self.get_rref, ) except AnnouncementNotMatched as e: - print('No plugin for storage-server "{nickname}" from plugins: {plugins}'.format(nickname=ann.get("nickname", "<unknown>"), plugins=e.args[0])) + self.log.error( + 'No plugin for storage-server "{nickname}" from plugins: {plugins}', + nickname=ann.get("nickname", "<unknown>"), + plugins=e.args[0], + ) else: return _FoolscapStorage.from_announcement( self._server_id, From 839aaea541a2d9504abe46a1b8ecb97e333f8971 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Wed, 8 Jun 2022 21:26:40 -0600 Subject: [PATCH 03/28] let misconfigured servers show up, and display information about missing plugins --- src/allmydata/storage_client.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 3fc18a908..a3974d4b9 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -661,6 +661,19 @@ class AnnouncementNotMatched(Exception): """ +@attr.s(auto_exc=True) +class MissingPlugin(Exception): + """ + A particular plugin was request, but is missing + """ + + plugin_name = attr.ib() + nickname = attr.ib() + + def __str__(self): + return "Missing plugin '{}' for server '{}'".format(self.plugin_name, self.nickname) + + def _storage_from_foolscap_plugin(node_config, config, announcement, get_rref): """ Construct an ``IStorageServer`` from the most locally-preferred plugin @@ -682,7 +695,7 @@ def _storage_from_foolscap_plugin(node_config, config, announcement, get_rref): try: plugin = plugins[plugin_name] except KeyError: - raise ValueError("{} not installed".format(plugin_name)) + raise MissingPlugin(plugin_name, announcement.get(u"nickname", "<unknown>")) for option in storage_options: if plugin_name == option[u"name"]: furl = option[u"storage-server-FURL"] @@ -773,6 +786,11 @@ class NativeStorageServer(service.MultiService): nickname=ann.get("nickname", "<unknown>"), plugins=e.args[0], ) + except MissingPlugin as e: + self.log.failure("Missing plugin") + ns = _NullStorage() + ns.longname = '<missing plugin "{}">'.format(e.args[0]) + return ns else: return _FoolscapStorage.from_announcement( self._server_id, From 6116b04ff7bd7a910651fb53f94b437f5595243f Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 10 Jun 2022 14:08:53 -0600 Subject: [PATCH 04/28] ignore incorrectly packaged autobahn versions --- setup.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/setup.py b/setup.py index c84d0ecde..b14893712 100644 --- a/setup.py +++ b/setup.py @@ -114,7 +114,9 @@ install_requires = [ "attrs >= 18.2.0", # WebSocket library for twisted and asyncio - "autobahn >= 19.5.2", + "autobahn >= 19.5.2, != 22.5.1, != 22.4.2, != 22.4.1" + # (the ignored versions above don't have autobahn.twisted.testing + # packaged properly) # Support for Python 3 transition "future >= 0.18.2", From c6fc82665c3fcf4dba9809be4405daba33f0d66c Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone <exarkun@twistedmatrix.com> Date: Tue, 29 Nov 2022 09:33:05 -0500 Subject: [PATCH 05/28] Pull `_make_storage_system` into a free function for easier testing --- src/allmydata/storage_client.py | 153 ++++++++++++++++++-------------- 1 file changed, 85 insertions(+), 68 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 410bfd28b..5dc4beb22 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -33,7 +33,7 @@ Ported to Python 3. from __future__ import annotations from six import ensure_text -from typing import Union +from typing import Union, Callable, Optional import re, time, hashlib from os import urandom from configparser import NoSectionError @@ -57,6 +57,7 @@ from twisted.plugin import ( from eliot import ( log_call, ) +from foolscap.ipb import IRemoteReference from foolscap.api import eventually, RemoteException from foolscap.reconnector import ( ReconnectionInfo, @@ -80,6 +81,9 @@ from allmydata.storage.http_client import ( ClientException as HTTPClientException, StorageClientMutables, ReadVector, TestWriteVectors, WriteVector, TestVector, ClientException ) +from .node import _Config + +_log = Logger() ANONYMOUS_STORAGE_NURLS = "anonymous-storage-NURLs" @@ -732,6 +736,85 @@ def _available_space_from_version(version): return available_space +def _make_storage_system( + node_config: _Config, + config: StorageClientConfig, + ann: dict, + server_id: bytes, + get_rref: Callable[[], Optional[IRemoteReference]], +) -> IFoolscapStorageServer: + """ + Create an object for interacting with the storage server described by + the given announcement. + + :param node_config: The node configuration to pass to any configured + storage plugins. + + :param config: Configuration specifying desired storage client behavior. + + :param ann: The storage announcement from the storage server we are meant + to communicate with. + + :param server_id: The unique identifier for the server. + + :param get_rref: A function which returns a remote reference to the + server-side object which implements this storage system, if one is + available (otherwise None). + + :return: An object enabling communication via Foolscap with the server + which generated the announcement. + """ + # Try to match the announcement against a plugin. + try: + furl, storage_server = _storage_from_foolscap_plugin( + node_config, + config, + ann, + # Pass in an accessor for our _rref attribute. The value of + # the attribute may change over time as connections are lost + # and re-established. The _StorageServer should always be + # able to get the most up-to-date value. + get_rref, + ) + except AnnouncementNotMatched as e: + _log.error( + 'No plugin for storage-server "{nickname}" from plugins: {plugins}', + nickname=ann.get("nickname", "<unknown>"), + plugins=e.args[0], + ) + except MissingPlugin as e: + _log.failure("Missing plugin") + ns = _NullStorage() + ns.longname = '<missing plugin "{}">'.format(e.args[0]) + return ns + else: + return _FoolscapStorage.from_announcement( + server_id, + furl, + ann, + storage_server, + ) + + # Try to match the announcement against the anonymous access scheme. + try: + furl = ann[u"anonymous-storage-FURL"] + except KeyError: + # Nope + pass + else: + # See comment above for the _storage_from_foolscap_plugin case + # about passing in get_rref. + storage_server = _StorageServer(get_rref=get_rref) + return _FoolscapStorage.from_announcement( + server_id, + furl, + ann, + storage_server, + ) + + # Nothing matched so we can't talk to this server. + return _null_storage + @implementer(IServer) class NativeStorageServer(service.MultiService): """I hold information about a storage server that we want to connect to. @@ -758,7 +841,6 @@ class NativeStorageServer(service.MultiService): }), "application-version": "unknown: no get_version()", }) - log = Logger() def __init__(self, server_id, ann, tub_maker, handler_overrides, node_config, config=StorageClientConfig()): service.MultiService.__init__(self) @@ -768,7 +850,7 @@ class NativeStorageServer(service.MultiService): self._tub_maker = tub_maker self._handler_overrides = handler_overrides - self._storage = self._make_storage_system(node_config, config, ann) + self._storage = _make_storage_system(node_config, config, ann, self._server_id, self.get_rref) self.last_connect_time = None self.last_loss_time = None @@ -778,71 +860,6 @@ class NativeStorageServer(service.MultiService): self._trigger_cb = None self._on_status_changed = ObserverList() - def _make_storage_system(self, node_config, config, ann): - """ - :param allmydata.node._Config node_config: The node configuration to pass - to any configured storage plugins. - - :param StorageClientConfig config: Configuration specifying desired - storage client behavior. - - :param dict ann: The storage announcement from the storage server we - are meant to communicate with. - - :return IFoolscapStorageServer: An object enabling communication via - Foolscap with the server which generated the announcement. - """ - # Try to match the announcement against a plugin. - try: - furl, storage_server = _storage_from_foolscap_plugin( - node_config, - config, - ann, - # Pass in an accessor for our _rref attribute. The value of - # the attribute may change over time as connections are lost - # and re-established. The _StorageServer should always be - # able to get the most up-to-date value. - self.get_rref, - ) - except AnnouncementNotMatched as e: - self.log.error( - 'No plugin for storage-server "{nickname}" from plugins: {plugins}', - nickname=ann.get("nickname", "<unknown>"), - plugins=e.args[0], - ) - except MissingPlugin as e: - self.log.failure("Missing plugin") - ns = _NullStorage() - ns.longname = '<missing plugin "{}">'.format(e.args[0]) - return ns - else: - return _FoolscapStorage.from_announcement( - self._server_id, - furl, - ann, - storage_server, - ) - - # Try to match the announcement against the anonymous access scheme. - try: - furl = ann[u"anonymous-storage-FURL"] - except KeyError: - # Nope - pass - else: - # See comment above for the _storage_from_foolscap_plugin case - # about passing in get_rref. - storage_server = _StorageServer(get_rref=self.get_rref) - return _FoolscapStorage.from_announcement( - self._server_id, - furl, - ann, - storage_server, - ) - - # Nothing matched so we can't talk to this server. - return _null_storage - def get_permutation_seed(self): return self._storage.permutation_seed def get_name(self): # keep methodname short From c4c9d1389ef10236227d5a58927cebbb0907c3a1 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone <exarkun@twistedmatrix.com> Date: Tue, 29 Nov 2022 09:47:10 -0500 Subject: [PATCH 06/28] Try (but fail) to demonstrate the longname behavior --- setup.py | 2 +- src/allmydata/storage_client.py | 8 ++++---- src/allmydata/test/test_storage_client.py | 20 +++++++++++++++++--- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/setup.py b/setup.py index 78f7042a9..480cb0d88 100644 --- a/setup.py +++ b/setup.py @@ -111,7 +111,7 @@ install_requires = [ "pyrsistent", # A great way to define types of values. - "attrs >= 18.2.0", + "attrs >= 20.1.0", # WebSocket library for twisted and asyncio "autobahn >= 22.4.3", diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 5dc4beb22..2ecd3bc0c 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -39,6 +39,7 @@ from os import urandom from configparser import NoSectionError import attr +from attr import define from hyperlink import DecodedURL from zope.interface import ( Attribute, @@ -637,6 +638,7 @@ class _FoolscapStorage(object): @implementer(IFoolscapStorageServer) +@define class _NullStorage(object): """ Abstraction for *not* communicating with a storage server of a type with @@ -650,7 +652,7 @@ class _NullStorage(object): lease_seed = hashlib.sha256(b"").digest() name = "<unsupported>" - longname = "<storage with unsupported protocol>" + longname: str = "<storage with unsupported protocol>" def connect_to(self, tub, got_connection): return NonReconnector() @@ -784,9 +786,7 @@ def _make_storage_system( ) except MissingPlugin as e: _log.failure("Missing plugin") - ns = _NullStorage() - ns.longname = '<missing plugin "{}">'.format(e.args[0]) - return ns + return _NullStorage('<missing plugin "{}">'.format(e.args[0])) else: return _FoolscapStorage.from_announcement( server_id, diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py index 1a84f35ec..0c05be2e6 100644 --- a/src/allmydata/test/test_storage_client.py +++ b/src/allmydata/test/test_storage_client.py @@ -159,7 +159,7 @@ class GetConnectionStatus(unittest.TestCase): self.assertTrue(IConnectionStatus.providedBy(connection_status)) -class UnrecognizedAnnouncement(unittest.TestCase): +class UnrecognizedAnnouncement(SyncTestCase): """ Tests for handling of announcements that aren't recognized and don't use *anonymous-storage-FURL*. @@ -169,9 +169,14 @@ class UnrecognizedAnnouncement(unittest.TestCase): an announcement generated by a storage server plugin which is not loaded in the client. """ + plugin_name = u"tahoe-lafs-testing-v1" ann = { - u"name": u"tahoe-lafs-testing-v1", - u"any-parameter": 12345, + u"storage-options": [ + { + u"name": plugin_name, + u"any-parameter": 12345, + }, + ], } server_id = b"abc" @@ -234,6 +239,15 @@ class UnrecognizedAnnouncement(unittest.TestCase): server.get_foolscap_write_enabler_seed() server.get_nickname() + def test_longname(self) -> None: + """ + ``NativeStorageServer.get_longname`` describes the missing plugin. + """ + server = self.native_storage_server() + self.assertThat( + server.get_longname(), + Equals('<missing plugin "{}">'.format(self.plugin_name)), + ) class PluginMatchedAnnouncement(SyncTestCase): From 347d11a83c3cb184a1f77cc1060f613a01cdb13f Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 2 Dec 2022 01:27:13 -0700 Subject: [PATCH 07/28] fix test, un-log error --- src/allmydata/test/test_storage_client.py | 25 ++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py index 0c05be2e6..04f2c7e29 100644 --- a/src/allmydata/test/test_storage_client.py +++ b/src/allmydata/test/test_storage_client.py @@ -77,6 +77,7 @@ from .common import ( UseNode, SameProcessStreamEndpointAssigner, MemoryIntroducerClient, + flush_logged_errors, ) from .common_web import ( do_http, @@ -92,6 +93,8 @@ from allmydata.storage_client import ( IFoolscapStorageServer, NativeStorageServer, StorageFarmBroker, + StorageClientConfig, + MissingPlugin, _FoolscapStorage, _NullStorage, ) @@ -159,7 +162,7 @@ class GetConnectionStatus(unittest.TestCase): self.assertTrue(IConnectionStatus.providedBy(connection_status)) -class UnrecognizedAnnouncement(SyncTestCase): +class UnrecognizedAnnouncement(unittest.TestCase): """ Tests for handling of announcements that aren't recognized and don't use *anonymous-storage-FURL*. @@ -183,7 +186,7 @@ class UnrecognizedAnnouncement(SyncTestCase): def _tub_maker(self, overrides): return Service() - def native_storage_server(self): + def native_storage_server(self, config=None): """ Make a ``NativeStorageServer`` out of an unrecognizable announcement. """ @@ -192,7 +195,8 @@ class UnrecognizedAnnouncement(SyncTestCase): self.ann, self._tub_maker, {}, - EMPTY_CLIENT_CONFIG, + node_config=EMPTY_CLIENT_CONFIG, + config=config or StorageClientConfig(), ) def test_no_exceptions(self): @@ -243,11 +247,18 @@ class UnrecognizedAnnouncement(SyncTestCase): """ ``NativeStorageServer.get_longname`` describes the missing plugin. """ - server = self.native_storage_server() - self.assertThat( - server.get_longname(), - Equals('<missing plugin "{}">'.format(self.plugin_name)), + server = self.native_storage_server( + StorageClientConfig( + storage_plugins={ + "nothing": {} + } + ) ) + self.assertEqual( + server.get_longname(), + '<missing plugin "nothing">', + ) + self.flushLoggedErrors(MissingPlugin) class PluginMatchedAnnouncement(SyncTestCase): From 22a7aacb9da710d8112d5f40a9bf5f7e82b2e138 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 2 Dec 2022 10:36:41 -0700 Subject: [PATCH 08/28] flake8 --- src/allmydata/test/test_storage_client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py index 04f2c7e29..a92e6723f 100644 --- a/src/allmydata/test/test_storage_client.py +++ b/src/allmydata/test/test_storage_client.py @@ -77,7 +77,6 @@ from .common import ( UseNode, SameProcessStreamEndpointAssigner, MemoryIntroducerClient, - flush_logged_errors, ) from .common_web import ( do_http, From 295e816d4ee2cfc27130f96994e0742849390009 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Wed, 9 Aug 2023 21:49:37 -0600 Subject: [PATCH 09/28] spell --- src/allmydata/storage_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 1f6b41b1c..9739091dc 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -761,7 +761,7 @@ class AnnouncementNotMatched(Exception): @attr.s(auto_exc=True) class MissingPlugin(Exception): """ - A particular plugin was request, but is missing + A particular plugin was requested but is missing """ plugin_name = attr.ib() From cf4fe0061cdddd254a850efc1f2949f0b49447f9 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Wed, 9 Aug 2023 22:27:55 -0600 Subject: [PATCH 10/28] refactor where plugins are loaded; use this to error early for users --- src/allmydata/client.py | 5 +++ src/allmydata/node.py | 2 + src/allmydata/scripts/tahoe_run.py | 14 +++++++ src/allmydata/storage_client.py | 67 +++++++++++++++++++++--------- 4 files changed, 69 insertions(+), 19 deletions(-) diff --git a/src/allmydata/client.py b/src/allmydata/client.py index aff2d5815..cfc0977a1 100644 --- a/src/allmydata/client.py +++ b/src/allmydata/client.py @@ -483,6 +483,11 @@ def create_storage_farm_broker(config: _Config, default_connection_handlers, foo storage_client_config = storage_client.StorageClientConfig.from_node_config( config, ) + # ensure that we can at least load all plugins that the + # configuration mentions; doing this early (i.e. before creating + # storage-clients themselves) allows us to exit in case of a + # problem. + storage_client_config.get_configured_storage_plugins() def tub_creator(handler_overrides=None, **kwargs): return node.create_tub( diff --git a/src/allmydata/node.py b/src/allmydata/node.py index 6c3082b50..5b06cb963 100644 --- a/src/allmydata/node.py +++ b/src/allmydata/node.py @@ -30,10 +30,12 @@ from twisted.python.filepath import ( from twisted.python import log as twlog from twisted.application import service from twisted.python.failure import Failure +from twisted.plugin import getPlugins from foolscap.api import Tub import foolscap.logging.log +from allmydata.interfaces import IFoolscapStoragePlugin from allmydata.util import log from allmydata.util import fileutil, iputil from allmydata.util.fileutil import abspath_expanduser_unicode diff --git a/src/allmydata/scripts/tahoe_run.py b/src/allmydata/scripts/tahoe_run.py index ff3ff9efd..eba5ae329 100644 --- a/src/allmydata/scripts/tahoe_run.py +++ b/src/allmydata/scripts/tahoe_run.py @@ -42,6 +42,9 @@ from allmydata.util.pid import ( from allmydata.storage.crawler import ( MigratePickleFileError, ) +from allmydata.storage_client import ( + MissingPlugin, +) from allmydata.node import ( PortAssignmentRequired, PrivacyError, @@ -197,6 +200,17 @@ class DaemonizeTheRealService(Service, HookMixin): self.basedir, ) ) + elif reason.check(MissingPlugin): + self.stderr.write( + "Missing Plugin\n" + "The configuration requests a plugin:\n" + "\n {}\n\n" + "...which cannot be found.\n" + "This typically means that some software hasn't been installed or the plugin couldn't be instantiated.\n\n" + .format( + reason.value.plugin_name, + ) + ) else: self.stderr.write("\nUnknown error, here's the traceback:\n") reason.printTraceback(self.stderr) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 9739091dc..24abe2a18 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -187,6 +187,30 @@ class StorageClientConfig(object): grid_manager_keys, ) + def get_configured_storage_plugins(self): + """ + :returns Dict[str, IFoolscapStoragePlugin]: a dict mapping names + to instances for all available plugins + + :raises MissingPlugin: if the configuration asks for a plugin + for which there is no corresponding instance (e.g. it is + not installed). + """ + plugins = { + plugin.name: plugin + for plugin + in getPlugins(IFoolscapStoragePlugin) + } + + configured = dict() + for plugin_name in self.storage_plugins: + try: + plugin = plugins[plugin_name] + except KeyError: + raise MissingPlugin(plugin_name) + configured[plugin_name] = plugin + return configured + @implementer(IStorageBroker) class StorageFarmBroker(service.MultiService): @@ -765,10 +789,9 @@ class MissingPlugin(Exception): """ plugin_name = attr.ib() - nickname = attr.ib() def __str__(self): - return "Missing plugin '{}' for server '{}'".format(self.plugin_name, self.nickname) + return "Missing plugin '{}'".format(self.plugin_name) def _storage_from_foolscap_plugin(node_config, config, announcement, get_rref): @@ -782,26 +805,32 @@ def _storage_from_foolscap_plugin(node_config, config, announcement, get_rref): :param dict announcement: The storage announcement for the storage server we should build """ - plugins = { - plugin.name: plugin - for plugin - in getPlugins(IFoolscapStoragePlugin) - } storage_options = announcement.get(u"storage-options", []) - for plugin_name, plugin_config in list(config.storage_plugins.items()): + plugins = config.get_configured_storage_plugins() + + # for every storage-option that we have enabled locally (in order + # of preference), see if the announcement asks for such a thing. + # if it does, great: we return that storage-client + # otherwise we've run out of options... + + for options in storage_options: try: - plugin = plugins[plugin_name] + plugin = plugins[options[u"name"]] except KeyError: - raise MissingPlugin(plugin_name, announcement.get(u"nickname", "<unknown>")) - for option in storage_options: - if plugin_name == option[u"name"]: - furl = option[u"storage-server-FURL"] - return furl, plugin.get_storage_client( - node_config, - option, - get_rref, - ) - plugin_names = ", ".join(sorted(list(config.storage_plugins.keys()))) + # we didn't configure this kind of plugin locally, so + # consider the next announced option + continue + + furl = options[u"storage-server-FURL"] + return furl, plugin.get_storage_client( + node_config, + options, + get_rref, + ) + + # none of the storage options in the announcement are configured + # locally; we can't make a storage-client. + plugin_names = ", ".join(sorted(plugins)) raise AnnouncementNotMatched(plugin_names) From d7cfb5dde9d11b52e8525d5666fbac355ba8eb1b Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Wed, 9 Aug 2023 23:21:28 -0600 Subject: [PATCH 11/28] show WebUI feedback when announcement-match fails --- src/allmydata/storage_client.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 24abe2a18..1b7b92acb 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -830,7 +830,7 @@ def _storage_from_foolscap_plugin(node_config, config, announcement, get_rref): # none of the storage options in the announcement are configured # locally; we can't make a storage-client. - plugin_names = ", ".join(sorted(plugins)) + plugin_names = ", ".join(sorted(option["name"] for option in storage_options)) raise AnnouncementNotMatched(plugin_names) @@ -872,6 +872,7 @@ def _make_storage_system( :return: An object enabling communication via Foolscap with the server which generated the announcement. """ + unmatched = None # Try to match the announcement against a plugin. try: furl, storage_server = _storage_from_foolscap_plugin( @@ -885,14 +886,10 @@ def _make_storage_system( get_rref, ) except AnnouncementNotMatched as e: - _log.error( - 'No plugin for storage-server "{nickname}" from plugins: {plugins}', - nickname=ann.get("nickname", "<unknown>"), - plugins=e.args[0], - ) - except MissingPlugin as e: - _log.failure("Missing plugin") - return _NullStorage('<missing plugin "{}">'.format(e.args[0])) + # show a more-specific error to the user for this server + # (Note this will only be shown if the server _doesn't_ offer + # anonymous service, which will match below) + unmatched = _NullStorage('{}: missing plugin "{}"'.format(server_id.decode("utf8"), str(e))) else: return _FoolscapStorage.from_announcement( server_id, @@ -918,8 +915,10 @@ def _make_storage_system( storage_server, ) - # Nothing matched so we can't talk to this server. - return _null_storage + # Nothing matched so we can't talk to this server. If we have a + # specific reason in "unmatched", use it; otherwise the generic + # one + return unmatched or _null_storage @implementer(IServer) class NativeStorageServer(service.MultiService): From 09ea172b940c607a990e6cd5d4bbb9f98075795e Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Thu, 10 Aug 2023 12:06:29 -0600 Subject: [PATCH 13/28] reformat multiline strings; don't output "storage.plugins = None" --- src/allmydata/test/common.py | 42 ++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/allmydata/test/common.py b/src/allmydata/test/common.py index d61bc28f1..744c17efa 100644 --- a/src/allmydata/test/common.py +++ b/src/allmydata/test/common.py @@ -307,13 +307,18 @@ class UseNode(object): if self.plugin_config is None: plugin_config_section = "" else: - plugin_config_section = """ -[storageclient.plugins.{storage_plugin}] -{config} -""".format( - storage_plugin=self.storage_plugin, - config=format_config_items(self.plugin_config), -) + plugin_config_section = + "[storageclient.plugins.{storage_plugin}]\n" + "{config}\n" + .format( + storage_plugin=self.storage_plugin, + config=format_config_items(self.plugin_config), + ) + + if self.storage_plugin is None: + plugins = "" + else: + plugins = "storage.plugins = {}".format(self.storage_plugin) write_introducer( self.basedir, @@ -340,18 +345,17 @@ class UseNode(object): self.config = config_from_string( self.basedir.asTextMode().path, "tub.port", -""" -[node] -{node_config} - -[client] -storage.plugins = {storage_plugin} -{plugin_config_section} -""".format( - storage_plugin=self.storage_plugin, - node_config=format_config_items(node_config), - plugin_config_section=plugin_config_section, -) + "[node]\n" + "{node_config}\n" + "\n" + "[client]\n" + "{plugins}\n" + "{plugin_config_section}\n" + .format( + plugins=plugins, + node_config=format_config_items(node_config), + plugin_config_section=plugin_config_section, + ) ) def create_node(self): From b07d9e90cbcd557821a75a1fd7571e2c169dee73 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Thu, 10 Aug 2023 12:07:03 -0600 Subject: [PATCH 14/28] correct test --- src/allmydata/test/test_storage_client.py | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py index e3b192a96..d719f227b 100644 --- a/src/allmydata/test/test_storage_client.py +++ b/src/allmydata/test/test_storage_client.py @@ -243,22 +243,18 @@ class UnrecognizedAnnouncement(unittest.TestCase): server.get_foolscap_write_enabler_seed() server.get_nickname() - def test_longname(self) -> None: + def test_missing_plugin(self) -> None: """ - ``NativeStorageServer.get_longname`` describes the missing plugin. + An exception is produced if the plugin is missing """ - server = self.native_storage_server( - StorageClientConfig( - storage_plugins={ - "nothing": {} - } + with self.assertRaises(MissingPlugin): + _ = self.native_storage_server( + StorageClientConfig( + storage_plugins={ + "nothing": {} + } + ) ) - ) - self.assertEqual( - server.get_longname(), - '<missing plugin "nothing">', - ) - self.flushLoggedErrors(MissingPlugin) class PluginMatchedAnnouncement(SyncTestCase): From e3e5b4bc8d5deacad91a3c25243b9f71eec3a63d Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Thu, 10 Aug 2023 12:19:11 -0600 Subject: [PATCH 15/28] typo --- src/allmydata/test/common.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/allmydata/test/common.py b/src/allmydata/test/common.py index 744c17efa..1186bd540 100644 --- a/src/allmydata/test/common.py +++ b/src/allmydata/test/common.py @@ -307,13 +307,12 @@ class UseNode(object): if self.plugin_config is None: plugin_config_section = "" else: - plugin_config_section = - "[storageclient.plugins.{storage_plugin}]\n" - "{config}\n" - .format( - storage_plugin=self.storage_plugin, - config=format_config_items(self.plugin_config), - ) + plugin_config_section = ( + "[storageclient.plugins.{storage_plugin}]\n" + "{config}\n").format( + storage_plugin=self.storage_plugin, + config=format_config_items(self.plugin_config), + ) if self.storage_plugin is None: plugins = "" From 60e873bbe48f94af3079a1a60e0d5159b73e4c87 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Thu, 10 Aug 2023 13:22:35 -0600 Subject: [PATCH 16/28] unused --- src/allmydata/node.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/allmydata/node.py b/src/allmydata/node.py index 5b06cb963..6c3082b50 100644 --- a/src/allmydata/node.py +++ b/src/allmydata/node.py @@ -30,12 +30,10 @@ from twisted.python.filepath import ( from twisted.python import log as twlog from twisted.application import service from twisted.python.failure import Failure -from twisted.plugin import getPlugins from foolscap.api import Tub import foolscap.logging.log -from allmydata.interfaces import IFoolscapStoragePlugin from allmydata.util import log from allmydata.util import fileutil, iputil from allmydata.util.fileutil import abspath_expanduser_unicode From 7322d8c0e60ecd33da155d7055c8917fc34aa83a Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Thu, 10 Aug 2023 14:28:55 -0600 Subject: [PATCH 17/28] better news --- newsfragments/3899.bugfix | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/newsfragments/3899.bugfix b/newsfragments/3899.bugfix index a55239c38..55d4fabd4 100644 --- a/newsfragments/3899.bugfix +++ b/newsfragments/3899.bugfix @@ -1 +1,4 @@ -Print a useful message when a storage-client cannot be matched to configuration +Provide better feedback from plugin configuration errors + +Local errors now print a useful message and exit. +Announcements that only contain invalid / unusable plugins now show a message in the Welcome page. From f51d49faa54c0fff3b2146bb6630e83da53484c5 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 11 Aug 2023 09:03:30 -0600 Subject: [PATCH 18/28] typing Co-authored-by: Jean-Paul Calderone <exarkun@twistedmatrix.com> --- src/allmydata/storage_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 1b7b92acb..7040ecd16 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -187,7 +187,7 @@ class StorageClientConfig(object): grid_manager_keys, ) - def get_configured_storage_plugins(self): + def get_configured_storage_plugins(self) -> dict[str, IFoolscapStoragePlugin]: """ :returns Dict[str, IFoolscapStoragePlugin]: a dict mapping names to instances for all available plugins From d81b64ba9e2dd8aa84a2812f702ef55cd1698f52 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 11 Aug 2023 09:05:16 -0600 Subject: [PATCH 19/28] docstring Co-authored-by: Jean-Paul Calderone <exarkun@twistedmatrix.com> --- src/allmydata/storage_client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 7040ecd16..a6a5336c6 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -189,8 +189,8 @@ class StorageClientConfig(object): def get_configured_storage_plugins(self) -> dict[str, IFoolscapStoragePlugin]: """ - :returns Dict[str, IFoolscapStoragePlugin]: a dict mapping names - to instances for all available plugins + :returns: a mapping from names to instances for all available + plugins :raises MissingPlugin: if the configuration asks for a plugin for which there is no corresponding instance (e.g. it is From c03076fe213382af5e754724f072fc50f9b61f49 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 11 Aug 2023 09:07:00 -0600 Subject: [PATCH 20/28] more robust comparison Co-authored-by: Jean-Paul Calderone <exarkun@twistedmatrix.com> --- src/allmydata/test/test_storage_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py index d719f227b..328e90499 100644 --- a/src/allmydata/test/test_storage_client.py +++ b/src/allmydata/test/test_storage_client.py @@ -196,7 +196,7 @@ class UnrecognizedAnnouncement(unittest.TestCase): self._tub_maker, {}, node_config=EMPTY_CLIENT_CONFIG, - config=config or StorageClientConfig(), + config=config if config is not None else StorageClientConfig(), ) def test_no_exceptions(self): From a0769f59dce7b3d70f2e4833b0e4405d8ad8e472 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 11 Aug 2023 09:07:18 -0600 Subject: [PATCH 21/28] naming Co-authored-by: Jean-Paul Calderone <exarkun@twistedmatrix.com> --- src/allmydata/test/test_storage_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py index 328e90499..5b2f80712 100644 --- a/src/allmydata/test/test_storage_client.py +++ b/src/allmydata/test/test_storage_client.py @@ -251,7 +251,7 @@ class UnrecognizedAnnouncement(unittest.TestCase): _ = self.native_storage_server( StorageClientConfig( storage_plugins={ - "nothing": {} + "missing-plugin-name": {} } ) ) From 2e76d554e2a1b6ecd090eedce64645a84e890710 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 11 Aug 2023 09:08:03 -0600 Subject: [PATCH 22/28] don't explicitly drop return Co-authored-by: Jean-Paul Calderone <exarkun@twistedmatrix.com> --- src/allmydata/test/test_storage_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py index 5b2f80712..f8db402d0 100644 --- a/src/allmydata/test/test_storage_client.py +++ b/src/allmydata/test/test_storage_client.py @@ -248,7 +248,7 @@ class UnrecognizedAnnouncement(unittest.TestCase): An exception is produced if the plugin is missing """ with self.assertRaises(MissingPlugin): - _ = self.native_storage_server( + self.native_storage_server( StorageClientConfig( storage_plugins={ "missing-plugin-name": {} From 375ee54c80bff6cb4327eec54e753a086055c4e5 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 11 Aug 2023 09:08:19 -0600 Subject: [PATCH 23/28] typing Co-authored-by: Jean-Paul Calderone <exarkun@twistedmatrix.com> --- src/allmydata/test/test_storage_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py index f8db402d0..97ce9fe68 100644 --- a/src/allmydata/test/test_storage_client.py +++ b/src/allmydata/test/test_storage_client.py @@ -186,7 +186,7 @@ class UnrecognizedAnnouncement(unittest.TestCase): def _tub_maker(self, overrides): return Service() - def native_storage_server(self, config=None): + def native_storage_server(self, config: Optional[StorageClientConfig] = None) -> NativeStorageServer: """ Make a ``NativeStorageServer`` out of an unrecognizable announcement. """ From c27b330984afdfc612f90707ccc1ffc2e2473042 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 11 Aug 2023 19:18:23 -0600 Subject: [PATCH 24/28] don't need fallback --- src/allmydata/storage_client.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 1b7b92acb..2a3b1dbad 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -772,8 +772,6 @@ class NonReconnector(object): def getReconnectionInfo(self): return ReconnectionInfo() -_null_storage = _NullStorage() - class AnnouncementNotMatched(Exception): """ @@ -915,10 +913,11 @@ def _make_storage_system( storage_server, ) - # Nothing matched so we can't talk to this server. If we have a - # specific reason in "unmatched", use it; otherwise the generic - # one - return unmatched or _null_storage + # Nothing matched so we can't talk to this server. (There should + # not be a way to get here without this local being valid) + assert unmatched is not None, "Expected unmatched plugin error" + return unmatched + @implementer(IServer) class NativeStorageServer(service.MultiService): From ffa589d6f827476ff7c7b98a7db52f34be4cf996 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 11 Aug 2023 21:19:02 -0600 Subject: [PATCH 25/28] import error --- src/allmydata/test/test_storage_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py index 97ce9fe68..604884eba 100644 --- a/src/allmydata/test/test_storage_client.py +++ b/src/allmydata/test/test_storage_client.py @@ -8,7 +8,7 @@ from json import ( loads, ) import hashlib -from typing import Union, Any +from typing import Union, Any, Optional from hyperlink import DecodedURL from fixtures import ( From 356a1d0f792ae2c5ea65105f7e9ffb0eb1321aa0 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 11 Aug 2023 22:01:21 -0600 Subject: [PATCH 26/28] don't know why dict_keys are so confusing to mypy --- src/allmydata/storage_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index c95d72dbf..75e717037 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -209,7 +209,7 @@ class StorageClientConfig(object): except KeyError: raise MissingPlugin(plugin_name) configured[plugin_name] = plugin - return configured + return configured # type: ignore @implementer(IStorageBroker) From a5b95273d7b3b420be6bc57ec9c4cd56897425d5 Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Fri, 11 Aug 2023 23:47:24 -0600 Subject: [PATCH 27/28] typing is .. good? --- src/allmydata/storage_client.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 75e717037..d35cd788b 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -33,7 +33,7 @@ Ported to Python 3. from __future__ import annotations from six import ensure_text -from typing import Union, Callable, Any, Optional, cast +from typing import Union, Callable, Any, Optional, cast, Dict from os import urandom import re import time @@ -202,14 +202,15 @@ class StorageClientConfig(object): in getPlugins(IFoolscapStoragePlugin) } - configured = dict() + # mypy doesn't like "str" in place of Any ... + configured: Dict[Any, IFoolscapStoragePlugin] = dict() for plugin_name in self.storage_plugins: try: plugin = plugins[plugin_name] except KeyError: raise MissingPlugin(plugin_name) configured[plugin_name] = plugin - return configured # type: ignore + return configured @implementer(IStorageBroker) From ad44958f0223c92b0133b4b65325ae540a54dd8a Mon Sep 17 00:00:00 2001 From: meejah <meejah@meejah.ca> Date: Sat, 12 Aug 2023 00:35:49 -0600 Subject: [PATCH 28/28] more kinds of whitespace --- src/allmydata/test/cli/test_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/cli/test_run.py b/src/allmydata/test/cli/test_run.py index 2adcfea19..ae0f92131 100644 --- a/src/allmydata/test/cli/test_run.py +++ b/src/allmydata/test/cli/test_run.py @@ -264,7 +264,7 @@ class RunTests(SyncTestCase): self.assertThat(runs, Equals([])) self.assertThat(result_code, Equals(1)) - good_file_content_re = re.compile(r"\w[0-9]*\w[0-9]*\w") + good_file_content_re = re.compile(r"\s[0-9]*\s[0-9]*\s", re.M) @given(text()) def test_pidfile_contents(self, content):