- Notifications
You must be signed in to change notification settings - Fork18.8k
Add CDISpecDirs to Info output#46004
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fdabd72 to9f6f0efCompare9f6f0ef toc1fb396CompareUh oh!
There was an error while loading.Please reload this page.
c1fb396 to4978d60Compareb8890de toc149ae8CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 taggedjson:",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 anil 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:
funcpromoteNil[S~[]E,Eany](sS)S {ifs==nil {returnS{}}returns}
(See here for an explanation on why the type argument is written~[]E.)
cmd/dockerd/daemon.go Outdated
| // If CDISpecDirs is set to an empty string, we set it to an empty slice. | ||
| conf.CDISpecDirs = []string{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices
| // 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 setconf.CDISpecDirs tonil if[] is specified in the config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Option C: update the daemon-config tests (not the Info API tests) toconsider 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[] tonil as code which consumesconf.CDISpecDirs is buggy by definition if it treats a nil slice differently from an empty slice. Besides, any code is likely to be exposed toconf.CDISpecDirs == nil anyway via unit tests which pass in a zero-valued or initialized-to-defaults config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh. I like that. Wasn't aware of that comparison. Will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated in latest.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 |
If that was true, the integration tests would be passing right now. An empty slice isnot equivalent to a nil value from the point of view of the integration tests.
Please don't update the tests to treat a nil value as equivalent to an empty slice. Outside of the Go ecosystem, |
96354b8 to4f397adCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the comments and feedback@corhere.
It may make sense to split this into two PRs. One with the improved initialisation of theCDISpecDirs member, and the other with the addition of this field tosystem info.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -73,6 +73,7 @@ type Info struct { | |||
| SecurityOptions []string | |||
| ProductLicense string `json:",omitempty"` | |||
| DefaultAddressPools []NetworkAddressPool `json:",omitempty"` | |||
| CDISpecDirs []string | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you also add the new field
- add the new field to the swagger
Line 4860 in69c9adb
SystemInfo: - add a bullet to the API version-historyhttps://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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also wondering if we still need a
omitemptyand 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 versionedInfo structs if it can be avoided.Theomitempty tag suppresses marshaling of empty, non-nil slices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 setCDISpecDirs to nil explicitly ifexperimental != true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like that idea; users could then uselen(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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have updated the implementation and tests to clear the setting ifexperimental != true. I have not removed the gating of the call toRegisterCDIDriver 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
4f397ad to9068959CompareSigned-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
9068959 toa65da24Compare| args = append(args, "--cdi-spec-dir="+specDir) | ||
| } | ||
| if tc.config != nil { | ||
| configPath := filepath.Join(t.TempDir(), "daemon.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 andt.TempDir() creates the leaf directory with mode 0777, but it also usesos.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 1There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This here
moby/integration/container/run_linux_test.go
Lines 229 to 242 ineb3ace9
| // 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() { | |
| iferr:=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) |
We could pull this into atestutils 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 thesystem info output.Signed-off-by: Evan Lezar <elezar@nvidia.com>
a65da24 to7a59913CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
|
Uh oh!
There was an error while loading.Please reload this page.
This PR makes the following changes:
"cdi-spec-dirs"in the daemon config or specifying--cdi-spec-dir=""will disable CDI injection even if experimental mode is enabled.cdi-spec-dirsis 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 perAdd support for CDI devices under Linux #45134 (comment).- What I did
Added the configured
cdi-spec-dirsto the system info output.- How I did it
loadDaemonCliConfigto handle the case where the option is not set to an empty slice.system.Infostruct with aCDISpecDirsfield that is filled from the config option whendaemon.SystemInfois called.- How to verify it
The change includes unit tests to ensure that the applied setting is returned.
- Description for the changelog
cdi-spec-dirsis empty or set to a single empty string using the command line arguments.CDISpecDirsto system info output.- A picture of a cute animal (not mandatory but encouraged)