Skip to content

runc worker does not always report the correct list of platforms #5740

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

Closed
4 tasks done
fiam opened this issue Feb 13, 2025 · 3 comments · Fixed by #5968
Closed
4 tasks done

runc worker does not always report the correct list of platforms #5740

fiam opened this issue Feb 13, 2025 · 3 comments · Fixed by #5968
Assignees
Labels
Milestone

Comments

@fiam
Copy link
Collaborator

fiam commented Feb 13, 2025

Contributing guidelines and issue reporting guide

Well-formed report checklist

  • I have found a bug that the documentation does not mention anything about my problem
  • I have found a bug that there are no open or closed issues that are related to my problem
  • I have provided version/information about my environment and done my best to provide a reproducer

Description of bug

Bug description

In architectures that support multiple platforms, runc worker might report only the default one. For example, on an arm64 host where the arm checks passes, it reports:

$ ./hacks/grpccurl.sh localhost:443 moby.buildkit.v1.Control.ListWorkers
{
  "record": [
    {
      "ID": "vao24g7dghf8k72gi74ljmarv",
      "Labels": {
        "org.mobyproject.buildkit.worker.executor": "oci",
        "org.mobyproject.buildkit.worker.hostname": "ip-10-130-30-112",
        "org.mobyproject.buildkit.worker.network": "host",
        "org.mobyproject.buildkit.worker.oci.process-mode": "sandbox",
        "org.mobyproject.buildkit.worker.selinux.enabled": "false",
        "org.mobyproject.buildkit.worker.snapshotter": "overlayfs"
      },
      "platforms": [
        {
          "Architecture": "arm64",
          "OS": "linux"
        }
      ],
      "GCPolicy": [
        {
          "all": true,
          "reservedSpace": "20000000000"
        }
      ],
      "BuildkitVersion": {
        "package": "github.com/moby/buildkit",
        "version": "v0.19.0",
        "revision": "49628b9df9b196db3371d745c64ac3d75283c4b4.m"
      }
    }
  ]
}

This should also include arm and arm/v6.

Reproduction

  1. Use an arm64 host
  2. Enable runc worker
  3. Launch BuildKit
  4. Call ListWorkers

Version information

v0.19.0

Additional information

This seems to happen because the runc worker initializes the list of supported platforms with just the default one. Later, when we call Worker.Platforms, the matcher for arm64 reports that it also includes arm and arm/v6 (which is not necessarily true), preventing them from being added to the list of platforms.

@fiam fiam changed the title runc worker does not report the correct list of platforms runc worker does not always report the correct list of platforms Feb 13, 2025
@tonistiigi
Copy link
Member

If the user doesn't set the platform in the config file, then it is initialized with a fully detected list of supported platforms https://github.com/moby/buildkit/blob/master/cmd/buildkitd/main.go#L528 . So this could only exist if some manual platform were set but not all the fallback platforms for the same manual ones. Theoretically, this could be useful to allow defining that you don't want the fallback behavior, but I'm not sure if it really was intentional.

@fiam
Copy link
Collaborator Author

fiam commented Feb 19, 2025

@tonistiigi I see, thanks! So the problem only happens when the platform lists is explicitly declared in the config, but it is also empty.

This checks for a nil slice https://github.com/moby/buildkit/blob/master/cmd/buildkitd/main.go#L528, but the worker checks that the slice has elements

if platformsStr := cfg.Platforms; len(platformsStr) != 0 {

Would it be sensible to add an omitempty in the configuration struct? Otherwise intiializing the config by serializing the structure will require setting the platforms manually.

@tonistiigi
Copy link
Member

Would it be sensible to add an omitempty in the configuration struct? Otherwise intiializing the config by serializing the structure will require setting the platforms manually.

ok

@thompson-shaun thompson-shaun added this to the v0.21.0 milestone Feb 20, 2025
@thompson-shaun thompson-shaun modified the milestones: v0.21.0, v0.22.0 Apr 7, 2025
fiam added a commit to fiam/buildkit that referenced this issue May 13, 2025
Previously, when no platforms were explicitly defined in the config,
serialization would emit an empty array instead of omitting the field.
When loading this cofig, this prevented the worker from falling back
to default platform detection during initialization. By using `omitempty`,
we ensure the field is excluded when empty, allowing defaults to be
correctly applied.

Fixes moby#5740.
fiam added a commit to fiam/buildkit that referenced this issue May 13, 2025
Previously, when no platforms were explicitly defined in the config,
serialization would emit an empty array instead of omitting the field.
When loading this cofig, this prevented the worker from falling back
to default platform detection during initialization. By using `omitempty`,
we ensure the field is excluded when empty, allowing defaults to be
correctly applied.

Fixes moby#5740.

Signed-off-by: Alberto Garcia Hierro <[email protected]>
@github-project-automation github-project-automation bot moved this from New to Accepted in Issue Triage May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants