deploy: Don't recompute verity checksums if not enabled #3326
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cgwalters
force-pushed
the
hack-deploy-no-verity
branch
from
7829003 to
67e81fd
Compare
allisonkarlitskaya
requested changes
Oct 28, 2024
This fixes a truly horrific performance bug when composefs is enabled, but fsverity is not supported by the filesystem. We'd fall back to doing *userspace* checksumming of all files at deployment time which was absolutely not expected or required. There's really an immense amount of technical debt here, such as the confusion between `ex-integity.composefs` vs the prepare-root config, how we handle "torn" states where some objects don't have verity enabled but some do, etc. The ostree composefs state has two modes: - signed: We need to enforce fsverity - unsigned: Best effort resilience So we fix this by making the deploy path to make verity "opportunistic" - if the ioctl gives us the data, then we add it to the composefs. However, this code path is also invoked when we're computing the expected composefs digest to inject as commit metadata, and *that* API must work regardless of whether the target repo has fsverity enabled as it may operate on a build server. One lucky thing in all of this: When I went to add the "checkout composefs" API I added a stub `GVariant` for options extensibility, which we now use. Signed-off-by: Colin Walters <[email protected]>
cgwalters
force-pushed
the
hack-deploy-no-verity
branch
from
67e81fd to
a6d07b6
Compare
|
One possible followup in #3327 |
|
Re this code alex wrote most of it originally... @alexlarsson mind giving this a quick skim from a high level? But we can't always get his time. |
cgwalters
commented
Oct 29, 2024
|
@allisonkarlitskaya you need to at least change your review status from "changes requested" to "approve" |
allisonkarlitskaya
approved these changes
Oct 29, 2024
This was referenced Oct 30, 2024
Merged
cgwalters
added a commit
to cgwalters/rpm-ostree
that referenced
this pull request
Nov 4, 2024
This effectively reverts coreos@c115600 I was wrong, if we don't have fsverity today on the repo (a common case) then this entails a full read of all files. For more information see ostreedev/ostree#3326
cgwalters
added a commit
to cgwalters/rpm-ostree
that referenced
this pull request
Nov 4, 2024
This effectively reverts coreos@c115600 I was wrong, if we don't have fsverity today on the repo (a common case) then this entails a full read of all files. For more information see ostreedev/ostree#3326 Signed-off-by: Colin Walters <[email protected]>
This was referenced Nov 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes a truly horrific performance bug when
composefs is enabled, but fsverity is not supported by the filesystem. We'd fall back to doing userspace checksumming of all files at deployment time which was absolutely not expected or required.
There's really an immense amount of technical debt here, such as the confusion between
ex-integity.composefsvs the prepare-root config, how we handle "torn" states where some objects don't have verity enabled but some do, etc.The ostree composefs state has two modes:
So we fix this by making the deploy path to make verity "opportunistic" - if the ioctl gives us the data, then we add it to the composefs.
However, this code path is also invoked when we're computing the expected composefs digest to inject
as commit metadata, and that API must work regardless of whether the target repo has fsverity enabled as it may operate on a build server.
One lucky thing in all of this: When I went to add the "checkout composefs" API I added a stub
GVariantfor options extensibility, which we now use.