nydus: update nydus-snapshotter dependency to v0.8.0

[changweige]

Nydus-snapshotter/converter does not record image's blobs digest on nydus image anymore since it is easy to overflow annotations' size limitation of Containerd. Nydus-snapshotter now relies on Containerd to GC.

[jedevc]

cc @imeoer

[imeoer]

Thanks! @changweige Can we also update the nydusd component version here and here ?

[changweige]

Thanks! @changweige Can we also update the nydusd component version here and here ?

What nydusd version are we suggesting now？

[jedevc]

Also, is there a reason we need to bump the AWS SDK and protobuf deps as well? Are these dependencies pulled from nydus? If not, we should avoid bumping deps that are out of scope for this change.

[imeoer]

What nydusd version are we suggesting now？

v2.1.6 should be ok.

[changweige]

Also, is there a reason we need to bump the AWS SDK and protobuf deps as well? Are these dependencies pulled from nydus? If not, we should avoid bumping deps that are out of scope for this change.

Sorry for the late reply. Buildkit should only depend on the package github.com/containerd/nydus-snapshotter/pkg/converter which does not depend on any AWS packages. So I am not sure why go mod tidy also bumps the AWS deps of Buildikit. I still need more time to investigate it.

[thaJeztah]

Sorry for the late reply. Buildkit should only depend on the package github.com/containerd/nydus-snapshotter/pkg/converter which does not depend on any AWS packages

Yeah, go modules unfortunately aren't that smart. the pkg/converter package does not have its own go.mod, which means it's part of the github.com/containerd/nydus-snapshotter module, and as such defines minimum versions required for all of the main module's go.mod. While pkg/converte may not use the AWS dependencies or anything else, go modules only looks at that after resolving the minimum version; basically;

1. it collects all the dependencies specified by all modules (go.mod) used by BuildKit (recursively)
2. for each, it determines the minimum required version (which is the highest version specified by any go.mod in 1.)
3. if any of those are used by BuildKit (direct, or indirect), it updates the version in BuildKit's go.mod

I tried to convince the Go maintainers that this can be really problematic in projects with a large dependency tree, but they considered this to be "not a problem", and "easy to fix" for everyone (but too much work for Go itself);

• proposal: cmd/go: go mod: limit version resolution to packages that are consumed  golang/go#52296

[changweige]

Sorry for the late reply. Buildkit should only depend on the package github.com/containerd/nydus-snapshotter/pkg/converter which does not depend on any AWS packages

Yeah, go modules unfortunately aren't that smart. the pkg/converter package does not have its own go.mod, which means it's part of the github.com/containerd/nydus-snapshotter module, and as such defines minimum versions required for all of the main module's go.mod. While pkg/converte may not use the AWS dependencies or anything else, go modules only looks at that after resolving the minimum version; basically;

1. it collects all the dependencies specified by all modules (go.mod) used by BuildKit (recursively)
2. for each, it determines the minimum required version (which is the highest version specified by any go.mod in 1.)
3. if any of those are used by BuildKit (direct, or indirect), it updates the version in BuildKit's go.mod

I tried to convince the Go maintainers that this can be really problematic in projects with a large dependency tree, but they considered this to be "not a problem", and "easy to fix" for everyone (but too much work for Go itself);

• proposal: cmd/go: go mod: limit version resolution to packages that are consumed  golang/go#52296

Thanks @thaJeztah for the helpful information. Sounds like a way to address this is to make pkg/converter a sub-module within github.com/containerd/nydus-snapshotter

[jedevc]

It should be fine actually - I was just wondering why those deps also got bumped, since it seemed out of scope for this PR.

I think it's fine to take the AWS SDK update with the nydus one 🎉

[thaJeztah]

Sounds like a way to address this is to make pkg/converter a sub-module within github.com/containerd/nydus-snapshotter

Possibly that would work, but "sub-modules" (more actually they are "multi-module repositories") can be a big pain as well, especially if there's dependencies between those modules within your repo so I'd be careful introducing one.

It should be fine actually - I was just wondering why those deps also got bumped, since it seemed out of scope for this PR.

Yes, I think it's probably fine; I just recalled that I also had to update the dependency as part of updating the docker dependency to v24.0.0; #3846

In that PR I split it to separate commits to make it a bit more granular.

[jedevc]

@changweige can you update the suggested nydus version as mentioned by @imeoer: #3814 (comment).

Otherwise, this LGTM.

[changweige]

Sounds like a way to address this is to make pkg/converter a sub-module within github.com/containerd/nydus-snapshotter

Possibly that would work, but "sub-modules" (more actually they are "multi-module repositories") can be a big pain as well, especially if there's dependencies between those modules within your repo so I'd be careful introducing one.

It should be fine actually - I was just wondering why those deps also got bumped, since it seemed out of scope for this PR.

Yes, I think it's probably fine; I just recalled that I also had to update the dependency as part of updating the docker dependency to v24.0.0; #3846

In that PR I split it to separate commits to make it a bit more granular.

@thaJeztah Thanks for sharing the granular way to make the vendor code update more clear. But I feel like it's hard to apply the same method to this PR since the nydus-snapshotter's API has changed within v0.8.0 which means I have to make changing go.mod and source code in the same commit atomically, otherwise, a certain commit can't be compiled 🥺

[changweige]

@changweige can you update the suggested nydus version as mentioned by @imeoer: #3814 (comment).

Otherwise, this LGTM.

Thanks @jedevc . I have already updated the suggestion

Needs "make generated-files"

[crazy-max]

@changweige You need to update generated files with make generated-files since protobuf deps update.

[changweige]

@changweige You need to update generated files with make generated-files since protobuf deps update.

@crazy-max Thanks for reminding me. I have already run make generated-files and committed the resulted changes.

[jedevc]

@changweige @imeoer it looks like since this merged we've got a lot more nydus related flakiness (also possibly related to #3786)

For example, see https://github.com/moby/buildkit/pull/3881/files:

• process "mkdir /sys/fs/cgroup/securitytest" did not complete successfully: exit code: 1
• failed to get compression blob "gzip": failed to get and ensure compression type of "gzip": failed to convert: unpack nydus blob: unpack nydus tar: unpack blob from nydus: can't find target image.blob by seeking tar: data not found

[imeoer]

it looks like since this merged we've got a lot more nydus related flakiness (also possibly related to #3786)

Thanks for the report, will try to fix in containerd/nydus-snapshotter#483.

[crazy-max]

• process "mkdir /sys/fs/cgroup/securitytest" did not complete successfully: exit code: 1

I think this one might also be related to #3786

Ah you already mentioned that issue @jedevc 😅