Skip to content

Commit 98fc717

Browse files
pluskymimi1vx
authored andcommitted
perf(refhosts): parse refhosts.yml once per report, not per testplatform
refhosts_from_tp built a fresh Refhosts store via the factory on every call (~1s ruamel parse of refhosts.yml each), and autoconnect/add_host call it once per testplatform -- so a K-testplatform template paid ~K seconds at session start. Reuse the already-memoized, thread-safe store from _get_refhosts_store (the same one the product-drift check uses) so the file is parsed once and shared. The store is read-only after build, so sharing one instance across testplatforms and the concurrent connect_targets fan-out is safe. Behavior is preserved: the factory funnels every resolver error into RefhostsResolveFailedError, which the memo swallows to None, matching the old `except RefhostsResolveFailedError: return`. One deliberate difference: a failed build is now cached and not retried for the report's lifetime (reload to recover) -- acceptable because resolver failures are effectively persistent and within one autoconnect loop caching is strictly better. Also restores static typing of the memoized store (Refhosts | None) so both call sites are ty-checked again. Tests: factory called once across testplatforms, a pre-built store is reused, and the sticky-None (no-retry-after-failure) property -- all revert-sensitive.
1 parent 33f4e17 commit 98fc717

3 files changed

Lines changed: 104 additions & 15 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ and this project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.ht
2424
now run concurrently instead of one after another, while results are read back
2525
in the original order so the rendered overview is unchanged. The openQA
2626
job-group list is fetched once up front and shared across the fan-out.
27+
- Loading a template / connecting reference hosts is faster for multi-target
28+
updates. `refhosts.yml` is now parsed once per report and shared across every
29+
testplatform (and with the product-drift check) instead of being re-parsed
30+
(~1s each) for every testplatform during autoconnect / `add_host`.
2731

2832
## 19.0.1 - 2026-07-17
2933

mtui/test_reports/testreport.py

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
from traceback import format_exc
1616
from typing import TYPE_CHECKING, Any, Literal
1717

18-
from ..hosts.refhost import Attributes, RefhostsFactory, RefhostsResolveFailedError
18+
from ..hosts.refhost import Attributes, RefhostsFactory
1919
from ..hosts.refhost import verify as product_verify
2020
from ..hosts.target import Target, TargetLockedError
2121
from ..hosts.target.hostgroup import HostsGroup
@@ -35,6 +35,7 @@
3535
if TYPE_CHECKING:
3636
from ..cli.prompter import Prompter
3737
from ..hosts.host_arbiter import HostArbiter
38+
from ..hosts.refhost import Refhosts
3839

3940
logger = getLogger("mtui.template.testreport")
4041

@@ -168,9 +169,11 @@ def __init__(
168169
# (see _verify_target_products); commands print these so they
169170
# reach MCP clients, which only see command stdout (not logs).
170171
self.product_warnings: dict[str, list[str]] = {}
171-
# Lazily-built, cached refhosts store for the product check, with
172-
# a lock because connect_targets runs connect_target concurrently.
173-
self._refhosts_store: Any = None
172+
# Lazily-built, cached refhosts store shared by host resolution
173+
# (refhosts_from_tp) and the product-drift check, with a lock
174+
# because connect_targets runs connect_target concurrently. A
175+
# failed build is cached and not retried for the report's lifetime.
176+
self._refhosts_store: Refhosts | None = None
174177
self._refhosts_store_built = False
175178
self._refhosts_store_lock = threading.Lock()
176179

@@ -449,12 +452,14 @@ def _autolock_new_target(self, target: Target) -> None:
449452
with suppress(TargetLockedError):
450453
target.lock(self.lock_comment)
451454

452-
def _get_refhosts_store(self):
455+
def _get_refhosts_store(self) -> "Refhosts | None":
453456
"""Build (once, thread-safe) the refhosts store, or None on failure.
454457
455-
connect_targets fans connect_target out across threads, so the
456-
first lookup may race; guard the one-time build with a lock and
457-
cache the (possibly ``None``) result.
458+
Shared by host resolution (:meth:`refhosts_from_tp`) and the
459+
product-drift check. connect_targets fans connect_target out
460+
across threads, so the first lookup may race; guard the one-time
461+
build with a lock and cache the (possibly ``None``) result. A
462+
failed build is cached and not retried for the report's lifetime.
458463
"""
459464
if self._refhosts_store_built:
460465
return self._refhosts_store
@@ -463,10 +468,7 @@ def _get_refhosts_store(self):
463468
try:
464469
self._refhosts_store = self.refhostsFactory(self.config)
465470
except Exception:
466-
logger.debug(
467-
"refhosts store unavailable for product check:\n%s",
468-
format_exc(),
469-
)
471+
logger.debug("refhosts store unavailable:\n%s", format_exc())
470472
self._refhosts_store = None
471473
self._refhosts_store_built = True
472474
return self._refhosts_store
@@ -782,9 +784,22 @@ def refhosts_from_tp(self, testplatform) -> None:
782784
testplatform: The test platform to get reference hosts from.
783785
784786
"""
785-
try:
786-
refhosts = self.refhostsFactory(self.config)
787-
except RefhostsResolveFailedError:
787+
# Reuse the once-built, thread-safe store instead of re-parsing
788+
# refhosts.yml (~1s) for every testplatform. autoconnect/add_host
789+
# call this once per testplatform, so a K-testplatform template
790+
# otherwise paid ~K seconds. The store is read-only after build, so
791+
# sharing one instance across testplatforms (and with the product
792+
# drift check) is safe. ``_get_refhosts_store`` returns None on any
793+
# resolver failure (the factory funnels every resolver error into
794+
# RefhostsResolveFailedError, which it swallows). Note the memo caches
795+
# a failed build: unlike the previous per-call factory build, a
796+
# transient resolver failure is not retried for the report's lifetime
797+
# (reload the template to recover). This is acceptable -- resolver
798+
# failures are effectively persistent (missing refhosts.yml + no
799+
# network), and within one autoconnect loop caching is strictly better
800+
# (one dead attempt instead of one per testplatform).
801+
refhosts = self._get_refhosts_store()
802+
if refhosts is None:
788803
return
789804

790805
try:

tests/test_testreport.py

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -890,6 +890,76 @@ def test_refhosts_from_tp_failed_resolve_swallows(tmp_path: Path) -> None:
890890
r.refhosts_from_tp("tp")
891891

892892

893+
def test_refhosts_from_tp_parses_refhosts_once_across_testplatforms(
894+
tmp_path: Path,
895+
) -> None:
896+
"""refhosts.yml is parsed once per report, not once per testplatform.
897+
898+
autoconnect/add_host resolve every testplatform through
899+
``refhosts_from_tp``; reusing the memoized ``_get_refhosts_store``
900+
collapses the K parses of a K-testplatform template to one. A revert to
901+
the per-call factory build makes ``call_count == K``.
902+
"""
903+
r = _make(tmp_path)
904+
r._arbiter = None
905+
r._owner = None
906+
r.refhostsFactory = MagicMock(
907+
return_value=_FakeRefhosts([_ph("h1", ("sles", "15", "x86_64", ()))])
908+
)
909+
910+
for minor in (5, 6, 7):
911+
r.refhosts_from_tp(f"base=sles(major=15,minor={minor});arch=[x86_64]")
912+
913+
assert r.refhostsFactory.call_count == 1
914+
assert r.hostnames == {"h1"}
915+
916+
917+
def test_refhosts_from_tp_reuses_prebuilt_store(tmp_path: Path) -> None:
918+
"""A store already built (e.g. by the product-drift check) is reused.
919+
920+
``refhosts_from_tp`` reads the same ``_get_refhosts_store`` memo the
921+
product-drift check populates, so once the store is built it never calls
922+
the factory again.
923+
"""
924+
r = _make(tmp_path)
925+
r._arbiter = None
926+
r._owner = None
927+
# Simulate the memo already built (as the product-drift check would).
928+
# _FakeRefhosts is a structural test double, not a Refhosts subclass.
929+
r._refhosts_store = _FakeRefhosts( # ty: ignore[invalid-assignment]
930+
[_ph("h1", ("sles", "15", "x86_64", ()))]
931+
)
932+
r._refhosts_store_built = True
933+
r.refhostsFactory = MagicMock()
934+
935+
r.refhosts_from_tp("base=sles(major=15,minor=5);arch=[x86_64]")
936+
937+
r.refhostsFactory.assert_not_called()
938+
assert r.hostnames == {"h1"}
939+
940+
941+
def test_refhosts_from_tp_caches_failed_resolve(tmp_path: Path) -> None:
942+
"""A failed store build is cached, not retried, for the report's lifetime.
943+
944+
Unlike the previous per-call factory build, the shared memo caches the
945+
failure: a second ``refhosts_from_tp`` does not re-invoke the factory
946+
(recovery needs a template reload). Pins the deliberate sticky-None
947+
behavior of the memoization.
948+
"""
949+
from mtui.hosts.refhost import RefhostsResolveFailedError
950+
951+
r = _make(tmp_path)
952+
r._arbiter = None
953+
r._owner = None
954+
r.refhostsFactory = MagicMock(side_effect=RefhostsResolveFailedError("bad"))
955+
956+
r.refhosts_from_tp("base=sles(major=15,minor=5);arch=[x86_64]")
957+
r.refhosts_from_tp("base=sles(major=15,minor=6);arch=[x86_64]")
958+
959+
assert r.refhostsFactory.call_count == 1
960+
assert r.hostnames == set()
961+
962+
893963
# ---------------------------------------------------------------------------
894964
# trivial pytest sanity for shutil-side metadata; never raises.
895965
# ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)