feat: fail if users try to run managed builds that would use a nonfunctional container #720

mattculler · 2025-01-28T01:00:48Z

Have you followed the guidelines for contributing?
Have you signed the CLA?
Have you successfully run tox?

New LXD versions were released that fix an issue we've seen where a mismatch between the support for cgroupsv1 and v2 between host and guest causes the container's systemd to become partially nonfunctional. Add checks to fail with a good error message if a user's system would launch such a container.

(CRAFT-3096)

mr-cal

Looks good overall. Seems possible to get this done with or without the loopback executor. I'd like to get @lengau's opinion on that, since he was talking about this when architecting craft-application.

craft_providers/lxd/lxc.py

craft_providers/bases/ubuntu.py

mattculler · 2025-01-28T18:27:30Z

Looks good overall. Seems possible to get this done with or without the loopback executor. I'd like to get @lengau's opinion on that, since he was talking about this when architecting craft-application.

Yeah you could. Taking a fresh look at it today I see a good way to refactor Base._get_os_release so the meat of it it reusable without Executor - going to try that instead.

mattculler · 2025-01-28T18:40:54Z

Looks good overall. Seems possible to get this done with or without the loopback executor. I'd like to get @lengau's opinion on that, since he was talking about this when architecting craft-application.

Yeah you could. Taking a fresh look at it today I see a good way to refactor Base._get_os_release so the meat of it it reusable without Executor - going to try that instead.

@mr-cal I think this is better, going this route and removing loopback executor unless you disagree: d2ff086

mr-cal · 2025-01-28T18:52:21Z

@mr-cal I think this is better, going this route and removing loopback executor unless you disagree: d2ff086

I like it. I'm interested in a loopback, but keeping this PR lean will speed up the dev and review process.

…rity

Also: - Fix text return for server version - Don't patch every BuilddBase, because that causes other BuilddBases to become MagicMocks, which breaks the issubclass check in ensure_guest_compatible. - Moved check obviates need for get_server_version calls

lengau · 2025-01-30T15:02:35Z

craft_providers/bases/ubuntu.py

+    # between cgroupv1 and v2 support.
+    # https://discourse.ubuntu.com/t/lxd-5-0-4-lts-has-been-released/49681#p-123331-support-for-ubuntu-oracular-containers-on-cgroupv2-hosts
+    if (
+        host_base_alias > BuilddBaseAlias.FOCAL


I'm not keen on having this be such an Ubuntu-specific solution. We should be able to make the logic generic and just have a data set that we can add to for managing this. For example:

BASE_MINIMUM_REQUIREMENTS = { BuilddBaseAlias.ORACULAR: { "lxd": [(5, 0, 4), (5, 21, 2)], "kernel": [(5, 15)], }, }

Agreed, this design would let us make changes in the future without needing to refactor.

I can make this change, but it does seem to me like we may be making this too generic.

If I do, then this data structure would really need to look like is something like this:

INVALID_VERSIONS = [ { "host": BuilddBase.FOCAL, "guest": BuilddBase.ORACULAR, "lxd": { (4,): (float('inf'), float('inf')), # everything 4.x and below fails (5, 0): (4,), (5, 21): (2,), (6,): (float('-inf'), float('-inf')), # everything 6.x and above passes }, "kernel": [(5, 15)],

This way, your logic can say something like (pseudocode):

for invalid in INVALID_VERSIONS: if host > invalid.host or guest < invalid.guest: continue lxd_version_in_family = lxd_version_match(lxd_version, invalid.lxd.keys()) if not lxd_version_in_family: # Current lxd version doesn't match any of the keys continue # This is a lxd version that may be affected by the bug if lxd_version_fail(lxd_version, lxd_version_in_family): raise Ex('You need a newer lxd for this host/guest') if kernel < invalid.kernel: raise Ex('You need a newer kernel for this host/guest')

Since you and Callahan both agreed that it should look something like this, I'll start down this path.

*Not exactly like that, I'm refining this design as I implement, but this is the gist.

Yeah, this is getting complex fast. Seems like we need a class or library to handle versioning more cleanly. I'd be fine to leave is as you originally implemented.

Also, this code may not be long-lived. It's going to be increasingly rare for someone to use these older minor versions of lxd.

@mr-cal The code I've got right now is working out a little more cleanly than the above would suggest, mainly by making some non-generic assumptions about lxd versions. I wasn't sold on this approach before, but I think what I'm settling on now is better - look for an update shortly.

Could we forget about 5.0.4 and just check if the version tuple >= (5, 21, 2)?

@lengau, what's your rationale for 5.21 and not 5.0? Both are LTS versions

These two commits implement a generic solution:
f4c8161
31dce45

But, I'm torn - I feel like the generic way it's harder to reason about what the code is doing, and we're unlikely to need to add to this INVALID_VERSIONS structure. Thoughts? @lengau @mr-cal ?

@mr-cal my rationale is that anything after 5.21.2 works, but some things greater than 5.0.4 don't work.

craft_providers/bases/ubuntu.py

lengau · 2025-01-30T15:08:50Z

craft_providers/bases/ubuntu.py

+    if lxd_major == 5:
+        # Major is 5, we care about patch versions given the minor
+        if lxd_minor == 0 and lxd_patch < 4:
+            raise lxd_exception


We can detect both the lxd version issue and the kernel version issue in one run and raise both problems simultaneously.

Yeah, this is probably the pattern I'll move towards. The error messages will be a little less specific, but I think that's an acceptable tradeoff here.

craft_providers/lxd/lxc.py

craft_providers/lxd/lxd_provider.py

craft_providers/bases/ubuntu.py

mr-cal

Looks good overall. Please re-request me after addressing Alex's comments

mr-cal · 2025-01-30T16:20:11Z

craft_providers/bases/ubuntu.py

+            raise lxd_exception
+        if lxd_minor == 21 and lxd_patch < 2:
+            raise lxd_exception
+    if lxd_major < 5:


Suggested change

if lxd_major < 5:

elif lxd_major < 5:

This logic will go away with the generic design Alex suggested above.

mr-cal · 2025-01-30T16:21:34Z

craft_providers/bases/ubuntu.py

+    # between cgroupv1 and v2 support.
+    # https://discourse.ubuntu.com/t/lxd-5-0-4-lts-has-been-released/49681#p-123331-support-for-ubuntu-oracular-containers-on-cgroupv2-hosts
+    if (
+        host_base_alias > BuilddBaseAlias.FOCAL


Agreed, this design would let us make changes in the future without needing to refactor.

…bout this

mr-cal

I'm +1 for your new design, readable and seems simpler since it separates this particular cgroups bug away from the if/else statements.

mr-cal · 2025-02-03T17:10:56Z

craft_providers/bases/checks.py

+"""
+
+
+INVALID_VERSIONS = [


Nice, this is very readable

craft_providers/bases/checks.py

tests/unit/bases/test_ensure_guest_compatible.py

lengau · 2025-02-03T17:35:50Z

craft_providers/bases/checks.py

+            (5, 0, 4),
+            (5, 21, 2),
+        ],
+        "kernel_less_than": (5, 15),


How about making this a list of triples as well and using the same function for matching lxd and kernels?

That symmetry would be sort of nice, but the complexity of the lxd logic isn't necessary for the kernel. We can't reason about any minor lxd versions under 5.x other than those listed, so we don't fail them. The kernel, on the other hand, is just a straight-up linear cutoff at 5.15.

It's not necessary now, but it could be necessary in the future, especially given OEM kernels.

feat: rough first pass - untested, want to get earlier feedback

077a8e2

mr-cal reviewed Jan 28, 2025

View reviewed changes

craft_providers/lxd/lxc.py Outdated Show resolved Hide resolved

craft_providers/bases/ubuntu.py Outdated Show resolved Hide resolved

feat: prototype approach without loopback executor

d2ff086

mattculler added 17 commits January 28, 2025 13:52

feat: use existing lxd version getter - also needs less parsing

5878afc

chore: use os-release variable

e8d7b9d

feat: remove loopback executor

0486218

fix: use lxc.get_version to address permission issues, rename for cla…

c14d62d

…rity

fix(tests): unit tests need to know about new call

2a4dd6f

chore(lint): black

84a71c7

fix: loopback executor was removed, directly use subprocess

4c2b3c0

fix: add default value

5a30bc4

fix(lint): black

a4776f8

chore: wordlist

982a130

fix: actually use custom wordlist

70bd295

feat(tests): add unit tests, make code a bit safer/neater

ff4904b

fix: pyright checks

76397c7

chore: remove debug output

71292c2

fix(tests): patch away host file access to fix test on windows

1636a8b

chore: black format

963329d

mattculler marked this pull request as ready for review January 30, 2025 04:53

mattculler requested a review from mr-cal January 30, 2025 04:53

lengau reviewed Jan 30, 2025

View reviewed changes

mattculler linked an issue Jan 30, 2025 that may be closed by this pull request

Set security.nesting=true for LXD projects #591

Open

mr-cal reviewed Jan 30, 2025

View reviewed changes

mattculler added 2 commits January 31, 2025 15:03

feat: redo logic in the generic way suggested by @lengau - not sure a…

f4c8161

…bout this

feat: forgot to git add new file in last commit

31dce45

mr-cal requested review from mr-cal and lengau February 3, 2025 14:38

mr-cal reviewed Feb 3, 2025

View reviewed changes

craft_providers/bases/checks.py

"""

INVALID_VERSIONS = [

Copy link

Collaborator

mr-cal Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is very readable

mattculler reacted with heart emoji

lengau reviewed Feb 3, 2025

View reviewed changes

craft_providers/bases/checks.py Outdated Show resolved Hide resolved

lengau reviewed Feb 3, 2025

View reviewed changes

tests/unit/bases/test_ensure_guest_compatible.py Outdated Show resolved Hide resolved

lengau reviewed Feb 3, 2025

View reviewed changes

mattculler added 6 commits February 3, 2025 14:43

chore: dedent, per @lengau

f293c0e

fix: remove comment

fc18692

chore: lint

53831e4

fix(tests): windows

ac5cd0f

fix: handle "LTS" case

3e137d7

fix: strip

21e3854

mattculler requested review from lengau and mr-cal February 4, 2025 02:24

lengau approved these changes Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fail if users try to run managed builds that would use a nonfunctional container #720

feat: fail if users try to run managed builds that would use a nonfunctional container #720

mattculler commented Jan 28, 2025

mr-cal left a comment

mattculler commented Jan 28, 2025

mattculler commented Jan 28, 2025 •

edited

Loading

mr-cal commented Jan 28, 2025

lengau Jan 30, 2025

mr-cal Jan 30, 2025

mattculler Jan 31, 2025

mattculler Jan 31, 2025

mr-cal Jan 31, 2025

mattculler Jan 31, 2025

lengau Jan 31, 2025

mr-cal Jan 31, 2025

mattculler Jan 31, 2025

lengau Feb 5, 2025

lengau Jan 30, 2025

mattculler Jan 31, 2025

mr-cal left a comment

mr-cal Jan 30, 2025

mattculler Jan 31, 2025

mr-cal Jan 30, 2025

mr-cal left a comment

mr-cal Feb 3, 2025

lengau Feb 3, 2025

mattculler Feb 3, 2025

lengau Feb 5, 2025

feat: fail if users try to run managed builds that would use a nonfunctional container #720

Are you sure you want to change the base?

feat: fail if users try to run managed builds that would use a nonfunctional container #720

Conversation

mattculler commented Jan 28, 2025

mr-cal left a comment

Choose a reason for hiding this comment

mattculler commented Jan 28, 2025

mattculler commented Jan 28, 2025 • edited Loading

mr-cal commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-cal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-cal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattculler commented Jan 28, 2025 •

edited

Loading