gc: resource leases #1690
gc: resource leases #1690
Conversation
|
@stevvooe gave the feedback of renaming these to leases instead of transactions |
95fda14 to
8c05c65
Compare
e17163b to
50bc32a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1690 +/- ##
==========================================
- Coverage 48.73% 48.59% -0.14%
==========================================
Files 27 28 +1
Lines 4073 4249 +176
==========================================
+ Hits 1985 2065 +80
- Misses 1670 1749 +79
- Partials 418 435 +17
Continue to review full report at Codecov.
|
api/services/leases/v1/leases.proto
Outdated
|
|
||
| map<string, string> labels = 3; | ||
|
|
||
| repeated Snapshot snapshots = 4; |
There was a problem hiding this comment.
Why are snapshots special?
There was a problem hiding this comment.
I was trying to avoid exposing another microformat for specifying resources. Today in the API we always have snapshotter and snapshot key broken out. We could combine it into a string, but then the client might need to parse it. We combine them into strings a few times in the backend, but do not persist them like that. Do you have an opinion or better suggestion for what to do here?
There was a problem hiding this comment.
What if we had an object reference:
message ObjectRef {
string plugin_type = 1;
string plugin_id = 2; // ie btrfs, local, etc.
string type = 3; // ie snapshot, blob
string key = 4; // identifier, digest, etc.
}
There was a problem hiding this comment.
I think a reference object makes sense. I am going to remove both of these fields for now and will add them with a proper reference type when I add support in ctr for managing leases
| } | ||
|
|
||
| // CreateLease creates a new lease | ||
| func (c *Client) CreateLease(ctx context.Context) (Lease, error) { |
There was a problem hiding this comment.
Should Lease be a pointer since it has the client on it?
There was a problem hiding this comment.
Doesn't seem necessary since the client is already a pointer
|
|
||
| // WithLease sets a given lease on the context | ||
| func WithLease(ctx context.Context, lid string) context.Context { | ||
| ctx = context.WithValue(ctx, leaseKey{}, lid) |
There was a problem hiding this comment.
What appends if a context already has a lease? Since the client is creating leases for some actions what will happen if I create a lease and call these methods?
There was a problem hiding this comment.
I was thinking about this, I might just add a helper function in the client which first checks if there is a lease, and uses it if it already has one, rather than always creating, setting, and deleting it.
There was a problem hiding this comment.
Maybe just make this function not add a new lease if one is already set?
There was a problem hiding this comment.
The only problem with that is this is called after CreateLease
There was a problem hiding this comment.
Should CreateLease return the context as well? Maybe lease implements context.Context?
There was a problem hiding this comment.
I added a commit which adds that helper function I was thinking of. We could change the interface to expose that function as the only way to create leases if it is more user friendly.
api/services/leases/v1/leases.proto
Outdated
|
|
||
| repeated Snapshot snapshots = 4; | ||
|
|
||
| repeated string content = 5; |
There was a problem hiding this comment.
Should this get the digest type?
f0f1074 to
723d624
Compare
|
Anymore interface or API feedback? We might need to improve the client feedback when |
|
@dmcgowan The GRPC API looks good. It it simple and let's us extend with future additions (ie pre-add all content client-side before including it). |
723d624 to
269c135
Compare
| @@ -41,6 +41,55 @@ func scanRoots(ctx context.Context, tx *bolt.Tx, nc chan<- gc.Node) error { | |||
| nbkt := v1bkt.Bucket(k) | |||
| ns := string(k) | |||
|
|
|||
| lbkt := nbkt.Bucket(bucketKeyObjectLeases) | |||
There was a problem hiding this comment.
L23:
labelGCRoot = []byte("containerd.io/gc.root")Do we need to keep gc.root?
There was a problem hiding this comment.
Yes, we will continue to support the gc.root label, it just doesn't need to be part of the normal client flows. Client's can still use these labels to retain content not otherwise referenced
| t := time.Now() | ||
| var b [3]byte | ||
| // Ignore read failures, just decreases uniqueness | ||
| rand.Read(b[:]) |
There was a problem hiding this comment.
git grep rand.Seed looks empty.
Please add seed initialization to cmd/containerd/main.go?
There was a problem hiding this comment.
This is using crypto/rand, the ignoring of the read failure accounts for cases such as not enough entropy. Since there is already a nanosecond granular time component, a failure to get a unique value here is not a huge concern.
api/services/leases/v1/leases.proto
Outdated
| } | ||
|
|
||
| message CreateRequest { | ||
| string id = 1; |
There was a problem hiding this comment.
Please add a doc string: when id is not set, the service creates a lease with a random id
|
SGTM |
269c135 to
22a7987
Compare
Signed-off-by: Derek McGowan <derek@mcgstyle.net>
Signed-off-by: Derek McGowan <derek@mcgstyle.net>
Signed-off-by: Derek McGowan <derek@mcgstyle.net>
22a7987 to
07885f1
Compare
|
LGTM |
| @@ -326,6 +326,10 @@ func (s *snapshotter) createSnapshot(ctx context.Context, key, parent string, re | |||
| return err | |||
| } | |||
|
|
|||
| if err := addSnapshotLease(ctx, tx, s.name, key); err != nil { | |||
There was a problem hiding this comment.
I am studying the lease. I have a question.
When we call sn.Prepare,we add the key to lease bucket.
On the time, the key in snapshot bucket agree with it in lease bucket.
But the key is not chainID. E.g. It is not chainID but "extract-%s %s" in ApplyLayer function.
When call sn.Commit, the key in the snapshot bucket be changed to chainID, but we don't change the key which is in lease bucket.
Is there a window where the gc will collect the snapshot.
@dmcgowan PTAL
func ApplyLayer(ctx context.Context, layer Layer, chain []digest.Digest, sn snapshot.Snapshotter, a diff.Differ
key := fmt.Sprintf("extract-%s %s", uniquePart(), chainID)
// Prepare snapshot with from parent, label as root
mounts, err := sn.Prepare(ctx, key, parent.String(), opts...)
if err = sn.Commit(ctx, chainID.String(), key, opts...); err != nil {
There was a problem hiding this comment.
@yanxuean you are correct, we should be adding the committed objects to the lease as well. Committed snapshots are usually protected by being referenced by images, containers, or other snapshots, however you are right to assume that there is a window before the references are created in which these could be GC'ed if they are not added to the lease. I will update this as part of another lease PR I am working on. Thanks for reviewing the code!
| return leases, nil | ||
| } | ||
|
|
||
| func (c *Client) withLease(ctx context.Context) (context.Context, func() error, error) { |
There was a problem hiding this comment.
Can we provide withLease() to the user for using? @dmcgowan
We need to import image from tar in cri-containerd. so we need it.
Or you can give me some suggestion. Thanks.
Adds a lease api for creating, deleting, and listing resource leases. These leases prevent the garbage collector from cleaning up resources created with the lease.
WIP: Please review API