Skip to content
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

allow controlling detected platforms cache timeout #4949

Merged
merged 1 commit into from
May 31, 2024

Conversation

tonistiigi
Copy link
Member

Because detecting emulator changes can be relatively expensive, avoid doing it very frequently. A new config parameter allows controlling if users prefer more or less frequent updates or want to disable detecting changes completely and only rely on emultor configuration at boot time.

@@ -245,10 +245,14 @@ func (w *Worker) Labels() map[string]string {

func (w *Worker) Platforms(noCache bool) []ocispecs.Platform {
if noCache {
matchers := make([]platforms.MatchComparer, len(w.WorkerOpt.Platforms))
for i, p := range w.WorkerOpt.Platforms {
matchers[i] = platforms.Only(p)
Copy link
Member Author

Choose a reason for hiding this comment

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

Although the effect of this isn't that significant compared to qemu detection logic, calling platforms.Only isn't completely free either as it internally allocates memory for the variant vector arrays.

@jedevc
Copy link
Member

jedevc commented May 29, 2024

This is a fix for docker/buildx#2474, correct?


"github.com/containerd/containerd/platforms"
"github.com/moby/buildkit/util/bklog"
ocispecs "github.com/opencontainers/image-spec/specs-go/v1"
)

var mu sync.Mutex
var arr []ocispecs.Platform
var CacheMaxAge = 20 * time.Second
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity. How often does this change during the lifetime of a buildkit process? I guess it would only change on an install of new binfmt handlers right?

Potentially, could we invalidate the cache based on the contents of /proc/sys/fs/binfmt_misc/? Just a thought, curious if you considered this option.

Copy link
Member Author

Choose a reason for hiding this comment

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

How often does this change during the lifetime of a buildkit process? I guess it would only change on an install of new binfmt handlers right?

Yeah, it depends on what the user is doing outside of buildkit.

Potentially, could we invalidate the cache based on the contents of /proc/sys/fs/binfmt_misc/?

binfmt_misc is usually not mounted in the environment where buildkit runs

@tonistiigi
Copy link
Member Author

This is a fix for docker/buildx#2474, correct?

Not directly. SupportedPlatforms does get called by ListWorkers though.

Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

type SystemConfig struct {
// PlatformCacheMaxAge controls how often supported platforms
// are refreshed by rescanning the system.
PlatformsCacheMaxAge *Duration `toml:"platformsCacheMaxAge"`
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to always use cache by setting platformsCacheMaxAge = -1 with this custom type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I tested this

Because detecting emulator changes can be relatively
expensive, avoid doing it very frequently. A new config
parameter allows controlling if users prefer more or
less frequent updates or want to disable detecting
changes completely and only rely on emultor configuration
at boot time.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi tonistiigi merged commit 29d0620 into moby:master May 31, 2024
77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants