bump up containerd (v1.0.0-beta.3) #155
Conversation
|
@AkihiroSuda Not sure if this is the cause for the error but this version should require root labels or leases so that created snapshots don't get garbage collected. |
AkihiroSuda
force-pushed
the
containerd-20171106
branch
from
36805c6
to
31f0964
Compare
AkihiroSuda
changed the title
AkihiroSuda
force-pushed
the
containerd-20171106
branch
2 times, most recently
from
bd5ff58
to
4930a1e
Compare
|
yes, snapshots were garbage collected. updated PR. |
AkihiroSuda
force-pushed
the
containerd-20171106
branch
2 times, most recently
from
a78ae37
to
99b3e28
Compare
cache/cacheimport/import.go
Outdated
| var chain []digest.Digest | ||
| for _, layer := range layers { | ||
| labels := map[string]string{ | ||
| "containerd.io/gc.root": time.Now().UTC().Format(time.RFC3339), |
There was a problem hiding this comment.
Should these timestamps be time.RFC3339Nano?
| @@ -31,8 +31,8 @@ import ( | |||
| type pullDeps struct { | |||
| Snapshotter ctdsnapshot.Snapshotter | |||
| ContentStore content.Store | |||
| Applier rootfs.Applier | |||
| Differ rootfs.MountDiffer | |||
| Applier diff.Differ | |||
There was a problem hiding this comment.
😢 I liked the small interfaces. I guess it doesn't make sense to keep them different in buildkit anymore as well then. (Can be follow-up).
There was a problem hiding this comment.
@dmcgowan WDYT about splitting Applier from Differ again?
There was a problem hiding this comment.
Are there many places we need both interfaces together? We can split and have a combined interface as well.
There was a problem hiding this comment.
@dmcgowan This is the place that creates all the dependencies that are sent to other objects, so nothing actually uses them in here. But, for example, before the pull component only depended on the apply function and image exporter only on the differ. Now there is no separation.
There was a problem hiding this comment.
opened containerd/containerd#1746
| @@ -35,7 +36,7 @@ type SourceOpt struct { | |||
| SessionManager *session.Manager | |||
| Snapshotter snapshot.Snapshotter | |||
| ContentStore content.Store | |||
| Applier rootfs.Applier | |||
| Applier diff.Differ | |||
There was a problem hiding this comment.
This naming seems quite weird now. Although, for the pull this is definitely more understandable as applier, not differ.
source/containerimage/pull.go
Outdated
| var chain []digest.Digest | ||
| for _, layer := range layers { | ||
| labels := map[string]string{ | ||
| "containerd.io/gc.root": time.Now().UTC().Format(time.RFC3339), |
There was a problem hiding this comment.
I think leases containerd/containerd#1690 may be more useful in here. We can have a lease per snapshot and if it has a matching blob put it under the same lease. Then on deletion we can just remove the lease and don't need to track them separately. Also, I think these labels need to be set up in the imageexporter - then the image should become the second root. I'm fine with merging this now and use a tracking issue/follow-up PR to discuss these cases.
There was a problem hiding this comment.
Also if I understand this correctly, currently the manifests would remain roots and that would make the cleanup tricky as buildkit actually doesn't care about the manifests much. The content blobs should remain under a lease with snapshot and if these blobs are part of the exported image, the regular chain of image to blobs (gc.ref.content. labels) is set up.
| c := &containers.Container{ | ||
| ID: id, | ||
| } | ||
| _, ok := namespaces.Namespace(ctx) |
There was a problem hiding this comment.
Can you add a comment about why this is needed? Possibly something that may need further discussion in containerd repo. Very confusing why a GenerateSpec() function requires a container and client parameter.
|
@AkihiroSuda Can you add the comment and make the change @stevvooe requested so we can merge this. We can leave the leases change to follow-up and #157 is already fixing the missing roots on the exporter. |
|
will update today, sorry for the delay |
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
AkihiroSuda
force-pushed
the
containerd-20171106
branch
from
99b3e28
to
c71a1ca
Compare
AkihiroSuda
changed the title
|
updated |
Close #141
Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp