-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
Add support to sig-proxy for podman-remote #15131
Conversation
c9d4365
to
a65d611
Compare
Quick review and the code LGTM, but the tests aren't a bit happy. |
pkg/domain/infra/tunnel/runtime.go
Outdated
// we terminate the proxy and let the defaults | ||
// play out. | ||
signal.StopCatch(sigBuffer) | ||
if err := syscall.Kill(syscall.Getpid(), s.(syscall.Signal)); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this have to happen on the server side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. @mheon am I right?
086a75f
to
c728e66
Compare
@edsantiago do you have an idea how should I test this. Should I run I saw there are some tests in https://github.com/containers/podman/blob/main/test/e2e/run_signal_test.go but I am not sure if they are related to the issue. In addition, when I run them on my local environment they fail. //cc @TomSweeneyRedHat |
The following works (says BYE) with your PR, does not work (silence) on main: $ bin/podman-remote run --name foo $IMAGE sh -c 'trap "echo BYE;exit 0" INT;while :;do sleep 0.1;done' &
...
$ kill -INT %1
...
$ podman logs foo
BYE Making that into a proper test -- capturing the PID (instead of %1), using randomized strings, and cleaning up -- is left as an exercise for the reader. HTH. |
@edsantiago I tried for too long to make this work in the system tests but it keeps failing. When I do it manually it works but running it through |
@boaz0 try this: @test "podman sigproxy" {
$PODMAN run -i --name foo $IMAGE sh -c 'trap "echo BYE;exit 0" INT;echo READY;while :;do sleep 0.1;done' &
local kidpid=$!
# Wait for container to appear
local timeout=5
while :;do
sleep 0.5
run_podman '?' container exists foo
if [[ $status -eq 0 ]]; then
break
fi
timeout=$((timeout - 1))
if [[ $timeout -eq 0 ]]; then
die "Timed out waiting for container to start"
fi
done
wait_for_ready foo
# Signal, and wait for container to exit
kill -INT $kidpid
local timeout=5
while :;do
sleep 0.5
run_podman logs foo
if [[ "$output" =~ BYE ]]; then
break
fi
timeout=$((timeout - 1))
if [[ $timeout -eq 0 ]]; then
die "Timed out waiting for BYE from container"
fi
done
run_podman rm -f -t0 foo
} Quick reminder that Another note: please squash your commits. Note to Podman team: the above works fine with podman-remote but, on the rootless server, spits out a red warning:
|
@edsantiago you're amazing. I have no idea how you figured it out but it works. |
@boaz0 blush, all it is is familiarity with one obscure little fraction of code. Still, thank you! @containers/podman-maintainers PTAL; the new imports are triggering bloat checks and Mac/Windows build failures. There might be a reason why those modules aren't imported in podman-remote :-( |
Code changes LGTM. Not sure what's causing the bloat... My assumption would be the terminal package, but I don't see what in there could be so big |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: boaz0, Luap99 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:
Approvers can indicate their approval by writing |
Yeah... the other two tests failed, all in remote, all in different places, and most with timeouts. There's almost certainly something broken in here. |
@edsantiago - yep that doesn't look good. I am first rebasing and will try to reproduce these on my local environment. |
a770cf6
to
3c962a3
Compare
Still failing in remote: # podman-remote --url unix:/tmp/podman_tmp_VdZP run --unsetenv-all --rm quay.io/libpod/testimage:20220615 /bin/printenv
timeout: sending signal TERM to command ‘/var/tmp/go/src/github.com/containers/podman/bin/podman-remote’
timeout: sending signal KILL to command ‘/var/tmp/go/src/github.com/containers/podman/bin/podman-remote’ I have a year-and-a-half catalog of podman flakes. This error is not in my catalog. That suggests that the problem is with this PR. |
cd9dec1
to
5ff7c88
Compare
OK I think I know what's the problem. Let's see if that fixes the problem. |
@edsantiago looks like it fixed the problem but now |
Signed-off-by: Boaz Shuster <[email protected]>
/lgtm |
@boaz0 thank you for your perseverance on this tricky one! |
closes #14707
Does this PR introduce a user-facing change?