launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

(The "Resubmit" vote is something that your reviewer might use to indicate that the MP needs to be entirely withdrawn and resubmitted, perhaps against a different branch.  It isn't something you do when you've made revisions in response to a review.)

Regarding tests at package build time, oops; I've fixed that in trunk.  Thanks for pointing it out.

Diff comments:

> 
> === modified file 'lpbuildd/target/lxd.py'
> --- lpbuildd/target/lxd.py	2018-06-12 23:23:13 +0000
> +++ lpbuildd/target/lxd.py	2018-10-25 07:43:01 +0000
> @@ -277,16 +277,31 @@
>              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("environment", {}).get(
> +                "driver_version")

While this was the first part of the train of thought in my previous review, I went on to say that I'd prefer just `lxc_version = self._client.host_info["environment"]["driver_version"]`, since all versions of LXD I can find have those keys.  Do you have a reason for using the `.get()` form?

> +        major, minor = [int(v) for v in lxc_version.split(".")[0:2]]
> +
> +        if major >= 3:
> +            raw_lxc_config.extend([
> +                ("lxc.apparmor.profile", "unconfined"),
> +                ("lxc.net.0.ipv4.address", ipv4_address),
> +                ("lxc.net.0.ipv4.gateway", self.ipv4_network.ip),
> +                ])
> +        else:
> +            raw_lxc_config.extend([
> +                ("lxc.aa_profile", "unconfined"),
> +                ("lxc.network.0.ipv4", ipv4_address),
> +                ("lxc.network.0.ipv4.gateway", self.ipv4_network.ip),
> +                ])
> +

Yes, though it's slightly more lines of code to do it this way and IMO less clear.  (I would refactor this to use a dict after landing it if you don't do it first.)

>          # Linux 4.4 on powerpc doesn't support all the seccomp bits that LXD
>          # needs.
>          if self.arch == "powerpc":
> 
> === 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-10-25 07:43:01 +0000
> @@ -181,22 +203,40 @@
>                  "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 ["2.0", "3.0"]:
> +            with MockPatch("pylxd.Client"):
> +                client = pylxd.Client()
> +                client.profiles.get.side_effect = FakeLXDAPIException
> +                client.host_info = {
> +                    "environment": {"driver_version": driver_version}
> +                    }
> +                LXD("1", "xenial", "amd64").create_profile()
> +                self.assert_correct_profile(
> +                        driver_version=driver_version or "3.0")

Ah, in that case, please use this approach instead (tested):

    with MockPatch("pylxd.Client"):
        for driver_version in ("2.0", "3.0"):
            client = pylxd.Client()
            client.reset_mock()
            ...

>  
>      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 ["2.0", "3.0"]:
> +            with MockPatch("pylxd.Client"):
> +                client = pylxd.Client()
> +                client.profiles.get.side_effect = FakeLXDAPIException
> +                client.host_info = {
> +                    "environment": {"driver_version": driver_version}
> +                    }
> +                LXD("1", "xenial", "powerpc").create_profile()
> +                self.assert_correct_profile(
> +                        extra_raw_lxc_config=[("lxc.seccomp", ""),],
> +                        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.