Repository navigation
[Resolves #1601] Share the stack output cache between resolvers - #1600
tiborhercz wants to merge 2 commits into
Conversation
|
Can you please tell us the motivation behind this change in the PRs description or commit message? or even better create an issue describing the problem and your proposed solution then link this PR to that issue. |
tiborhercz
changed the title
3daac04 to
43b335a
Compare
The lru_cache on StackOutputBase._get_stack_outputs is on an instance method, so self is part of the key. Every !stack_output tag is its own resolver, so the cache never hits across resolvers and N stacks that read one output make N DescribeStacks calls. Replace it with a class-level dict keyed on stack name, profile, region and role, behind a lock. A failed call is not cached. clear_cache() resets it for tests.
43b335a to
457e5e5
Compare
Thanks for the nudge, was still working on it. I opened #1601 with the problem and the proposed fix, and linked it from the title and the description. Short version: the I also squashed the branch to one commit in the |
There was a problem hiding this comment.
🟡 Changes recommended
The stack-output cache can mix outputs across accounts or regions, deadlock during resolver-backed role initialization, and retain stale outputs across plans, while its global lock also serializes unrelated requests.
4 open issues
What changed in this PR
This PR addresses #1601 by sharing stack-output results across resolver instances, reducing redundant CloudFormation requests that the earlier instance-keyed cache did not prevent.
Changes:
- Replaces
lru_cachewith a shared dictionary protected by a lock. - Separates fetching from caching and adds
clear_cache()for test isolation. - Adds regression tests for cross-resolver reuse and retrying failed fetches.
| File | Description |
|---|---|
tests/test_resolvers/test_stack_output.py |
Resets the cache and tests shared results and failed-fetch retries. |
tests/test_resolvers/test_stack_output_cache.py |
Tests repeated resolution and reuse across ten resolver instances. |
sceptre/resolvers/stack_output.py |
Implements shared, synchronized output caching and cache clearing. |
Review effort: Balanced
| ) | ||
|
|
||
| @functools.lru_cache(maxsize=4096) | ||
| _outputs_cache = {} |
There was a problem hiding this comment.
Good point. SceptrePlan._execute now clears the cache before it runs, so each plan starts empty. Added a test for it.
|
|
||
| @functools.lru_cache(maxsize=4096) | ||
| _outputs_cache = {} | ||
| _outputs_cache_lock = threading.Lock() |
There was a problem hiding this comment.
Yes, sceptre_role as !stack_output would deadlock here. The connection manager is now read before any lock and the fetch runs under a per-key RLock. Added a test that re-enters from inside a fetch.
| :returns: A formatted version of the Stack outputs. | ||
| :rtype: dict | ||
| """ | ||
| key = (stack_name, profile, region, sceptre_role) |
There was a problem hiding this comment.
You're right, (None, None, None) means "use the stack defaults" in call(). The key now uses the profile/region/role of the stack's connection manager in that case, same rule as _determine_session_args. Added a test with two profiles reading the same external stack name.
| with self._outputs_cache_lock: | ||
| if key not in self._outputs_cache: | ||
| self._outputs_cache[key] = self._fetch_stack_outputs( | ||
| stack_name, profile, region, sceptre_role | ||
| ) |
There was a problem hiding this comment.
Changed to a lock per key, the global one only guards the dicts now. Added tests: two stacks fetch in parallel, and a second caller of the same key waits for the running fetch and gets its result.
|
@zaro0508 Addressed the Copilot comments |
|
@tiborhercz can you please merge with master branch and push an update? |
Resolves #1601.
#1263 asked for stack outputs to be fetched once per stack, and #1312 added
functools.lru_cacheonStackOutputBase._get_stack_outputsfor that. Theproblem is that it's an instance method, so
selfis part of the cache key.Resolverdoesn't define__eq__/__hash__, and every!stack_outputtagis its own resolver instance, so the cache never hits across resolvers. Ten
stacks reading the same output still make ten DescribeStacks calls. On a big
project that's enough to get throttled by CloudFormation and spend most of
the launch in the retry back-off.
I added
tests/test_resolvers/test_stack_output_cache.py, which builds tenStackOutputresolvers the way the config reader does and resolves them.On master it fails with:
The fix replaces the
lru_cachewith a class-level dict keyed on stack name,profile, region and role, behind a lock so the first fetch of a stack only
happens once when a whole batch of threads starts at the same time. A failed
call isn't cached.
clear_cache()resets it for tests.PR Checklist
[Resolve #issue-number].poetry run tox) are passing.poetry run pre-commit run --all-files).and description in grammatically correct, complete sentences.
Approver/Reviewer Checklist
Other Information
Guide to writing a good commit