canonical-ubuntu-qa team mailing list archive

[Skia <mp+456773@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> diff --git a/charms/focal/autopkgtest-cloud-worker/lib/systemd.py b/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
> index 0f1cc34..95ed829 100644
> --- a/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
> +++ b/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
> @@ -250,6 +250,24 @@ def update_lxd_dropins(arch, ip, n):
>      reload()
>  
>  
> +def remove_old_lxd_dropins(target_lxd_config):
> +    (
> +        _,
> +        lxd_unit_object_paths,
> +        _,
> +    ) = get_units()
> +    all_lxd_arches = set(
> +        list(lxd_unit_object_paths.keys()) + list(target_lxd_config.keys())
> +    )
> +    for arch in all_lxd_arches:
> +        ips = target_lxd_config[arch].keys()
> +        for lxd_path in lxd_unit_object_paths:
> +            if "autopkgtest@" in lxd_path and [
> +                ip for ip in ips if ip not in lxd_path

I'm not entirely sure, but I think this test will always succeed, as long as you have at least two different IPs, since at least one of them will **not** be in `lxd_path`, thus will end up in the list you're building, making that list to evaluate to True.
I think a test that would work would rather have a form like `not any([ip in lxd_path for ip in ips])`.

> +            ]:
> +                os.remove(lxd_path)

Adding a log here would certainly help make sure the condition above is correct, and would help investigate if something weird happens.

> +
> +
>  def enable_timer(region, arch, releases):
>      unit_names = [
>          "build-adt-image@{}-{}-{}.timer".format(release, region, arch)


-- 
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/456773
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~andersson123/autopkgtest-cloud:lxd-cleanup-srv-files into autopkgtest-cloud:master.