container: Remove RWLayer#46771
Conversation
vvoland
added
area/daemon
vvoland
force-pushed
the
container-remove-rwlayer
branch
4 times, most recently
from
7920ea3 to
d309522
Compare
vvoland
force-pushed
the
container-remove-rwlayer
branch
from
d309522 to
1154e9b
Compare
Don't store the RWLayer in the Container object - it's an implementation detail of the graphdriver ImageService and it shouldn't be exposed. The ID of the layer is always the same as the container ID so it can be used to move the direct RWLayer accesses to the graphdriver's ImageService implementation. Signed-off-by: Paweł Gronowski <[email protected]>
vvoland
force-pushed
the
container-remove-rwlayer
branch
from
1154e9b to
e6a6591
Compare
vvoland
requested review from
corhere,
cpuguy83,
dmcgowan,
laurazard,
neersighted,
rumpl and
thaJeztah
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Just some rambling from glancing over the changes.
|
|
||
| // CreateLayer creates a filesystem layer for a container. | ||
| // called from create.go | ||
| // TODO: accept an opt struct instead of container? |
There was a problem hiding this comment.
I see this TODO was removed; as we're changing the signature, is that still something we should consider?
| } | ||
|
|
||
| // ReleaseLayer releases a layer allowing it to be removed | ||
| // ReleaseLayer releases the snapshot allowing it to be removed. |
There was a problem hiding this comment.
This is confusing; ReleaseLayer releasing a snapshot (not a "layer"). How deeply is this function engrained elsewhere? Should we either have separate functions for containerd <--> graphdriver, or would a different (more generic) name work for the context it's used in?
| ls := i.client.LeasesService() | ||
| lease := leases.Lease{ | ||
| ID: ctr.ID, | ||
| } | ||
| err := ls.Delete(ctx, lease, leases.SynchronousDelete) |
There was a problem hiding this comment.
nit; I guess we can inline all of the above here (I think that's still readable, even if we inline the client?)
| ls := i.client.LeasesService() | |
| lease := leases.Lease{ | |
| ID: ctr.ID, | |
| } | |
| err := ls.Delete(ctx, lease, leases.SynchronousDelete) | |
| err := i.client.LeasesService().Delete(ctx, leases.Lease{ID: ctr.ID}, leases.SynchronousDelete) |
| if err != nil && cerrdefs.IsNotFound(err) { | ||
| return nil | ||
| } | ||
| return err |
There was a problem hiding this comment.
I'm guessing the err != nil is even redundant here (but perhaps more readable this way 😅)
Alternatively, we could do the reverse, which would make the final return explicit that there's no error;
| if err != nil && cerrdefs.IsNotFound(err) { | |
| return nil | |
| } | |
| return err | |
| if err != nil && !cerrdefs.IsNotFound(err) { | |
| return err | |
| } | |
| return nil |
| @@ -86,10 +87,8 @@ func (i *ImageService) CountImages(ctx context.Context) int { | |||
| } | |||
|
|
|||
| // CreateLayer creates a filesystem layer for a container. | |||
There was a problem hiding this comment.
I guess the same applies here; if this is creating a snapshot.
Wondering if we should consider removing the abstraction, and instead had separate methods for "Snapshot" vs "graphdriver".
| // called from create.go | ||
| // TODO: accept an opt struct instead of container? | ||
| func (i *ImageService) CreateLayer(container *container.Container, initFunc layer.MountInit) (layer.RWLayer, error) { | ||
| func (i *ImageService) CreateLayer(ctx context.Context, container *container.Container, _ string, _ *ocispec.Platform, setupInit func(string) error) error { |
There was a problem hiding this comment.
Related to the "abstraction". From this it really looks like the abstraction no longer really "works". We're now have to change the signature to effectively be for "snapshot", and more and more stubbing for graphdrivers. Perhaps diversifying starts to be a better options 🤔 (which would also allow for more descriptive naming, clearer on intent)
| is "gotest.tools/v3/assert/cmp" | ||
| ) | ||
|
|
||
| func TestGetInspectData(t *testing.T) { |
There was a problem hiding this comment.
This test was no longer relevant? Or is there something else covering this?
|
2 participants
Don't store the RWLayer in the Container object - it's an implementation detail of the graphdriver ImageService and it shouldn't be exposed. The ID of the layer is always the same as the container ID so it can be used to move the direct RWLayer accesses to the graphdriver's ImageService implementation.
- What I did
Removed
RWLayerfield fromContainerstruct and removed some uglyif daemon.UsesSnapshotter()branches.- How I did it
Instead of storing
RWLayerand accessing it when needed, obtain it directly from the layerStore. The ID of the RW layer always matches the container ID.- How to verify it
CI.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)