-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
a4a6eb8
to
bc3443a
Compare
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>
bc3443a
to
f5ca0c5
Compare
I rebased this after #3827 was merged. |
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 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. |
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.
LGTM; tested with Moby and it seems to resolve the issue with not being able to GC blobs.
type nsFallbackStore struct { | ||
main *Store | ||
fb *Store | ||
} |
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.
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:
buildkit/session/content/attachable.go
Lines 21 to 41 in 8dd5f87
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:
buildkit/session/content/caller.go
Lines 16 to 31 in 8dd5f87
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?
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