Skip to content

daemon: Discover devices and include in system info #49980

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
Merged
vvoland merged 5 commits into from
May 16, 2025
Merged

daemon: Discover devices and include in system info #49980

vvoland merged 5 commits into from
May 16, 2025

Conversation

@vvoland
Copy link
Contributor

@vvoland vvoland commented May 14, 2025 •
edited
Loading

Add ability for the device driver to implement a device discovery mechanism and expose discovered devices in the docker info output.

Currently it's only implemented for CDI devices.

- How to verify it
New tests

Example /info output:

...
  "DiscoveredDevices": [
    {
      "Source": "cdi",
      "Name": "test.com/device=mygpu0",
    }
  ],
...

- Human readable description for the release notes

`GET /info` now includes a `DiscoveredDevices` field. This is an array of `DeviceInfo` objects, each providing details about a device discovered by a device driver.

TODO: Make CLI print it (docker info now returns DiscoveredDevices providing a result of CDI device discovery.)

@vvoland vvoland added this to the 28.2.0 milestone May 14, 2025
@vvoland vvoland self-assigned this May 14, 2025
@vvoland vvoland added kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny impact/changelog area/daemon Core Engine area/go-sdk area/cdi labels May 14, 2025
@vvoland vvoland mentioned this pull request May 14, 2025
Merged
thaJeztah
thaJeztah reviewed May 14, 2025
View reviewed changes
@vvoland vvoland force-pushed the devices-discovery branch from b4cb42c to aa737a3 Compare May 14, 2025 13:50
tonistiigi
tonistiigi reviewed May 14, 2025
View reviewed changes
thaJeztah
thaJeztah reviewed May 14, 2025
View reviewed changes
@vvoland vvoland force-pushed the devices-discovery branch from aa737a3 to 50d3900 Compare May 14, 2025 17:00
vvoland
vvoland commented May 14, 2025
View reviewed changes
api/types/system/info.go Outdated
Comment on lines 169 to 171
// Name is the unique identifier for the device.
// Example: CDI FQDN like "vendor.com/gpu=0", or other driver-specific device ID
Name string `json:"Name"`
Copy link
Contributor Author

@vvoland vvoland May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm, on a second look, Name is probably too CDI-centric.
Perhaps ID would make more sense here to keep consistency with the actual DeviceRequest struct we use in our API.

moby/api/types/container/hostconfig.go

Line 263 in 2154b9c

DeviceIDs []string // List of device IDs as recognizable by the device driver

@thaJeztah @tonistiigi WDYT?

Copy link
Member

@tonistiigi tonistiigi May 15, 2025

Choose a reason for hiding this comment

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

ok with calling it smth else. It should have the value that I can use in --device. Doesn't Source make more sense as Type?

Copy link
Contributor Author

@vvoland vvoland May 15, 2025

Choose a reason for hiding this comment

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

Source is the name of the device driver that discovered the device though. So that would be a cdi or nvidia. I think calling it Type would be more ambiguous?

@thompson-shaun thompson-shaun added the release-blocker PRs we want to block a release on label May 15, 2025
thaJeztah
thaJeztah reviewed May 15, 2025
View reviewed changes
@vvoland vvoland force-pushed the devices-discovery branch from 50d3900 to 08b40cb Compare May 16, 2025 10:50
@vvoland
Copy link
Contributor Author

vvoland commented May 16, 2025

Changed Name to ID and updated API version to 1.50.

@thaJeztah thaJeztah mentioned this pull request May 16, 2025
Merged
thaJeztah
thaJeztah reviewed May 16, 2025
View reviewed changes
api/common.go
const (
// DefaultVersion of the current REST API.
DefaultVersion = "1.49"
DefaultVersion = "1.50"
Copy link
Member

@thaJeztah thaJeztah May 16, 2025

Choose a reason for hiding this comment

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

We also need to update versions mentioned in the swagger file, e.g.

moby/api/swagger.yaml

Lines 58 to 60 in c04dec1

If you omit the version-prefix, the current version of the API (v1.49) is used.
For example, calling `/info` is the same as calling `/v1.49/info`. Using the
API without a version-prefix is deprecated and will be removed in a future release.

"cleanest" would be to to both that, as well as the version-history.md in a single commit, like 4390ab2

Copy link
Contributor Author

@vvoland vvoland May 16, 2025

Choose a reason for hiding this comment

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

Done, thanks!

thaJeztah
thaJeztah reviewed May 16, 2025
View reviewed changes
thaJeztah
thaJeztah approved these changes May 16, 2025
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

code changes LGTM

left a comment for the API version bump (and missing bump in swagger)

vvoland added 2 commits May 16, 2025 17:03
@vvoland
daemon: Fix error log when CDI cache creation failed
23bbfea 
Logger was created but no consumed.

Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland
testutil/daemon: Don't use devcontainers daemon.json
b70b496 
Prevent the daemon spawned for integration tests from sourcing the
daemon configuration intended interactive dev shell usage.

Before this change, integration tests would fail to create a daemon
with different configuration provided via cli flags (like `--feature`)
if they're already specified in the default daemon.json.

Signed-off-by: Paweł Gronowski <[email protected]>
vvoland added 3 commits May 16, 2025 17:03
@vvoland
api: bump API version to 1.50
f95a7c4 
Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland
daemon: Discover devices and include in system info
9095698 
Add ability for the device driver to implement a device discovery
mechanism and expose discovered devices in the `docker info` output.

Currently it's only implemented for CDI devices.

Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland
client/info_test: Use gotest.tools asserts
c1b2be0 
Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland vvoland force-pushed the devices-discovery branch from 08b40cb to c1b2be0 Compare May 16, 2025 15:03
thaJeztah
thaJeztah approved these changes May 16, 2025
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@vvoland vvoland merged commit 81116f7 into moby:master May 16, 2025
141 checks passed
@vvoland vvoland mentioned this pull request May 16, 2025
Merged
@vvoland vvoland moved this from Up next to Complete in 🔦 Maintainer spotlight May 22, 2025
@vvoland vvoland mentioned this pull request Jun 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi

Assignees

@dmcgowan dmcgowan

@vvoland vvoland

Labels

area/api API area/cdi area/daemon Core Engine area/go-sdk impact/api impact/changelog kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny release-blocker PRs we want to block a release on

Projects

🔦 Maintainer spotlight
Archived in project

Milestone

28.2.0

Development

Successfully merging this pull request may close these issues.

5 participants

@vvoland @dmcgowan @tonistiigi @thaJeztah @thompson-shaun