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

Jump to bottom

Add options to specify containerd runtime (alternative) #4279

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Add options to specify containerd runtime (alternative) #4279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8bfd280
Add options to specify containerd runtime
jedevc Aug 13, 2023
ef8cb31
chore: add todo note to remove runj specifics
jedevc Sep 26, 2023
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
6 changes: 6 additions & 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 ContainerdRuntime `toml:"runtime"`
GCConfig
NetworkConfig
Snapshotter string `toml:"snapshotter"`
Expand All @@ -128,6 +129,11 @@ type ContainerdConfig struct {
Rootless bool `toml:"rootless"`
}

type ContainerdRuntime struct {
Name string `toml:"name"`
Options map[string]interface{} `toml:"options"`
}

type GCPolicy struct {
All bool `toml:"all"`
KeepBytes DiskSpace `toml:"keepBytes"`
Expand Down
5 changes: 5 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 @@ -42,6 +42,9 @@ foo="bar"
namespace="non-default"
platforms=["linux/amd64"]
address="containerd.sock"
[worker.containerd.runtime]
name="exotic"
options.foo="bar"
[[worker.containerd.gcpolicy]]
all=true
filters=["foo==bar"]
Expand Down Expand Up @@ -103,6 +106,8 @@ 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.Name)
require.Equal(t, "bar", cfg.Workers.Containerd.Runtime.Options["foo"])
require.Equal(t, 3, len(cfg.Workers.Containerd.GCPolicy))

require.Nil(t, cfg.Workers.Containerd.GC)
Expand Down
58 changes: 57 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,19 +6,25 @@ package main
import (
"context"
"os"
"runtime"
"strconv"
"strings"
"time"

ctd "github.com/containerd/containerd"
"github.com/containerd/containerd/defaults"
runtimeoptions "github.com/containerd/containerd/pkg/runtimeoptions/v1"
"github.com/containerd/containerd/pkg/userns"
"github.com/containerd/containerd/plugin"
runcoptions "github.com/containerd/containerd/runtime/v2/runc/options"
"github.com/moby/buildkit/cmd/buildkitd/config"
"github.com/moby/buildkit/util/bklog"
"github.com/moby/buildkit/util/network/cniprovider"
"github.com/moby/buildkit/util/network/netproviders"
"github.com/moby/buildkit/worker"
"github.com/moby/buildkit/worker/base"
"github.com/moby/buildkit/worker/containerd"
"github.com/pelletier/go-toml"
"github.com/pkg/errors"
"github.com/urfave/cli"
"golang.org/x/sync/semaphore"
Expand Down Expand Up @@ -46,6 +52,15 @@ func init() {
defaultConf.Workers.Containerd.Namespace = defaultContainerdNamespace
}

if defaultConf.Workers.Containerd.Runtime.Name == "" {
if runtime.GOOS == "freebsd" {
// TODO: this can be removed once containerd/containerd#8964 is included
defaultConf.Workers.Containerd.Runtime.Name = "wtf.sbk.runj.v1"
} else {
defaultConf.Workers.Containerd.Runtime.Name = defaults.DefaultRuntime
}
}

flags := []cli.Flag{
cli.StringFlag{
Name: "containerd-worker",
Expand Down Expand Up @@ -74,6 +89,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.Name,
Hidden: true,
},
cli.StringFlag{
Name: "containerd-worker-net",
Usage: "worker network type (auto, cni or host)",
Expand Down Expand Up @@ -202,6 +223,12 @@ 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.Name == "" {
cfg.Workers.Containerd.Runtime = config.ContainerdRuntime{
Name: 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 +302,26 @@ 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))

var runtime *containerd.RuntimeInfo
if cfg.Runtime.Name != "" {
opts := getRuntimeOptionsType(cfg.Runtime.Name)

t, err := toml.TreeFromMap(cfg.Runtime.Options)
if err != nil {
return nil, errors.Wrapf(err, "failed to parse runtime options config")
}
err = t.Unmarshal(opts)
if err != nil {
return nil, errors.Wrapf(err, "failed to parse runtime options config")
}

runtime = &containerd.RuntimeInfo{
Name: cfg.Runtime.Name,
Options: opts,
}
}
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, runtime, ctd.WithTimeout(60*time.Second))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -320,3 +366,13 @@ func validContainerdSocket(cfg config.ContainerdConfig) bool {
}
return true
}

// getRuntimeOptionsType gets empty runtime options by the runtime type name.
func getRuntimeOptionsType(t string) interface{} {
switch t {
case plugin.RuntimeRuncV2:
return &runcoptions.Options{}
default:
return &runtimeoptions.Options{}
}
}
Comment on lines +370 to +378
Copy link
Member Author

Choose a reason for hiding this comment

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

This is pulled pretty much directly from https://github.com/containerd/containerd/blob/c33249cbe6cb5a5e877647626472d16174b83f85/pkg/cri/sbserver/helpers.go#L289-L299.

Because of how this works, this means that we'd need to add support for each containerd runtime whose options we want to support, which is a little bit fiddly IMO.

@AkihiroSuda is there a different way to work around this? Or alternatively, could we potentially expose this logic as public in containerd?

Copy link
Member

Choose a reason for hiding this comment

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

Or alternatively, could we potentially expose this logic as public in containerd?

SGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, options should be an opaque type. Otherwise, this is just a leaky abstraction and we only pretend that runtimes are easily swappable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just came across this (because it broke Windows RUN due to the aforementioned fiddly changes being needed, see #4364)

The only way I can see of avoiding this fiddlyness (which as-noted, containerd/cri also suffers from) would be to amend the containerd v2 runtime ABI to require runtimes to support both their own Options type, and accept runtimeoptions.Options and parse their own type out of either the ConfigPath or ConfigBody.

I suggested that hcsshim support that in microsoft/hcsshim#1941 and have more notes on the idea there, but I'm not super-sold on it being a great approach. It also doesn't help a lot when an application needs to do more than reserialise a user-provided config block into an Options struct. But for use-cases like this (and containerd/cri) that's what we're trying to do, so it fits the bill well.

One improvement I did suggest is that the thing that defines the Options struct should also export the name, so that if-chains like this are correctly coupled. But that assumes the struct is containerd-specific; it is in hcsshim, but I don't know off-hand if runc also uses this struct elsewhere and the runtime name would be irrelevant. (plugin.RuntimeRuncV2 is historical legacy, AFAIK; the list of runtimes in plugin shouldn't be growing.)

I suspect that if the default option was nil, then at least it would not fail when a runtime whose options type is not known is used, as long as that runtime does not require any settings; hcsshim meets this criteria, it correctly handles the "No options struct" case, it only barfs when it gets one it cannot deserialize.

In the end, making something like this public from containerd isn't a bad idea, but it only helps for cases that containerd knows about, and there'll certainly be situations where you need to handle a new runtime locally like this either because the containerd change is too new to vendor, or because the update to containerd is stuck in a PR.

Right now, containerd itself doesn't know about any of these types; containerd/cri does because it does the same thing we do here (except it also populates runtimeoptions.Options.ConfigBytes) to handle its own config, and ctr happens to know how to pass "Debug" into hcsshim. And similar to my comment above, containerd probably shouldn't start to know about all the possible runtime types, but no specifically-better solution comes to mind.

5 changes: 5 additions & 0 deletions docs/buildkitd.toml.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ insecure-entitlements = [ "network.host", "security.insecure" ]
[worker.containerd.labels]
"foo" = "bar"

# configure the containerd runtime
[worker.containerd.runtime]
runtime = "io.containerd.runc.v2"
Copy link
Contributor

Choose a reason for hiding this comment

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

BUG: this should be name = "..."

options = { BinaryName = "runc" }

[[worker.containerd.gcpolicy]]
keepBytes = 512000000
keepDuration = 172800
Expand Down
17 changes: 14 additions & 3 deletions executor/containerdexecutor/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ type containerdExecutor struct {
selinux bool
traceSocket string
rootless bool
runtime *RuntimeInfo
}

// OnCreateRuntimer provides an alternative to OCI hooks for applying network
Expand All @@ -53,8 +54,13 @@ type OnCreateRuntimer interface {
OnCreateRuntime(pid uint32) error
}

type RuntimeInfo struct {
Name string
Options any
}

// New creates a new executor backed by connection to containerd API
func New(client *containerd.Client, root, cgroup string, networkProviders map[pb.NetMode]network.Provider, dnsConfig *oci.DNSConfig, apparmorProfile string, selinux bool, traceSocket string, rootless bool) executor.Executor {
func New(client *containerd.Client, root, cgroup string, networkProviders map[pb.NetMode]network.Provider, dnsConfig *oci.DNSConfig, apparmorProfile string, selinux bool, traceSocket string, rootless bool, runtime *RuntimeInfo) executor.Executor {
// clean up old hosts/resolv.conf file. ignore errors
os.RemoveAll(filepath.Join(root, "hosts"))
os.RemoveAll(filepath.Join(root, "resolv.conf"))
Expand All @@ -70,6 +76,7 @@ func New(client *containerd.Client, root, cgroup string, networkProviders map[pb
selinux: selinux,
traceSocket: traceSocket,
rootless: rootless,
runtime: runtime,
}
}

Expand Down Expand Up @@ -145,9 +152,13 @@ func (w *containerdExecutor) Run(ctx context.Context, id string, root executor.M
defer releaseSpec()
}

container, err := w.client.NewContainer(ctx, id,
opts := []containerd.NewContainerOpts{
containerd.WithSpec(spec),
)
}
if w.runtime != nil {
opts = append(opts, containerd.WithRuntime(w.runtime.Name, w.runtime.Options))
}
container, err := w.client.NewContainer(ctx, id, opts...)
if err != nil {
return nil, err
}
Expand Down
177 changes: 177 additions & 0 deletions vendor/github.com/containerd/containerd/pkg/runtimeoptions/v1/api.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading