Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
enhancements: Propose CoreOS layering #7
enhancements: Propose CoreOS layering #7
Changes from all commits
b2ec69f
1fe203b
b7608f8
03e25fa
1bcd3b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if there was a halfway approach where we do support the
Dockerfile
approach but the only supported operation is still to just apply a Butane config, potentially with some locally referenced files that were copied from a previous build stage.Personally, my primary concerns with the "freehand"
Dockerfile
approach is that:--allow-base-overrides
or something?Focusing on Butane configs as the layering mechanism in my opinion helps with both of those because it's more declarative and so easier to analyze, and it's the same configuration language we've been speaking so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I'm OK with this too, and I know we can always tweak things going forward (though it's always harder to add restrictions than it is to remove them). Just wanted to point out what IMO are some non-negligible risks of this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not quite true, because we will still have the original base image. Reverting back to the "golden FCOS base" will also be a just an
rpm-ostree rebase
away.Additionally, because we will have
yum|dnf -> rpm-ostree
in this image (xref https://coreos.github.io/rpm-ostree/cliwrap/) we can actually still impose theinstall
vsoverride
semantic.Further, tooling can perform filesystem and rpmdb diffing between layers - we have all that code today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, today one can use any tool to generate Ignition, which definitely happens (e.g.
openshift-install
generates Ignition via some custom Terraform stuff) and we support that.More broadly I think we are in a position of needing to support "low level" as well as arbitrary configuration mechanisms, but we also try to have opinions on higher level tooling.
So here
Dockerfile
is that lowest common denominator for arbitrary, low level (kinda) mechanism - but we can streamline higher level workflows inside of that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but the power a user wields in their
Dockerfile
sees it all as just one filesystem. E.g. nothing stops them from doingCOPY my-libc.so /usr/lib64/libc.so
and the build system will be happy to build that. Whereas if it's Butane based, we can immediately tell they're modifying base content and fail the build. So the failure happens at image build time instead of deploy and reboot time. Of course, we do want to provide flexibility to modify base content, but IMO it should be explicitly opted in.Combining this with ostreedev/ostree-rs-ext#159, maybe we could have this
ostree container finalize
step take that switch? E.g.ostree container finalize --allow-overrides
.Hmm, can you expand on the UX you're thinking of here?
dnf install
will automatically upgrade an already installed package so we'd have to require some added switch (related: coreos/rpm-ostree#2844 (comment)).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the cliwrap status quo today errors actually when someone types
dnf install
. I know this is a perpetually confusing thing but so far cliwrap does not involve necessarily invoking traditionalyum|dnf
logic. It opens the door to that of course.So specifically if e.g. someone times
RUN yum install usbguard
in theirDockerfile
, we would error out, and they'd have to doRUN rpm-ostree install usbguard
which would run through all of the same logic that exists today which would only layer, not upgrade dependencies. (There's...a lot implied in this whole "rpm-ostree in container builds" flow, really worth splitting out to a separate issue; will look at that)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real status quo right now to be clear is that
dnf install usbguard
will give "command not found" andrpm-ostree install usbguard
will error out because you're in a container. Andrpm -Uvh https://example.com/usbguard.rpm
will work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coreos/rpm-ostree#3227
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, I like that.
What I'm trying to make sure here is that OCP customers who will be interacting with this feature are still subject to basic rules by default so that our QA efforts aren't trivially invalidated. I.e. I don't want them to be able to change
/usr
base content just as easily as they can/etc
content. If the only way OCP customers interact with this is through Butane or MCs for example, then I think that's fine. Otherwise, I think we should make them opt into modifying base content explicitly (and maybe e.g. we only support them doing this if they have a support exception).Taking this feature in isolation (e.g. for FCOS users), typing
rpm-ostree rebase $my_pullspec
already implies some risk acceptance (though there's still value in keeping guardrails there too of course). But in the OCP case, the MCO will be what's running that command.