tests to cover locks and parallel execution #2551 #2552
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: muayyad-alsadi If they are not already assigned, you can assign the PR to them by writing 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 |
|
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. |
openshift-ci-robot
added
the
needs-ok-to-test
|
Can one of the admins verify this patch?
|
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
and removed
needs-ok-to-test
|
bot, add author to whitelist |
|
@muayyad-alsadi thanks a bunch for the PR. I've just kicked off the tests but I don't think you've signed this PR and the test system will not be happy with that. If you indeed did not sign it, please do |
| @@ -90,6 +90,52 @@ podman run $image java 2>&1 || echo $? | |||
| ######## | |||
| podman rm --all | |||
|
|
|||
| ######## | |||
There was a problem hiding this comment.
Could you copy lines 83 -> 91 (run java when it's not there and clean out containers) and move that below your new section? I think we should still keep the clean out container lines 88->91 where they are, but then remove the run java when it's not there section, at lines 83->86.
If that's not clear, no worries, I'll tend to it post merge.
|
@muayyad-alsadi Just to be sure you're aware, this script is only run "by hand" and is not part of the CI test system. I think it's a nice addition regardless, but if you want something to run with every PR, you'll need to work something up in the go tests that live in https://github.com/containers/libpod/tree/master/test/e2e |
|
you need to sign your commit, |
Signed-off-by: alsadi <[email protected]>
| # assert we have 10 running containers | ||
| count=$( podman ps -q | wc -l ) | ||
| [ "x$count" == "x10" ] || { echo "was expecting 10 running containers"; exit -1; } | ||
|
|
There was a problem hiding this comment.
Well this is interesting. The following removal fails for me with:
Error: unable to find container c1: more than one result for ID or name c1: container already exists
Error: unable to find container c1: more than one result for ID or name c1: container already exists
Unless I put a several second sleep here. I then tried to changed lines 130 -> 133 with podman rmi -a -f, that seems to work, but takes a very long time, 30+ seconds of no feedback.
Adding a '-f' to the rm command on line 130 seems to make the error happen less frequently, but it's still pops from time to time.
@mheon, @vrothberg thoughts? Is this pulling up a lock issue?
There was a problem hiding this comment.
Also fwiw, I stopped the script in a few places, and looked at the created containers and we'd no duplicates, so I think at least the error could use some massaging.
@muayyad-alsadi these two comments I don't suspect are an issue with your script, rather they may have found something interesting in Podman.
There was a problem hiding this comment.
@TomSweeneyRedHat oh, this is an interesting one - 1 in 256 chance that we get a container ID that starts with C1, in which case we have a container c1xxxxxx... and a container with the name c1 - and rm can't tell which one we wanted to remove.
Need to check Docker for expected behavior here.
There was a problem hiding this comment.
yes, this is the exact case those tests are made to catch. and I guess #2553 is also related.
it happened once, but with yesterday version of podman it did not report error.
There was a problem hiding this comment.
container ID that starts with C1, in which case we have a container c1xxxxxx...
I can change it to c_1 to fix that if you like.
There was a problem hiding this comment.
@muayyad-alsadi I think this might well be a bug - Docker handles this by deleting the container named c1 even if there is an ID match as well.
There was a problem hiding this comment.
I think I'm going to submit a bugfix PR for this.
There was a problem hiding this comment.
Opened #2563 to fix our handling of this
…od into alsadi-tests-2019036
|
@muayyad-alsadi: The following test failed for commit a0f241b, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
@baude I did, is it ok now? |
are those in e2e run as root or rootless? I can add my tests there later. |
|
@muayyad-alsadi the e2e/tests should run as root, right @baude? |
@baude @TomSweeneyRedHat IMHO we should do rootless checks too it's one of the best values of podman. Maybe we need subsets of root test (due to limited privilege) in that case a flag might be useful. |
github-actions
bot
added
the
locked - please file new issue/PR
creates 10 containers in parallel having no image (
podman rmi)removes them in parallel
again but having the image existing before we start