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

volumes: allow more options for devpts #12124

Merged
merged 2 commits into from
Oct 28, 2021
Merged

volumes: allow more options for devpts #12124

merged 2 commits into from
Oct 28, 2021

Conversation

giuseppe
Copy link
Member

What this PR does / why we need it:

allow to pass down more options that are supported by the kernel.

Discussion here: containers/toolbox#568

How to verify it

there is a new test

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2021
@giuseppe giuseppe mentioned this pull request Oct 28, 2021
Open
vrothberg
vrothberg reviewed Oct 28, 2021
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.

Other than the indent nit, LGTM

docs/source/markdown/podman-create.1.md Outdated

· mode: permission mask for the file (default 600).

· max: maximum number of PTYs (default 1048576).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
· max: maximum number of PTYs (default 1048576).
· max: maximum number of PTYs (default 1048576).

docs/source/markdown/podman-run.1.md Outdated Show resolved Hide resolved
@giuseppe giuseppe force-pushed the allow-devpts-options branch from c4c1c93 to c5668e2 Compare October 28, 2021 08:22
@rhatdan
Copy link
Member

rhatdan commented Oct 28, 2021

Would this be better to do with the --mount options? Can it already be done with it?

@rhatdan
Copy link
Member

rhatdan commented Oct 28, 2021

Never mind, I should read the entire PR before jumping to conclusions.

@rhatdan
Copy link
Member

rhatdan commented Oct 28, 2021

LGTM once test works.

@giuseppe giuseppe force-pushed the allow-devpts-options branch from c5668e2 to d273a44 Compare October 28, 2021 09:45
@giuseppe
Copy link
Member Author

LGTM once test works.

looks like runc doesn't honor uid. I'll drop testing uid

@rhatdan
Copy link
Member

rhatdan commented Oct 28, 2021

@kolyshkin @AkihiroSuda FYI

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 28, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AkihiroSuda, giuseppe

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

@AkihiroSuda
Copy link
Collaborator

LGTM once test works.

looks like runc doesn't honor uid. I'll drop testing uid

Could you open an issue in runc repo?

@giuseppe giuseppe force-pushed the allow-devpts-options branch from d273a44 to 81e4946 Compare October 28, 2021 10:37
@giuseppe
Copy link
Member Author

Could you open an issue in runc repo?

not sure it is an issue, it seems to be done on purpose by fixStdioPermissions. Perhaps I need to change the test to look at the mount table

@giuseppe
volumes: do not pass mount opt as formatter string
e6286fb 
otherwise passing a formatter string as an option causes a weird
error message:

$ podman run --mount type=devpts,destination=/dev/pts,%sfoo ...
Error: %!s(MISSING)foo: invalid mount option

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe
volumes: allow more options for devpts
4e9e6f2 
allow to pass down more options that are supported by the kernel.

Discussion here: containers/toolbox#568

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the allow-devpts-options branch from 81e4946 to 4e9e6f2 Compare October 28, 2021 13:30
@giuseppe
Copy link
Member Author

/retest

@giuseppe
Copy link
Member Author

tests are finally green

ashley-cui
ashley-cui reviewed Oct 28, 2021
View reviewed changes
Copy link
Member

@ashley-cui ashley-cui left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2021
@openshift-merge-robot openshift-merge-robot merged commit f7ca045 into containers:main Oct 28, 2021
@edsantiago edsantiago mentioned this pull request Nov 3, 2021
Closed
@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ashley-cui ashley-cui ashley-cui left review comments

@vrothberg vrothberg vrothberg left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

@ashley-cui ashley-cui

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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.
6 participants
@giuseppe @rhatdan @AkihiroSuda @vrothberg @ashley-cui @openshift-merge-robot