Skip to content

Mount (accessible) host devices in --privileged rootless containers#42638

Merged
thaJeztah merged 1 commit intomoby:masterfrom
eliaskoromilas:host-devices
Mar 24, 2022
Merged

Mount (accessible) host devices in --privileged rootless containers#42638
thaJeztah merged 1 commit intomoby:masterfrom
eliaskoromilas:host-devices

Conversation

@eliaskoromilas
Copy link
Copy Markdown
Contributor

@eliaskoromilas eliaskoromilas commented Jul 14, 2021 •
edited
Loading

fixes #42406

- What I did

- How I did it

  • Removed the condition that skipped that process in rootless mode.
  • containerd/oci.HostDevices() ignores permission errors when running in a user namespace.

- How to verify it

Integration tests are included.

- Description for the changelog

Mount (accessible) host devices in --privileged rootless containers

@thaJeztah thaJeztah added area/rootless Rootless Mode impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/2-code-review labels Jul 14, 2021
@AkihiroSuda AkihiroSuda self-assigned this Jul 14, 2021
@eliaskoromilas
Copy link
Copy Markdown
Contributor Author

eliaskoromilas commented Jul 15, 2021

rebase

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jul 15, 2021

Can we have integration tests too?

AkihiroSuda
AkihiroSuda reviewed Jul 15, 2021
View reviewed changes
Comment thread daemon/device_unix.go Outdated
thaJeztah
thaJeztah reviewed Jul 15, 2021
View reviewed changes
Comment thread daemon/device_unix_test.go Outdated
@eliaskoromilas
Copy link
Copy Markdown
Contributor Author

eliaskoromilas commented Oct 5, 2021

rebase

@AkihiroSuda AkihiroSuda added this to the 21.xx milestone Oct 5, 2021
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Oct 5, 2021

Please squash commits

@eliaskoromilas
Copy link
Copy Markdown
Contributor Author

eliaskoromilas commented Oct 5, 2021

Should we skip these tests somehow? It fails for every privileged container test.

https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-42638/12/pipeline/348#step-351-log-796

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 21, 2021

Should we skip these tests somehow? It fails for every privileged container test.

This should be fixed on master; I kicked CI again, but if it fails again, try doing a rebase 👍

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 21, 2021

@AkihiroSuda ptal

@eliaskoromilas
Copy link
Copy Markdown
Contributor Author

eliaskoromilas commented Oct 21, 2021

This should be fixed on master; I kicked CI again, but if it fails again, try doing a rebase 👍

My tests pass now. CI still fails like master.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 21, 2021

Ah, yes, two tests that are failing; TestHealthKillContainer is the remaining test to fix (we finally found the cause for it, and have an idea how to fix it); the TestNetworkDBCRUDTableEntries is a known flaky (sometimes fails).

We can ignore those test failures

thaJeztah
thaJeztah reviewed Dec 1, 2021
View reviewed changes
Comment thread daemon/device_unix.go Outdated
Comment thread daemon/device_unix.go Outdated
Comment thread daemon/device_unix.go Outdated
This was referenced Dec 1, 2021
Merged
Merged
Merged
thaJeztah
thaJeztah reviewed Dec 1, 2021
View reviewed changes
Comment thread daemon/device_unix_test.go Outdated
@eliaskoromilas
Copy link
Copy Markdown
Contributor Author

eliaskoromilas commented Dec 15, 2021

I will rebase this after vendoring containerd.

@eliaskoromilas
Copy link
Copy Markdown
Contributor Author

eliaskoromilas commented Jan 7, 2022 •
edited
Loading

@thaJeztah Should we vendor github.com/containerd/containerd v1.5.9 (I thought it was backported there) v1.6.0 now?

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Mar 24, 2022

oh! almost forgot we had this one; thanks for the rebase, and good that it's all working as expected with the changes in containerd 👍 🎉

thaJeztah
thaJeztah approved these changes Mar 24, 2022
View reviewed changes
Copy link
Copy Markdown
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!

@thaJeztah thaJeztah merged commit d967ffb into moby:master Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi

Assignees

@AkihiroSuda AkihiroSuda

Labels

area/rootless Rootless Mode impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/2-code-review

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

Host devices not available inside --privileged rootless containers

3 participants

@eliaskoromilas @AkihiroSuda @thaJeztah