Re: [Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
Review: Approve
Diff comments:
>
> === modified file 'lpbuildd/target/lxd.py'
> --- lpbuildd/target/lxd.py2018-06-12 23:23:13 +
> +++ lpbuildd/target/lxd.py2018-10-25 09:55:10 +
> @@ -277,16 +277,30 @@
> 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["environment"]["driver_version"]
> +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),
> +])
> +
I tried, and then I realised that of course there are (intentionally) duplicate
keys, so a dict doesn't make sense. Apologies for the misdirection; I'll leave
this as it is.
> # Linux 4.4 on powerpc doesn't support all the seccomp bits that LXD
> # needs.
> if self.arch == "powerpc":
--
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
[Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
The proposal to merge lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd has been updated. Status: Needs review => Merged For more details, see: 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
Re: [Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
@cjwatson, thanks for the clarification on the "resubmit". I'm not sure how you
want the re-factoring on `raw_lxc_config` to look like, so maybe it's better if
you just mold it to your liking. Thanks.
Diff comments:
>
> === modified file 'lpbuildd/target/lxd.py'
> --- lpbuildd/target/lxd.py2018-06-12 23:23:13 +
> +++ lpbuildd/target/lxd.py2018-10-25 09:55:10 +
> @@ -277,16 +277,30 @@
> 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["environment"]["driver_version"]
> +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),
> +])
> +
I'm not entirely sure, how you want this to look like. Feel free to change it
as you see fit. Thanks!
> # Linux 4.4 on powerpc doesn't support all the seccomp bits that LXD
> # needs.
> if self.arch == "powerpc":
--
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
Re: [Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
(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.py2018-06-12 23:23:13 +
> +++ lpbuildd/target/lxd.py2018-10-25 07:43:01 +
> @@ -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 +
> +++ lpbuildd/target/tests/test_lxd.py 2018-10-25 07:43:01 +
> @@ -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", ""),],
> +
[Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
The proposal to merge lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd has been updated. Description changed to: This patch makes the buildd's LXD backend work on Bionic: * Based on LXD version modify raw.lxc settings (some parameter names have changed) * Fix a problem with header checks where uid and gid are set to 0, but need to be strings * Explicitly specify a root device, which seems to be mandatory now For more details, see: 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
Re: [Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
Is there a reason why only tests in lpbuildd/tests are run during package build? -- 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
Re: [Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
Review: Resubmit
@cjwatson, ready for re-review.
Diff comments:
>
> === modified file 'lpbuildd/target/lxd.py'
> --- lpbuildd/target/lxd.py2018-06-12 23:23:13 +
> +++ lpbuildd/target/lxd.py2018-10-25 07:43:01 +
> @@ -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")
> +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),
> +])
> +
The suggestion was to use a dict and sort later. I suppose sorting the list is
going to do the same.
> # 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 +
> +++ lpbuildd/target/tests/test_lxd.py 2018-10-25 07:43:01 +
> @@ -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")
The suggestion was to take the fixture out of the loop, but it needs to be
reset on each iteration, else checks such as
client.profiles.get.assert_called_once_with("lpbuildd") will break.
>
> 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.
___
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
Re: [Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
Review: Needs Fixing
Diff comments:
> === modified file 'lpbuildd/target/lxd.py'
> --- lpbuildd/target/lxd.py2018-06-12 23:23:13 +
> +++ lpbuildd/target/lxd.py2018-09-17 13:33:57 +
> @@ -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 +
> +++ lpbuildd/target/tests/test_lxd.py 2018-09-17 13:33:57 +
> @@ -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",
Re: [Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
@cjwatson, please review. -- 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
[Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
The proposal to merge lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd has been updated. Description changed to: This patch makes the buildd's LXD backend work on Bionic: * Use client host_info, apt cache, lxd --version in that order to find LXD version * Based on LXD version modify raw.lxc settings (some parameter names have changed) * Fix a problem with header checks where uid and gid are set to 0, but need to be strings * Explicitly specify a root device, which seems to be mandatory now Note: in the current state, the packager should add an explicit dependency on python-apt. For more details, see: 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
Re: [Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
Thanks a lot for the initial review! When you find the time, maybe you can skim over this and see if it is better now. I'll see what I can do about the tests. -- 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
[Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
The proposal to merge lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd has been updated. Description changed to: This patch makes the buildd's LXD backend work on Bionic: * Use client host_info, apt cache, lxd --version in that order to find LXD version * Based on LXD version modify raw.lxc settings (some parameter names have changed) * Fix a problem with header checks where uid and gid are set to 0, but need to be strings * Explicitly specify a root device, which seems to be mandatory now For more details, see: 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
Re: [Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
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.py2018-06-12 23:23:13 +
> +++ lpbuildd/target/lxd.py2018-09-05 13:55:08 +
> @@ -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
[Launchpad-reviewers] [Merge] lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd
Tobias Koch has proposed merging
lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd.
Commit message:
bionic compatibility
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~tobijk/launchpad-buildd/launchpad-buildd-bionic/+merge/354331
This patch makes launchpad-buildd work on Bionic.
* Introduce a utility function that allows checking OS VERSION_ID from
/etc/os-release
* Based on major version id (e.g. 18 for Bionic) select matching raw.lxc
settings (some parameter names have changed)
* Fix a problem with header checks where uid and gid are set to 0
* Explicitly specify a root device, which seems to be mandatory now
--
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~tobijk/launchpad-buildd/launchpad-buildd-bionic into lp:launchpad-buildd.
=== modified file 'lpbuildd/target/lxd.py'
--- lpbuildd/target/lxd.py 2018-06-12 23:23:13 +
+++ lpbuildd/target/lxd.py 2018-09-05 13:55:08 +
@@ -28,6 +28,7 @@
from lpbuildd.util import (
set_personality,
shell_escape,
+get_os_release
)
@@ -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:
+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),
+]
# Linux 4.4 on powerpc doesn't support all the seccomp bits that LXD
# needs.
if self.arch == "powerpc":
@@ -305,6 +321,11 @@
"parent": self.bridge_name,
"type": "nic",
},
+"root": {
+"path": "/",
+"pool": "default",
+"type": "disk",
+},
}
self.client.profiles.create(self.profile_name, config, devices)
@@ -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")
+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()
@@ -481,8 +506,8 @@
data = source_file.read()
mode = stat.S_IMODE(os.fstat(source_file.fileno()).st_mode)
headers = {
-"X-LXD-uid": 0,
-"X-LXD-gid": 0,
+"X-LXD-uid": "0",
+"X-LXD-gid": "0",
"X-LXD-mode": "%#o" % mode,
}
try:
=== modified file 'lpbuildd/util.py'
--- lpbuildd/util.py 2017-11-09 12:15:39 +
+++ lpbuildd/util.py 2018-09-05 13:55:08 +
@@ -44,3 +44,27 @@
setarch_cmd.append("--uname-2.6")
return setarch_cmd + args
+
+
+def get_os_release():
+version_id = None
+
+with file("/etc/os-release", "r") as fp:
+for line in fp:
+try:
+key, value = line.strip().split("=", 1)
+except ValueError:
+continue
+
+if key == "VERSION_ID":
+version_id = value.strip('"')
+break
+
+if not version_id:
+raise RuntimeError("Cannot
