Skip to content

Fix Docker API DeviceMapping for CDI devices#28495

Open
Honny1 wants to merge 1 commit intocontainers:mainfrom
Honny1:fix-device-compat-api
Open

Fix Docker API DeviceMapping for CDI devices#28495
Honny1 wants to merge 1 commit intocontainers:mainfrom
Honny1:fix-device-compat-api

Conversation

@Honny1
Copy link
Copy Markdown
Member

@Honny1 Honny1 commented Apr 13, 2026

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

  • Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
    commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
    the sign-off email address. See CONTRIBUTING.md
    for more information.
  • Referenced issues using Fixes: #00000 in commit message (if applicable)
  • Tests have been added/updated (or no tests are needed)
  • Documentation has been updated (or no documentation changes are needed)
  • All commits pass make validatepr (format/lint checks)
  • Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

Compat API Container create no longer turns CDI-qualified HostConfig.Devices entries into invalid `host:container:permissions` strings when optional fields are empty, so compose-style clients can pass CDI selectors reliably. DeviceRequests with driver cdi are now matched case-insensitively.

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Apr 13, 2026
@Honny1 Honny1 marked this pull request as ready for review April 13, 2026 13:19
@Honny1 Honny1 force-pushed the fix-device-compat-api branch from 4c035d2 to 202ce2c Compare April 13, 2026 13:47
Copy link
Copy Markdown
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

an e2e compose test like test/compose/cdi_device/docker-compose.yml would make sense, I guess just add a second device to the config and then pass it as normal device in compose?

}
for _, r := range cc.HostConfig.Resources.DeviceRequests {
if r.Driver == "cdi" {
if strings.EqualFold(r.Driver, "cdi") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My original plan was to take a defensive approach, but I realize now that it doesn't make sense. I will fix it.

@Honny1
Copy link
Copy Markdown
Member Author

Honny1 commented Apr 13, 2026

an e2e compose test like test/compose/cdi_device/docker-compose.yml would make sense, I guess just add a second device to the config and then pass it as normal device in compose?

I'm not sure about this. Newer versions (since docker/compose#12184) of Compose send CDI devices through cc.HostConfig.Resources.DeviceRequests, so the new test will always pass.

@Honny1 Honny1 force-pushed the fix-device-compat-api branch from 202ce2c to 73775ce Compare April 13, 2026 18:26
@Honny1
Copy link
Copy Markdown
Member Author

Honny1 commented Apr 13, 2026

I am not sure if this change fixes the reported issues, as I cannot directly verify them. The main goal of this change is to make defensive improvements for edge cases that might occur with older versions of Compose or other clients.

Signed-off-by: Jan Rodák <hony.com@seznam.cz>
@mheon
Copy link
Copy Markdown
Member

mheon commented Apr 13, 2026

It seems like a reasonable change regardless. I wonder if the people seeing this are all on Compose v1? We dropped all our tests for that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/api-change Change to remote API; merits scrutiny

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants