cmd/run: add `--preserve-fds` command-line argument #1067

allisonkarlitskaya · 2022-06-16T21:38:56Z

This mirrors the --preserve-fds argument of podman.

Welcome to "I've never written anything in Go before" hour :)

This mirrors the --preserve-fds argument of podman. Fixes containers#1066 containers#1067 Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>

softwarefactory-project-zuul · 2022-06-16T21:56:27Z

Build succeeded.

✔️ unit-test SUCCESS in 6m 38s
✔️ system-test-fedora-rawhide SUCCESS in 10m 25s
✔️ system-test-fedora-36 SUCCESS in 10m 01s
✔️ system-test-fedora-35 SUCCESS in 10m 18s
✔️ system-test-fedora-34 SUCCESS in 10m 01s

allisonkarlitskaya · 2022-09-09T13:14:40Z

@debarshiray ping?

debarshiray

Thanks for working on this @allisonkarlitskaya ! My apologies - it fell off my radar.

It looks mostly good to me. It will need some rebasing, but I will take care of it, because right now I am staring at the need for podman exec --preserve-fds ... myself for the internal use of Toolbx. :)

src/cmd/run.go

This mirrors the --preserve-fds argument of podman. Fixes containers#1066 containers#1067 Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>

debarshiray

Thanks for the updates!

debarshiray · 2022-11-11T09:13:53Z

src/cmd/run.go

@@ -481,6 +488,10 @@ func constructExecArgs(container string,

 	execArgs = append(execArgs, envOptions...)

+	if runFlags.preserveFDs != 0 {


I suspect that this should be > 0 because that's also what Podman uses. Otherwise, we will only error out later from podman exec ... itself:

$ toolbox run --preserve-fds -12 true Error: failed to invoke 'podman exec' in container fedora-toolbox-36 $ echo $? 125

This makes me wonder if we should be using an unsigned integer flag. :)

I see that newer versions of Podman do use an unsigned integer flag.

It's not obvious from the commit message, but that's where it changed.

Damn! Converting an unsigned integer to a string is surprisingly annoying. :)

strconv.Itoa takes a signed int, which would require a cast, and there's no unsigned counterpart. There's strconv.FormatUint which takes an unsigned uint64, which is better, but would still require a cast.

So, fmt.Sprint it is, if we want to avoid the casting. It's a bit more expensive than the other two functions, but we don't have to worry about it unless it's proven to be a performance bottle neck.

src/cmd/run.go

softwarefactory-project-zuul · 2022-11-11T09:52:55Z

Build failed.

❌ unit-test FAILURE in 8m 06s
❌ unit-test-migration-path-for-coreos-toolbox FAILURE in 7m 41s
✔️ system-test-fedora-rawhide SUCCESS in 34m 44s
✔️ system-test-fedora-36 SUCCESS in 11m 56s
✔️ system-test-fedora-35 SUCCESS in 11m 28s

allisonkarlitskaya · 2022-11-11T13:38:39Z

As per our conversation on IRC, I'll step back and let @debarshiray address the outstanding issues.

debarshiray · 2022-11-14T19:35:24Z

As per our conversation on IRC, I'll step back and let @debarshiray address the outstanding issues.

I added some tests. It took longer than I expected because the test harness reserves two extra file descriptors.

softwarefactory-project-zuul · 2022-11-14T20:03:49Z

Build failed.

✔️ unit-test SUCCESS in 7m 19s
❌ unit-test-migration-path-for-coreos-toolbox RETRY_LIMIT in 7m 12s
✔️ system-test-fedora-rawhide SUCCESS in 29m 29s
✔️ system-test-fedora-36 SUCCESS in 9m 45s
✔️ system-test-fedora-35 SUCCESS in 10m 34s

debarshiray · 2022-11-14T20:04:06Z

recheck

softwarefactory-project-zuul · 2022-11-14T20:34:07Z

Build failed.

✔️ unit-test SUCCESS in 7m 40s
❌ unit-test-migration-path-for-coreos-toolbox RETRY_LIMIT in 7m 35s
❌ system-test-fedora-rawhide FAILURE in 28m 46s
✔️ system-test-fedora-36 SUCCESS in 9m 53s
✔️ system-test-fedora-35 SUCCESS in 10m 52s

debarshiray · 2022-11-14T21:08:14Z

Damn:

#   Trying to pull quay.io/toolbox_tests/registry:latest...
#   Getting image source signatures
...
#   Error: writing blob: storing blob to file "/var/tmp/storage1659742604/3": happened during read: unexpected EOF

debarshiray · 2022-11-14T21:08:21Z

recheck

This mirrors the --preserve-fds option of Podman. Converting an unsigned 'uint', which is what Podman uses for its --preserve-fds option, to a string is surprisingly annoying. strconv.Itoa [1] takes a signed 'int', which would require a cast, and there's no unsigned counterpart. There's strconv.FormatUint [2] which takes an unsigned 'uint64', which is better, but would still require a cast. So, fmt.Sprint [3] it is, if the cast is to be avoided. It's more expensive than the other two functions, but there's no need to worry unless it's proven to be a performance bottle neck. Some changes by Debarshi Ray. [1] https://pkg.go.dev/strconv#Itoa [2] https://pkg.go.dev/strconv#FormatUint [3] https://pkg.go.dev/fmt#Sprint containers#1066 Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>

debarshiray · 2022-11-14T21:31:50Z

I suspect that unit-test-migration-path-for-coreos-toolbox was failing because I forgot to update the runCommand call in src/rootMigration.go and the code was failing to compile.

softwarefactory-project-zuul · 2022-11-14T21:59:10Z

Build failed.

✔️ unit-test SUCCESS in 7m 06s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 7m 13s
❌ system-test-fedora-rawhide FAILURE in 28m 46s
✔️ system-test-fedora-36 SUCCESS in 9m 33s
✔️ system-test-fedora-35 SUCCESS in 10m 31s

Note that file descriptors 3 and 4 are reserved by Bats. The former is used for adding custom text to the Test Anything Protocol (or TAP) stream [1] and the latter for tracing [2]. [1] https://bats-core.readthedocs.io/en/stable/writing-tests.html#file-descriptor-3-read-this-if-bats-hangs https://bats-core.readthedocs.io/en/stable/writing-tests.html#printing-to-the-terminal [2] Bats commit 635700cd2282b754 bats-core/bats-core#467 bats-core/bats-core#488 containers#1066

softwarefactory-project-zuul · 2022-11-14T22:40:29Z

Build succeeded.

✔️ unit-test SUCCESS in 7m 09s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 7m 17s
✔️ system-test-fedora-rawhide SUCCESS in 27m 53s
✔️ system-test-fedora-36 SUCCESS in 9m 35s
✔️ system-test-fedora-35 SUCCESS in 10m 29s

debarshiray · 2022-11-15T07:12:55Z

Thanks for working on this, @allisonkarlitskaya !

allisonkarlitskaya force-pushed the preserve-fds branch from 212eae3 to 1ee68e6 Compare June 16, 2022 21:41

allisonkarlitskaya force-pushed the preserve-fds branch from 1ee68e6 to 22f753a Compare June 16, 2022 21:45

akdev1l mentioned this pull request Aug 19, 2022

cmd/run: add --preserve-fds command-line argument akdev1l/toolbox#6

Merged

debarshiray reviewed Nov 10, 2022

View reviewed changes

src/cmd/run.go Outdated Show resolved Hide resolved

src/cmd/run.go Outdated Show resolved Hide resolved

debarshiray reviewed Nov 11, 2022

View reviewed changes

src/cmd/run.go Outdated Show resolved Hide resolved

src/cmd/run.go Outdated Show resolved Hide resolved

allisonkarlitskaya force-pushed the preserve-fds branch from 22f753a to 518056b Compare November 11, 2022 09:12

debarshiray reviewed Nov 11, 2022

View reviewed changes

debarshiray force-pushed the preserve-fds branch 2 times, most recently from ce516aa to ed43d39 Compare November 14, 2022 19:33

debarshiray force-pushed the preserve-fds branch from ed43d39 to 11cd1b2 Compare November 14, 2022 21:29

debarshiray force-pushed the preserve-fds branch from 11cd1b2 to 9bf9f97 Compare November 14, 2022 22:11

debarshiray merged commit 9bf9f97 into containers:main Nov 15, 2022

		@@ -481,6 +488,10 @@ func constructExecArgs(container string,

		execArgs = append(execArgs, envOptions...)

		if runFlags.preserveFDs != 0 {

cmd/run: add --preserve-fds command-line argument #1067

cmd/run: add --preserve-fds command-line argument #1067

Uh oh!

Conversation

allisonkarlitskaya commented Jun 16, 2022

Uh oh!

softwarefactory-project-zuul bot commented Jun 16, 2022

Uh oh!

allisonkarlitskaya commented Sep 9, 2022

Uh oh!

debarshiray left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

debarshiray left a comment

Choose a reason for hiding this comment

Uh oh!

debarshiray Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

debarshiray Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

debarshiray Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

softwarefactory-project-zuul bot commented Nov 11, 2022

Uh oh!

allisonkarlitskaya commented Nov 11, 2022

Uh oh!

debarshiray commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

softwarefactory-project-zuul bot commented Nov 14, 2022

Uh oh!

debarshiray commented Nov 14, 2022

Uh oh!

softwarefactory-project-zuul bot commented Nov 14, 2022

Uh oh!

debarshiray commented Nov 14, 2022

Uh oh!

debarshiray commented Nov 14, 2022

Uh oh!

debarshiray commented Nov 14, 2022

Uh oh!

softwarefactory-project-zuul bot commented Nov 14, 2022

Uh oh!

softwarefactory-project-zuul bot commented Nov 14, 2022

Uh oh!

debarshiray commented Nov 15, 2022

Uh oh!

Uh oh!

cmd/run: add `--preserve-fds` command-line argument #1067

cmd/run: add `--preserve-fds` command-line argument #1067

debarshiray Nov 14, 2022 •

edited

Loading

debarshiray commented Nov 14, 2022 •

edited

Loading