Update cimfs snapshotter & differ for new hcsshim interface #10033
Conversation
|
Hi @ambarve. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
ambarve
force-pushed
the
cimfs_layer_refactor
branch
from
be2ed19
to
e0f787b
Compare
| @@ -1 +1 @@ | |||
| v0.12.0 | |||
| 1d406d0eac5573287ba7b46a04a58275410137ac | |||
There was a problem hiding this comment.
Is it possible to get a release with the changes? 2.0 doesn't seem too far off so it'd be nice to stay on a tag
There was a problem hiding this comment.
Hmm, I wasn't planning to get this in containerd 2.0 since the rc builds are already out. That's why we didn't get a release on hcsshim. Do we still need a tag if we don't want to get this in 2.0?
There was a problem hiding this comment.
Maybe I've forgotten the release process but I'd think if this got checked in anytime before we decide to tag 2.0 then it'd be part of the release
There was a problem hiding this comment.
Yeah, it does look like merging this right now will get it into 2.0. I have confirmed that getting this into 2.0 shouldn't be a problem.
There was a problem hiding this comment.
Gotcha.
I have confirmed that getting this into 2.0 shouldn't be a problem.
You mean the commit instead of a tag?
There was a problem hiding this comment.
We don't need to get this into 2.0, we can merge this after the final 2.0 tag is cut. At that time, we can merge with a commit ID of hcsshim and then when containerd is preparing for a 2.1 release we can make a separate hcsshim tag at that time and vendor that. Does that sound okay?
There was a problem hiding this comment.
Sounds fine to me at least, I'm not sure if any other maintainers would prefer us stay on a tag.
There was a problem hiding this comment.
I agree it would be best to keep a tagged release here.
|
Needs a rebase |
ambarve
force-pushed
the
cimfs_layer_refactor
branch
from
e0f787b
to
abedcaf
Compare
go.mod
Outdated
| @@ -7,7 +7,7 @@ require ( | |||
| github.com/AdaLogics/go-fuzz-headers v0.0.0-20230811130428-ced1acdcaa24 | |||
| github.com/AdamKorcz/go-118-fuzz-build v0.0.0-20230306123547-8075edf89bb0 | |||
| github.com/Microsoft/go-winio v0.6.2 | |||
| github.com/Microsoft/hcsshim v0.12.3 | |||
| github.com/Microsoft/hcsshim v0.12.1-0.20240326215926-1d406d0eac55 | |||
There was a problem hiding this comment.
I think we'd prefer to vendor a tagged release.
core/mount/mount_windows.go
Outdated
| @@ -130,6 +141,40 @@ func (m *Mount) GetParentPaths() ([]string, error) { | |||
| return parentLayerPaths, nil | |||
| } | |||
|
|
|||
| // gets the paths of the parent cims of this mount | |||
| func (m *Mount) GetParentCimPaths() ([]string, error) { | |||
There was a problem hiding this comment.
I would prefer these functions be defined in the cimfs snapshotter package, and take a Mount as input, rather than being attached to the type. We probably don't want Mount to end up with functions hanging off of it for specific mount types, it should stay generic.
plugins/snapshots/windows/cimfs.go
Outdated
| } | ||
| return false, err | ||
| func (s *cimFSSnapshotter) parentIDsToCimPaths(parentIDs []string) []string { | ||
| cimPaths := []string{} |
There was a problem hiding this comment.
nit: make([]string, 0, len(parentIDs))
| } else if ok { | ||
| cimUsage, err := cimfs.GetCimUsage(ctx, getCimLayerPath(s.cimDir, id)) | ||
| if info.Kind == snapshots.KindCommitted { | ||
| // Committed MUST be a cimfs layer |
There was a problem hiding this comment.
I think we are saying not just that committed -> cimfs, but also cimfs -> committed, right?
There was a problem hiding this comment.
Yes, here the comment just suggests that a committed snapshot means we should add the usage of cim files too.
| } | ||
| // if this is an image layer being extracted include a cim path in which the layer | ||
| // will be extracted. | ||
| if strings.Contains(key, snapshots.UnpackKeyPrefix) { |
There was a problem hiding this comment.
It would be nice to avoid keying more stuff off of UnpackKeyPrefix. Is there any way we can avoid this?
There was a problem hiding this comment.
I have updated this to use the snapshot digest label that is passed with each Prepare call during image extraction. I think it is slightly better than checking for the key prefix. Let me know what you think.
There was a problem hiding this comment.
Actually, there are two other places (Commit & Mounts) where we still need to use the UnpackKeyPrefix. I am trying to figure out if we can easily change it there too. But if not, would you be okay with making this change in a separate PR?
ambarve
force-pushed
the
cimfs_layer_refactor
branch
from
abedcaf
to
ca0c82e
Compare
hcsshim recently [updated](microsoft/hcsshim@1d406d0) the interface of APIs that are used for importing OCI layers. Plus, it now expects that the CimFS snapshotter mounts contain the full cim paths for parent layers. This change updates the cimfs differ & snapshotter to use that new interface. Signed-off-by: Amit Barve <ambarve@microsoft.com>
ambarve
force-pushed
the
cimfs_layer_refactor
branch
from
ca0c82e
to
6e1f0ee
Compare
hcsshim recently updated the interface of APIs that are used for importing OCI layers. Plus it now expects that the CimFS snapshotter mounts contain the full cim paths for parent layers. This change updates the cimfs differ & snapshotter to use that new interface.