Make ContainerCreateTimeout configurable at runtime handler level #9499
Conversation
|
Hi @snir911. Thanks for your PR. I'm waiting for a cri-o 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-sigs/prow repository. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9499 +/- ##
==========================================
+ Coverage 63.85% 63.90% +0.04%
==========================================
Files 205 205
Lines 28699 28722 +23
==========================================
+ Hits 18327 18355 +28
+ Misses 8746 8743 -3
+ Partials 1626 1624 -2 🚀 New features to boost your workflow:
|
f74dbd0 to
a0408f0
Compare
|
@snir911, what happens when the ContainerCreateTimeout is set to, let's say, 600 (10 minutes), but kubelet's runtimeRequestTimeout is lower than that? What I think that would happen is that kubelet's runtimeRequestTimeout would be the value respected in this case, which makes me think it's at least worth it adding to the documentation that the value set here depends also on the kubelet's value. |
|
That's right, I'll mention it, thanks @fidencio ! |
|
/ok-to-test |
|
/release-note-edit |
|
@bitoku: /release-note-edit must be used with a single release note block. DetailsIn response to this:
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-sigs/prow repository. |
a732847 to
5097166
Compare
37ad46f to
abf1b0f
Compare
|
@saschagrunert @bitoku recent changes includes:
Another look will be appreciated, thanks |
I thought this is needed especially for runtime_vm. It doesn't make sense to disable the test for vm. |
@bitoku You’re correct. However, at the moment I have no idea how to simulate the hanging CreateContainer call in this test infra. When containerd-shim-v2 is used, the CreateContainer call is performed over ttrpc, so I can’t intercept it with a simple wrapper like we do in the runtime_oci case. If it is possible, it will probably require a more complex test scenario, so it might make sense to implement it separately. |
|
@snir911 |
Signed-off-by: Snir Schreiber <[email protected]>
This allows different runtime handlers to have different container creation timeouts, useful for VM-based runtimes that may need longer timeouts than OCI runtimes. Signed-off-by: Snir Schreiber <[email protected]> Fixes: cri-o#9151
Previously, r.task.Create() used r.ctx (background context) which had no timeout, this may cause the goroutine to continue running even after the select timeout was reached. Signed-off-by: Snir Schreiber <[email protected]>
- Add container_create_timeout to crio.conf.5.md man page documentation - Add container_create_timeout to configuration template in template.go Signed-off-by: Snir Schreiber <[email protected]>
- Add ValidateContainerCreateTimeout unit tests covering: * Default timeout when not configured (240s) * Valid configured timeout values * Minimum timeout enforcement (30s) * Zero and negative value handling * Large timeout values * Different timeouts per runtime handler Fixes: cri-o#9151 Signed-off-by: Snir Schreiber <[email protected]>
Add comprehensive BATS integration tests for the container_create_timeout feature introduced in commit 63131b6. These tests verify: - Default timeout value (240s) is applied when not configured - Custom timeout values are properly set and respected - Minimum timeout enforcement (30s minimum) - Different runtime handlers can have different timeout values - Verify the container create timeout is actually being triggered Assisted-by: Claude AI Signed-off-by: Snir Schreiber <[email protected]>
@bitoku done, BTW how the two pending checks can be triggered? |
|
They seem running. let me wait for the tests to be green. |
|
/lgtm @snir911 Thank you for your contribution and patience! |
|
/retest |
1 similar comment
|
/retest |
This allows different runtime handlers to have different container creation timeouts,
useful for VM-based runtimes that may need longer timeouts than OCI runtimes.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Fixes #9151 and additional minor fixes
Which issue(s) this PR fixes:
Fixes #9151
Special notes for your reviewer:
Does this PR introduce a user-facing change?