From 3b71197fb85c66c9269855a1e1962345deae72b7 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 25 Jul 2023 13:49:42 +0200 Subject: [PATCH 1/3] Set default CDI spec dirs after parsing args Signed-off-by: Evan Lezar --- cmd/dockerd/daemon.go | 12 +++++++ cmd/dockerd/daemon_test.go | 70 ++++++++++++++++++++++++++++++++++++++ daemon/config/config.go | 2 -- 3 files changed, 82 insertions(+), 2 deletions(-) diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go index e9f979c699..2451d7f32d 100644 --- a/cmd/dockerd/daemon.go +++ b/cmd/dockerd/daemon.go @@ -544,6 +544,18 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) { return nil, err } + if conf.CDISpecDirs == nil { + // If the CDISpecDirs is not set at this stage, we set it to the default. + conf.CDISpecDirs = append([]string(nil), cdi.DefaultSpecDirs...) + } else if len(conf.CDISpecDirs) == 1 && conf.CDISpecDirs[0] == "" { + // If CDISpecDirs is set to an empty string, we clear it to ensure that CDI is disabled. + conf.CDISpecDirs = nil + } + if !conf.Experimental { + // If experimental mode is not set, we clear the CDISpecDirs to ensure that CDI is disabled. + conf.CDISpecDirs = nil + } + return conf, nil } diff --git a/cmd/dockerd/daemon_test.go b/cmd/dockerd/daemon_test.go index 55b301f8db..3bc9d628d2 100644 --- a/cmd/dockerd/daemon_test.go +++ b/cmd/dockerd/daemon_test.go @@ -5,6 +5,7 @@ import ( "github.com/containerd/containerd/log" "github.com/docker/docker/daemon/config" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/sirupsen/logrus" "github.com/spf13/pflag" "gotest.tools/v3/assert" @@ -223,3 +224,72 @@ func TestConfigureDaemonLogs(t *testing.T) { // TODO (thaJeztah): add more aliases in log package assert.Check(t, is.Equal(logrus.WarnLevel, log.GetLevel())) } + +func TestCDISpecDirs(t *testing.T) { + testCases := []struct { + description string + configContent string + experimental bool + specDirs []string + expectedCDISpecDirs []string + }{ + { + description: "experimental and no spec dirs specified returns default", + specDirs: nil, + experimental: true, + expectedCDISpecDirs: []string{"/etc/cdi", "/var/run/cdi"}, + }, + { + description: "experimental and specified spec dirs are returned", + specDirs: []string{"/foo/bar", "/baz/qux"}, + experimental: true, + expectedCDISpecDirs: []string{"/foo/bar", "/baz/qux"}, + }, + { + description: "experimental and empty string as spec dir returns empty slice", + specDirs: []string{""}, + experimental: true, + expectedCDISpecDirs: []string{}, + }, + { + description: "experimental and empty config option returns empty slice", + configContent: `{"cdi-spec-dirs": []}`, + experimental: true, + expectedCDISpecDirs: []string{}, + }, + { + description: "non-experimental and no spec dirs specified returns no cdi spec dirs", + specDirs: nil, + experimental: false, + expectedCDISpecDirs: nil, + }, + { + description: "non-experimental and specified spec dirs returns no cdi spec dirs", + specDirs: []string{"/foo/bar", "/baz/qux"}, + experimental: false, + expectedCDISpecDirs: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + tempFile := fs.NewFile(t, "config", fs.WithContent(tc.configContent)) + defer tempFile.Remove() + + opts := defaultOptions(t, tempFile.Path()) + + flags := opts.flags + for _, specDir := range tc.specDirs { + assert.Check(t, flags.Set("cdi-spec-dir", specDir)) + } + if tc.experimental { + assert.Check(t, flags.Set("experimental", "true")) + } + + loadedConfig, err := loadDaemonCliConfig(opts) + assert.NilError(t, err) + + assert.Check(t, is.DeepEqual(tc.expectedCDISpecDirs, loadedConfig.CDISpecDirs, cmpopts.EquateEmpty())) + }) + } +} diff --git a/daemon/config/config.go b/daemon/config/config.go index 04edc579ec..c0865a7ec9 100644 --- a/daemon/config/config.go +++ b/daemon/config/config.go @@ -14,7 +14,6 @@ import ( "golang.org/x/text/encoding/unicode" "golang.org/x/text/transform" - "github.com/container-orchestrated-devices/container-device-interface/pkg/cdi" "github.com/containerd/containerd/log" "github.com/docker/docker/opts" "github.com/docker/docker/registry" @@ -288,7 +287,6 @@ func New() (*Config, error) { ContainerdNamespace: DefaultContainersNamespace, ContainerdPluginNamespace: DefaultPluginNamespace, DefaultRuntime: StockRuntimeName, - CDISpecDirs: append([]string(nil), cdi.DefaultSpecDirs...), }, } From bbb92555628b1236d78009a006843f6e73420023 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 1 Aug 2023 17:57:30 +0200 Subject: [PATCH 2/3] Skip CDI driver registration if CDISpecDirs is empty Signed-off-by: Evan Lezar --- cmd/dockerd/daemon.go | 4 +--- daemon/cdi.go | 41 +++++++++++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go index 2451d7f32d..267007792a 100644 --- a/cmd/dockerd/daemon.go +++ b/cmd/dockerd/daemon.go @@ -251,9 +251,7 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) { // - Support needs to be added to the cdi package for injecting Windows devices: https://github.com/container-orchestrated-devices/container-device-interface/issues/28 // - The DeviceRequests API must be extended to non-linux platforms. if runtime.GOOS == "linux" && cli.Config.Experimental { - daemon.RegisterCDIDriver( - cdi.WithSpecDirs(cli.Config.CDISpecDirs...), - ) + daemon.RegisterCDIDriver(cli.Config.CDISpecDirs...) } cli.d = d diff --git a/daemon/cdi.go b/daemon/cdi.go index cf77b978e0..1b4a0534ae 100644 --- a/daemon/cdi.go +++ b/daemon/cdi.go @@ -18,21 +18,29 @@ type cdiHandler struct { // RegisterCDIDriver registers the CDI device driver. // The driver injects CDI devices into an incoming OCI spec and is called for DeviceRequests associated with CDI devices. -func RegisterCDIDriver(opts ...cdi.Option) { - cache, err := cdi.NewCache(opts...) +// If the list of CDI spec directories is empty, the driver is not registered. +func RegisterCDIDriver(cdiSpecDirs ...string) { + driver := newCDIDeviceDriver(cdiSpecDirs...) + + registerDeviceDriver("cdi", driver) +} + +// newCDIDeviceDriver creates a new CDI device driver. +// If the creation of the CDI cache fails, a driver is returned that will return an error on an injection request. +func newCDIDeviceDriver(cdiSpecDirs ...string) *deviceDriver { + cache, err := createCDICache(cdiSpecDirs...) if err != nil { - log.G(context.TODO()).WithError(err).Error("CDI registry initialization failed") + log.G(context.TODO()).WithError(err) // We create a spec updater that always returns an error. // This error will be returned only when a CDI device is requested. - // This ensures that daemon startup is not blocked by a CDI registry initialization failure. + // This ensures that daemon startup is not blocked by a CDI registry initialization failure or being disabled + // by configuratrion. errorOnUpdateSpec := func(s *specs.Spec, dev *deviceInstance) error { - return fmt.Errorf("CDI device injection failed due to registry initialization failure: %w", err) + return fmt.Errorf("CDI device injection failed: %w", err) } - driver := &deviceDriver{ + return &deviceDriver{ updateSpec: errorOnUpdateSpec, } - registerDeviceDriver("cdi", driver) - return } // We construct a spec updates that injects CDI devices into the OCI spec using the initialized registry. @@ -40,11 +48,24 @@ func RegisterCDIDriver(opts ...cdi.Option) { registry: cache, } - driver := &deviceDriver{ + return &deviceDriver{ updateSpec: c.injectCDIDevices, } +} - registerDeviceDriver("cdi", driver) +// createCDICache creates a CDI cache for the specified CDI specification directories. +// If the list of CDI specification directories is empty or the creation of the CDI cache fails, an error is returned. +func createCDICache(cdiSpecDirs ...string) (*cdi.Cache, error) { + if len(cdiSpecDirs) == 0 { + return nil, fmt.Errorf("No CDI specification directories specified") + } + + cache, err := cdi.NewCache(cdi.WithSpecDirs(cdiSpecDirs...)) + if err != nil { + return nil, fmt.Errorf("CDI registry initialization failure: %w", err) + } + + return cache, nil } // injectCDIDevices injects a set of CDI devices into the specified OCI specification. From 7a59913b1a8b302fa169234aa86e143f18de4073 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 18 Jul 2023 01:50:08 +0200 Subject: [PATCH 3/3] Add CDISpecDirs to Info output This change adds the configured CDI spec directories to the system info output. Signed-off-by: Evan Lezar --- api/swagger.yaml | 18 ++++++ api/types/system/info.go | 1 + daemon/info.go | 10 ++++ docs/api/version-history.md | 14 +++-- integration/container/cdi_test.go | 97 +++++++++++++++++++++++++++++++ 5 files changed, 135 insertions(+), 5 deletions(-) diff --git a/api/swagger.yaml b/api/swagger.yaml index 201e043e02..e6f579adf7 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -5301,7 +5301,25 @@ definitions: - "WARNING: No memory limit support" - "WARNING: bridge-nf-call-iptables is disabled" - "WARNING: bridge-nf-call-ip6tables is disabled" + CDISpecDirs: + description: | + List of directories where (Container Device Interface) CDI + specifications are located. + These specifications define vendor-specific modifications to an OCI + runtime specification for a container being created. + + An empty list indicates that CDI device injection is disabled. + + Note that since using CDI device injection requires the daemon to have + experimental enabled. For non-experimental daemons an empty list will + always be returned. + type: "array" + items: + type: "string" + example: + - "/etc/cdi" + - "/var/run/cdi" # PluginsInfo is a temp struct holding Plugins name # registered with docker daemon. It is used by Info struct diff --git a/api/types/system/info.go b/api/types/system/info.go index bea649035f..09dbbd0926 100644 --- a/api/types/system/info.go +++ b/api/types/system/info.go @@ -73,6 +73,7 @@ type Info struct { SecurityOptions []string ProductLicense string `json:",omitempty"` DefaultAddressPools []NetworkAddressPool `json:",omitempty"` + CDISpecDirs []string // Legacy API fields for older API versions. legacyFields diff --git a/daemon/info.go b/daemon/info.go index 3fc7105ec3..ca0dbc4bd3 100644 --- a/daemon/info.go +++ b/daemon/info.go @@ -62,6 +62,7 @@ func (daemon *Daemon) SystemInfo() *system.Info { NoProxy: getConfigOrEnv(cfg.NoProxy, "NO_PROXY", "no_proxy"), LiveRestoreEnabled: cfg.LiveRestoreEnabled, Isolation: daemon.defaultIsolation, + CDISpecDirs: promoteNil(cfg.CDISpecDirs), } daemon.fillContainerStates(v) @@ -309,3 +310,12 @@ func getConfigOrEnv(config string, env ...string) string { } return getEnvAny(env...) } + +// promoteNil converts a nil slice to an empty slice of that type. +// A non-nil slice is returned as is. +func promoteNil[S ~[]E, E any](s S) S { + if s == nil { + return S{} + } + return s +} diff --git a/docs/api/version-history.md b/docs/api/version-history.md index 1d6cfe8210..dbc65dc22f 100644 --- a/docs/api/version-history.md +++ b/docs/api/version-history.md @@ -34,6 +34,10 @@ keywords: "API, Docker, rcli, REST, documentation" `BindOptions.ReadOnlyNonRecursive` and `BindOptions.ReadOnlyForceRecursive` to customize the behavior. * `POST /containers/create` now accepts a `HealthConfig.StartInterval` to set the interval for health checks during the start period. +* `GET /info` now includes a `CDISpecDirs` field indicating the configured CDI + specifications directories. The use of the applied setting requires the daemon + to have expermental enabled, and for non-experimental daemons an empty list is + always returned. ## v1.43 API changes @@ -103,7 +107,7 @@ keywords: "API, Docker, rcli, REST, documentation" a default. This change is not versioned, and affects all API versions if the daemon has - this patch. + this patch. * `GET /_ping` and `HEAD /_ping` now return a `Swarm` header, which allows a client to detect if Swarm is enabled on the daemon, without having to call additional endpoints. @@ -126,7 +130,7 @@ keywords: "API, Docker, rcli, REST, documentation" versioned, and affects all API versions if the daemon has this patch. * `GET /containers/{id}/attach`, `GET /exec/{id}/start`, `GET /containers/{id}/logs` `GET /services/{id}/logs` and `GET /tasks/{id}/logs` now set Content-Type header - to `application/vnd.docker.multiplexed-stream` when a multiplexed stdout/stderr + to `application/vnd.docker.multiplexed-stream` when a multiplexed stdout/stderr stream is sent to client, `application/vnd.docker.raw-stream` otherwise. * `POST /volumes/create` now accepts a new `ClusterVolumeSpec` to create a cluster volume (CNI). This option can only be used if the daemon is a Swarm manager. @@ -139,7 +143,7 @@ keywords: "API, Docker, rcli, REST, documentation" * Volume information returned by `GET /volumes/{name}`, `GET /volumes` and `GET /system/df` can now contain a `ClusterVolume` if the volume is a cluster volume (requires the daemon to be a Swarm manager). -* The `Volume` type, as returned by `Added new `ClusterVolume` fields +* The `Volume` type, as returned by `Added new `ClusterVolume` fields * Added a new `PUT /volumes{name}` endpoint to update cluster volumes (CNI). Cluster volumes are only supported if the daemon is a Swarm manager. * `GET /containers/{name}/attach/ws` endpoint now accepts `stdin`, `stdout` and @@ -355,7 +359,7 @@ keywords: "API, Docker, rcli, REST, documentation" [Docker Engine API v1.36](https://docs.docker.com/engine/api/v1.36/) documentation -* `Get /events` now return `exec_die` event when an exec process terminates. +* `Get /events` now return `exec_die` event when an exec process terminates. ## v1.35 API changes @@ -563,7 +567,7 @@ keywords: "API, Docker, rcli, REST, documentation" * `POST /services/create` and `POST /services/(id or name)/update` now accept the `TTY` parameter, which allocate a pseudo-TTY in container. * `POST /services/create` and `POST /services/(id or name)/update` now accept the `DNSConfig` parameter, which specifies DNS related configurations in resolver configuration file (resolv.conf) through `Nameservers`, `Search`, and `Options`. * `POST /services/create` and `POST /services/(id or name)/update` now support - `node.platform.arch` and `node.platform.os` constraints in the services + `node.platform.arch` and `node.platform.os` constraints in the services `TaskSpec.Placement.Constraints` field. * `GET /networks/(id or name)` now includes IP and name of all peers nodes for swarm mode overlay networks. * `GET /plugins` list plugins. diff --git a/integration/container/cdi_test.go b/integration/container/cdi_test.go index 91dcbc3476..822f3eeb4c 100644 --- a/integration/container/cdi_test.go +++ b/integration/container/cdi_test.go @@ -3,6 +3,7 @@ package container // import "github.com/docker/docker/integration/container" import ( "bytes" "context" + "encoding/json" "io" "os" "path/filepath" @@ -62,3 +63,99 @@ func TestCreateWithCDIDevices(t *testing.T) { outlines := strings.Split(actualStdout.String(), "\n") assert.Assert(t, is.Contains(outlines, "FOO=injected")) } + +func TestCDISpecDirsAreInSystemInfo(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType == "windows") // d.Start fails on Windows with `protocol not available` + // TODO: This restriction can be relaxed with https://github.com/moby/moby/pull/46158 + skip.If(t, testEnv.IsRootless, "the t.TempDir test creates a folder with incorrect permissions for rootless") + + testCases := []struct { + description string + config map[string]interface{} + experimental bool + specDirs []string + expectedInfoCDISpecDirs []string + }{ + { + description: "experimental no spec dirs specified returns default", + experimental: true, + specDirs: nil, + expectedInfoCDISpecDirs: []string{"/etc/cdi", "/var/run/cdi"}, + }, + { + description: "experimental specified spec dirs are returned", + experimental: true, + specDirs: []string{"/foo/bar", "/baz/qux"}, + expectedInfoCDISpecDirs: []string{"/foo/bar", "/baz/qux"}, + }, + { + description: "experimental empty string as spec dir returns empty slice", + experimental: true, + specDirs: []string{""}, + expectedInfoCDISpecDirs: []string{}, + }, + { + description: "experimental empty config option returns empty slice", + experimental: true, + config: map[string]interface{}{"cdi-spec-dirs": []string{}}, + expectedInfoCDISpecDirs: []string{}, + }, + { + description: "non-experimental no spec dirs specified returns empty slice", + experimental: false, + specDirs: nil, + expectedInfoCDISpecDirs: []string{}, + }, + { + description: "non-experimental specified spec dirs returns empty slice", + experimental: false, + specDirs: []string{"/foo/bar", "/baz/qux"}, + expectedInfoCDISpecDirs: []string{}, + }, + { + description: "non-experimental empty string as spec dir returns empty slice", + experimental: false, + specDirs: []string{""}, + expectedInfoCDISpecDirs: []string{}, + }, + { + description: "non-experimental empty config option returns empty slice", + experimental: false, + config: map[string]interface{}{"cdi-spec-dirs": []string{}}, + expectedInfoCDISpecDirs: []string{}, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + var opts []daemon.Option + if tc.experimental { + opts = append(opts, daemon.WithExperimental()) + } + d := daemon.New(t, opts...) + + var args []string + for _, specDir := range tc.specDirs { + args = append(args, "--cdi-spec-dir="+specDir) + } + if tc.config != nil { + configPath := filepath.Join(t.TempDir(), "daemon.json") + + configFile, err := os.Create(configPath) + assert.NilError(t, err) + defer configFile.Close() + + err = json.NewEncoder(configFile).Encode(tc.config) + assert.NilError(t, err) + + args = append(args, "--config-file="+configPath) + } + d.Start(t, args...) + defer d.Stop(t) + + info := d.Info(t) + + assert.Check(t, is.DeepEqual(tc.expectedInfoCDISpecDirs, info.CDISpecDirs)) + }) + } +}