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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
