On Fri, Aug 14, 2026 at 11:17 AM Dumitru Ceara <[email protected]> wrote:
> 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]> > Thank you Dumitru, I did that and applied the patch to main. > > 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 > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
