libpod: specify a detach keys sequence in libpod.conf #3324

marcov · 2019-06-13T17:43:42Z

Add the ability of specifying a detach keys sequence in libpod.conf

Signed-off-by: Marco Vedovati mvedovati@suse.com

openshift-ci-robot · 2019-06-13T17:44:03Z

Hi @marcov. Thanks for your PR.

I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rh-atomic-bot · 2019-06-13T17:52:45Z

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

mheon · 2019-06-13T18:22:30Z

docs/podman-attach.1.md

Change to specifying it in the **libpod.conf** file: see...

mheon · 2019-06-13T18:23:03Z

docs/podman-create.1.md

Same change as the run manpage

mheon · 2019-06-13T18:24:09Z

libpod/runtime.go

Completely unrelated to this PR, but what is this - doing in the TOML string... @baude you added this, right?

mheon · 2019-06-13T18:26:54Z

utils/utils.go

I'd prefer it if we kept this around, but set it to the runtime's DetachKeys instead of hard-coding C-p C-q. That way, we don't have to unconditionally pass the keys in as part of StartAndAttach() - we get the defaults if not overridden.

I'm not sure how can I retrieve the runtime DetachKeys here without making an import loop

Pass then in as a separate argument, I think (keys, defaultKeys []byte)

Alternatively, push determining the keys up the stack into attachContainerSocket(), which has a runtime available to get the keys

Get the default keys, rather

I preferred moving the choice in attachContainerSocket, so that term.ToBytes can be called only once.

mheon · 2019-06-13T18:27:14Z

/ok-to-test
/approve

TomSweeneyRedHat · 2019-06-13T20:13:57Z

docs/podman-run.1.md

I'm not a fan of the word "You" in a man page. Can you drop it and just cap "Configure"

I can drop this "You", note however that there are other "You" around in the doc.

docs/podman-create.1.md

docs/podman-attach.1.md

cmd/podman/common.go

cmd/podman/attach.go

cmd/podman/start.go

baude · 2019-06-17T13:22:28Z

this will need to also be implemented for the remote-client as well ... more specifically, reading from its conf file.

marcov · 2019-06-17T13:28:20Z

I have pushed one more commit to fix a bug found while testing these changes: when starting with attach (using podman run or podman start -a), podman would hang if attach fails before start, e.g. if the detach keys passed are invalid.

mheon · 2019-06-17T13:36:56Z

Prow failures look like infra flakes

mheon · 2019-06-17T13:42:32Z

Code LGTM

rh-atomic-bot · 2019-06-26T00:38:14Z

☔ The latest upstream changes (presumably #3419) made this pull request unmergeable. Please resolve the merge conflicts.

Add the ability of specifying a detach keys sequence in libpod.conf Signed-off-by: Marco Vedovati <mvedovati@suse.com>

Signed-off-by: Marco Vedovati <mvedovati@suse.com>

When a container is attached upon start, the WaitGroup counter may never be decremented if an error is raised before start, causing the caller to hang. Synchronize with the start & attach goroutine using a channel, to be able to detect failures before start. Signed-off-by: Marco Vedovati <mvedovati@suse.com>

openshift-ci-robot · 2019-06-26T08:19:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marcov, mheon

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:

~~OWNERS~~ [mheon]

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

marcov · 2019-06-26T08:36:57Z

@mheon @TomSweeneyRedHat, I updated and rebased this PR based on your reviews, let me know if you expect something else to be changed/added.

TomSweeneyRedHat · 2019-06-26T12:41:44Z

Updated changes LGTM, thanks @marcov . Definitely need a head nod from @mheon though as I think he still had two open questions.

mheon · 2019-07-01T13:43:17Z

Sorry for the delay here.
/lgtm

openshift-ci-robot requested review from jwhonce and mrunalp June 13, 2019 17:43

openshift-ci-robot added the size/M label Jun 13, 2019

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 13, 2019

mheon reviewed Jun 13, 2019

View reviewed changes

docs/podman-attach.1.md Outdated

Copy link

Member

mheon Jun 13, 2019

Choose a reason for hiding this comment

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

Change to specifying it in the **libpod.conf** file: see...

mheon reviewed Jun 13, 2019

View reviewed changes

docs/podman-create.1.md Outdated

Copy link

Member

mheon Jun 13, 2019

Choose a reason for hiding this comment

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

Same change as the run manpage

mheon reviewed Jun 13, 2019

View reviewed changes

openshift-ci-robot added ok-to-test approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 13, 2019

TomSweeneyRedHat reviewed Jun 13, 2019

View reviewed changes

docs/podman-create.1.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Jun 13, 2019

View reviewed changes

docs/podman-attach.1.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Jun 13, 2019

View reviewed changes

cmd/podman/common.go Outdated Show resolved Hide resolved

jwhonce suggested changes Jun 13, 2019

View reviewed changes

cmd/podman/attach.go Outdated Show resolved Hide resolved

cmd/podman/start.go Outdated Show resolved Hide resolved

marcov force-pushed the detach-keys-configurable branch from d1042b1 to a5ec7f9 Compare June 17, 2019 13:24

openshift-ci-robot added size/L and removed size/M labels Jun 17, 2019

marcov force-pushed the detach-keys-configurable branch from a5ec7f9 to 79d1c6c Compare June 17, 2019 13:32

marcov force-pushed the detach-keys-configurable branch from 79d1c6c to 263ae52 Compare June 17, 2019 14:22

libpod: specify a detach keys sequence in libpod.conf

7e3f1c2

Add the ability of specifying a detach keys sequence in libpod.conf Signed-off-by: Marco Vedovati <mvedovati@suse.com>

marcov added 2 commits June 26, 2019 10:12

podman: clarify the format of --detach-keys argument

6e9b490

Signed-off-by: Marco Vedovati <mvedovati@suse.com>

marcov force-pushed the detach-keys-configurable branch from 263ae52 to 4f56964 Compare June 26, 2019 08:17

openshift-ci-robot assigned mheon Jul 1, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2019

openshift-merge-robot merged commit 1507788 into containers:master Jul 1, 2019

This was referenced Jul 1, 2019

use conmon for exec #3143

Merged

build: allow to build without cgo on RISC-V #3437

Merged

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 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023

libpod: specify a detach keys sequence in libpod.conf #3324

libpod: specify a detach keys sequence in libpod.conf #3324

Uh oh!

Conversation

marcov commented Jun 13, 2019

Uh oh!

openshift-ci-robot commented Jun 13, 2019

Uh oh!

rh-atomic-bot commented Jun 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Jun 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

baude commented Jun 17, 2019

Uh oh!

marcov commented Jun 17, 2019

Uh oh!

mheon commented Jun 17, 2019

Uh oh!

mheon commented Jun 17, 2019

Uh oh!

rh-atomic-bot commented Jun 26, 2019

Uh oh!

openshift-ci-robot commented Jun 26, 2019

Uh oh!

marcov commented Jun 26, 2019

Uh oh!

TomSweeneyRedHat commented Jun 26, 2019

Uh oh!

mheon commented Jul 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants