-
Notifications
You must be signed in to change notification settings - Fork 671
[Feat] Add commit compression type support #4061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CI is failing right now - bustage will be fixed by #4058 |
9d87a91
to
6112618
Compare
@yankay @AkihiroSuda please take a look, thanks. |
CI is failing |
@lengrongfu could we have a test? eg: commit with zstd, then use that image. |
Yes, I test, can use zstd commit image to deployment pod. |
@AkihiroSuda @apostasie Please take a look, thanks ~ |
Thanks @lengrongfu We need a test that can run on the CI. Eg, like in: https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/container/container_commit_test.go |
@lengrongfu Code is fine (beside a couple of minor nits above), but my main issues right now are:
|
In containerd v1 becase it don't support |
07414b3
to
3e355e4
Compare
0f5db22
to
521abab
Compare
@apostasie about |
Thanks a lot @lengrongfu ! I do have a small series of comments here - mainly typos, and a suggestion for the test. Let me know what you think and then it should be good for final review. |
1718ad0
to
9e78423
Compare
@apostasie Modified according to your suggestion, thank you for your work. |
Thanks @lengrongfu LGTM. CI failure is unrelated. Now to maintainers - @AkihiroSuda @yankay @fahedouch PTAL |
var dinf dockercompat.Info | ||
err := json.Unmarshal([]byte(stdout), &dinf) | ||
assert.NilError(helpers.T(), err, "failed to parse docker info") | ||
matched, err := regexp.MatchString(`^v2\.\d+`, dinf.ServerVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
semver should be used
nerdctl/pkg/taskutil/taskutil.go
Line 144 in 772bb09
} else if sv.LessThan(semver.MustParse("1.6.0-0")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -416,3 +417,15 @@ var RemapIDs = &test.Requirement{ | |||
return false, "snapshotter does not support ID remapping" | |||
}, | |||
} | |||
|
|||
var ContainerdVersionV2 = &test.Requirement{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var ContainerdVersionV2 = &test.Requirement{ | |
func ContainerdVersion(v string) = &test.Requirement{ |
So that we do not need to add ContainerdVersionV2_1
, ContainerdVersionV2_2
, ContainerdVersionV3
, ... for each of the versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test.Requirement
is struct, how to set to func ContainerdVersion(v string)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to make it a function that returns *test.Requirement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, thanks.
c114e8b
to
13dec2d
Compare
thanks @lengrongfu . The documentation update is good, but it might be helpful to add a brief explanation of when to use each compression type (e.g., zstd is generally better for compression ratio but might not be as widely supported). |
Signed-off-by: rongfu.leng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Fix #4060
Use zstd compression can shorten commit time. current containerd v2 having support
zstd
commit type, but containerd v1 version don't support.Test case:
to view blob type.