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

logging: new mode -l passthrough-tty #20774

Merged
merged 1 commit into from
Feb 29, 2024
Merged

logging: new mode -l passthrough-tty #20774

merged 1 commit into from
Feb 29, 2024

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Nov 24, 2023 •
edited
Loading

it works in a similar way to passthrough but it allows to be used also on a TTY.

conmon support: containers/conmon#465

Closes: #20767

Does this PR introduce a user-facing change?

Add a new logging mode --passthrough-tty that works like passthrough but can be used with TTYs

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 24, 2023
@giuseppe giuseppe mentioned this pull request Nov 24, 2023
Closed
@giuseppe giuseppe force-pushed the passthrough-tty branch 2 times, most recently from 0e1ab70 to 8567079 Compare December 1, 2023 17:26
@rhatdan
Copy link
Member

rhatdan commented Dec 3, 2023

@giuseppe is this still a draft?

@giuseppe giuseppe marked this pull request as ready for review December 3, 2023 20:15
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2023
@giuseppe
Copy link
Member Author

giuseppe commented Dec 3, 2023

no this is ready, I was waiting for the conmon change to merge first

flouthoc
flouthoc approved these changes Dec 4, 2023
View reviewed changes
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@halfline
Copy link

halfline commented Dec 4, 2023

given passthrough-tty got folded into passthrough in the conmon merge request, should the same thing happen here?

@giuseppe
Copy link
Member Author

giuseppe commented Dec 4, 2023

given passthrough-tty got folded into passthrough in the conmon merge request, should the same thing happen here?

personally I think it is better to keep it as a separate mode, to avoid potential security issues with passthrough mode. Now it is explicit that the user expects the std streams to potentially be terminals and accept the risk

Luap99
Luap99 reviewed Dec 6, 2023
View reviewed changes
test/system/035-logs.bats Outdated
Comment on lines 444 to 450
# check if conmon supports -l passthrough-tty, we can drop this check once conmon gets in the CI images.
run_podman info -f '{{.Host.Conmon.Path}}'
conmon_path="$output"

if ! $conmon_path -l passthrough --cid foo --cuuid foo --runtime /bin/true; then
skip "conmon does not support -l passthrough-tty"
fi
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer applicable I assume?

Luap99
Luap99 reviewed Dec 6, 2023
View reviewed changes
test/system/035-logs.bats Outdated
Comment on lines 440 to 442
if ! tty -s; then
skip "not running in a tty"
fi
Copy link
Member

Choose a reason for hiding this comment

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

looking at CI this seems to always trigger so it is unable to catch any regression in CI thus effectively making the test pointless. Can we setup a tty for this test somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

found a way to test it with python, it is hacky but seems to work fine locally

@giuseppe giuseppe force-pushed the passthrough-tty branch 2 times, most recently from c37f0aa to 5bd2bb4 Compare December 6, 2023 21:41
@edsantiago
Copy link
Member

Hi, sorry I haven't been watching this closely, before you spend too much time on this, can you look at 450-interactive.bats?

@giuseppe
Copy link
Member Author

giuseppe commented Dec 6, 2023

Hi, sorry I haven't been watching this closely, before you spend too much time on this, can you look at 450-interactive.bats?

thanks, that is surely better!

I've moved the test to 450-interactive.bats and used the existing infrastructure

edsantiago
edsantiago reviewed Dec 6, 2023
View reviewed changes
test/system/450-interactive.bats Outdated
Comment on lines 109 to 111
if ! tty -s; then
skip "not running in a tty"
fi
Copy link
Member

Choose a reason for hiding this comment

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

ummmmmm....

Copy link
Member Author

Choose a reason for hiding this comment

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

dropped now

edsantiago
edsantiago reviewed Dec 6, 2023
View reviewed changes
test/system/450-interactive.bats Outdated
run_podman info -f '{{.Host.Conmon.Path}}'
conmon_path="$output"

if ! $conmon_path -l passthrough --cid foo --cuuid foo --runtime /bin/true; then
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what version of conmon supports this, and where it is in the bodhi pipeline? I just spun up a rawhide VM, and even in updates-testing, I can't get a conmon that supports this. I would like to hold this PR until I can run this test against a truly working conmon.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is not released yet: containers/conmon#465

Fine by me, let's hold it until the next conmon release

@giuseppe
Copy link
Member Author

/hold until we have a working conmon in the CI

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2023
@edsantiago
Copy link
Member

Ping. We have new VMs. Conmon is now 2.1.6+ds1-1 on Debian, and 2:2.1.8-2 on all Fedorae. Is that the required version?

@giuseppe
Copy link
Member Author

giuseppe commented Jan 8, 2024

I've rebased and cleaned up the test but unfortunately 2.1.8 is not enough. We need at least 2.1.9 (which has a regression, so it should be 2.1.10).

@rhatdan
Copy link
Member

rhatdan commented Jan 21, 2024

@giuseppe Could you rebase.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 8126808d4d96f79312a8c278e6d8652e8f1f49b4. @martinpitt, @jelly, @mvollmer please check.

@giuseppe
logging: new mode -l passthrough-tty
950f612 
it works in a similar way to passthrough but it allows to be used also
on a TTY.

conmon support: containers/conmon#465

Closes: containers#20767

Signed-off-by: Giuseppe Scrivano <[email protected]>
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 950f612. @martinpitt, @jelly, @mvollmer please check.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, flouthoc, 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:
  • OWNERS [edsantiago,flouthoc,giuseppe]

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

@giuseppe
Copy link
Member Author

@containers/podman-maintainers PTAL

@Luap99
Copy link
Member

Luap99 commented Feb 29, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 29, 2024
@Luap99
Copy link
Member

Luap99 commented Feb 29, 2024

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 29, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 690b671 into containers:main Feb 29, 2024
93 checks passed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 30, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@flouthoc flouthoc flouthoc approved these changes

@edsantiago edsantiago edsantiago approved these changes

Assignees

@Luap99 Luap99

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. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow passing inheriting PTY from caller
7 participants
@giuseppe @rhatdan @halfline @edsantiago @Luap99 @flouthoc @TomSweeneyRedHat