Skip to content
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

Expose block and character devices with play kube #14266

Merged
merged 3 commits into from
May 23, 2022
Merged

Expose block and character devices with play kube #14266

merged 3 commits into from
May 23, 2022

Conversation

tupyy
Copy link
Contributor

@tupyy tupyy commented May 17, 2022

This PR addresses issue #13951.

The aim of the PR is to be able to expose devices inside containers using podman play kube. The user can specify which device will be exposed using volumes and volumeMounts in the pod manifest:

apiVersion: v1
kind: Pod
metadata:
  name: f35
spec:
  containers:
    - name: f35
      image: fedora:35
      command: ["/bin/bash", "-c", "sleep 300"]
      volumeMounts:
      - mountPath: /dev/loop1
        name: loop
      - mountPath: /dev/video0
        name: video
  volumes:
  - name: loop
    hostPath:
      path: /dev/loop1
      type: BlockDevice
  - name: video 
    hostPath:
      path: /dev/video0
      type: CharDevice

Remark: The issue #13951 refers to the need to use container device interface with podman play kube. This PR is not addressing this need.

Signed-off-by: Cosmin Tupangiu [email protected]

@tupyy tupyy changed the title Add blockdevice play kube Expose block and character devices with play kube May 17, 2022
@tupyy
Copy link
Contributor Author

tupyy commented May 17, 2022

/assign @mheon

@tupyy tupyy force-pushed the add-blockdevice-play-kube branch from 5c86313 to b2c6eeb Compare May 17, 2022 08:48
@tupyy
expose block and char devices with play kube
0c9b0e2 
[NO NEW TESTS NEEDED]

Signed-off-by: Cosmin Tupangiu <[email protected]>
@tupyy tupyy force-pushed the add-blockdevice-play-kube branch from b2c6eeb to 0c9b0e2 Compare May 17, 2022 09:16
vrothberg
vrothberg approved these changes May 17, 2022
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Thank you for contributing!

Did you consider writing tests? I'd love have some. Creating block and char devices should work just fine in our CI.

pkg/specgen/generate/kube/kube.go Outdated Show resolved Hide resolved
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tupyy, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2022
@mheon
Copy link
Member

mheon commented May 17, 2022

Code LGTM. Tests would definitely be preferred though.

@tupyy
Copy link
Contributor Author

tupyy commented May 17, 2022

I can add tests no problem. But it requires creating the actual devices with root permission. do you think it is ok for the CI pipeline?

@rhatdan
Copy link
Member

rhatdan commented May 17, 2022

You will have to run only rootful tests and make sure you remove the devices when the pod completes.

@vrothberg
Copy link
Member

vrothberg commented May 17, 2022 •
edited
Loading

Yes, you can use SkipIfRootless("explanation") in test/e2e. This way, the tests are skipped for non-root runs.

@rhatdan rhatdan added the kube label May 17, 2022
vikas-goel
vikas-goel suggested changes May 17, 2022
View reviewed changes
pkg/specgen/generate/kube/volume.go Outdated Show resolved Hide resolved
pkg/specgen/generate/kube/volume.go Outdated Show resolved Hide resolved
@tupyy
add tests and fix bug when char device pass the test as block device
f5c8c09 
- add test
- fix bug when a character device set in a volume as a block device
  is seen as block device in _pkg/specgen/generate/kube/volume.go_.
  At this stage the type does not matter much because the devices are
recreated at lower layer but the bug allowed a CharDevice volume to be
passed to lower layer as a BlockDevice.

Signed-off-by: Cosmin Tupangiu <[email protected]>
@TomSweeneyRedHat
Copy link
Member

LGTM
I've restarted the failing test which looked flakey to me.

@tupyy
fix tests by randomize the device folder name
4960a17 
e2e tests tends to fail when running with multiple nodes because
the same device folder name is used accross all nodes

Signed-off-by: Cosmin Tupangiu <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented May 23, 2022

/lgtm
Great job @tupyy

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2022
@openshift-merge-robot openshift-merge-robot merged commit e11d8d4 into containers:main May 23, 2022
tupyy added a commit to tupyy/podman that referenced this pull request May 23, 2022
@tupyy
Update _play kube_ doc following PR containers#14266 merged
ac15442 
Signed-off-by: Cosmin Tupangiu <[email protected]>
@tupyy tupyy mentioned this pull request May 23, 2022
Merged
openshift-merge-robot added a commit that referenced this pull request May 23, 2022
@openshift-merge-robot
Merge pull request #14323 from tupyy/main
3798478 
Update _play kube_ doc following PR #14266 merging
cdoern pushed a commit to cdoern/podman that referenced this pull request May 27, 2022
@tupyy
Update _play kube_ doc following PR containers#14266 merged
800e996 
Signed-off-by: Cosmin Tupangiu <[email protected]>
@tupyy tupyy mentioned this pull request Jun 6, 2022
Closed
@dmotte
Copy link

dmotte commented Jun 15, 2023

Is this feature the equivalent of the --device flag? E.g. podman run ... --device /dev/video0?

Because unfortunately it's not working in my specific use case

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mheon mheon mheon left review comments

@vikas-goel vikas-goel vikas-goel requested changes

@vrothberg vrothberg vrothberg approved these changes

Assignees

@rhatdan rhatdan

@mheon mheon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kube lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tupyy @mheon @rhatdan @vrothberg @TomSweeneyRedHat @dmotte @vikas-goel @openshift-merge-robot