Conversation
|
Hi @kiashok. Thanks for your PR. I'm waiting for a containerd 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. DetailsInstructions 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. |
| t.Log("Start the container") | ||
| require.NoError(t, runtimeService.StartContainer(cn)) | ||
| // Wait few seconds for the container to be completely initialized | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
Hmm, isn't CreateContainer synchronous? What does it return if we call StopContainer too early?
There was a problem hiding this comment.
(I think its more about how the underlying compute system deals with it? )
its scheduling dependent - Its possible to hit a window where the container is not running yet but the kill has been sent immediately
There was a problem hiding this comment.
Yea I'd expect this too.. I'd think if start returns we should expect the workload to be running. I'd imagine anything thats not that case is a bug (if the command you asked to run is expected to stay alive and isn't "echo foo" or some short lived task obviously).
There was a problem hiding this comment.
Not sure I am able to repro the window after start container where the container is not already running and we have killed it. Looks like StartContainer() is a synchronous call as well. Can add an assert after run to make sure the state of container is expected. Thoughts @mikebrow ?
There was a problem hiding this comment.
waiting 5sec will probably work .. but isn't a guarantee that the code you ran in command is complete.. e.g. the command includes a sleep 5sec before proceeding with the meat of what you are testing in the container.. or gc or swapper thrash or device interrupt...
There was a problem hiding this comment.
I've not looked around for the cplatpublic.azurecr.io/args-escaped-test-image-ns code that's being tested..
There was a problem hiding this comment.
if you know what to look for in result status or what not.. you could loop here waiting for it
There was a problem hiding this comment.
Looked over the runtimeService.StartContainer() code again and I see that if it returns, the container has started successfully. If any windows exists where the container is not running, I think the 5 sec delay is more than sufficient in this case as the test image is only looking for how CMD and ENTRYPOINTs are overriden and if they start up as expected
- Rename test name - Add a tag to the container image used in the tests instead of the latest tag - Add a 5 second delay between container start and stop to ensure that the container is fully initialized Signed-off-by: Kirtana Ashok <[email protected]>
kiashok
force-pushed
the
argsEscapedTestFix
branch
from
8bf1f9f to
e0b817e
Compare
@dcantah could you please take a look when you have time? :) Thank you! |
|
@dmcgowan could you please take a look if you have some time? Thanks! :) |
5 participants
This PR addresses the review comments on the port PR #8247 .
container is running