Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include OS version and features in OCI platforms #4387

Merged

Conversation

TBBle
Copy link
Collaborator

@TBBle TBBle commented Oct 27, 2023

This fixes a bunch of places where these OCI fields were being stripped or lost during processing.

It hasn't been visible until now as they are nil on all Linux platforms as far as I know, but WCOW needs them to select the appropriate image for FROM from a manifest list, and also probably in other places that I haven't tested.

Draft because of one TODO to be completed/improved, and because I suspect this will warrant discussion. I'd also like to see if CI passes, in case I'm wrong about this not affecting Linux at all, e.g., unexpected digest shifts.

solver/llbsolver/vertex.go Outdated Show resolved Hide resolved
@profnandaa
Copy link
Collaborator

Thanks for the fix!

@TBBle TBBle force-pushed the include-OS-version-and-features-in-platforms branch 2 times, most recently from 20f2fe1 to ca6d8c5 Compare November 2, 2023 14:57
@TBBle TBBle marked this pull request as ready for review November 2, 2023 14:57
client/llb/marshal.go Outdated Show resolved Hide resolved
Trivially created by looking for every reference to .Variant and adding
OSVersion and OSFeatures, except the ones related to the string
representation of a Platform instance.

I then went through and ensured every assignment of OSFeatures that
might leak out, i.e., not local-only or for marhsalling purposes, uses
the append-to-nil idiom to avoid sharing the slice storage and allowing
accidental mutation after-the-fact.

Signed-off-by: Paul "TBBle" Hampson <[email protected]>
@TBBle TBBle force-pushed the include-OS-version-and-features-in-platforms branch from ca6d8c5 to 98e0d8d Compare November 3, 2023 03:20
@TBBle
Copy link
Collaborator Author

TBBle commented Nov 3, 2023

Single test failure in TestRuncWorkerExec, running ps -o pid,comm in a container. I can't see how that could be related to my changes, particularly since I don't think the runc worker ever cares about the Platform object. Didn't occur on rerun.

@tonistiigi tonistiigi merged commit 6a0cd7c into moby:master Nov 4, 2023
57 checks passed
@TBBle TBBle deleted the include-OS-version-and-features-in-platforms branch November 4, 2023 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants