Add CDISpecDirs to Info output by elezar · Pull Request #46004 · moby/moby

elezar · 2023-07-18T00:11:21Z

This PR makes the following changes:

An empty "cdi-spec-dirs" in the daemon config or specifying --cdi-spec-dir="" will disable CDI injection even if experimental mode is enabled.
The configured value for cdi-spec-dirs is added to the system info output. Note that an empty slice is explicitly output. This is a follow-up to the changes adding CDI support as per Add support for CDI devices under Linux #45134 (comment).

- What I did

Added the configured cdi-spec-dirs to the system info output.

- How I did it

Moved the initialisation to the default options to loadDaemonCliConfig to handle the case where the option is not set to an empty slice.
Updated the system.Info struct with a CDISpecDirs field that is filled from the config option when daemon.SystemInfo is called.

- How to verify it

The change includes unit tests to ensure that the applied setting is returned.

- Description for the changelog

Disable CDI device injection if cdi-spec-dirs is empty or set to a single empty string using the command line arguments.
Added CDISpecDirs to system info output.

- A picture of a cute animal (not mandatory but encouraged)

daemon/info.go

corhere

As I explained on Slack, the integration tests are failing because of how zero-length slices are being round-tripped through JSON. As the struct field is tagged json:",omitempty", nil and zero-length slice fields marshal to an undefined JSON property. And an undefined JSON property unmarshals to a no-op (i.e. leave the struct field untouched) so when unmarshalling to a zero-valued struct, the result is a nil slice.

To make the Info API endpoint output {"CDISpecDirs": []} and get the integration tests to pass, remove the struct tag and arrange to have the (system.Info).CDISpecDirs field always be non-nil. A generic function could be used to do so ergonomically:

func promoteNil[S ~[]E, E any](s S) S {
	if s == nil {
		return S{}
	}
	return s
}

(See here for an explanation on why the type argument is written ~[]E.)

corhere · 2023-07-26T16:28:48Z

cmd/dockerd/daemon.go

+		// If CDISpecDirs is set to an empty string, we set it to an empty slice.
+		conf.CDISpecDirs = []string{}


https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices

Suggested change

// If CDISpecDirs is set to an empty string, we set it to an empty slice.

conf.CDISpecDirs = []string{}

// If CDISpecDirs is set to an empty string, we set it to an empty slice.

conf.CDISpecDirs = nil

@corhere, looking at this again, I don't think that this suggestion is applicable here. We want the case of "cdi-spec-dirs": [""] (set by passing --cdi-spec-dir="" to the daemon command line) to be equivalent to "cdi-spec-dirs": [] in terms of the semantics around disabling CDI injection.

This was also reflected in the unit tests:

{ description: "empty string as spec dir returns nil", specDirs: []string{""}, expectedCDISpecDirs: []string{}, }, { description: "empty config option returns nil", configContent: `{"cdi-spec-dirs": []}`, expectedCDISpecDirs: []string{}, },

which fail with the code as it is.

My question is then, do we update the tests to reflect that this is now set to nil, or is it important to be consistent? If it is the latter, do we rever the change requested here, or do we also explicitly set conf.CDISpecDirs to nil if [] is specified in the config?

Option C: update the daemon-config tests (not the Info API tests) to consider all zero-length slices as equal to not over-fit the config interface contract. I don't think it's necessary to coerce an unmarshaled [] to nil as code which consumes conf.CDISpecDirs is buggy by definition if it treats a nil slice differently from an empty slice. Besides, any code is likely to be exposed to conf.CDISpecDirs == nil anyway via unit tests which pass in a zero-valued or initialized-to-defaults config.

Oh. I like that. Wasn't aware of that comparison. Will update.

Updated in latest.

cmd/dockerd/daemon.go

cmd/dockerd/daemon_test.go

elezar · 2023-07-26T18:39:26Z

Thanks for the round of reviews. I'm on PTO until early next week, so will have a look again then. Thinking about it quickly, a nil value and an empty slice are equivalent from the point of view of the integration tests, so updating their checks may make the most sense. Will address this on Tuesday

corhere · 2023-07-26T18:56:44Z

Thinking about it quickly, a nil value and an empty slice are equivalent from the point of view of the integration tests,

If that was true, the integration tests would be passing right now. An empty slice is not equivalent to a nil value from the point of view of the integration tests.

so updating their checks may make the most sense.

Please don't update the tests to treat a nil value as equivalent to an empty slice. Outside of the Go ecosystem, {"CDISpecDirs": []} is not the same as {"CDISpecDirs": null} is not the same as {}. A test which conflates the three would neglect to catch accidentally introducing a breaking API change, which would not be a very good test. As far as I'm concerned, the integration tests are correct and complete as currently written. Update the implementation to get the tests to pass.

elezar

Thanks for the comments and feedback @corhere.

It may make sense to split this into two PRs. One with the improved initialisation of the CDISpecDirs member, and the other with the addition of this field to system info.

daemon/info.go

thaJeztah · 2023-08-01T16:18:47Z

api/types/system/info.go

 	SecurityOptions     []string
 	ProductLicense      string               `json:",omitempty"`
 	DefaultAddressPools []NetworkAddressPool `json:",omitempty"`
+	CDISpecDirs         []string


Can you also add the new field

add the new field to the swagger

moby/api/swagger.yaml

Line 4860 in 69c9adb

SystemInfo:

add a bullet to the API version-history https://github.com/moby/moby/blob/69c9adb7d3868cb0560b1ffcef798015c5a70510/docs/api/version-history.md#v144-api-changes

For both, we probably should mention that it requires the daemon to have experimental enabled (for this release)

Also wondering if we still need aomitempty and to omit the field for API < 1.44 (as this fields "doesn't exist" in older API versions).

We're generally more relaxed in returning additional fields (as we do maintain an "open schema", so happy to hear from others on that.

Also wondering if we still need aomitempty and to omit the field for API < 1.44 (as this fields "doesn't exist" in older API versions).

I sure hope not. I wouldn't want to juggle multiple versioned Info structs if it can be avoided. The omitempty tag suppresses marshaling of empty, non-nil slices.

For both, we probably should mention that it requires the daemon to have experimental enabled (for this release)

This got me thinking. Should we also set CDISpecDirs to nil explicitly if experimental != true?

I like that idea; users could then use len(Info.CDISpecDirs) > 0 as the signal that CDI support is enabled on the daemon, without needing to know whether the feature has graduated from experimental.

I have updated the implementation and tests to clear the setting if experimental != true. I have not removed the gating of the call to RegisterCDIDriver for the time being but can if anyone feels strongly about it. The downside is that we would have to make two changes once we graduate this to a non-experimental feature.

daemon/cdi.go

daemon/info.go

Signed-off-by: Evan Lezar <[email protected]>

corhere · 2023-08-03T18:00:24Z

integration/container/cdi_test.go

+				args = append(args, "--cdi-spec-dir="+specDir)
+			}
+			if tc.config != nil {
+				configPath := filepath.Join(t.TempDir(), "daemon.json")


The tests are failing with a rootless daemon because the daemon doesn't have permission to read the file. os.Create() creates the file with mode 0666 and t.TempDir() creates the leaf directory with mode 0777, but it also uses os.MkdirTemp() to create the parent of that directory, which creates it with mode 0700. I'm not sure what other tests do for this situation; the easiest thing to do would be to simply skip the tests on rootless.

daemon.go:313: [d1f3a08e0cab1] unable to configure the Docker daemon with file /tmp/TestCDISpecDirsAreInSystemInfoexperimental_empty_config_option_returns_empty_slice4240331815/001/daemon.json: open /tmp/TestCDISpecDirsAreInSystemInfoexperimental_empty_config_option_returns_empty_slice4240331815/001/daemon.json: permission denied daemon.go:313: [d1f3a08e0cab1] [rootlesskit:child ] error: command [/usr/local/bin/dockerd-rootless.sh --data-root /go/src/github.com/docker/docker/bundles/test-integration/TestCDISpecDirsAreInSystemInfo/experimental_empty_config_option_returns_empty_slice/d1f3a08e0cab1/root --exec-root /tmp/dxr/d1f3a08e0cab1 --pidfile /go/src/github.com/docker/docker/bundles/test-integration/TestCDISpecDirsAreInSystemInfo/experimental_empty_config_option_returns_empty_slice/d1f3a08e0cab1/docker.pid --userland-proxy=true --containerd-namespace d1f3a08e0cab1 --containerd-plugins-namespace d1f3a08e0cab1p --experimental --host unix:///tmp/docker-integration/d1f3a08e0cab1.sock --debug --storage-driver overlay2 --config-file=/tmp/TestCDISpecDirsAreInSystemInfoexperimental_empty_config_option_returns_empty_slice4240331815/001/daemon.json] exited: exit status 1

This here

moby/integration/container/run_linux_test.go

Lines 229 to 242 in eb3ace9

// t.TempDir() can't be used here as the temporary directory returned by

// that function cannot be accessed by the fake-root user for rootless

// Docker. It creates a nested hierarchy of directories where the

// outermost has permission 0700.

shimDir, err := os.MkdirTemp("", t.Name())

assert.Assert(t, err)

t.Cleanup(func() {

if err := os.RemoveAll(shimDir); err != nil {

t.Errorf("shimDir RemoveAll cleanup: %v", err)

}

})

assert.Assert(t, os.Chmod(shimDir, 0o777))

shimDir, err = filepath.Abs(shimDir)

assert.Assert(t, err)

looks like the correct way to handle it.

We could pull this into a testutils package that we could reuse (created #46158), but for now I'll just skip the rootless tests.

This change adds the configured CDI spec directories to the system info output. Signed-off-by: Evan Lezar <[email protected]>

thaJeztah

LGTM

thaJeztah · 2023-08-22T12:07:32Z

opened info: add CDI spec directories to output docker/cli#4510 to add this information to the output of docker info

elezar force-pushed the add-cdi-spec-dirs-to-info branch from fdabd72 to 9f6f0ef Compare July 18, 2023 00:20

elezar mentioned this pull request Jul 18, 2023

Graduate injection of CDI devices from experimental to GA #45192

Closed

7 tasks

elezar force-pushed the add-cdi-spec-dirs-to-info branch from 9f6f0ef to c1fb396 Compare July 18, 2023 01:21

elezar commented Jul 18, 2023

View reviewed changes

daemon/info.go Outdated Show resolved Hide resolved

elezar force-pushed the add-cdi-spec-dirs-to-info branch from c1fb396 to 4978d60 Compare July 18, 2023 06:03

elezar marked this pull request as ready for review July 18, 2023 14:23

thaJeztah added this to the 25.0.0 milestone Jul 20, 2023

neersighted mentioned this pull request Jul 24, 2023

25.0 Release Tracking #45926

Closed

26 tasks

elezar force-pushed the add-cdi-spec-dirs-to-info branch 3 times, most recently from b8890de to c149ae8 Compare July 26, 2023 12:44

corhere reviewed Jul 26, 2023

View reviewed changes

thaJeztah added area/api API status/2-code-review impact/api impact/changelog labels Jul 27, 2023

elezar force-pushed the add-cdi-spec-dirs-to-info branch 2 times, most recently from 96354b8 to 4f397ad Compare August 1, 2023 16:05

elezar commented Aug 1, 2023

View reviewed changes

daemon/info.go Show resolved Hide resolved

thaJeztah reviewed Aug 1, 2023

View reviewed changes

corhere reviewed Aug 2, 2023

View reviewed changes

daemon/cdi.go Outdated Show resolved Hide resolved

daemon/info.go Show resolved Hide resolved

elezar force-pushed the add-cdi-spec-dirs-to-info branch from 4f397ad to 9068959 Compare August 2, 2023 09:33

elezar added 2 commits August 3, 2023 12:21

Set default CDI spec dirs after parsing args

3b71197

Signed-off-by: Evan Lezar <[email protected]>

Skip CDI driver registration if CDISpecDirs is empty

bbb9255

Signed-off-by: Evan Lezar <[email protected]>

elezar force-pushed the add-cdi-spec-dirs-to-info branch from 9068959 to a65da24 Compare August 3, 2023 11:22

corhere reviewed Aug 3, 2023

View reviewed changes

Add CDISpecDirs to Info output

7a59913

This change adds the configured CDI spec directories to the system info output. Signed-off-by: Evan Lezar <[email protected]>

elezar force-pushed the add-cdi-spec-dirs-to-info branch from a65da24 to 7a59913 Compare August 4, 2023 09:46

corhere approved these changes Aug 4, 2023

View reviewed changes

thaJeztah approved these changes Aug 7, 2023

View reviewed changes

thaJeztah merged commit 4b19b2f into moby:master Aug 7, 2023

thaJeztah added the impact/documentation label Aug 7, 2023

thaJeztah added the docs/revisit label Aug 22, 2023

thaJeztah mentioned this pull request Aug 22, 2023

info: add CDI spec directories to output docker/cli#4510

Merged

elezar mentioned this pull request Aug 25, 2023

Add documentation for CDI support docker/cli#4525

Closed

5 tasks

elezar deleted the add-cdi-spec-dirs-to-info branch September 8, 2023 10:13

thaJeztah mentioned this pull request Jan 17, 2024

cdi: use separate feature-flag #47087

Merged

		// If CDISpecDirs is set to an empty string, we set it to an empty slice.
		conf.CDISpecDirs = []string{}

	// t.TempDir() can't be used here as the temporary directory returned by
	// that function cannot be accessed by the fake-root user for rootless
	// Docker. It creates a nested hierarchy of directories where the
	// outermost has permission 0700.
	shimDir, err := os.MkdirTemp("", t.Name())
	assert.Assert(t, err)
	t.Cleanup(func() {
	if err := os.RemoveAll(shimDir); err != nil {
	t.Errorf("shimDir RemoveAll cleanup: %v", err)
	}
	})
	assert.Assert(t, os.Chmod(shimDir, 0o777))
	shimDir, err = filepath.Abs(shimDir)
	assert.Assert(t, err)

Conversation

elezar commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elezar commented Jul 26, 2023

Uh oh!

corhere commented Jul 26, 2023

Uh oh!

elezar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elezar commented Jul 18, 2023 •

edited

Loading