Skip to content

builder: fall back to defaultKeepStorage if keepStorage is unset for GC policy#49062

Merged
thaJeztah merged 1 commit intomoby:masterfrom
dvdksn:builder-gcpolicy-empty-keepbytes
Dec 18, 2024
Merged

builder: fall back to defaultKeepStorage if keepStorage is unset for GC policy#49062
thaJeztah merged 1 commit intomoby:masterfrom
dvdksn:builder-gcpolicy-empty-keepbytes

Conversation

@dvdksn
Copy link
Copy Markdown
Contributor

@dvdksn dvdksn commented Dec 11, 2024

- What I did

Builder GC policies without a keepStorage option set should fall back to using the defaultKeepStorage value. But currently, the policy's keepStorage is parsed unconditionally, meaning that if it's unset, an error occurs:

could not get builder GC policy: invalid size: ''

The only way to get a policy to use defaultKeepStorage limit for GC would be to set it to "0".

This patch is to make sure policy keepStorage is conditionally parsed, if set, otherwise the defaultKeepStorage is used.

I also modified related error messages:

could not get builder GC policy: failed to parse defaultKeepStorage: invalid size: 'foo'
could not get builder GC policy: failed to parse keepStorage: invalid size: 'bar'

- How I did it

- How to verify it

Tested manually;

{
        "builder": {
                "gc": {
                        "enabled": true,
                        "defaultKeepStorage": "10GB",
                        "policy": [ { "filter": ["unused-for=2200h"] } ] 
                }
        }
}
$ docker buildx inspect
...
GC Policy rule#0:
 All:            false
 Filters:        
 Keep Duration:  2200h0m0.5074347s
 Reserved Space: 10GiB

- Description for the changelog

- Builder GC policies without a `keepStorage` value now inherit the `defaultKeepStorage` limit as intended.

- A picture of a cute animal (not mandatory but encouraged)

@dvdksn dvdksn requested a review from tonistiigi as a code owner December 11, 2024 10:01
@dvdksn dvdksn force-pushed the builder-gcpolicy-empty-keepbytes branch from 90ae132 to b08ff81 Compare December 11, 2024 10:03
@dvdksn dvdksn changed the title builder: fall back to defaultKeepBytes if keepBytes is unset for GC p… builder: fall back to defaultKeepStorage if keepStorage is unset for GC policy Dec 11, 2024
@thaJeztah thaJeztah added this to the 28.0.0 milestone Dec 17, 2024
thaJeztah
thaJeztah approved these changes Dec 17, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 17, 2024

cc @crazy-max @jsternberg in case this was intentional, and keepStorage to be meant a required option to be set (in that case, we should probably return a different error)

jsternberg
jsternberg approved these changes Dec 18, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@jsternberg jsternberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 18, 2024

Thanks! Yup looked good to me as well; just a quick sanity check in case I missed something, thanks!!

@thaJeztah thaJeztah merged commit 40f58b3 into moby:master Dec 18, 2024
@dvdksn dvdksn deleted the builder-gcpolicy-empty-keepbytes branch December 18, 2024 19:16
@thaJeztah thaJeztah mentioned this pull request Dec 19, 2024
Merged
@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Dec 19, 2024 •
edited
Loading

That's not how this is supposed to work. DefaultKeepStorage and Rules and exclusive properties. If you set DefaultKeepStorage then this is used as a base to generate default rules, but this does not mean all these generated rules will use DefaultKeepStorage as storage limit. If the user sets custom rules then they define their own storage limit, having zero there is completely normal - the filter can be based on other properties.

https://github.com/moby/buildkit/blob/master/cmd/buildkitd/main.go#L930-L949

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 19, 2024

@tonistiigi the code before this PR would produce an obscure error if the field was left empty in the config (which was the case @dvdksn ran into), should it be required to set it to 0, or should empty values be allowed, but in that case no fallback?

(for the latter case, we may need to look what type we currently use, as it may not be possible to distinguish "default" from "not set" - I'd have to double check that).

@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Dec 19, 2024

Fixing the parse error is correct, but if the storage value is not set by the user, then it should be 0, not DefaultKeepStorage.

@dvdksn
Copy link
Copy Markdown
Contributor Author

dvdksn commented Dec 19, 2024 •
edited
Loading

@tonistiigi how do you feel about making defaultKeepStorage the fallback though? I find it odd that the defaults are not used at all if you define a custom rule. What about this:

  • If a rule's keepStorage is omitted, default to keepStorage if set, or zero otherwise
  • If you want to use a zero-limit for a rule and defaultKeepStorage is set to a higher value, set keepStorage to zero for the rule(s)

@thaJeztah thaJeztah mentioned this pull request Dec 19, 2024
Merged
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 19, 2024

@tonis this PR looking like the correct logic?

@tonis
Copy link
Copy Markdown

tonis commented Dec 19, 2024 via email

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 19, 2024

Oh! Sorry for that! I blame GitHub autocomplete 🙈🙈

(nice to meet you though!)

@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Dec 19, 2024

@dvdksn I don't think this makes sense because even if you just set defaultKeepStorage and no rules, then the rules that are generated do not all have their storage set to defaultKeepStorage (the cache mounts etc use a fraction of that). It might make sense to at least warn though if user does set defaultKeepStorage and GCPolicy array both, to let them know that the default is ignored because they defined manual rules instead.

@dvdksn
Copy link
Copy Markdown
Contributor Author

dvdksn commented Dec 19, 2024

Yes that warning sounds like a good addition!

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

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi is a code owner

+1 more reviewer

@jsternberg jsternberg jsternberg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/builder/buildkit Build area/builder Build area/daemon Core Engine kind/bugfix PR's that fix bugs process/cherry-picked status/2-code-review

Projects

None yet

Milestone

28.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@dvdksn @thaJeztah @tonistiigi @tonis @jsternberg