Skip to content

volumes: allow more options for devpts - #12124

Merged
openshift-merge-robot merged 2 commits into
podman-container-tools:mainfrom
giuseppe:allow-devpts-options
Oct 28, 2021
Merged

volumes: allow more options for devpts#12124
openshift-merge-robot merged 2 commits into
podman-container-tools:mainfrom
giuseppe:allow-devpts-options

Conversation

@giuseppe

@giuseppe giuseppe commented Oct 28, 2021

Copy link
Copy Markdown
Contributor

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

@vrothberg vrothberg left a comment

Copy link
Copy Markdown
Contributor

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

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

@vrothberg vrothberg Oct 28, 2021

Copy link
Copy Markdown
Contributor

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

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

rhatdan commented Oct 28, 2021

Copy link
Copy Markdown
Contributor

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

@rhatdan

rhatdan commented Oct 28, 2021

Copy link
Copy Markdown
Contributor

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

@rhatdan

rhatdan commented Oct 28, 2021

Copy link
Copy Markdown
Contributor

LGTM once test works.

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

giuseppe commented Oct 28, 2021

Copy link
Copy Markdown
Contributor Author

LGTM once test works.

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

@rhatdan

rhatdan commented Oct 28, 2021

Copy link
Copy Markdown
Contributor

@kolyshkin @AkihiroSuda FYI

@openshift-ci

openshift-ci Bot commented Oct 28, 2021

Copy link
Copy Markdown
Contributor

[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

Details 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

AkihiroSuda commented Oct 28, 2021

Copy link
Copy Markdown
Contributor

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

giuseppe commented Oct 28, 2021

Copy link
Copy Markdown
Contributor 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 <gscrivan@redhat.com>
@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 <gscrivan@redhat.com>
@giuseppe
giuseppe force-pushed the allow-devpts-options branch from 81e4946 to 4e9e6f2 Compare October 28, 2021 13:30
@giuseppe

giuseppe commented Oct 28, 2021

Copy link
Copy Markdown
Contributor Author

/retest

@giuseppe

giuseppe commented Oct 28, 2021

Copy link
Copy Markdown
Contributor Author

tests are finally green

ashley-cui
ashley-cui reviewed Oct 28, 2021
View reviewed changes

@ashley-cui ashley-cui left a comment

Copy link
Copy Markdown
Contributor

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 podman-container-tools: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
+2 more reviewers
@vrothberg vrothberg vrothberg left review comments
@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes
Reviewers whose approvals may not affect merge requirements

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