-
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
test to cover parallel execution and networking #2570
test to cover parallel execution and networking #2570
Conversation
Signed-off-by: alsadi <alsadi@gmail.com>
Signed-off-by: alsadi <alsadi@gmail.com>
Hi @muayyad-alsadi. 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 Once the patch is verified, the new status will be reflected by the 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. |
/approved |
LGTM |
test/test_podman_baseline.sh
Outdated
[ "x$txt1"=="x$txt2" ] && echo "PASS" | ||
txt2=$( podman run --rm --net host busybox wget -qO - http://localhost:8080/index2.txt ) | ||
[ "x$txt1"=="x$txt2" ] && echo "PASS" | ||
# poadman run --rm --net container:myweb --add-host myweb:127.0.0.1 busybox wget -qO - http://myweb/index.txt |
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.
Should be podman, not podman and unless you plan to use it at some point, I'd just remove it.
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.
yes, I'll uncomment it soon when it works, see my comment here. @haircommander fixed it but I did not yet validate it
test/test_podman_baseline.sh
Outdated
[ "x$txt1"=="x$txt2" ] && echo "PASS" | ||
txt2=$( podman run --rm --net host busybox wget -qO - http://localhost:8080/index.txt ) | ||
[ "x$txt1"=="x$txt2" ] && echo "PASS" | ||
txt2=$( podman run --rm --net host busybox wget -qO - http://localhost:8080/index2.txt ) |
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.
needs run -d
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.
no it does not. I'm not taking the container id, I'm testing the output of wget -qO -
, passing -d would make it go background and no output is stored in txt2
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.
When I ran it, the test hung there for me, that's why I thought it needed the -d
...
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.
please try now
test/test_podman_baseline.sh
Outdated
echo $txt | podman exec -i myweb sh -c "cat > /var/www/index2.txt" | ||
txt2=$( podman exec myweb cat /var/www/index2.txt ) | ||
[ "x$txt1"=="x$txt2" ] && echo "PASS" | ||
txt2=$( podman run --rm --net host busybox wget -qO - http://localhost:8080/index.txt ) |
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.
needs run -d
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.
it does not, you can try it.
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 find some typos I'll fix them.
test/test_podman_baseline.sh
Outdated
echo -e "\ndone\n" | ||
# assert we have 0 running containers | ||
count=$( podman ps -q | wc -l ) | ||
[ "x$count" == "x0" ] || { echo "was expecting 0 running containers"; exit -1; } |
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 forgot to mention/ask for this yesterday, my apologies. I like this test to not stop if there's an error unless the '-e' param is passed in. For this line and the others where you exit on error, can you rework it slightly to something like:
[ "x$count" == "x0" ] || { echo "was expecting 0 running containers";-}
if [ "$showerror" eq 1 ]; then
[ "x$count" == "x0" ] || { exit -1; }
fi
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'll fix that
Really nice addition to the test. Couple of comments and then gtg. |
Signed-off-by: alsadi <alsadi@gmail.com>
[ "x$txt1" == "x$txt2" ] && echo "PASS2" || { echo "FAIL2"; port_test_failed=1; } | ||
txt2=$( podman run --rm --net host busybox wget -qO - http://localhost:$HOST_PORT/index2.txt ) | ||
[ "x$txt1" == "x$txt2" ] && echo "PASS3" || { echo "FAIL3"; port_test_failed=1; } | ||
# podman run --rm --net container:myweb --add-host myweb:127.0.0.1 busybox wget -qO - http://myweb/index.txt |
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.
if the @haircommander fix worked, we can have
txt2=$( podman run --rm --net container:myweb --add-host myweb:127.0.0.1 busybox wget -qO - http://myweb/index.txt )
[ "x$txt1" == "x$txt2" ] && echo "PASS4" || { echo "FAIL4"; port_test_failed=1; }
/ok-to-test |
count=$( podman ps -q | wc -l ) | ||
[ "x$count" == "x0" ] && echo "PASS" || { echo "FAIL, expecting 0 found $count"; prun_test_failed=1; } | ||
[ "0$prun_test_failed" -eq 1 ] && [ "0$showerror" -eq 1 ] && { | ||
echo "was expecting 0 running containers"; |
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.
@muayyad-alsadi it's much happier now. Thanks for the changes.
If you've the time,can you have each of the echo's for errors like this line print if you had the failure? If running it all but not stopping on errors, I still would like to see the errors.
If not, no worries. I'd a thought today to add color to the error lines. I can make the change myself when I go through that exercise.
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.
Let's add those when I revisit it to add the commented out tests about --net container:
One hopefully small question, otherwise LGTM. |
OK, LGTM then. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: muayyad-alsadi, rhatdan 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 |
creates 10 containers in parallel having no image (podman rmi)
removes them in parallel
do the same again but having the image existing before we start
this opens host port 8080, do some test the close it, if you want to customize this please tell me.
sorry I closed #2552 by deleting the previous branch mistake while doing a rebase.
some details are in #2551