snapshot: add Close() #1728
snapshot: add Close() #1728
Conversation
snapshot/snapshotter.go
Outdated
| @@ -296,6 +296,9 @@ type Snapshotter interface { | |||
| // Walk all snapshots in the snapshotter. For each snapshot in the | |||
| // snapshotter, the function will be called. | |||
| Walk(ctx context.Context, fn func(context.Context, Info) error) error | |||
|
|
|||
| // Close closes the snapshotter. | |||
| Close(ctx context.Context) error | |||
There was a problem hiding this comment.
ctx would not be used in most cases, but I kept this ctx for potential support for cancellation
AkihiroSuda
force-pushed
the
snapshot-closable
branch
2 times, most recently
from
0bf8865
to
041dc33
Compare
|
What does a snapshotter do on close? What is the contract? |
|
@stevvooe My impression is that it releases the internal resources and makes it so that a new snapshotter instance can be created using the same state directory. |
|
I agree with having a |
|
@tonistiigi @dmcgowan Agreed, but let's put that down in writing. Notice that most snapshot methods document their contract. For example, should it always be required to call close? What if close is called twice? |
AkihiroSuda
force-pushed
the
snapshot-closable
branch
from
041dc33
to
914fd3d
Compare
|
removed |
AkihiroSuda
force-pushed
the
snapshot-closable
branch
from
914fd3d
to
47ca82f
Compare
Codecov Report
@@ Coverage Diff @@
## master #1728 +/- ##
==========================================
+ Coverage 48.58% 48.68% +0.09%
==========================================
Files 28 28
Lines 4250 4258 +8
==========================================
+ Hits 2065 2073 +8
Misses 1750 1750
Partials 435 435
Continue to review full report at Codecov.
|
snapshot/snapshotter.go
Outdated
| @@ -296,6 +296,15 @@ type Snapshotter interface { | |||
| // Walk all snapshots in the snapshotter. For each snapshot in the | |||
| // snapshotter, the function will be called. | |||
| Walk(ctx context.Context, fn func(context.Context, Info) error) error | |||
|
|
|||
| // Close releases the internal resources and makes it so that a new | |||
| // snapshotter instance can be created using the same state directory. | |||
There was a problem hiding this comment.
This seems like an implementation detail. Can't multiple snapshotter instance be made from a single state directory?
snapshot/snapshotter.go
Outdated
| // Close releases the internal resources and makes it so that a new | ||
| // snapshotter instance can be created using the same state directory. | ||
| // | ||
| // The most expected usecase of this function is unit testing. |
There was a problem hiding this comment.
I don't think this should be the primary use case of a close function. Should this not be the lifecycle endpoint of the snapshotter?
AkihiroSuda
force-pushed
the
snapshot-closable
branch
from
47ca82f
to
ff1ccd6
Compare
AkihiroSuda
changed the title
|
@stevvooe updated |
snapshot/snapshotter.go
Outdated
| // Close is expected to be called on the end of the lifecycle of the snapshotter, | ||
| // but not mandatory. | ||
| // | ||
| // Without calling Close, a new snapshotter instance may not be |
There was a problem hiding this comment.
Again, this is an implementation detail and doesn't belong here. Snapshotters do not know about state directories and other details about how they're implemented.
snapshot/snapshotter.go
Outdated
| // Without calling Close, a new snapshotter instance may not be | ||
| // created using the same state directory, depending on the implementation. | ||
| // | ||
| // The behavior of Close after the first call is undefined. (see io.Closer godoc) |
There was a problem hiding this comment.
Let's make it safe to call this multiple times.
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
AkihiroSuda
force-pushed
the
snapshot-closable
branch
from
ff1ccd6
to
4feb6f2
Compare
|
@stevvooe |
|
LGTM |
For containerd/containerd#1728 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
For containerd/containerd#1728 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
For containerd/containerd#1728 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp
ref: https://github.com/moby/buildkit/pull/155/files#diff-86435c84a0aac9739ad7fcc45ca98579R220
cc @tonistiigi