Unified cache v1 - #66
matyas7dub wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate cache-key correctness issues remain, along with minor shell behavior fixes.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
Unifies test-suite caching under .cache/ and improves logging, configuration handling, and cache determinism.
Changes:
- Adds persistent and per-run cache utilities.
- Caches merged, VLAN-tagged, and TREx-generated artifacts.
- Improves timing, traffic statistics, typing, fixtures, and documentation.
- Updates shell behavior and performance-test logging.
| File | Summary |
|---|---|
util/trex_util.py |
Caches merged PCAPs; cache keys should include source fingerprints. |
util/test_runner.py |
Adds duration and traffic logging. |
util/suricata_manager.py |
Updates startup timing. |
util/suri_util.py |
Adds aggregated statistics and debug logging. |
util/make-graphs.py |
Handles missing parameters safely. |
util/config_builder.py |
Refactors configuration handling and typing. |
util/cache_util.py |
Adds cache management helpers. |
util/add_vlan.py |
Caches VLAN-tagged PCAPs; keys should include source fingerprints. |
README.md |
Documents cache behavior. |
pytest_start.sh |
Updates debug tracing and usage handling. |
performance_tests/web_50_sites/test_web_50_sites.py |
Removes duplicate run logging. |
performance_tests/pcap_replay/test_pcap_replay.py |
Removes duplicate run logging. |
performance_tests/nfs_smb_simple/test_nfs_smb_simple.py |
Removes duplicate run logging. |
performance_tests/https_simple/test_https_simple.py |
Removes duplicate run logging. |
performance_tests/http_simple/test_http_simple.py |
Removes duplicate run logging. |
performance_tests/http_https_smb_simple/test_http_https_smb_simple.py |
Removes duplicate run logging. |
conftest.py |
Adds cache lifecycle and formatting helpers. |
assets/trex/traffic_profiles/trex_client_manager.py |
Adds cached TREx artifacts and traffic logging; cache keys should include the TREx version. |
.gitignore |
Ignores .cache/. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Will be adding a V2 with an updated docstring in |
|
|
||
| cached_profile_path = try_cache("stf_profile.yaml", key_parts) | ||
| if cached_profile_path is not None: | ||
| self._stf_config_path = cached_profile_path | ||
| return self._stf_config_path | ||
|
|
||
| self._stf_config_path = cache_path("stf_profile.yaml", *key_parts) | ||
| with open(self._stf_config_path, mode="w+") as f: |
There was a problem hiding this comment.
We have heard about magical constants, but maybeee we could use one placeholder value for all "stf_profile.yaml"? Just a minor nitpick though.
There was a problem hiding this comment.
I suppose it is not really a magical constant, but I will put it behind a target_name variable like I did in some other functions. I just forgot here.
| filename = _cache_name(name, *key_parts) | ||
| for base in (RUN_DIR, PERSISTENT_DIR): | ||
| candidate = base / filename | ||
| if candidate.is_file(): |
There was a problem hiding this comment.
.is_file() does not make sure that the cached file is complete.
There was a problem hiding this comment.
The cache assumes it is read only (which might not be true all the time, but that is what the per-run cache is for), so the hash in the file name should be enough to ensure consistency. Though I guess it wouldn't be too difficult to keep a manifest, which would store the checksum and maybe some other metadata.
| def _cache_key(*parts: object) -> str: | ||
| """Return a short hash derived from the given parts.""" | ||
| return hashlib.md5("|".join(str(p) for p in parts).encode()).hexdigest()[:_KEY_LENGTH] |
There was a problem hiding this comment.
MD5 for sure is and truncating it down to _KEY_LENGTH doesn't change that. It does increase the chance of collisions, but that should still be absurdly unlikely.
Depends on #65
This primarily adds
cache_util.pyin the last commit (all other commits are from the previous PR).The goal is to unify caching behavior across the test suite, which means that we can have better debug logging and it is easier to manage just
.cacheinstead of manytmpfolders.Also the caching behavior itself should be more robust, since now it is easy to generate a deterministic name from the inputs.