Skip to content

[1.1] libcontainer: relax getenv_int sanity check#3494

Merged
kolyshkin merged 1 commit intoopencontainers:release-1.1from
eriksjolund:1.1-backport-3489
Jun 2, 2022
Merged

[1.1] libcontainer: relax getenv_int sanity check#3494
kolyshkin merged 1 commit intoopencontainers:release-1.1from
eriksjolund:1.1-backport-3489

Conversation

@eriksjolund
Copy link
Copy Markdown
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 PR to release-1.1 label Jun 1, 2022
kolyshkin
kolyshkin previously approved these changes Jun 1, 2022
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 PR to release-1.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants