[KEP-3085] kubelet - extend RuntimeHelper interface with OnPodSandboxReady to update PodReadyToStartContainers condition correctly
#134660
Conversation
k8s-ci-robot
added
release-note
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
k8s-ci-robot
added
needs-priority
k8s-ci-robot
removed
the
do-not-merge/needs-sig
github-project-automation
bot
moved this to Triage
in SIG Node: code and documentation PRs
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Priyankasaggu11929 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 |
Priyankasaggu11929
changed the title
Priyankasaggu11929
force-pushed
the
kep-3085-add-callback
branch
from
a87d62c to
b1c982a
Compare
|
Windows unit test failure is tracked here #134300 |
|
cc: @SergeyKanzhelev @haircommander for review too. Thanks! |
pkg/kubelet/kubelet.go
Outdated
| klet.allocationManager.SetContainerRuntime(runtime) | ||
|
|
||
| // Register pod sandbox ready callback with runtime manager. | ||
| // The Type assertion is used because the `SetPodSandboxReadyCallback` method |
There was a problem hiding this comment.
why should it not be part of the interface?
There was a problem hiding this comment.
Hello @haircommander, do you mean adding the OnPodSandboxReady callback to RuntimeHelper interface directly and and calling it as m.runtimeHelper.OnPodSandboxReady() in containerruntime.SyncPod()?
I tried that approach and that was cleaner as well.
My only thought was if extending the RuntimeHelper interface was fine, provided all components implementing it will now need to define this additional function as well.
But now I think that is simpler approach, since within Kubernetes codebase, there're only 2 places which implements this interface (Kubelet and FakeRuntimeHelper).
I'll update the PR.
There was a problem hiding this comment.
747aaf4 addresses this.
@haircommander, please review again. Thanks!
|
What about DRA? Can we include it in the description as well. I wonder if there are more places where it should be mentioned that DRA allocation is also included into the time before this condition is set |
| // SyncPod syncs the running pod into the desired pod by executing following steps: | ||
| // | ||
| // 1. Compute sandbox and container changes. | ||
| // 2. Kill pod sandbox if necessary. | ||
| // 3. Kill any containers that should not be running. | ||
| // 4. Create sandbox if necessary. | ||
| // 5. Create ephemeral containers. | ||
| // 6. Create init containers. | ||
| // 7. Resize running containers (if InPlacePodVerticalScaling==true) | ||
| // 8. Create normal containers. | ||
| // 5. Invoke sandbox ready callback to Kubelet to update pod status | ||
| // 6. Create ephemeral containers. | ||
| // 7. Create init containers. | ||
| // 8. Resize running containers (if InPlacePodVerticalScaling==true) | ||
| // 9. Create normal containers. |
There was a problem hiding this comment.
Hello @SergeyKanzhelev,
What about DRA? Can we include it in the description as well. I wonder if there are more places where it should be mentioned that DRA allocation is also included into the time before this condition is set
I was preparing a separate PR I will update this PR itself to add tests that validates this order between the DRA allocate calls and PodReadytoStartContainers condition.
I can also extend the documentation comments over SyncPod function to clarify the above order.
Plus, I am also planning to update the Pod Lifecycle Docs to mention this.
Would that be sufficient? Or I'm missing some more places? Thanks!
There was a problem hiding this comment.
b566e88 add tests for validating order of DRA Allocate calls and PodReadyToStartContainers.
@SergeyKanzhelev, please review. Thanks!
Priyankasaggu11929
force-pushed
the
kep-3085-add-callback
branch
from
b1c982a to
c89f907
Compare
Priyankasaggu11929
changed the title
OnPodSandboxReady to update PodReadyToStartContainers condition correctly
…ce to update `PodReadyToStartContainers` condition immediately after sandbox creation This is to address the bug (gh-issue 134460), which reported that currently `PodReadyToStartContainers` condition is only set to `True` after the container image pull is completed. so, if the image size is big and image pull takes significant time to finish, the pod status managaer is blocked and the condition remaind `False`. The commit implements the following changes, to allow kubelet to update the `PodReadyToStartContainers` pod condition immediately after all three requirements (pod sandbox, networking, volume)are ready, but before container images are pulled or containers are created. * add `OnPodSandboxReady` method to the `RuntimeHelper` interface in `container/helpers.go` * implement the `OnPodSandboxReady` method in Kubelet * inside `(containerRuntime).SyncPod`, after sandbox creation and network configuration, invoke `runtimeHelper.OnPodSandboxReady()` directly (this method retrieves current pod status, generates updated API status, and notifies the status manager to sync to the API server) This implementation is gated under `PodReadyToStartContainersCondition` feature gate, and fails gracefully, i.e, it only logs error and continues the pod creation process to make sure that these new changes don't block pod startup.
Priyankasaggu11929
force-pushed
the
kep-3085-add-callback
branch
from
c89f907 to
c4f7894
Compare
…adyToStartContainers condition
Priyankasaggu11929
force-pushed
the
kep-3085-add-callback
branch
from
c4f7894 to
b566e88
Compare
|
Hello @haircommander @SergeyKanzhelev - just another ping for requesting review, to make sure I address any more follow up review items in time for the 1.35 upcoming code freeze timeline. Thank you! |
4 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Ongoing work for KEP-3085 to prepare for stable graduation.
First part (747aaf4) of the PR implements the following changes, to allow kubelet to update the
PodReadyToStartContainerspod condition immediately after all three requirements (pod sandbox, networking, volume)are ready, but before container images are pulled or containers are created.OnPodSandboxReadymethod to theRuntimeHelperinterface incontainer/helpers.goOnPodSandboxReadymethod in Kubelet(containerRuntime).SyncPod, after sandbox creation and network configuration, invokeruntimeHelper.OnPodSandboxReady()directly(this method retrieves current pod status, generates updated API status, and notifies the status manager to sync to the API server)
Second Part (b566e88) of the PR is adding tests to verify invocation of
OnPodSandboxReadymethod andPodReadyToStartContainerscondition. These tests also validates the order between the DRA allocate calls and PodReadytoStartContainers condition.The new code implementation is gated under
PodReadyToStartContainersConditionfeature gate, and fails gracefully, i.e, it only logs error and continues the pod creation process to make sure that these new changes don't block pod startup.Which issue(s) this PR is related to:
Fixes #134460
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: