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

llbsolver: move history blobs to a separate namespace #3833

Merged
merged 2 commits into from
May 24, 2023

Conversation

tonistiigi
Copy link
Member

Migrate history objects to separate namespace to holding
reference to a blob does not interfere with the GC labels
held for the same blobs by the containerd image store.

fixes #3797

@vvoland

Migrate history objects to separate namespace to holding
reference to a blob does not interfer with the GC labels
held for same blobs by the containerd image store.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Linter does not understand these and shows bogus warnings.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi tonistiigi marked this pull request as ready for review May 12, 2023 06:54
@tonistiigi
Copy link
Member Author

I rebased this after #3827 was merged.

Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

I was a bit worried about the migration at init but it's quite fast, even for large history.

@jedevc jedevc self-requested a review May 17, 2023 16:24
@tonistiigi tonistiigi added this to the v0.12.0 milestone May 19, 2023
@tonistiigi
Copy link
Member Author

I was a bit worried about the migration at init but it's quite fast, even for large history.

The data doesn't actually move. It is just juggling with the lease objects.

Copy link
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM; tested with Moby and it seems to resolve the issue with not being able to GC blobs.

@crazy-max crazy-max merged commit ecae985 into moby:master May 24, 2023
54 checks passed
Comment on lines +100 to +103
type nsFallbackStore struct {
main *Store
fb *Store
}
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, could we potentially use an implementation that was more similar to the content store multiplexing we already have in buildkit (added in #615)?

On the client:

type attachableContentStore struct {
stores map[string]content.Store
}
func (cs *attachableContentStore) choose(ctx context.Context) (content.Store, error) {
md, ok := metadata.FromIncomingContext(ctx)
if !ok {
return nil, errors.Wrap(errdefs.ErrInvalidArgument, "request lacks metadata")
}
values := md[GRPCHeaderID]
if len(values) == 0 {
return nil, errors.Wrapf(errdefs.ErrInvalidArgument, "request lacks metadata %q", GRPCHeaderID)
}
id := values[0]
store, ok := cs.stores[id]
if !ok {
return nil, errors.Wrapf(errdefs.ErrNotFound, "unknown store %s", id)
}
return store, nil
}

On the server:
type callerContentStore struct {
store content.Store
storeID string
}
func (cs *callerContentStore) choose(ctx context.Context) context.Context {
nsheader := metadata.Pairs(GRPCHeaderID, cs.storeID)
md, ok := metadata.FromOutgoingContext(ctx) // merge with outgoing context.
if !ok {
md = nsheader
} else {
// order ensures the latest is first in this list.
md = metadata.Join(nsheader, md)
}
return metadata.NewOutgoingContext(ctx, md)
}

I think we need to still need to have the fallback store as the default, to emulate v0.11 buildkit behavior, but I think maybe moving forwards the client should be able to dictate which store to use?

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.

Copy History API objects to a different namespace
4 participants