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.

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

-sequence is `ctrl-p,ctrl-q`. You configure the key sequence using the --detach-keys option
+sequence is `ctrl-p,ctrl-q`.
+You configure the keys sequence using the **--detach-keys** option, or specifying
+it in a libpod file: see **libpod.conf(5)** for more information.


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

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

docs/podman-create.1.md

-You configure the key sequence using the **--detach-keys** option or a configuration file.
-See **config-json(5)** for documentation on using a configuration file.
+You configure the keys sequence using the **--detach-keys** option, or specifying
+it in a libpod file: see **libpod.conf(5)** for more information.


Same change as the run manpage

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

libpod/runtime.go

@@ -233,6 +237,8 @@ type RuntimeConfig struct {
 	EventsLogger string `toml:"events_logger"`
 	// EventsLogFilePath is where the events log is stored.
 	EventsLogFilePath string `toml:-"events_logfile_path"`


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

@@ -59,11 +59,6 @@ var ErrDetach = errors.New("detached from container")

 // CopyDetachable is similar to io.Copy but support a detach key sequence to break out.
 func CopyDetachable(dst io.Writer, src io.Reader, keys []byte) (written int64, err error) {
-	if len(keys) == 0 {
-		// Default keys : ctrl-p,ctrl-q
-		keys = []byte{16, 17}


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

@@ -181,12 +181,14 @@ detached container with **podman attach**.

 When attached in the tty mode, you can detach from the container (and leave it
 running) using a configurable key sequence. The default sequence is `ctrl-p,ctrl-q`.
-You configure the key sequence using the **--detach-keys** option or a configuration file.
-See **config-json(5)** for documentation on using a configuration file.
+You configure the keys sequence using the **--detach-keys** option, or specifying


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

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

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 requested 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marcov commented Jun 13, 2019

openshift-ci-robot commented Jun 13, 2019

rh-atomic-bot commented Jun 13, 2019

mheon Jun 13, 2019

mheon Jun 13, 2019

mheon Jun 13, 2019

mheon Jun 13, 2019

marcov Jun 13, 2019

mheon Jun 13, 2019

mheon Jun 13, 2019

mheon Jun 13, 2019

marcov Jun 17, 2019

mheon commented Jun 13, 2019

TomSweeneyRedHat Jun 13, 2019

marcov Jun 13, 2019

baude commented Jun 17, 2019

marcov commented Jun 17, 2019

mheon commented Jun 17, 2019

mheon commented Jun 17, 2019

rh-atomic-bot commented Jun 26, 2019

openshift-ci-robot commented Jun 26, 2019

marcov commented Jun 26, 2019

TomSweeneyRedHat commented Jun 26, 2019

mheon commented Jul 1, 2019

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

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

Conversation

marcov commented Jun 13, 2019

openshift-ci-robot commented Jun 13, 2019

rh-atomic-bot commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baude commented Jun 17, 2019

marcov commented Jun 17, 2019

mheon commented Jun 17, 2019

mheon commented Jun 17, 2019

rh-atomic-bot commented Jun 26, 2019

openshift-ci-robot commented Jun 26, 2019

marcov commented Jun 26, 2019

TomSweeneyRedHat commented Jun 26, 2019

mheon commented Jul 1, 2019