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.

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

[1.1] libcontainer: relax getenv_int sanity check #3494

Merged

Conversation

eriksjolund
Copy link
Contributor

@eriksjolund eriksjolund commented May 27, 2022

This is a backport of #3489 to release-1.1 branch.

Remove upper bound in integer sanity check
to not restrict the number of socket-activated
sockets passed in.

@kolyshkin kolyshkin added this to the 1.1.3 milestone Jun 1, 2022
@kolyshkin kolyshkin added the backport/1.1-pr A backport to 1.1.x release. label Jun 1, 2022
kolyshkin
kolyshkin previously approved these changes Jun 1, 2022
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor

kolyshkin commented Jun 1, 2022

Changelog entry for #3494 added. Guess we can do a 1.1.3 once that one is merged.

Any comments wrt changelog contents?

@eriksjolund
Copy link
Contributor Author

I see the changelog
"Socket activation was failing when more than 3 sockets were used."
Shouldn't it be
"Socket activation was failing when more than 2 sockets were used."
?

See this example

[test@asus ~]$ bash script.sh
31061f1f1fec51f80a84dcc511819420dc92372deb6fdf2ae18b7b14290b98cd

Testing runc

i=4001
hello
i=4002
hello
i=4003
Error: no container with name or ID "echo2" found: no such container
[test@asus ~]$ 

The file script.sh

#!/bin/bash
set -o errexit
podman pull -q ghcr.io/eriksjolund/socket-activate-echo
for runtime in runc; do
    echo 
    echo Testing $runtime
    echo
  listenargs=""  
  for i in {4001..4003}; do
    echo i=$i
    listenargs="$listenargs -l 127.0.0.1:$i"
    systemd-socket-activate $listenargs podman --runtime $runtime run -q --rm --name echo2 --pull=never --replace ghcr.io/eriksjolund/socket-activate-echo /socket-activate-echo 2> /tmp/stderr.$runtime.$i &
    sleep 1
    echo hello | socat - tcp4:127.0.0.1:4001
    if pgrep -u $USER systemd-socket-activate; then pkill systemd-socket-activate; fi
    podman --runtime $runtime container rm -t0 -f echo2 > /dev/null
    sleep 1
  done
done

Another thing:
The commit message contains the text

(cherry picked from commit 00af87d)

That commit is not relevant anymore. I just want to draw attention to the fact.
It does not matter to me if it stays like that. Just modify it as you want (or remove it).

@thaJeztah
Copy link
Member

The commit message contains the text

(cherry picked from commit 00af87d)
That commit is not relevant anymore.

Ah, yes, I usually keep the cherry pick in draft until the original is merged; I guess the original PR was rebased at some point?

Would be nice if you could re-do the cherry pick and/or update it (the intent of that line is to have an easy way to lead back where it came from, so it's good to have it accurate

@thaJeztah
Copy link
Member

Oh, but it still matches? I guess I misunderstood your comment 🤭☺️

Remove upper bound in integer sanity check
to not restrict the number of socket-activated
sockets passed in.

Closes opencontainers#3488

Signed-off-by: Erik Sjölund <erik.sjolund@gmail.com>
(cherry picked from commit 03a210d)
Signed-off-by: Erik Sjölund <erik.sjolund@gmail.com>
@eriksjolund
Copy link
Contributor Author

I guess the original PR was rebased at some point?

Yes, and then I forced-pushed it from the command-line.
Anyway, I have now cherry-picked the new commit.

@thaJeztah
Copy link
Member

Yes, and then I forced-pushed it from the command-line.

Ah, yes, I got confused again, because GitHub still showed the old commit (probably it was still cached)

GitHub actions seemed to have an outage / glitch; I kicked it again.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin can you re-LGTM?

@thaJeztah thaJeztah requested a review from kolyshkin June 2, 2022 10:14
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin kolyshkin merged commit da9b9d9 into opencontainers:release-1.1 Jun 2, 2022
@kolyshkin kolyshkin mentioned this pull request Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.1-pr A backport to 1.1.x release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants