KEP-3386: Kubelet Evented PLEG for Better Performance

[harche]

Signed-off-by: Harshal Patil [email protected]

• One-line PR description: Kubelet Evented PLEG for Better Performance

• Issue link: Kubelet Evented PLEG for Better Performance #3386

• Other comments:

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[haircommander]

I think we should add one where a cluster admin wants their nodes to perform with as little infrastructure overhead as possible

[ruiwen-zhao]

"the Kubelet monitors changes to container and image state"?

[haircommander]

why not use a featuregate instead?

[mrunalp]

Yes, we should use a feature gate here 👍

[mrunalp]

I think we should narrow down the scope for the first iteration to just the container lifecycle events.

[mrunalp]

increased performance -> reduced overhead of kubelet and the CRI runtime.

[mrunalp]

We can say List/Watch model instead of evented.

[mrunalp]

nit: Reduce -> reduce

[mrunalp]

typo: eliminate

[mrunalp]

relist at reduced...

[mrunalp]

nit: Drop Luckily

[mrunalp]

We can ensure that the existing tests function and add any addtional tests as needed to mitigate risks.

[haircommander]

if we do the fallback method described above, this will need updating

[haircommander]

what about missed events? it's not clear to me grpc does multiplexing, so the CRI will have to keep track of which client it has sent which events to. if crictl gets an events endpoint, and the server doesnt' support multiplexing, kubelet may not get an event

[haircommander]

speaking offline with @harche and @mrunalp it sounds like this is not a problem with the grpc implementation of events

[deads2k]

How can a cluster-admin know this is happening in the cluster at scale? Is there something exposed which shows a difference between the state anticipated based on the event stream and state produced when the re-list happens? I think that's computable and necessary to know the watch has failed, even if it is a significantly trailing indicator.

[mrunalp]

We can add an alert when we observe that there a delta that crosses a threshold to detect events missed when compared to a relist.

[haircommander]

I have added some notes about kube_pod_missed_events metric the kubelet should emit

[deads2k]

the PRR lgtm, looks like the sig still has some review to to, but I can approve just the PRR

/approve

[mrunalp]

/lgtm

[ruiwen-zhao]

/lgtm

[derekwaynecarr]

/approve
/lgtm

I think the write-up is good to pursue alpha.

As we go into implementation, the exact metric name will need to be visited.

@vinaykul you may want to track this as well for in-place resize awareness.

[derekwaynecarr]

we may want to refine this metric name during implementation, but i agree with concept.

[derekwaynecarr]

we have a metric today that is pleg_discard_events for awareness, so it may be worth a pass to see if the name will confuse with existing metrics or not.

[derekwaynecarr]

the kubelet internal pleg has an event for ContainerChanged and PodSync. we seem to ignore ContainerChanged now in the code base, and as long as any container modification is synchronous and atomic, it is fine. the one use case that we need to be careful with is resource requirement changes. if update container resources call changes cpu, memory requirements and only cpu is applied, memory cannot, we may find ourselves in a partial state.

Can we leave as a future discussion that we may see a need to support CONTAINER_CHANGED_EVENT as we work through edge cases encountered with in-place resource resizing or any other interaction that may change the state of a running container?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, derekwaynecarr, harche

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:

• keps/prod-readiness/OWNERS [deads2k]
• keps/sig-node/OWNERS [derekwaynecarr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mikebrow]

LGTM in general to get started..

[mikebrow]

issues:

I'm not convinced of the benefit of supporting N-different subscriptions per each container/pod to this partial set of PLEG events. Seems more likely that we need one subscription for all events for the k8s.io namespace?

The filter id is in error.

GetEventsFilter {
     // ID of the container, sandbox.
     string id = 1;

You can't combine container and sandbox ids like this.

Same on the response messages.. pod lifecycle events should not be overloaded with container lifecycle events.

[vinaykul]

/approve /lgtm

I think the write-up is good to pursue alpha.

As we go into implementation, the exact metric name will need to be visited.

@vinaykul you may want to track this as well for in-place resize awareness.

@derekwaynecarr Yes this is on my radar - supporting pod resize use case is currently a stretch goal but hopefully we can bring it in for beta. I'd like to throw out some of the PLEG changes I currently have.

[ruiwen-zhao]

issues:

I'm not convinced of the benefit of supporting N-different subscriptions per each container/pod to this partial set of PLEG events. Seems more likely that we need one subscription for all events for the k8s.io namespace?

The filter id is in error.

GetEventsFilter {
     // ID of the container, sandbox.
     string id = 1;

You can't combine container and sandbox ids like this.

Same on the response messages.. pod lifecycle events should not be overloaded with container lifecycle events.

1. re: one subscription for all events for the k8s.io namespace?

Although not explicitly mentioned in the KEP, I think it is technically possible that the kubelet subscribes events for all containers without using any filter, and kubelet figures out how to update the status for each container. On the container runtime side, container runtime should only return events in k8s.io namespace to the GetContainerEvents CRI. Does that make sense? Or should namespace be a parameter of this API?

@harche @mrunalp please correct me if I am assuming too much here.

2. re: pod lifecycle events should not be overloaded with container lifecycle events

Can you elaborate why they cannot be mixed? More specifically, why containerId == sandboxId is not a good indicator to sandboxes? Looking at the containerd code [1], it does look like containerd creates sandbox container with sandboxId.

[1] https://github.com/containerd/containerd/blob/fbf76c201fbced37ab3a322f5ee4cb0f1c60cef4/pkg/cri/server/sandbox_run.go#L221

[mikebrow]

container runtime should only return events in k8s.io namespace to the GetContainerEvents CRI. Does that make sense?

It does make sense.. we (containerd) can scope our events to just the ones for the k8s.io namespace for this default CRI connection.

If we wanted to allow for non-default subs for monitoring other container events outside the ones allocated in the k8s.io namespace this would be the place.. but I'm fine with punting that to some future item or just not doing it that way.

Can you elaborate why they cannot be mixed?

Sure, we support truncated index lookups for containers and pods. The podsandbox id passed in has to be full or if "truncated", has to match one and only one pod. Same for containers. We keep separate lists of sandboxes and containers each with their own index lookup. You can't request status of a pod using the pause container id of the pod.

[mikebrow]

I think it is technically possible that the kubelet subscribes events for all containers without using any filter

nod.. suspected the filter part wasn't necessary... probably more useful to crictl/debugging scenarios.

[mikebrow]

.. is there a proposal for synchronizing the PLEG event subscription notifications and the macro/sync cycle CRI status verification? Trying to figure out if we also need to add timestamps regarding time of status in the status response for each pod/container, thus allowing one to identify/resolve pleg events with respect to the sync cycle status timings.

[harche]

.. is there a proposal for synchronizing the PLEG event subscription notifications and the macro/sync cycle CRI status verification? Trying to figure out if we also need to add timestamps regarding time of status in the status response for each pod/container, thus allowing one to identify/resolve pleg events with respect to the sync cycle status timings.

There is no other enhancement around evented PLEG apart from this one at this point in time.

[smarterclayton]

When a gRPC connection is closed (due to a kubelet failure), how does the Kubelet "catch up" to the latest events?

I would assume that we would have to "ListContainerStatuses", then start the events channel for all events that occur after the "List". That requires coordination and buffering on the CRI side - a way to find all container statuses newer than the time the List was returned. That ensures that in the event of a Kubelet failure we don't miss any events.

I expected to see it described here how that would be solved, but I'm missing it. Is it because we're assuming the created_at attribute of the Container Event can be filtered on? If so, how are we filtering that in the CRI and how is the Kubelet comparing the results of the List call to the start point of the events? CRI GetContainerEvents does not have an argument that allows us to get "events after time T".

Can someone point me to where this is described?

[swghosh]

The current implementation in Generic PLEG uses computeEvents that is called upon periodic relisting and which generates PLEG events based upon last updated pod status present in cache. Since, in case of Evented PLEG we aim to still re-use the relisting functionality of Generic PLEG (though with relisting being called at a higher frequency than usual 1s) and since the pod status(es) are persisted to common cache, that func should be still able to generate the events since the last update / gRPC connection failure (which forces a relist as well).

[smarterclayton]

I'd like to see that explicitly described in the KEP so that in the future can refer to it. Right now I'm not confident in the design, and rather than debate implementation in the review (which might get lost) I'd just want to be sure that the KEP captures "how we expect to reliably ensure ordered deliver of events". Doesn't have to be as detailed as pseudocode, but I want to make sure the expectation is right.

I.e. a section in the KEP:

The pod worker expects to receive in order status updates in order, where no events are missed or delivered out of order (receiving a delete before create, for instance). We also expect to bound latency observing container updates as part of the motivation for this design. To achieve that, we will: . If the Kubelet restarts, or if the container runtime restarts, we expect to .

If necessary, just describing it in comments here first or in a proposed change to the KEP would be enough for me to review and then we can avoid iterating too much on implementation.

[ruiwen-zhao]

+1 to what Clayton suggests.

A little bit more on "how we expect to reliably ensure ordered deliver of events": We should be clear on how CRI should buffer container events. For example, if kubelet stops reading from grpc stream for any reason, we can't expect CRI to buffer the events indefinitely. Some events will be discarded if kubelet stops for too long. Kubelet should be able to handle this case.

Also, on the same front, regarding @swghosh's comment on

that func should be still able to generate the events since the last update / gRPC connection failure (which forces a relist as well).

Will kubelet need to reconcile the relist result and the container events, because relist always gives the latest status but events could be stale? For example, consider the following

1. time-0: kubelet starts having trouble reading from grpc stream. No container events can be read by kubelet.
2. time-1: containerA starts on container runtime. CRI tries to sends event {ContainerA starts} but fails. The event is buffered by CRI.
3. time-2: containerA stops on container runtime. CRI tries to sends event {ContainerA stops} but fails. The event is buffered by CRI.
4. time-3: kubelet relists and knows that containerA has stopped.
5. time-4: kubelet resumes reading from grpc stream, reads {ContainerA starts}, {ContainerA stops}, and thinks containerA starts again and then stops.

Is the sequence above possible? Maybe this should be discussed on the PR, so I added a comment there as well (https://github.com/kubernetes/kubernetes/pull/111384/files#r974723039)

[harche]

Thanks @ruiwen-zhao and @smarterclayton for these important points. We experimented with a fix that should address these issues, and will update KEP and the implementation as soon as possible.

[mikebrow]

if it helps... state changes of a container occur at points in time that can easily be recorded into the container runtime state machine of the container and provided along with container status, and container events, ... createdOn, runningAt/unknownAsOf, exitedAt, deletedAt, updatedOn... see possible state transitions as of today: https://github.com/containerd/containerd/blob/main/pkg/cri/store/container/status.go#L31-L61

[mikebrow]

both kubelet and the container runtime may be restarted resulting in unexpected/unknown states of shim managed containers, esp. while the container runtime is "restarting" https://github.com/containerd/containerd/blob/main/pkg/cri/server/restart.go