From ad68b154d4cbecdbc2e96e58d9d3c4153e014eeb Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 24 Mar 2017 16:30:15 +0100 Subject: [PATCH 1/6] Make ToolConfWatcher watch ToolCache If any item in the tool cache has changed (local tool update, repository update, tool deleted) the toolbox will be reloaded and any tool changes are applied. Should fix https://github.com/galaxyproject/galaxy/issues/3813. --- lib/galaxy/tools/toolbox/base.py | 2 +- lib/galaxy/tools/toolbox/cache.py | 25 +++++++++++++++++++++++-- lib/galaxy/tools/toolbox/watcher.py | 13 +++++++++---- test/unit/tools/test_toolbox.py | 1 + 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/tools/toolbox/base.py b/lib/galaxy/tools/toolbox/base.py index 402e490bb47..b432f3df69c 100644 --- a/lib/galaxy/tools/toolbox/base.py +++ b/lib/galaxy/tools/toolbox/base.py @@ -79,7 +79,7 @@ class AbstractToolBox( Dictifiable, ManagesIntegratedToolPanelMixin, object ): if tool_conf_watcher: self._tool_conf_watcher = tool_conf_watcher # Avoids (re-)starting threads in uwsgi else: - self._tool_conf_watcher = get_tool_conf_watcher(lambda: self.handle_reload_toolbox()) + self._tool_conf_watcher = get_tool_conf_watcher(reload_callback=lambda: self.handle_reload_toolbox(), tool_cache=self.app.tool_cache) self._filter_factory = FilterFactory( self ) self._tool_tag_manager = tool_tag_manager( app ) self._init_tools_from_configs( config_filenames ) diff --git a/lib/galaxy/tools/toolbox/cache.py b/lib/galaxy/tools/toolbox/cache.py index ee576d1d865..e1036f1bec7 100644 --- a/lib/galaxy/tools/toolbox/cache.py +++ b/lib/galaxy/tools/toolbox/cache.py @@ -1,4 +1,5 @@ import os +import time from galaxy.util.hash_util import md5_hash_file @@ -13,16 +14,34 @@ class ToolCache(object): self._hash_by_tool_paths = {} self._tools_by_path = {} self._tool_paths_by_id = {} + self._mod_time_by_path = {} def cleanup(self): - """Remove uninstalled tools from tool cache if they are not on disk anymore or if their content has changed.""" - paths_to_cleanup = {path: tool.all_ids for path, tool in self._tools_by_path.items() if not os.path.exists(path) or md5_hash_file(path) != self._hash_by_tool_paths[path]} + """ + Remove uninstalled tools from tool cache if they are not on disk anymore or if their content has changed. + + Returns list of tool_ids that have been removed. + """ + paths_to_cleanup = {path: tool.all_ids for path, tool in self._tools_by_path.items() if self._should_cleanup(path)} + removed_tool_ids = [] for config_filename, tool_ids in paths_to_cleanup.items(): + removed_tool_ids.extend(tool_ids) del self._hash_by_tool_paths[config_filename] del self._tools_by_path[config_filename] for tool_id in tool_ids: if tool_id in self._tool_paths_by_id: del self._tool_paths_by_id[tool_id] + return removed_tool_ids + + def _should_cleanup(self, config_filename): + """Return True of `config_filename` does not exist or if modtime and hash have changes, else return False.""" + if not os.path.exists(config_filename): + return True + new_mtime = time.ctime(os.path.getmtime(config_filename)) + if self._mod_time_by_path[config_filename] != new_mtime: + if md5_hash_file(config_filename) != self._hash_by_tool_paths[config_filename]: + return True + return False def get_tool(self, config_filename): """ Get the tool from the cache if the tool is up to date. @@ -35,10 +54,12 @@ class ToolCache(object): del self._hash_by_tool_paths[config_filename] del self._tool_paths_by_id[tool_id] del self._tools_by_path[config_filename] + del self._mod_time_by_path[config_filename] def cache_tool(self, config_filename, tool): tool_hash = md5_hash_file(config_filename) tool_id = str( tool.id ) self._hash_by_tool_paths[config_filename] = tool_hash + self._mod_time_by_path[config_filename] = time.ctime(os.path.getmtime(config_filename)) self._tool_paths_by_id[tool_id] = config_filename self._tools_by_path[config_filename] = tool diff --git a/lib/galaxy/tools/toolbox/watcher.py b/lib/galaxy/tools/toolbox/watcher.py index 862b30017ee..cbd9fa75a4f 100644 --- a/lib/galaxy/tools/toolbox/watcher.py +++ b/lib/galaxy/tools/toolbox/watcher.py @@ -48,8 +48,8 @@ def get_observer_class(config_value, default, monitor_what_str): return observer_class -def get_tool_conf_watcher(reload_callback): - return ToolConfWatcher(reload_callback) +def get_tool_conf_watcher(reload_callback, tool_cache=None): + return ToolConfWatcher(reload_callback=reload_callback, tool_cache=tool_cache) def get_tool_data_dir_watcher(tool_data_tables, config): @@ -73,8 +73,9 @@ def get_tool_watcher(toolbox, config): class ToolConfWatcher(object): - def __init__(self, reload_callback): + def __init__(self, reload_callback, tool_cache=None): self.paths = {} + self.cache = tool_cache self._active = False self._lock = threading.Lock() self.thread = threading.Thread(target=self.check, name="ToolConfWatcher.thread") @@ -92,6 +93,7 @@ class ToolConfWatcher(object): self.thread.join() def check(self): + """Check for changes in self.paths or self.cache and call the event handler.""" hashes = { key: None for key in self.paths.keys() } while self._active: do_reload = False @@ -113,7 +115,10 @@ class ToolConfWatcher(object): hashes[path] = new_hash log.debug("The file '%s' has changes.", path) do_reload = True - + if not do_reload and self.cache: + removed_ids = self.cache.cleanup() + if removed_ids: + do_reload = True if do_reload: with self._lock: t = threading.Thread(target=self.event_handler.on_any_event) diff --git a/test/unit/tools/test_toolbox.py b/test/unit/tools/test_toolbox.py index 6e9a2729fea..a3856640aa1 100644 --- a/test/unit/tools/test_toolbox.py +++ b/test/unit/tools/test_toolbox.py @@ -57,6 +57,7 @@ class BaseToolBoxTestCase( unittest.TestCase, tools_support.UsesApp, tools_supp self.reindexed = False self.setup_app( mock_model=False ) install_model = mapping.init( "sqlite:///:memory:", create_tables=True ) + self.app.tool_cache = None self.app.install_model = install_model self.app.reindex_tool_search = self.__reindex itp_config = os.path.join(self.test_directory, "integrated_tool_panel.xml") From c91c988c852e5a30e1c7c69168b5df770933dcd5 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 24 Mar 2017 17:08:31 +0100 Subject: [PATCH 2/6] Fix ToolConfWatcher test: can only watch existing files --- test/unit/tools/test_watcher.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/tools/test_watcher.py b/test/unit/tools/test_watcher.py index 7054856dc48..b2f3dce1fd7 100644 --- a/test/unit/tools/test_watcher.py +++ b/test/unit/tools/test_watcher.py @@ -40,6 +40,7 @@ def test_tool_conf_watcher(): with __test_directory() as t: tool_conf_path = path.join(t, "test_conf.xml") + open(tool_conf_path, "w").write("a") conf_watcher.watch_file(tool_conf_path) time.sleep(1) open(tool_conf_path, "w").write("b") From 5be7ebe780b487175563eb4661955a07df1d30cd Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 24 Mar 2017 17:13:44 +0100 Subject: [PATCH 3/6] Only call tool_cache.cleanup if tool_cache exists --- lib/galaxy/queue_worker.py | 3 ++- test/unit/tools/test_toolbox.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/queue_worker.py b/lib/galaxy/queue_worker.py index f3f8245b7e9..167be3e2339 100644 --- a/lib/galaxy/queue_worker.py +++ b/lib/galaxy/queue_worker.py @@ -87,7 +87,8 @@ def reload_tool(app, **kwargs): def reload_toolbox(app, **kwargs): log.debug("Executing toolbox reload on '%s'", app.config.server_name) reload_count = app.toolbox._reload_count - app.tool_cache.cleanup() + if app.tool_cache: + app.tool_cache.cleanup() app.toolbox = _get_new_toolbox(app) app.toolbox._reload_count = reload_count + 1 diff --git a/test/unit/tools/test_toolbox.py b/test/unit/tools/test_toolbox.py index a3856640aa1..70a50f7f246 100644 --- a/test/unit/tools/test_toolbox.py +++ b/test/unit/tools/test_toolbox.py @@ -430,6 +430,7 @@ class SimplifiedToolBox( ToolBox ): def __init__( self, test_case ): app = test_case.app # Handle app/config stuff needed by toolbox but not by tools. + app.tool_cache = None app.job_config.get_tool_resource_parameters = lambda tool_id: None app.config.update_integrated_tool_panel = True config_files = test_case.config_files From 4d9cf642a80025e28ea0276f8c41fcd9d9a53903 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 24 Mar 2017 17:53:46 +0100 Subject: [PATCH 4/6] Fix toolshed startup by setting tool_cache to `None` (the toolshed loads an empty toolbox) --- lib/galaxy/tools/toolbox/base.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/tools/toolbox/base.py b/lib/galaxy/tools/toolbox/base.py index b432f3df69c..28a73fa4ab0 100644 --- a/lib/galaxy/tools/toolbox/base.py +++ b/lib/galaxy/tools/toolbox/base.py @@ -79,7 +79,13 @@ class AbstractToolBox( Dictifiable, ManagesIntegratedToolPanelMixin, object ): if tool_conf_watcher: self._tool_conf_watcher = tool_conf_watcher # Avoids (re-)starting threads in uwsgi else: - self._tool_conf_watcher = get_tool_conf_watcher(reload_callback=lambda: self.handle_reload_toolbox(), tool_cache=self.app.tool_cache) + if hasattr(self.app, 'tool_cache'): + # Normal galaxy instances should have a tool_cache, + # but the toolshed does not. + tool_cache = self.app.tool_cache + else: + tool_cache = None + self._tool_conf_watcher = get_tool_conf_watcher(reload_callback=lambda: self.handle_reload_toolbox(), tool_cache=tool_cache) self._filter_factory = FilterFactory( self ) self._tool_tag_manager = tool_tag_manager( app ) self._init_tools_from_configs( config_filenames ) From 0bf2493233fd0afbc07da6620740536938d8d059 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 24 Mar 2017 18:26:48 +0100 Subject: [PATCH 5/6] Wrap tool_cache cleanup in try/except clause --- lib/galaxy/tools/toolbox/cache.py | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/lib/galaxy/tools/toolbox/cache.py b/lib/galaxy/tools/toolbox/cache.py index e1036f1bec7..5dbf7d2fcae 100644 --- a/lib/galaxy/tools/toolbox/cache.py +++ b/lib/galaxy/tools/toolbox/cache.py @@ -22,24 +22,29 @@ class ToolCache(object): Returns list of tool_ids that have been removed. """ - paths_to_cleanup = {path: tool.all_ids for path, tool in self._tools_by_path.items() if self._should_cleanup(path)} - removed_tool_ids = [] - for config_filename, tool_ids in paths_to_cleanup.items(): - removed_tool_ids.extend(tool_ids) - del self._hash_by_tool_paths[config_filename] - del self._tools_by_path[config_filename] - for tool_id in tool_ids: - if tool_id in self._tool_paths_by_id: - del self._tool_paths_by_id[tool_id] - return removed_tool_ids + try: + paths_to_cleanup = {path: tool.all_ids for path, tool in self._tools_by_path.items() if self._should_cleanup(path)} + removed_tool_ids = [] + for config_filename, tool_ids in paths_to_cleanup.items(): + removed_tool_ids.extend(tool_ids) + del self._hash_by_tool_paths[config_filename] + del self._tools_by_path[config_filename] + for tool_id in tool_ids: + if tool_id in self._tool_paths_by_id: + del self._tool_paths_by_id[tool_id] + return removed_tool_ids + except Exception: + # If by chance the file is being removed while calculating the hash or modtime + # we don't want the thread to die. + return [] def _should_cleanup(self, config_filename): """Return True of `config_filename` does not exist or if modtime and hash have changes, else return False.""" if not os.path.exists(config_filename): return True new_mtime = time.ctime(os.path.getmtime(config_filename)) - if self._mod_time_by_path[config_filename] != new_mtime: - if md5_hash_file(config_filename) != self._hash_by_tool_paths[config_filename]: + if self._mod_time_by_path.get(config_filename) != new_mtime: + if md5_hash_file(config_filename) != self._hash_by_tool_paths.get(config_filename): return True return False From 197aa402d72895d370307ce9724c86b892cded96 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 25 Mar 2017 12:03:53 +0100 Subject: [PATCH 6/6] Improve ToolCache cleanup logic Pull `removed_tool_ids` out of try/except, so that we can return `removed_tool_ids` even if an exception occured. Fix a typo in the `_should_cleanup` docstring. Many thanks for the suggestions @nsoranzo. --- lib/galaxy/tools/toolbox/cache.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/galaxy/tools/toolbox/cache.py b/lib/galaxy/tools/toolbox/cache.py index 5dbf7d2fcae..22b1fc882c6 100644 --- a/lib/galaxy/tools/toolbox/cache.py +++ b/lib/galaxy/tools/toolbox/cache.py @@ -1,5 +1,4 @@ import os -import time from galaxy.util.hash_util import md5_hash_file @@ -22,28 +21,28 @@ class ToolCache(object): Returns list of tool_ids that have been removed. """ + removed_tool_ids = [] try: paths_to_cleanup = {path: tool.all_ids for path, tool in self._tools_by_path.items() if self._should_cleanup(path)} - removed_tool_ids = [] for config_filename, tool_ids in paths_to_cleanup.items(): - removed_tool_ids.extend(tool_ids) del self._hash_by_tool_paths[config_filename] del self._tools_by_path[config_filename] for tool_id in tool_ids: if tool_id in self._tool_paths_by_id: del self._tool_paths_by_id[tool_id] - return removed_tool_ids + removed_tool_ids.extend(tool_ids) except Exception: # If by chance the file is being removed while calculating the hash or modtime # we don't want the thread to die. - return [] + pass + return removed_tool_ids def _should_cleanup(self, config_filename): - """Return True of `config_filename` does not exist or if modtime and hash have changes, else return False.""" + """Return True if `config_filename` does not exist or if modtime and hash have changes, else return False.""" if not os.path.exists(config_filename): return True - new_mtime = time.ctime(os.path.getmtime(config_filename)) - if self._mod_time_by_path.get(config_filename) != new_mtime: + new_mtime = os.path.getmtime(config_filename) + if self._mod_time_by_path.get(config_filename) < new_mtime: if md5_hash_file(config_filename) != self._hash_by_tool_paths.get(config_filename): return True return False @@ -65,6 +64,6 @@ class ToolCache(object): tool_hash = md5_hash_file(config_filename) tool_id = str( tool.id ) self._hash_by_tool_paths[config_filename] = tool_hash - self._mod_time_by_path[config_filename] = time.ctime(os.path.getmtime(config_filename)) + self._mod_time_by_path[config_filename] = os.path.getmtime(config_filename) self._tool_paths_by_id[tool_id] = config_filename self._tools_by_path[config_filename] = tool