launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



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-17 13:33:57 +0000
> @@ -277,16 +278,34 @@
>              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),
>              ]
> +
> +        lxc_version = self._client.host_info.get("driver_version")

I can't find any environment where driver_version exists at the top level of `client.host_info`.  Shouldn't this be more like this?

    lxc_version = self._client.host_info.get("environment", {}).get("driver_version")

At least, this works for me on both xenial and bionic, whereas the code I'm commenting on works on neither; it also works if lxd is installed from a snap rather than from a .deb.

> +
> +        if lxc_version:
> +            major, minor = [int(v) for v in lxc_version.split(".")[0:2]]
> +        else:
> +            major, minor = get_lxd_version()

I wonder if this fallback is really necessary?  With the fix above, we never seem to need the fallback.

So in fact, rather than the `.get` sequence above, I'd prefer just:

    lxc_version = self._client.host_info["environment"]["driver_version"]
    major, minor = [int(v) for v in lxc_version.split(".")[:2]]

... and then drop all the fallback code.  That would make this branch quite a lot simpler.  I'd also quite strongly prefer not to use apt to look up lxd's version even as a fallback, as there's a good chance that we'll switch over to using lxd via the snap in future.

> +
> +        if major >= 3:
> +            raw_lxc_config.insert(0, ("lxc.apparmor.profile", "unconfined"))
> +            raw_lxc_config.extend([
> +                ("lxc.net.0.ipv4.address", ipv4_address),
> +                ("lxc.net.0.ipv4.gateway", self.ipv4_network.ip),
> +                ])
> +        else:
> +            raw_lxc_config.insert(0, ("lxc.aa_profile", "unconfined"))
> +            raw_lxc_config.extend([
> +                ("lxc.network.0.ipv4", ipv4_address),
> +                ("lxc.network.0.ipv4.gateway", self.ipv4_network.ip),
> +                ])

It might be worth just building up a dict and then using `... for key, value in sorted(raw_lxc_config.items())` at the end, rather than having to invest thought in maintaining the list in sorted order here.

> +
>          # Linux 4.4 on powerpc doesn't support all the seccomp bits that LXD
>          # needs.
>          if self.arch == "powerpc":
> @@ -341,7 +366,17 @@
>              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")
> +
> +        resolv_conf = "/etc/resolv.conf"
> +
> +        if os.path.islink(resolv_conf):
> +            resolv_conf = os.readlink(resolv_conf)

`os.readlink` only reads one level of symbolic links, and doesn't canonicalise the result, so you can get confusing results if there's a relative symlink there.  Can this be `resolv_conf = os.path.realpath(resolv_conf)` instead?

> +            if os.path.basename(resolv_conf) == "stub-resolv.conf" and \
> +                    os.path.isfile("/run/systemd/resolve/resolv.conf"):

We always prefer this style over trailing backslashes where possible:

    if (os.path.basename(resolv_conf) == "stub-resolv.conf" and
            os.path.isfile("/run/systemd/resolve/resolv.conf")):
        ...

Also, should the first test perhaps just be `resolv_conf == "/run/systemd/resolve/stub-resolv.conf"` rather than assuming that anything with that basename is owned by systemd?

> +                resolv_conf = "/run/systemd/resolve/resolv.conf"
> +
> +        self.copy_in(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()
> 
> === modified file 'lpbuildd/target/tests/test_lxd.py'
> --- lpbuildd/target/tests/test_lxd.py	2018-05-08 09:37:16 +0000
> +++ lpbuildd/target/tests/test_lxd.py	2018-09-17 13:33:57 +0000
> @@ -181,22 +200,39 @@
>                  "type": "nic",
>                  },
>              }
> +        if driver_version == "3.0":
> +            expected_devices["root"] = {
> +                "path": "/",
> +                "pool": "default",
> +                "type": "disk",
> +                }
>          client.profiles.create.assert_called_once_with(
>              "lpbuildd", expected_config, expected_devices)
>  
>      def test_create_profile_amd64(self):
> -        self.useFixture(MockPatch("pylxd.Client"))
> -        client = pylxd.Client()
> -        client.profiles.get.side_effect = FakeLXDAPIException
> -        LXD("1", "xenial", "amd64").create_profile()
> -        self.assert_correct_profile()
> +        for driver_version in [None, "2.0", "3.0"]:
> +            self.useFixture(MockPatch("pylxd.Client"))
> +            if not driver_version:
> +                self.useFixture(MockPatch("lpbuildd.target.lxd.get_lxd_version",
> +                    lambda: (3, 0)))

Using fixtures in a loop is a bit confusing and best avoided.  The get_lxd_version business goes away if you take my suggestion above, and the pylxd.Client mock can safely be moved out of the loop.

(If we had to keep the get_lxd_version business for some reason, then putting the loop body inside `with fixture:` is better than doing `self.useFixture(fixture)` each time round the loop.)

> +            client = pylxd.Client()
> +            client.profiles.get.side_effect = FakeLXDAPIException
> +            client.host_info = {"driver_version": driver_version}
> +            LXD("1", "xenial", "amd64").create_profile()
> +            self.assert_correct_profile(driver_version=driver_version or "3.0")
>  
>      def test_create_profile_powerpc(self):
> -        self.useFixture(MockPatch("pylxd.Client"))
> -        client = pylxd.Client()
> -        client.profiles.get.side_effect = FakeLXDAPIException
> -        LXD("1", "xenial", "powerpc").create_profile()
> -        self.assert_correct_profile("lxc.seccomp=\n")
> +        for driver_version in [None, "2.0", "3.0"]:
> +            self.useFixture(MockPatch("pylxd.Client"))
> +            if not driver_version:
> +                self.useFixture(MockPatch("lpbuildd.target.lxd.get_lxd_version",
> +                    lambda: (3, 0)))
> +            client = pylxd.Client()
> +            client.profiles.get.side_effect = FakeLXDAPIException
> +            client.host_info = {"driver_version": driver_version}
> +            LXD("1", "xenial", "powerpc").create_profile()
> +            self.assert_correct_profile("lxc.seccomp=\n",
> +                    driver_version=driver_version or "3.0")
>  
>      def test_start(self):
>          fs_fixture = self.useFixture(FakeFilesystem())


-- 
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.