-
Notifications
You must be signed in to change notification settings - Fork 305
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
ostree/summary: Generate an ostree-metadata ref when updating summary #1158
ostree/summary: Generate an ostree-metadata ref when updating summary #1158
Conversation
The original introduction of |
@rh-atomic-bot, retest this please |
g_autoptr(OstreeMutableTree) mtree = NULL; | ||
g_autoptr(OstreeRepoFile) repo_file = NULL; | ||
g_autoptr(GVariantDict) new_summary_commit_dict = NULL; | ||
g_autoptr(GVariant) new_summary_commit = NULL; |
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.
We prefer declare-and-initialize for new code; doesn't have to be done in this patch but someone will probably do it later if not.
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.
This code was copied from the almost-identical implementation I did in flatpak; so I’d prefer not to rewrite it on import.
src/ostree/ot-builtin-summary.c
Outdated
return FALSE; | ||
|
||
mtree = ostree_mutable_tree_new (); | ||
if (!ostree_repo_write_dfd_to_mtree (repo, AT_FDCWD, ".", mtree, NULL, NULL, error)) |
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.
Hmm? I don't think we should be relying on the process' cwd here, right? It looks like that cwd is the entire unit test directory at least from the tests.
Are you intending to include any content in the metadata ref? If not, the libarchive code has some "create empty directory" code.
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.
Oops. The intention is to not include any content in the commit, correct. I can’t see anything immediately obvious for this in the libarchive code. Which code do you mean, exactly?
I need to fix flatpak to not use AT_FDCWD, "."
too, once this is done.
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.
It's this bit:
/* If we didn't import anything at all, and autocreation of parents
* is enabled, automatically create a root directory. This is
* useful primarily when importing Docker image layers, which can
* just be metadata.
*/
if (opts->autocreate_parents &&
ostree_mutable_tree_get_metadata_checksum (mtree) == NULL)
{
glnx_unref_object GFileInfo *fi = g_file_info_new ();
g_file_info_set_attribute_uint32 (fi, "unix::uid", 0);
g_file_info_set_attribute_uint32 (fi, "unix::gid", 0);
g_file_info_set_attribute_uint32 (fi, "unix::mode", DEFAULT_DIRMODE);
g_autoptr(GFileInfo) mfi = NULL;
(void)_ostree_repo_commit_modifier_apply (self, modifier, "/",
fi, &mfi);
if (!aic_ensure_parent_dir_with_file_info (&aictx, mtree, "/", mfi, NULL,
cancellable, error))
goto out;
}
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.
Great, thanks. I’ve updated the patch and added a test.
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.
I’ll fix flatpak tomorrow; need to head off now.
src/ostree/ot-builtin-summary.c
Outdated
if (!ostree_repo_write_mtree (repo, mtree, (GFile **) &repo_file, NULL, error)) | ||
return FALSE; | ||
|
||
if (!ostree_repo_write_commit (repo, old_ostree_metadata_checksum, |
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.
What's the rationale behind the commit having a parent? In what cases would one want old metadata?
I guess with this administrators could do ostree --repo=repo prune --only-branch=ostree-metadata --depth=0
when they wanted, but still.
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.
@pwithnall Did you have an opinion on this issue? (Ah, I see the unit tests rely on it)
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 unit tests don’t rely on it inasmuch as they test that the code implements it.
The rationale for metadata commits having a parent is that it allows the metadata history of the repository to be represented, at fairly low cost. As you say, admins can always prune it later if they want.
const char *collection_id = NULL; | ||
#endif /* OSTREE_ENABLE_EXPERIMENTAL_API */ | ||
|
||
/* Write out a new metadata commit for the repository. */ |
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.
Can you explain a bit what metadata is being included here? It looks it's basically a stub, but callers could use --add-metadata
?
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.
additional_metadata
is passed into the vardict for new_summary_commit
, so the stored metadata is identical to that in the summary
file (plus the collection/ref binding keys). By default, that is indeed nothing, unless the user provides an --add-metadata
option.
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.
Hmm, but if the intention is to get rid of the summary file down the line, don't we want the list of refs/deltas?
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 intention is not to get rid of the summary file down the line, since it remains a useful cache for the list of refs/deltas. The ostree-metadata
ref is only replacing the additional metadata from the summary
.
Is it OK if this waits till the next release cycle? |
Sure, although we might carry it as a patch in our OSTree package until then, so if you have any architectural issues to raise with it, please do so now. I don’t want our OSTree package to end up diverging from upstream. |
I'm going to mark this as WIP since it feels like it needs a bit of design. Are we agreed that this replaces the whole summary file down the line? Maybe we'll need infrastructure to re-synthesize something that looks summary-file like for the pull code? One thing I'd hope we can achieve here is split up the metadata into separate chunks to scale better. |
Nope, the intention is that this only replaces the additional metadata in the The deltas are currently stored in the additional metadata map (right? I haven’t checked), so I guess they end up moving to |
(Would it help to have a call about this?) |
One thing that occurred to me from our discussion is that if we want to deprecate the summary file, perhaps we need an |
What are you suggesting this |
Signed-off-by: Philip Withnall <[email protected]>
There is no error handling to do, so just return everywhere instead. Signed-off-by: Philip Withnall <[email protected]>
This is the new way of publishing repository metadata, rather than as additional-metadata in the summary file. The use of an ostree-metadata ref means that the metadata from multiple upstream collections is not conflated when doing P2P mirroring of many repositories. The new ref is only generated if the repository has a collection ID set. The old summary file continues to be generated for backwards compatibility (and because it continues to be the canonical ref → checksum map for the repository). The new code is only used if configured with --enable-experimental-api. Includes unit tests. Signed-off-by: Philip Withnall <[email protected]>
0a13fd6
to
f126610
Compare
Rebased against master and squashed in the fixups. Ready for a second round of review. |
{ | ||
const char *key_id = *iter; | ||
|
||
if (!ostree_repo_sign_commit (repo, |
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.
Generating, signing, and committing in one go makes doing signing asynchronously harder. At least in Fedora releng does signing async. But this is something we can enhance later.
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.
If opt_key_ids == NULL
then signing won’t happen as part of this utility call.
There is no error handling to do, so just return everywhere instead. Signed-off-by: Philip Withnall <[email protected]> Closes: #1158 Approved by: cgwalters
This is the new way of publishing repository metadata, rather than as additional-metadata in the summary file. The use of an ostree-metadata ref means that the metadata from multiple upstream collections is not conflated when doing P2P mirroring of many repositories. The new ref is only generated if the repository has a collection ID set. The old summary file continues to be generated for backwards compatibility (and because it continues to be the canonical ref → checksum map for the repository). The new code is only used if configured with --enable-experimental-api. Includes unit tests. Signed-off-by: Philip Withnall <[email protected]> Closes: #1158 Approved by: cgwalters
☀️ Test successful - status-atomicjenkins |
When building the ostree-metadata branch (which only happens when configured with --enable-p2p), we are supposed to create empty commits which contain only metadata. However, the code to do this was wrong, and was instead pulling in all the files from the current working directory and committing them. Fix that code to actually create an empty commit. This could have been a fairly serious bug were it not for the fact that nobody’s using this code because it’s all experimental. Spotted as part of ostreedev/ostree#1158. Signed-off-by: Philip Withnall <[email protected]>
When building the ostree-metadata branch (which only happens when configured with --enable-p2p), we are supposed to create empty commits which contain only metadata. However, the code to do this was wrong, and was instead pulling in all the files from the current working directory and committing them. Fix that code to actually create an empty commit. This could have been a fairly serious bug were it not for the fact that nobody’s using this code because it’s all experimental. Spotted as part of ostreedev/ostree#1158. Signed-off-by: Philip Withnall <[email protected]> Closes: #1066 Approved by: alexlarsson
When building the ostree-metadata branch (which only happens when configured with --enable-p2p), we are supposed to create empty commits which contain only metadata. However, the code to do this was wrong, and was instead pulling in all the files from the current working directory and committing them. Fix that code to actually create an empty commit. This could have been a fairly serious bug were it not for the fact that nobody’s using this code because it’s all experimental. Spotted as part of ostreedev/ostree#1158. Signed-off-by: Philip Withnall <[email protected]> Closes: #1066 Approved by: alexlarsson
This is the new way of publishing repository metadata, rather than as
additional-metadata in the summary file. The use of an ostree-metadata
ref means that the metadata from multiple upstream collections is not
conflated when doing P2P mirroring of many repositories.
The new ref is only generated if the repository has a collection ID set.
The old summary file continues to be generated for backwards
compatibility (and because it continues to be the canonical ref →
checksum map for the repository).
The new code is only used if configured with --enable-experimental-api.
Includes unit tests.
Signed-off-by: Philip Withnall [email protected]