launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

Thanks for working on this!

As well as the inline comments I've left, I'm pretty sure that this is going to need changes to the test suite.

Diff comments:

> === modified file 'lpbuildd/target/lxd.py'
> --- lpbuildd/target/lxd.py	2018-06-12 23:23:13 +0000
> +++ lpbuildd/target/lxd.py	2018-09-05 13:55:08 +0000
> @@ -28,6 +28,7 @@
>  from lpbuildd.util import (
>      set_personality,
>      shell_escape,
> +    get_os_release

Please keep these imports sorted, and make sure there's a comma at the end of each line.

>      )
>  
>  
> @@ -276,17 +277,32 @@
>          else:
>              old_profile.delete()
>  
> -        raw_lxc_config = [
> -            ("lxc.aa_profile", "unconfined"),
> -            ("lxc.cap.drop", ""),
> -            ("lxc.cap.drop", "sys_time sys_module"),
> -            ("lxc.cgroup.devices.deny", ""),
> -            ("lxc.cgroup.devices.allow", ""),
> -            ("lxc.mount.auto", ""),
> -            ("lxc.mount.auto", "proc:rw sys:rw"),
> -            ("lxc.network.0.ipv4", ipv4_address),
> -            ("lxc.network.0.ipv4.gateway", self.ipv4_network.ip),
> -            ]
> +        major, minor = get_os_release()
> +
> +        if major >= 18:

You're effectively using the OS release as a proxy for properties of LXD here.  However, the OS release is a bad proxy to use for this, because at some point we may well want to switch to the LXD snap.  Could you please find a way to make the behaviour conditional on the LXD version instead?  You should be able to dig that out of `self._client.host_info`.

> +            raw_lxc_config = [
> +                ("lxc.apparmor.profile", "unconfined"),
> +                ("lxc.cap.drop", ""),
> +                ("lxc.cap.drop", "sys_time sys_module"),
> +                ("lxc.cgroup.devices.deny", ""),
> +                ("lxc.cgroup.devices.allow", ""),
> +                ("lxc.mount.auto", ""),
> +                ("lxc.mount.auto", "proc:rw sys:rw"),
> +                ("lxc.net.0.ipv4.address", ipv4_address),
> +                ("lxc.net.0.ipv4.gateway", self.ipv4_network.ip),
> +                ]
> +        else:
> +            raw_lxc_config = [
> +                ("lxc.aa_profile", "unconfined"),
> +                ("lxc.cap.drop", ""),
> +                ("lxc.cap.drop", "sys_time sys_module"),
> +                ("lxc.cgroup.devices.deny", ""),
> +                ("lxc.cgroup.devices.allow", ""),
> +                ("lxc.mount.auto", ""),
> +                ("lxc.mount.auto", "proc:rw sys:rw"),
> +                ("lxc.network.0.ipv4", ipv4_address),
> +                ("lxc.network.0.ipv4.gateway", self.ipv4_network.ip),
> +                ]

Please could you reduce the duplication here?  Most of the entries are identical, so you should be able to build up a list with conditional and unconditional `.append` and `.extend` calls.

>          # Linux 4.4 on powerpc doesn't support all the seccomp bits that LXD
>          # needs.
>          if self.arch == "powerpc":
> @@ -341,7 +362,11 @@
>              hostname_file.flush()
>              os.fchmod(hostname_file.fileno(), 0o644)
>              self.copy_in(hostname_file.name, "/etc/hostname")
> -        self.copy_in("/etc/resolv.conf", "/etc/resolv.conf")
> +        if os.path.exists("/run/systemd/resolve/resolv.conf"):
> +            self.copy_in("/run/systemd/resolve/resolv.conf",
> +                    "/etc/resolv.conf")

Should this possibly depend instead on whether /etc/resolv.conf is a symlink, and if so on what the target of the symlink is?

> +        else:
> +            self.copy_in("/etc/resolv.conf", "/etc/resolv.conf")
>          with tempfile.NamedTemporaryFile(mode="w+") as policy_rc_d_file:
>              policy_rc_d_file.write(policy_rc_d)
>              policy_rc_d_file.flush()


-- 
https://code.launchpad.net/~tobijk/launchpad-buildd/launchpad-buildd-bionic/+merge/354331
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd.