should never happen on containerd.. a sandbox might be a vm type... and containerd does not overload containerid and podsandbox ids.. a podsandbox has it's own id and it's own lifecycle in containerd. or is this just a variable naming issue?

We are just updating kubelet's internal cache. This is very similar to how the existing Generic PLEG handles the pod deletion from the kubelet cache. In case of Generic PLEG, they will delete the pod from the cache if it doesn't show up during the relisting. While in our case, we get explicit delete event from the runtime so we also go ahead and delete the pod from kube cache. We just make sure that the delete event we got was for the sandbox container and not the constituent container(s) of the pod before deleting the entire pod from the cache.

ok.. my concern I guess is small windows / disconnects between the idea of the sandbox container being deleted and the sandbox being deleted.. from (https://github.com/containerd/containerd/blob/main/pkg/cri/server/sandbox_remove.go#L96-L112)

	// Delete sandbox container.
	if err := sandbox.Container.Delete(ctx, containerd.WithSnapshotCleanup); err != nil {
		if !errdefs.IsNotFound(err) {
			return nil, fmt.Errorf("failed to delete sandbox container %q: %w", id, err)
		}
		log.G(ctx).Tracef("Remove called for sandbox container %q that does not exist", id)
	}

	// Remove sandbox from sandbox store. Note that once the sandbox is successfully
	// deleted:
	// 1) ListPodSandbox will not include this sandbox.
	// 2) PodSandboxStatus and StopPodSandbox will return error.
	// 3) On-going operations which have held the reference will not be affected.
	c.sandboxStore.Delete(id)

	// Release the sandbox name reserved for the sandbox.
	c.sandboxNameIndex.ReleaseByKey(id)

Din't quite get the concern that might be caused due to this. In the current implementation, the pod cache will be updated once the container with ID as that of sandbox ID gets deleted, which seems natural (although we do understand that it actually doesn't indicate the deletion of the pod sandbox). The pod sandbox may still remain active though the sandbox container exits, but since our implementation is event-driven and we're not using relisting, this condition was used to just update the cache.

cc: @rphillips

Din't quite get the concern that might be caused due to this. In the current implementation, the pod cache will be updated once the container with ID as that of sandbox ID gets deleted

container runtime is containerd...

mike@ubnt:~/crictl-test$ sudo crictl run container-config.json sandbox-config.json 
301bd43e2bfb139188af9d5156c6be76fd7aace714423d2aa7f12a07d9dd66b2
mike@ubnt:~/crictl-test$ sudo crictl pods
POD ID              CREATED             STATE               NAME                NAMESPACE           ATTEMPT             RUNTIME
dcc2efa57c400       17 seconds ago      Ready               busybox-sandbox     default             1                   (default)

note pod id is dcc

mike@ubnt:~/crictl-test$ sudo crictl ps --all
CONTAINER           IMAGE               CREATED             STATE               NAME                ATTEMPT             POD ID
301bd43e2bfb1       busybox:1.35.0      21 seconds ago      Running             busybox             0                   dcc2efa57c400

note container id for container inside the pod is 301 and it's pod is dcc

, which seems natural (although we do understand that it actually doesn't indicate the deletion of the pod sandbox).

doesn't seem natural to me, it seems early, for example if the sequence for pod construction is create id, then network namespace then attach that to the pause container,.... during removal the sequenc will be remove the pause container then the network namespace.. then destroy meta store and return pod id in a transaction..

The pod sandbox may still remain active though the sandbox container exits, but since our implementation is event-driven and we're not using relisting, this condition was used to just update the cache.

It's not a "may" thing the sandbox container is just one possible part of the sandbox, and is not a requirement for pod sandboxes. In fact there will come a time where there will be no sandbox container for sandboxed pods. Only a matter of time, we (mostly maksym) are currently working on a sandboxed feature in containerd.

There should be pod events to indicate the pod has exited. Unless we are saying we want kubelet to manage tracking all the checkpointed/inited parts of the pod sandbox, individually?

from the ctr (containerd tool) here you can see the pause container and the container:

mike@ubnt:~/crictl-test$ sudo ctr -n k8s.io c ls
CONTAINER                                                           IMAGE                               RUNTIME                  
301bd43e2bfb139188af9d5156c6be76fd7aace714423d2aa7f12a07d9dd66b2    docker.io/library/busybox:1.35.0    io.containerd.runc.v2    
dcc2efa57c400dbf98935f11e37ffb8c5544a3ae28bf530ef8a9fc5dc26e60d4    k8s.gcr.io/pause:3.7                io.containerd.runc.v2 

now kill the pause task, ls the containers showing the container is still there, and delete the pause container using the ctr tool:

mike@ubnt:~/crictl-test$ sudo ctr -n k8s.io t kill -s KILL dcc2efa57c400dbf98935f11e37ffb8c5544a3ae28bf530ef8a9fc5dc26e60d4
mike@ubnt:~/crictl-test$ sudo ctr -n k8s.io c ls
CONTAINER                                                           IMAGE                               RUNTIME                  
301bd43e2bfb139188af9d5156c6be76fd7aace714423d2aa7f12a07d9dd66b2    docker.io/library/busybox:1.35.0    io.containerd.runc.v2    
dcc2efa57c400dbf98935f11e37ffb8c5544a3ae28bf530ef8a9fc5dc26e60d4    k8s.gcr.io/pause:3.7                io.containerd.runc.v2    
mike@ubnt:~/crictl-test$ sudo ctr -n k8s.io c del dcc2efa57c400dbf98935f11e37ffb8c5544a3ae28bf530ef8a9fc5dc26e60d4
mike@ubnt:~/crictl-test$ sudo ctr -n k8s.io c ls
CONTAINER                                                           IMAGE                               RUNTIME                  
301bd43e2bfb139188af9d5156c6be76fd7aace714423d2aa7f12a07d9dd66b2    docker.io/library/busybox:1.35.0    io.containerd.runc.v2    

note pause container is deleted

now come in through CRI to see what's still around:

mike@ubnt:~/crictl-test$ sudo crictl ps -a
CONTAINER           IMAGE               CREATED             STATE               NAME                ATTEMPT             POD ID
301bd43e2bfb1       busybox:1.35.0      19 minutes ago      Exited              busybox             0                   dcc2efa57c400
mike@ubnt:~/crictl-test$ sudo crictl pods

note container has exited

check pod list

POD ID              CREATED             STATE               NAME                NAMESPACE           ATTEMPT             RUNTIME
dcc2efa57c400       19 minutes ago      NotReady            busybox-sandbox     default             1                   (default)

note pod is still there, and now in notready state

I suppose one could use the information about the pause exit as a point in time "update" to a cache moving the pod to the "exiting" state, if kubelet is going to accept said new state?

Thanks @mikebrow for that perspective.

nit: doc comment,

Set updates the cache by setting the PodStatus for the pod only if the data is newer than the cache based on the provided time stamp. Returns if the cache was updated

updated.

nit: maybe rename val to cachedVal to make it clear that val is coming from the cache.

yeah, thanks. Updated to use cachedVal.

nit: just link to the commit url on github directly here, no reason to use bit.ly

Updated.

Sorry if this was already mentioned, but why is this mutex necessery? What fields is it guarding? Isn't this function only called by the go-routine spawned in the wait.Until in Start()?

agree with @bobbypage it would be important to clarify this before beta.

nit: s/ingored/ignored

Thanks, corrected the typo.

I might be misunderstanding the logic here, but does this mean that after eventedPlegMaxStreamRetries we completely stop using evented pleg and fallback to generic PLEG?

Do we ever again re-attempt to restart evented pleg after the retry limit is reached? It seems like we should... for example take the case that you stop the container runtime for some time to perform some maintenance, during this time we could easily reach up to 5 retries (the current eventedPlegMaxStreamRetries). However after starting the container runtime, the connection could be established but it seems like the evented pleg would never re-start and re-connect? Maybe this needs some type of backoff logic instead?

That's a good point. For now, we would require kubelet to be restarted in this scenario as we do not have a very deterministic way of retrying to re-establish the gRPC streaming connection with the runtime.

Either we have to consider a reasonable time period for retrying within this mechanism or have a way for runtime to inform kubelet that connection should be re-established. IMHO, we could consider this for beta. Thanks!

nit: s/podUid/podUID here and below, most of the existing kubelet logs use PodUID (different casing), might be worth to stay consistent in case folks are using this to extract logs (https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/kubernetes/kubernetes%24+file:%5Epkg/kubelet+klog.*podUID.*&patternType=regexp)

Done!

I don't fully understand why for ContainerEventType_CONTAINER_DELETED_EVENT (deletion of the sandbox container), we don't also check the timestamp of the event (i.e. e.cache.Set(podID, status, err, time.Unix(event.GetCreatedAt(), 0))). Is there a reason we don't do this check only for the sandbox container deletion? If so, would be great to add a comment to explain why.

Updated, thanks.

not 100% sure if it's needed here and below, but do you need to call event.String() to ensure you get string representation of the event in the log?

Yeah, used event.String() now.

I had the same question when I was reviewing this, I was also trying to understand the reason for the difference here since it is not immediately clear :). Can we maybe summarize the reasoning for this change as a comment in the code?

this 3m value is the same value that used to be in generic but has moved here with the same value and is passed down, so its not a behavior change, etc.

just noting to myself this is the same code as elsewhere just extracted for reuse.

Yes, that's correct.

is this a bug in the container runtime itself? if we encounter this, can we really trust anything? is it possible to determine this in the implementation of PodSandboxStatus?

timestamp in the PodSandboxStatus is introduced as a part of this PR. If someone uses runtime which doesn't have the CRI changes from this PR, the timestamp value will not be set. This is not a bug in the runtime, but rather the older runtime is not aware that PodSandboxStatus has timestamp field.

e.g. The CI job pull-kubernetes-e2e-gce-alpha-features will enable all feature gates in k8s, which would include this evented pleg feature as well, but will end up using a containerd which doesn't have evented pleg yet. It would be impossible to pass that job (and hence merge this PR) without this condition.

this tripped me up for a second. the behavior here is the same as without the EventedPleg, but rather than an else block, we have the negative to probably make it easier to read in an actual code editor rather than github ;-)

It's correct that this is behavior is same as without the Evented PLEG. However, I am not sure we have a way to put it in the else block, because there is no if anywhere there. The if evented pleg == true condition appears above is inside the loop. But part of that loop also needs to be executed for generic pleg as well.

i think we can do better here before beta.

the health check should validate that we have an actual connection with container runtime and that we are able to keep up with the rate of emitted events.

I see we added Relist() to support the evented pleg being able to force the generic pleg to relist.
I see we added Update() to support the evented pleg telling the generic pleg to use different timings.
I see we added Stop() to support fall back on failure as well from evented pleg to generic pleg.

Before we go to beta, can we have two interfaces?

PodLifecycleEventGenerator stays with just Start, Watch, and Healthy as this is all the rest of the kubelet needs to know about. We add a PodLifecycleEventGeneratorHandler and it adds Stop , Update, and Relist so these compositions can be kept internal.

Thanks, that's approach looks good.

PR to address this comment - #113825

thanks for the comment, i was similar to @bobbypage when catching up on latest.

nit: naming on this would be clearer if it was the following:

genericPlegRelistPeriodWhenEventedPlegInUse

Thanks, will rename this variable.

before beta, we should clarify how we respond to these error conditions. notably when is the evented pleg re-established versus just fall back to generic behavior.

I agree. Thanks.

agree with @bobbypage it would be important to clarify this before beta.