lazy boot: answer the shed-conf walk from the index, not the filesystem

Walking a CVMFS shed conf, the eager _load_tool_tag_set paid two per-tool
costs the lazy toolbox doesn't need: an os.path.exists stat (network
round trip on CVMFS — minutes over ~10k tools) and, since conf-provided
repositories have no install-DB row by design, one 'Attempted to load
tool shed tool, but the repository ... was not found in database'
warning per tool per boot.

Two overridable hooks in AbstractToolBox, both eager-behavior-preserving:
_tool_file_on_disk (the existence gate) and _missing_repository_log_level
(the warning's severity). LazyToolBox answers both from an identity-cached
set of indexed source_paths: the populator established existence when it
stored the source and materialisation reads the raw source from the
store, so the stat proves nothing; the missing install-DB row is expected,
so it logs at debug. Unindexed paths keep exact eager behavior.

Claude-Session: https://claude.ai/code/session_018L7ZmCv2ubKA3JNeSL8Pkr
This commit is contained in:
mvdbeek
2026-07-28 17:27:30 +02:00
parent 7df072fa05
commit 68f579b638
3 changed files with 107 additions and 2 deletions
+22 -2
View File
@@ -990,7 +990,7 @@ class AbstractToolBox(ManagesIntegratedToolPanelMixin):
template_kwds = self._path_template_kwds()
path = string.Template(path_template).safe_substitute(**template_kwds)
concrete_path = os.path.join(tool_path, path)
if not os.path.exists(concrete_path):
if not self._tool_file_on_disk(concrete_path):
# This is a lot faster than attempting to load a non-existing tool
raise OSError(ENOENT, os.strerror(ENOENT))
tool_shed_repository = None
@@ -1047,6 +1047,26 @@ class AbstractToolBox(ManagesIntegratedToolPanelMixin):
except Exception:
log.exception("Error reading tool from path: %s", path)
def _tool_file_on_disk(self, path: str) -> bool:
"""Existence gate for a conf-referenced tool file.
A per-tool stat dominates the walk of a large shed conf on a
network filesystem (CVMFS); the lazy toolbox overrides this to
answer from its index instead.
"""
return os.path.exists(path)
def _missing_repository_log_level(self, path: str) -> int:
"""Severity for a shed tool whose repository has no install-DB row.
Warning by default — for the eager toolbox that usually means lost
install records. The lazy toolbox downgrades index-covered tools:
conf-provided repositories (a CVMFS shed conf) are absent from the
install database by design, and one warning per tool is thousands
of lines per boot there.
"""
return logging.WARNING
def get_tool_repository_from_xml_item(
self, elem: "Element", path: str
) -> Union[ToolConfRepository, "ToolShedRepository"]:
@@ -1087,7 +1107,7 @@ class AbstractToolBox(ManagesIntegratedToolPanelMixin):
"Attempted to load tool shed tool, but the repository with name '%s' from owner '%s' was not found "
"in database. Tool will be loaded without install database."
)
log.warning(msg, repository_name, repository_owner)
log.log(self._missing_repository_log_level(path), msg, repository_name, repository_owner)
# Figure out path to repository on disk given the tool shed info and the path to the tool contained in the repo
assert installed_changeset_revision
repository_path = os.path.join(
+53
View File
@@ -496,6 +496,10 @@ class LazyToolBox(ToolBox):
# via ``_rebuild_shed_short_id_map``.
self._shed_short_id_to_guids: dict[str, set[str]] = {}
self._store_watcher: ToolSourceStoreWatcher | None = None
# Identity-keyed cache of every indexed ``source_path`` — see
# ``_index_source_paths``. Set before ``super().__init__`` because
# the eager walk consults it through ``_tool_file_on_disk``.
self._index_source_paths_cache: tuple[ToolIndex, set[str]] | None = None
# Eager init — its ``_init_tools_from_configs`` is overridden so the
# walk goes through our ``create_tool`` seam and hands back LazyTool
@@ -1008,6 +1012,55 @@ class LazyToolBox(ToolBox):
self._rebuild_shed_short_id_map()
return self._resolve_index_entry(config_file, guid)
def _index_source_paths(self) -> set[str]:
"""Every ``source_path`` the current in-memory index covers.
Cached against the identity of ``_tool_index``, so any reload
(which always assigns a fresh ``ToolIndex``) refreshes it without
reset plumbing. In-place entry removals can leave a stale extra
path here — that only skips one stat for a tool ``create_tool``
will then resolve or self-heal, so it is harmless.
"""
index = self._tool_index
if index is None:
return set()
cached = self._index_source_paths_cache
if cached is not None and cached[0] is index:
return cached[1]
paths: set[str] = set()
for versions in index.entries_by_version.values():
for entry in versions.values():
if entry.source_path:
paths.add(entry.source_path)
for entry in index.entries.values():
if entry.source_path:
paths.add(entry.source_path)
self._index_source_paths_cache = (index, paths)
return paths
def _tool_file_on_disk(self, path: str) -> bool:
"""Answer the eager walk's existence gate from the index.
The populator established existence when it stored the source, and
materialisation reads the raw source from the store, not the file —
so for an index-covered tool the per-tool stat proves nothing and,
on a CVMFS-resident shed conf, costs minutes of boot time.
"""
if os.path.abspath(path) in self._index_source_paths():
return True
return os.path.exists(path)
def _missing_repository_log_level(self, path: str) -> int:
"""Index-covered shed tools expectedly lack install-DB rows.
A conf-provided repository (CVMFS shed conf) is never installed
through the install database, so the eager warning would repeat
for every one of its tools on every boot.
"""
if os.path.abspath(path) in self._index_source_paths():
return logging.DEBUG
return logging.WARNING
def _resolve_index_entry(self, config_file, guid: str | None) -> ToolIndexEntry | None:
"""Find a matching index entry for the (config_file, guid) pair, or ``None``."""
if self._tool_index is None:
+32
View File
@@ -509,3 +509,35 @@ def test_remove_tool_by_id_broadcasts_reload_to_peers(monkeypatch):
box._store.remove_index_entry.assert_called_once_with("doomed")
assert calls == [("reload_tool_source_cache", {"noop_self": True})]
assert "doomed" not in box._tools_by_id
def test_tool_file_on_disk_answers_from_index(tmp_path):
box = _seam_box()
box._index_source_paths_cache = None
indexed_path = "/cvmfs/nowhere.example.org/shed_tools/repos/o/n/rev/t1.xml"
box._tool_index.add_entry(_entry(id="t1", source_path=indexed_path))
assert box._tool_file_on_disk(indexed_path) is True
assert box._tool_file_on_disk(str(tmp_path / "missing.xml")) is False
on_disk = tmp_path / "real.xml"
on_disk.write_text("<tool/>")
assert box._tool_file_on_disk(str(on_disk)) is True
def test_missing_repository_log_level_downgrades_indexed_paths():
box = _seam_box()
box._index_source_paths_cache = None
indexed_path = "/cvmfs/nowhere.example.org/shed_tools/repos/o/n/rev/t1.xml"
box._tool_index.add_entry(_entry(id="t1", source_path=indexed_path))
assert box._missing_repository_log_level(indexed_path) == logging.DEBUG
assert box._missing_repository_log_level("/elsewhere/t2.xml") == logging.WARNING
def test_index_source_paths_refresh_on_index_swap():
box = _seam_box()
box._index_source_paths_cache = None
box._tool_index.add_entry(_entry(id="t1", source_path="/a/t1.xml"))
assert "/a/t1.xml" in box._index_source_paths()
swapped = ToolIndex()
swapped.add_entry(_entry(id="t2", source_path="/b/t2.xml"))
box._tool_index = swapped
assert box._index_source_paths() == {"/b/t2.xml"}