Harden artifacts collection and make its failures observable - #3579
Merged
Conversation
`iterdir()` returns a generator, which is always truthy, so the empty-directory check never fired. Empty destination directories were kept and a success message was logged even when no artifacts were copied. Use `any()` to actually check for content.
Artifacts for the same cluster instance can be saved more than once, e.g. on cluster respin and again at session end. The destination directory name is derived from the cluster instance id, so the second save hit an existing directory and `mkdir` raised `FileExistsError`, aborting the teardown. Append a random suffix when the directory already exists.
Directory copies already use `ignore_dangling_symlinks=True`, but the flat file copies didn't have an equivalent guard. A broken symlink matched by the glob patterns made `shutil.copy` raise `FileNotFoundError` and aborted the whole artifacts save.
Log the full path of the saved coverage file, consistent with `save_start_script_coverage`.
The destination directory name contains a fresh random suffix, so it can't exist beforehand and the `rmtree` branch was dead code.
The cluster may still be running when artifacts are saved, so a file matched by the glob patterns can be rotated or deleted before it is copied. The `is_file()` guard alone only narrowed that window. Wrap the copy in `try/except OSError` so one problematic file doesn't abort the whole artifacts save, and log every skipped or failed file so dropped artifacts leave a trace.
The random-suffix fallback fired silently, hiding a repeated save of the same cluster instance. Log a warning so a double save upstream is diagnosable and the duplicate directories are explained.
An empty state dir is anomalous and was only signalled by the absence of the success log line. Log a warning before removing the empty destination directory.
Log the concrete destination directory instead of its parent, consistent with the other save functions.
Cover files and dirs copy, cluster instance id in the destination dir name, skipping of dangling symlinks and directories matching the file globs, empty state dir handling and the random suffix on destination dir name collision.
The `nodes` and `shelley` directories are copied while the cluster may still be running, so `shutil.copytree` is subject to the same race as the per-file copies and its failure had a much bigger blast radius: it aborted saving of all remaining cluster instances, and skipped both stopping of the clusters and copying of the collected artifacts to the artifacts base dir. Wrap the copy in `try/except OSError` with a warning, same as the per-file copies. Also drop the `ignore_dangling_symlinks` argument, which is a no-op when `symlinks=True`.
A systemic copy failure (disk full, permissions) produced per-file warnings followed by the misleading 'No cluster artifacts found' message, pointing a debugger at the wrong cause. Track copy failures and log an error instead when copies were attempted but nothing was saved.
FIFOs make `shutil.copy` fail fast, character devices make it hang, so 'fail or hang on' describes the skip more precisely than 'fail or block on'. The `ignore_dangling_symlinks` argument in `copy_artifacts` is a no-op when `symlinks=True`.
Add tests for the `except OSError` branches (single file copy failure, subdirectory copy failure, total save failure) that the existing tests short-circuited before via the `is_file()` guard. Populate the `shelley` dir in the state dir fixture so both entries of `dirs_to_copy` are exercised. Drop the `caplog.at_level` wrappers that suppressed INFO records and buy nothing since the project sets `log_level = INFO`, and fix the `_get_saved_dirs` docstring, which claimed to return only directories.
The added error handling pushed `save_cluster_artifacts` over the complexity limit (C901). Move the file and directory copy loops to a `_copy_state_dir_content` helper that returns the failure count.
Artifact collection inside the cluster restart loop is best-effort diagnostics, but any uncaught error from it failed the whole restart attempt. Wrap it in try/except with a log message, same as the neighboring stop script handling.
`save_start_script_coverage` had the same check-then-copy race that was fixed for the cluster artifacts: the log file can disappear between the `exists()` check and the copy. Log a warning and return `None` instead of raising in the teardown paths.
`self.log` is a no-op when `SCHEDULING_LOG` is not set, which is the common case for local runs, so a swallowed artifact save failure left no record anywhere while the state dir got deleted right after. Log via `LOGGER.exception` too, so the failure and its traceback land in pytest output. Also split the two independent saves into separate try blocks, so a failure in the cheap coverage copy can't prevent saving the far more valuable cluster artifacts.
An exception from saving one cluster instance's artifacts aborted the loop in `save_all_clusters_artifacts`, skipping the remaining instances and propagating out of session teardown, which then skipped stopping of the clusters and copying of the collected artifacts to the artifacts base dir. Guard each save separately and log the failure with traceback.
`except OSError` also covers destination-side failures (unwritable dir, disk full), not only the source file disappearing. Name both paths in the warning, mention the destination in the comment, and document the three `None` return cases in the docstring.
Cover the happy path, disabled coverage collection and the new copy-failure path that logs a warning and returns `None`.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens test artifact collection in teardown/restart flows to avoid aborting cleanup on partial failures, while improving observability via clearer logging and adding unit coverage for the artifacts module.
Changes:
- Make
save_cluster_artifactsand related copy operations tolerant of per-file/per-dir failures and fix empty-destination detection. - Wrap artifact collection in best-effort exception handling in cluster restart (
cluster_getter.py) and manager teardown (manager.py). - Add
framework_tests/test_artifacts.pyunit tests covering artifact copy, skip, and failure/collision paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| framework_tests/test_artifacts.py | Adds unit tests covering artifact saving behavior and failure modes. |
| cardano_node_tests/utils/artifacts.py | Fixes artifact copy logic, improves log messages, and makes copy steps more resilient. |
| cardano_node_tests/cluster_management/manager.py | Ensures artifact collection failures don’t prevent saving artifacts for remaining clusters. |
| cardano_node_tests/cluster_management/cluster_getter.py | Prevents artifact-save failures from aborting cluster restart attempts; logs failures. |
Suppressed comments (1)
cardano_node_tests/cluster_management/cluster_getter.py:273
- This
LOGGER.exceptionmessage also lacks cluster instance context, so failures can’t be easily correlated to a specificcNrestart attempt. Includec{self.cluster_instance_num}(and optionallystate_dir) in the message.
LOGGER.exception("Failed to save cluster artifacts.")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The setup I/O (reading the cluster instance id, creating the destination dir, listing it) could still raise past the per-file tolerance, and not all teardown callers guard the function (e.g. the `DEV_CLUSTER_RUNNING` path in session teardown). Catch `OSError` around the whole save and log it with traceback. Addresses PR #3579 review feedback.
Include the cluster instance number in the artifact save failure messages, so failures can be attributed when multiple instances are restarting. Addresses PR #3579 review feedback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes several bugs and silent failure modes in test artifacts collection
(
cardano_node_tests/utils/artifacts.py) and its callers, and adds unittests for the module.
Artifact collection runs in teardown paths, often while the cluster is
still running. Previously a single problematic file could abort the whole
save, and several failure modes either crashed teardown or passed without
any trace in the logs.
Bug fixes
save_cluster_artifactstreated every destination dir as non-empty:if not destdir.iterdir()is always false for a generator, so emptydirs were kept and a success message was logged even when nothing was
copied.
session end) crashed with
FileExistsError. Now a random suffix isappended and a warning is logged.
rotated/deleted mid-save aborted the whole artifacts save. Per-file and
per-directory copies are now tolerant: each failure logs a warning and
the save continues.
(
cluster_getter.py) aborted the restart attempt, and in sessionteardown (
manager.py) it skipped saving of the remaining clusterinstances, stopping of the clusters and the final copy to the artifacts
base dir. Artifact collection is now best-effort in both places, with
failures logged with traceback via the standard logger (visible even
when
SCHEDULING_LOGis not set).save_start_script_coveragehad the same check-then-copy race and nowlogs a warning and returns
Noneinstead of raising.Observability
distinct from the "state dir had no artifacts" warning.
Cleanups
rmtreebranch incopy_artifactsand a no-opignore_dangling_symlinksargument._copy_state_dir_contenthelper.Tests
New
framework_tests/test_artifacts.pywith 12 unit tests coveringsave_cluster_artifacts(copy, naming, skips, failure paths, collisions)and
save_start_script_coverage. No cluster needed.