Skip to content

Fix Docker API DeviceMapping for CDI devices#28495

Merged
mheon merged 1 commit into
podman-container-tools:mainfrom
Honny1:fix-device-compat-api
Apr 14, 2026
Merged

Fix Docker API DeviceMapping for CDI devices#28495
mheon merged 1 commit into
podman-container-tools:mainfrom
Honny1:fix-device-compat-api

Conversation

@Honny1

@Honny1 Honny1 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

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

@Luap99 Luap99 left a comment

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.

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
Contributor 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

Honny1 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor Author

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

Honny1 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor Author

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

mheon commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

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...

@Luap99 Luap99 left a comment

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.

LGTM

@mheon

mheon commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Sure, LGTM

@mheon mheon merged commit e5f4849 into podman-container-tools:main Apr 14, 2026
83 checks passed
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