On 8/13/26 9:07 AM, Ales Musil wrote:
> The upgrade test cleanup used to conditionally remove the entire
> upgrade-testsuite.dir based on test success and debug flags.
> This included the base-repo source checkout with all object
> files and binaries, making the CI artifact unnecessarily large.
>
> Align the upgrade test cleanup with regular test suites where
> autotest already handles per-test directory cleanup (keeping
> them on failure or with -d, removing on success).
>
> Add collect_logs() to copy the test log and per-test result
> directories from base-repo/tests/ into upgrade-testsuite.dir/
> so they are preserved. Errors during collection are logged
> but do not prevent cleanup from proceeding.
>
> Add remove_base_directory() to unconditionally remove the
> base-repo source checkout after logs have been collected.
> The cleanup handler now simply calls both functions without
> any conditional logic.
>
> Simplify remove_upgrade_test_directory() to only handle
> startup cleanup of leftovers from a previous run.
>
> Assisted-by: Claude Opus 4.6, OpenCode
> Signed-off-by: Ales Musil <[email protected]>
> ---
Hi Ales,
Thanks for the improvement!
> .ci/ovn_upgrade_test.py | 12 ++++-----
> .ci/ovn_upgrade_utils.py | 54 +++++++++++++++++++++++++++-------------
> 2 files changed, 42 insertions(+), 24 deletions(-)
>
> diff --git a/.ci/ovn_upgrade_test.py b/.ci/ovn_upgrade_test.py
> index e38145cc2..3c2f7832c 100755
> --- a/.ci/ovn_upgrade_test.py
> +++ b/.ci/ovn_upgrade_test.py
> @@ -1,7 +1,6 @@
> #!/usr/bin/env python3
>
> import atexit
> -import os
> import signal
> import sys
> from pathlib import Path
> @@ -15,6 +14,8 @@ from ovn_upgrade_utils import (
> ovn_upgrade_save_current_binaries,
> ovn_upgrade_extract_info,
> run_upgrade_workflow,
> + collect_logs,
> + remove_base_directory,
> remove_upgrade_test_directory,
> UpgradeConfig
> )
> @@ -45,11 +46,8 @@ def main():
> test_success = False
>
> def cleanup():
> - flags = os.environ.get('TESTSUITEFLAGS', '')
> - if '-d' in flags or '--debug' in flags or not test_success:
> - log(f"Keeping {config.path.upgrade_dir} for debugging")
> - else:
> - remove_upgrade_test_directory(config)
> + collect_logs(config)
> + remove_base_directory(config)
>
> atexit.register(cleanup)
> signal.signal(signal.SIGINT, lambda s, f: sys.exit(1))
> @@ -93,7 +91,7 @@ def main():
> log("UPGRADE TESTS PASSED")
> else:
> log("UPGRADE TESTS FAILED")
> - log(f"Check: {config.file.test_log}")
> + log(f"Check: {config.path.upgrade_dir}")
> log("=" * 70)
>
> return 0 if test_success else 1
> diff --git a/.ci/ovn_upgrade_utils.py b/.ci/ovn_upgrade_utils.py
> index b96696e1a..5804a017d 100755
> --- a/.ci/ovn_upgrade_utils.py
> +++ b/.ci/ovn_upgrade_utils.py
> @@ -14,8 +14,7 @@ import shlex
> import sys
>
> UPGRADE_DIR = 'tests/upgrade-testsuite.dir'
> -SYSTEM_TESTS_LOGS = 'tests/system-kmod-testsuite.log'
> -SYSTEM_TESTS_DIR = 'tests/system-kmod-testsuite.dir'
> +SYSTEM_TESTS_GLOB = 'tests/system-kmod-testsuite.*'
> BASE_REPO_DIR = 'base-repo'
> BINARIES_DIR = 'ovn-upgrade-binaries'
> BUILD_LOG = 'build-base.log'
> @@ -41,13 +40,12 @@ class PathConfig:
> upgrade_dir: Path # Path where all upgrade-tests related files are
> stored
> base_dir: Path # Path for base branch i.e. from which we upgrade
> binaries_dir: Path # Path for binaries from dst branch
> - test_dir: Path # Path for system tests run by upgrade tests.
> + test_glob: str # Glob for test results in base_dir
>
>
> @dataclass
> class FileConfig:
> git_log: Path
> - test_log: Path
> build_log: Path
> new_egress: Path
> m4_defines: Path
> @@ -85,11 +83,10 @@ class UpgradeConfig:
> binaries_dir=upgrade_dir / BINARIES_DIR,
> base_dir=base_dir,
> upgrade_dir=upgrade_dir,
> - test_dir=base_dir / SYSTEM_TESTS_DIR,
> + test_glob=str(base_dir / SYSTEM_TESTS_GLOB),
> )
>
> file_obj = FileConfig(
> - test_log=base_dir / SYSTEM_TESTS_LOGS,
> build_log=upgrade_dir / BUILD_LOG,
> git_log=upgrade_dir / GIT_LOG,
> new_egress=upgrade_dir / NEW_EGRESS,
> @@ -659,22 +656,45 @@ def run_upgrade_workflow(config):
> return True
>
>
> +def collect_logs(config):
> + """Move base-repo test logs into upgrade-testsuite.dir.
> +
> + Moves system-kmod-testsuite.* from base-repo/tests/ into
> + upgrade-testsuite.dir/ so they are preserved after base-repo
> + is removed.
> + """
> + upgrade_dir = config.path.upgrade_dir
> +
> + if not run_shell_command(f"sudo mv {config.path.test_glob}
> {upgrade_dir}"):
> + log(f"Failed to collect test logs from {config.path.test_glob}")
> + else:
> + log(f"Test logs collected in {upgrade_dir}")
> +
> +
> +def remove_base_directory(config):
> + """Remove the base-repo source checkout.
> +
> + Called after collect_logs() to remove the full source tree
I think I'd merge the contents of this function into collect_logs(). We
always call them together anyway.
With this addressed or not (I'll leave it up to you), this patch looks
good to me, thanks!
Acked-by: Dumitru Ceara <[email protected]>
Regards,
Dumitru
> + with object files and binaries that would otherwise bloat the
> + CI artifact.
> + """
> + base_dir = config.path.base_dir
> + if base_dir.exists():
> + log(f"Removing {base_dir}...")
> + run_command(f"sudo rm -rf {base_dir}")
> +
> +
> def remove_upgrade_test_directory(config):
> + """Remove the entire upgrade-testsuite.dir.
> +
> + Used at startup to clear leftovers from a previous run.
> + """
> upgrade_dir = config.path.upgrade_dir
> - test_dir = config.path.test_dir
> - test_log = config.file.test_log
>
> if not upgrade_dir.exists():
> return True
>
> log(f"Removing old {upgrade_dir}...")
> + result = run_command(f"sudo rm -rf {upgrade_dir}")
>
> - run_command(f"sudo rm -rf {test_dir}")
> - run_command(f"sudo rm -f {test_log}")
> -
> - try:
> - shutil.rmtree(upgrade_dir)
> - return True
> - except OSError as e:
> - log(f"Failed to remove {upgrade_dir}: {e}")
> - return False
> + return result.returncode == 0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
