feat: use buildkit API for clean up build cache within `system prune` by STRRL · Pull Request #1360 · containerd/nerdctl

STRRL · 2022-09-07T08:19:54Z

Signed-off-by: Zhou Zhiqiang im@strrl.dev

This PR does not change the logic of nerdctl builder prune, please let me know if we should also update it.

prev #1284

close #1279

STRRL · 2022-09-07T08:28:57Z

I think this PR is ready for review! PTAL @junnplus @AkihiroSuda @fahedouch

STRRL · 2022-09-07T08:31:39Z

#1284 (comment)

After integrating with buildkit API directly, I noticed that the size of nerdctl increased from 29M to 32M.

But introducing buildkit API would bring some dependency issue, for example, buildkit pinned docker with github.com/docker/docker => github.com/docker/docker v20.10.3-0.20220831131523-b5a0d7a188ac+incompatible, so we also have to pin on that version. I have tried to use v20.10.7 but it failed to compile because of some API changes.

ktock · 2022-09-26T07:08:21Z

needs rebase

STRRL · 2022-09-28T13:55:32Z

needs rebase

Updated, PTAL!

AkihiroSuda · 2022-10-19T02:47:37Z

Needs rebase

STRRL · 2022-12-05T12:44:19Z

CI Windows/containerd-1.6 failed because it does not run buildkitd with windows container runtime.

What should I do? @AkihiroSuda

AkihiroSuda · 2022-12-06T01:17:14Z

CI Windows/containerd-1.6 failed because it does not run buildkitd with windows container runtime.

What should I do? @AkihiroSuda

The test should be skipped when buildkitd is missing.

Also please squash commits

STRRL · 2022-12-06T04:33:19Z

Also please squash commits

Hi @AkihiroSuda, it seems we already used "Squash and Merge" so the origin commits would not be taken in the main branch as is. Or are there any other things that I should consider?

AkihiroSuda · 2022-12-06T04:35:00Z

it seems we already used "Squash and Merge"

Untrue. We do not use this button, as we want to keep the commits including DCO sign-off as-is

STRRL · 2022-12-06T04:37:12Z

it seems we already used "Squash and Merge"

Untrue. We do not use this button, as we want to keep the commits including DCO sign-off as-is

Oops, got that, I would try to rebase the commits on the latest main branch.

Thanks for your explanation. ❤️

STRRL · 2022-12-08T13:16:44Z

Hi @AkihiroSuda, @Zheaoli, there are 2 unrelated testcase failed

TestRunWithJsonFileLogDriver in test-integration-rootless (22.04, main)
TestRunWithJsonFileLogDriver, TestRunWithJsonFileLogDriverAndLogPathOpt in test-integration-rootless (22.04, v1.6.10)

Maybe they are flaky tests, and give it a retry? PTAL, thanks!

AkihiroSuda · 2022-12-09T00:04:23Z

 	base.Cmd("images").AssertNoOut("alpine")
+
+	if rootlessutil.IsRootless() {
+		t.Skip("test skipped for buildkitd is not available with rootless containers")


Doesn't seem true

#1360 (comment)

I remember that calling buildctl would be failed in the rootless container integration tests. So I append these lines of code.

What is the error on rootless?

I remember that calling buildctl would be failed in the rootless container integration tests.

I could not reproduce that error on rootless container integration tests with my local machine now. 🙈

I would remove these lines.

AkihiroSuda · 2022-12-09T00:06:47Z

Commits don't seem squashed

STRRL · 2022-12-09T12:18:28Z

Commits don't seem squashed

Do I need to squash them into only 1 commit?

AkihiroSuda · 2022-12-12T03:10:23Z

Commits don't seem squashed

Do I need to squash them into only 1 commit?

1 topic = 1 commit.
This doesn't necessary mean that 1 PR = 1 commit, though.

Signed-off-by: Zhou Zhiqiang <im@strrl.dev>

STRRL · 2022-12-14T07:51:40Z

I have

removed the testcase condiitons for rootles containers
squashed into 1 commit

PTAL @AkihiroSuda ❤️

AkihiroSuda

Thanks

AkihiroSuda · 2022-12-16T18:02:07Z

BuildKit v0.11 will support buildctl du --format json, so maybe we will reopen #1284 after its GA

STRRL · 2022-12-21T12:09:36Z

BuildKit v0.11 will support buildctl du --format json, so maybe we will reopen #1284 after its GA

So you mean we still prefer to parse the JSON output rather than importing buildkit as a library?

AkihiroSuda · 2022-12-22T08:43:05Z

BuildKit v0.11 will support buildctl du --format json, so maybe we will reopen #1284 after its GA

So you mean we still prefer to parse the JSON output rather than importing buildkit as a library?

Yes, that might be better to avoid the replace() hell in go.mod

STRRL marked this pull request as ready for review September 7, 2022 08:28

STRRL changed the title ~~feat: use buildkit API for clean up build cache~~ feat: use buildkit API for clean up build cache within system prune Sep 7, 2022

AkihiroSuda requested a review from ktock September 7, 2022 09:52

ktock reviewed Sep 8, 2022

View reviewed changes

Comment thread cmd/nerdctl/system_prune.go

Comment thread cmd/nerdctl/system_prune.go Outdated

Comment thread cmd/nerdctl/system_prune.go

manugupt1 reviewed Sep 8, 2022

View reviewed changes

Comment thread pkg/infoutil/infoutil_linux.go

Comment thread cmd/nerdctl/system_prune.go

AkihiroSuda requested a review from ktock September 26, 2022 01:46

ktock reviewed Sep 26, 2022

View reviewed changes

Comment thread cmd/nerdctl/system_prune.go Outdated

Comment thread cmd/nerdctl/system_prune.go Outdated

Comment thread cmd/nerdctl/system_prune.go

STRRL force-pushed the system-prune-cleanup-cache-with-buildkit branch from fc47a3e to c3083b2 Compare September 28, 2022 13:55

STRRL requested review from ktock and manugupt1 and removed request for ktock and manugupt1 September 28, 2022 13:55

STRRL force-pushed the system-prune-cleanup-cache-with-buildkit branch from af75547 to 3ce94eb Compare September 29, 2022 06:02

ktock reviewed Sep 29, 2022

View reviewed changes

Comment thread cmd/nerdctl/system_prune_linux_test.go

STRRL force-pushed the system-prune-cleanup-cache-with-buildkit branch from a57c864 to 20e8662 Compare December 5, 2022 12:16

STRRL force-pushed the system-prune-cleanup-cache-with-buildkit branch from 2712b5a to 31f7e87 Compare December 5, 2022 13:46

AkihiroSuda reviewed Dec 6, 2022

View reviewed changes

Comment thread go.mod Outdated

AkihiroSuda reviewed Dec 6, 2022

View reviewed changes

Comment thread go.mod Outdated

STRRL force-pushed the system-prune-cleanup-cache-with-buildkit branch 3 times, most recently from 7beb541 to bd96b9a Compare December 6, 2022 04:59

AkihiroSuda requested a review from ktock December 9, 2022 00:03

AkihiroSuda added this to the v1.1.1 (tentative) milestone Dec 9, 2022

AkihiroSuda reviewed Dec 9, 2022

View reviewed changes

ktock reviewed Dec 9, 2022

View reviewed changes

Comment thread go.mod Outdated

STRRL force-pushed the system-prune-cleanup-cache-with-buildkit branch 3 times, most recently from 934b05c to 2024bae Compare December 14, 2022 07:47

feat: use buildkit API for clean up build cache

0575de8

Signed-off-by: Zhou Zhiqiang <im@strrl.dev>

STRRL force-pushed the system-prune-cleanup-cache-with-buildkit branch from 2024bae to 0575de8 Compare December 14, 2022 07:49

AkihiroSuda approved these changes Dec 16, 2022

View reviewed changes

AkihiroSuda merged commit 1c49ccb into containerd:main Dec 16, 2022

Conversation

STRRL commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

STRRL commented Sep 7, 2022

Uh oh!

STRRL commented Sep 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ktock commented Sep 26, 2022

Uh oh!

STRRL commented Sep 28, 2022

Uh oh!

Uh oh!

AkihiroSuda commented Oct 19, 2022

Uh oh!

STRRL commented Dec 5, 2022

Uh oh!

AkihiroSuda commented Dec 6, 2022

Uh oh!

Uh oh!

Uh oh!

STRRL commented Dec 6, 2022

Uh oh!

AkihiroSuda commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

STRRL commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

STRRL commented Dec 8, 2022

Uh oh!

AkihiroSuda Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

STRRL Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

STRRL Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Dec 9, 2022

Uh oh!

Uh oh!

STRRL commented Dec 9, 2022

Uh oh!

AkihiroSuda commented Dec 12, 2022

Uh oh!

STRRL commented Dec 14, 2022

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Dec 16, 2022

Uh oh!

STRRL commented Dec 21, 2022

Uh oh!

AkihiroSuda commented Dec 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

STRRL commented Sep 7, 2022 •

edited

Loading

AkihiroSuda commented Dec 6, 2022 •

edited

Loading

STRRL commented Dec 6, 2022 •

edited

Loading