Make ContainerCreateTimeout configurable at runtime handler level #9499
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
|
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. 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9499 +/- ##
==========================================
- Coverage 67.04% 66.97% -0.08%
==========================================
Files 202 202
Lines 28159 28255 +96
==========================================
+ Hits 18880 18924 +44
- Misses 7702 7739 +37
- Partials 1577 1592 +15 🚀 New features to boost your workflow:
|
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 <ssheribe@redhat.com> 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 <ssheribe@redhat.com>
Signed-off-by: Snir Schreiber <ssheribe@redhat.com>
This eliminates duplication and makes the struct cleaner with a single source of truth for configuration values from the handler. Signed-off-by: Snir Schreiber <ssheribe@redhat.com>
- 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 <ssheribe@redhat.com>
- 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 <ssheribe@redhat.com>
|
@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 ! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: littlejawa, snir911 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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?