Skip to content

feat: system prune also cleanup build cache#1284

Closed
STRRL wants to merge 4 commits into
containerd:masterfrom
STRRL:system-prune-cleanup-cache
Closed

feat: system prune also cleanup build cache#1284
STRRL wants to merge 4 commits into
containerd:masterfrom
STRRL:system-prune-cleanup-cache

Conversation

@STRRL
Copy link
Copy Markdown
Contributor

@STRRL STRRL commented Jul 31, 2022

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

close #1279

Changes:

  • call builderPruneAction() within command system prune

@STRRL
Copy link
Copy Markdown
Contributor Author

STRRL commented Jul 31, 2022

PTAL!

@Zheaoli @AkihiroSuda

@AkihiroSuda
Copy link
Copy Markdown
Member

Signed-off-by: STRRL

Thanks, but please use real name

Comment thread cmd/nerdctl/system_prune.go Outdated
Signed-off-by: Zhou Zhiqiang <im@strrl.dev>
@STRRL STRRL force-pushed the system-prune-cleanup-cache branch from fd1e190 to 2eb359e Compare July 31, 2022 08:38
@STRRL
Copy link
Copy Markdown
Contributor Author

STRRL commented Jul 31, 2022

Signed-off-by: STRRL

Thanks, but please use real name

Updated!

STRRL added 3 commits August 3, 2022 18:16
Signed-off-by: Zhou Zhiqiang <im@strrl.dev>
Signed-off-by: Zhou Zhiqiang <im@strrl.dev>
Signed-off-by: Zhou Zhiqiang <im@strrl.dev>
@STRRL STRRL force-pushed the system-prune-cleanup-cache branch from 5f57eb2 to 332b68f Compare August 3, 2022 12:27
@STRRL
Copy link
Copy Markdown
Contributor Author

STRRL commented Aug 3, 2022

Hi @AkihiroSuda @junnplus , I meet some trouble when using TabReader to parse the output of buildctl prune.

The output of the buildctl prune is not soo standard, it would use both \t and white-space as the delimiter:

image

I also updated the testcase, so it would be failed with the current TabReader implementation because it extracts values based on the character position.

@STRRL
Copy link
Copy Markdown
Contributor Author

STRRL commented Aug 3, 2022

What should I do now? Make a new implementation like TabReader but only for buildctl prune? or wait moby/buildkit#2992 getting merged?

Comment thread cmd/nerdctl/system_prune.go
@STRRL
Copy link
Copy Markdown
Contributor Author

STRRL commented Aug 3, 2022

What should I do now? Make a new implementation like TabReader but only for buildctl prune? or wait moby/buildkit#2992 getting merged?

I just noticed that PR already get merged! 🎉

I think I could update code based on the json output!

@junnplus
Copy link
Copy Markdown
Member

junnplus commented Aug 3, 2022

I think I could update code based on the json output!

Are we not compatible with older versions?

@STRRL
Copy link
Copy Markdown
Contributor Author

STRRL commented Aug 3, 2022

I think I could update code based on the json output!

Are we not compatible with older versions?

Well, if we want to be compatible with the older version of buildctl, it seems I must build the parser for resolving the output of buildctl prune. 🤔

Or maybe using the BuildKit go API would be the better way?

PTAL @junnplus @AkihiroSuda

@junnplus
Copy link
Copy Markdown
Member

junnplus commented Aug 3, 2022

The output of the buildctl prune is not soo standard, it would use both \t and white-space as the delimiter:

You can convert \t to whitespace.

@AkihiroSuda
Copy link
Copy Markdown
Member

Parse the JSON if the JSON output is available, otherwise we can just print the bare output from buildctl builder prune

@junnplus
Copy link
Copy Markdown
Member

junnplus commented Aug 3, 2022

Parse the JSON if the JSON output is available, otherwise we can just print the bare output from buildctl builder prune

Honestly, I wish we could be compatible with older versions because most users are using them.

Comment thread pkg/tabutil/tabutil.go
@STRRL
Copy link
Copy Markdown
Contributor Author

STRRL commented Aug 3, 2022

The output of the buildctl prune is not soo standard, it would use both \t and white-space as the delimiter:

You can convert \t to whitespace.

Parse the JSON if the JSON output is available, otherwise we can just print the bare output from buildctl builder prune

What about using BuildKit Go API? I prefer to use that instead of parsing the text, because go API seems more stable and easy to use. 🤔

@junnplus
Copy link
Copy Markdown
Member

junnplus commented Aug 3, 2022

What about using BuildKit Go API? I prefer to use that instead of parsing the text, because go API seems more stable and easy to use. 🤔

SGTM.

@fahedouch fahedouch self-requested a review August 3, 2022 21:52
@STRRL
Copy link
Copy Markdown
Contributor Author

STRRL commented Aug 11, 2022

What about using BuildKit Go API? I prefer to use that instead of parsing the text, because go API seems more stable and easy to use. thinking

SGTM.

Hi @junnplus @AkihiroSuda @fahedouch, I took a look at using buildkit go API instead of using buildctl for pruning cache. It seems it would take more changes, but I still think that worth it.

I think I would create a tracking issue about:

  • migrate the existed build prune using buildkit API
  • then finished the work about system prune

What do you think about it?

@AkihiroSuda
Copy link
Copy Markdown
Member

Using buildkit API sounds good but fearing it may increase the binary footprint

Cc @ktock

@STRRL
Copy link
Copy Markdown
Contributor Author

STRRL commented Sep 7, 2022

It has been a long time since this PR not active, I would continue the work in another PR with new codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nerdctl system prune should prune build caches too

3 participants