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.

Sign up for GitHub

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

Add option to specify containerd runtime #4141

Closed
wants to merge 1 commit into from
Closed

Add option to specify containerd runtime #4141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/buildkitd/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ type ContainerdConfig struct {
Labels map[string]string `toml:"labels"`
Platforms []string `toml:"platforms"`
Namespace string `toml:"namespace"`
Runtime string `toml:"runtime"`
jedevc marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this support options for runc binary path, cgroup driver, etc?
I also guess this should be map[string]interface{} to support multiple runtime classes in the future.

i.e., The config structure should be similar to the config structure of containerd/CRI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how to pass that to containerd. What I just wanted to achieve is to be able to override client.defaultRuntime. Because there are cough, cough platforms where runtime isn't mature enough to be declared The Default.

The lack of such configuration option leaded to FreeBSD runtime being hardcoded in buildkit. I'm in the same boat with macOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, my idea is: with these changes, one can configure runtime options in containerd config, while using buildkit to select needed runtime. Or, option added here can just be used to specify path path to runtime binary. Exposing the full runtime configuration through buildkit config is too much extending the scope of the task.

Copy link
Member

Choose a reason for hiding this comment

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

Some related work in moby;

I need to refresh my memory (but perhaps the code describes it), but ISTR, we only allowed paths etc to be configured for the legacy bits, and otherwise only allow a (fully qualified?) reference to be specified, or at least from the client side (to prevent a arbitrary paths to be passed that are not in the server's PATH).

Copy link
Member

Choose a reason for hiding this comment

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

I think I missed this originally. We should probably make sure to cover this is in a first pass - I wrote up a possible implementation: jedevc@22d9bb1. @AkihiroSuda @tonistiigi wdyt? (I took inspiration from how CRI does it today). @slonopotamus you happy to pull that change into your branch (or I can force-push)?

Without a way to configure the runtime, I think the feature is quite limited (e.g. I have a use case for wanting to override the runc binary used by the io.containerd.runc.v2 runtime, which is possible by using the binary_name option). Containerd doesn't seem to have any other way to configure this, so we need to allow this configuration in buildkit if we want to allow users this kind of freedom.

Even if we don't take this now, we need to provide a place in config fields to allow this kind of configuration. Previously we had:

[worker.containerd]
runtime = "io.containerd.runc.v2"

I think something like this should work easily:

[worker.containerd.runtime]
name = "io.containerd.runc.v2"
options = { binary_name = "foobar" }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jedevc I'm perfectly fine if you create a separate PR. I just need a way to specify which runtime to use, it isn't really important what lines one would need to write into config file, it is easy to adapt.

Copy link
Member

Choose a reason for hiding this comment

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

Opened in #4279.

GCConfig
NetworkConfig
Snapshotter string `toml:"snapshotter"`
Expand Down
2 changes: 2 additions & 0 deletions cmd/buildkitd/config/load_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ foo="bar"

[worker.containerd]
namespace="non-default"
runtime="exotic"
platforms=["linux/amd64"]
address="containerd.sock"
[[worker.containerd.gcpolicy]]
Expand Down Expand Up @@ -103,6 +104,7 @@ searchDomains=["example.com"]

require.Equal(t, 0, len(cfg.Workers.OCI.GCPolicy))
require.Equal(t, "non-default", cfg.Workers.Containerd.Namespace)
require.Equal(t, "exotic", cfg.Workers.Containerd.Runtime)
require.Equal(t, 3, len(cfg.Workers.Containerd.GCPolicy))

require.Nil(t, cfg.Workers.Containerd.GC)
Expand Down
22 changes: 21 additions & 1 deletion cmd/buildkitd/main_containerd_worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ package main
import (
"context"
"os"
"runtime"
"strconv"
"strings"
"time"

ctd "github.com/containerd/containerd"
"github.com/containerd/containerd/defaults"
"github.com/containerd/containerd/pkg/userns"
"github.com/moby/buildkit/cmd/buildkitd/config"
"github.com/moby/buildkit/util/bklog"
Expand Down Expand Up @@ -46,6 +48,14 @@ func init() {
defaultConf.Workers.Containerd.Namespace = defaultContainerdNamespace
}

if defaultConf.Workers.Containerd.Runtime == "" {
if runtime.GOOS == "freebsd" {
defaultConf.Workers.Containerd.Runtime = "wtf.sbk.runj.v1"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a TODO saying it should be removed once containerd/containerd#8964 comes with a containerd release?

} else {
defaultConf.Workers.Containerd.Runtime = defaults.DefaultRuntime
}
}

flags := []cli.Flag{
cli.StringFlag{
Name: "containerd-worker",
Expand Down Expand Up @@ -74,6 +84,12 @@ func init() {
Value: defaultConf.Workers.Containerd.Namespace,
Hidden: true,
},
cli.StringFlag{
Name: "containerd-worker-runtime",
Usage: "override containerd runtime",
Value: defaultConf.Workers.Containerd.Runtime,
Hidden: true,
},
cli.StringFlag{
Name: "containerd-worker-net",
Usage: "worker network type (auto, cni or host)",
Expand Down Expand Up @@ -202,6 +218,10 @@ func applyContainerdFlags(c *cli.Context, cfg *config.Config) error {
cfg.Workers.Containerd.Namespace = c.GlobalString("containerd-worker-namespace")
}

if c.GlobalIsSet("containerd-worker-runtime") || cfg.Workers.Containerd.Runtime == "" {
cfg.Workers.Containerd.Runtime = c.GlobalString("containerd-worker-runtime")
}

if c.GlobalIsSet("containerd-worker-gc") {
v := c.GlobalBool("containerd-worker-gc")
cfg.Workers.Containerd.GC = &v
Expand Down Expand Up @@ -275,7 +295,7 @@ func containerdWorkerInitializer(c *cli.Context, common workerInitializerOpt) ([
if cfg.Snapshotter != "" {
snapshotter = cfg.Snapshotter
}
opt, err := containerd.NewWorkerOpt(common.config.Root, cfg.Address, snapshotter, cfg.Namespace, cfg.Rootless, cfg.Labels, dns, nc, common.config.Workers.Containerd.ApparmorProfile, common.config.Workers.Containerd.SELinux, parallelismSem, common.traceSocket, ctd.WithTimeout(60*time.Second))
opt, err := containerd.NewWorkerOpt(common.config.Root, cfg.Address, snapshotter, cfg.Namespace, cfg.Rootless, cfg.Labels, dns, nc, common.config.Workers.Containerd.ApparmorProfile, common.config.Workers.Containerd.SELinux, parallelismSem, common.traceSocket, ctd.WithTimeout(60*time.Second), ctd.WithDefaultRuntime(cfg.Runtime))
if err != nil {
return nil, err
}
Expand Down
1 change: 1 addition & 0 deletions docs/buildkitd.toml.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ insecure-entitlements = [ "network.host", "security.insecure" ]
enabled = true
platforms = [ "linux/amd64", "linux/arm64" ]
namespace = "buildkit"
runtime = "io.containerd.runc.v2"
gc = true
# gckeepstorage sets storage limit for default gc profile, in MB.
gckeepstorage = 9000
Expand Down
4 changes: 0 additions & 4 deletions worker/containerd/containerd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"os"
"path/filepath"
"runtime"
"strconv"
"strings"

Expand All @@ -30,9 +29,6 @@ import (
// NewWorkerOpt creates a WorkerOpt.
func NewWorkerOpt(root string, address, snapshotterName, ns string, rootless bool, labels map[string]string, dns *oci.DNSConfig, nopt netproviders.Opt, apparmorProfile string, selinux bool, parallelismSem *semaphore.Weighted, traceSocket string, opts ...containerd.ClientOpt) (base.WorkerOpt, error) {
opts = append(opts, containerd.WithDefaultNamespace(ns))
if runtime.GOOS == "freebsd" {
opts = append(opts, containerd.WithDefaultRuntime("wtf.sbk.runj.v1"))
}

client, err := containerd.New(address, opts...)
if err != nil {
Expand Down