Add container event support to containerd #7073

ruiwen-zhao · 2022-06-17T18:25:09Z

This PR adds container event support (kubernetes/enhancements#3386) to containerd, by implementing GetContainerEvents() CRI API that supports single-caller and returns all container events (i.e. not filtered by container ID or other filters) to the caller.

Tests Done

Integration test TestContainerEvents added.
Manually tested by
a. Creating a k8s cluster with k8s version evented pleg support and containerd from this PR.
b. Deploying an simple Job and verifying the pod status turned to Running -> Terminating, and then disappeared

To-do list before un-drafting:

add sandbox server support
Merge the CRI change in k/k repo

k8s-ci-robot · 2022-06-17T18:25:18Z

Hi @ruiwen-zhao. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

samuelkarp · 2022-06-17T18:35:39Z

/ok-to-test

mxpv · 2022-06-17T19:11:06Z

pkg/cri/server/container_events.go

+)
+
+func (c *criService) GetContainerEvents(r *runtime.GetEventsRequest, s runtime.RuntimeService_GetContainerEventsServer) error {
+	// TODO: replace with a real implementation that broadcasts containerEventsChan to all subscribers.


Would it make sense to use containerd events here?
https://github.com/containerd/containerd/blob/main/events/events.go#L79
It supports broadcasting and has notion of filters.

thanks Maksym! I thought about it acutally but I thought events.go was for specific containerd-internal use cases and could not be easily generalized. Some questions I had:

events/events.go is just some interfaces, and I would still need to write my own implementation of broadcasting. Is that right?

events.go seems to have some implementations but looks like it spins up an event service and relies local grpc to sub/pub. I am not sure if this is an overkill for my usecase here. (My change only touches CRI server and does not touch the containerd service itself).

There is an implementation as well - https://github.com/containerd/containerd/blob/main/events/exchange/exchange.go

Event service already launched by containerd any way and some events already used by CRI (https://github.com/containerd/containerd/blob/main/pkg/cri/server/events.go#L105).
So I'm just wondering whether its possible to reuse existing code for a similar task instead of building another one.

a simple implementation to start I think for this POC would be ok I think, then maybe look at refactoring after if needed? I think he's reusing container exit/stopped off the cri events code correctly, I like how that looks below. I would rather not do this message api as a broadcast-able filter-able by container id subscription for all event types .. not in the alpha of the api anyhow. IMO this get msg subs api should be about the orchestrator asking for its PLEG messages for its namespace, it's owned containers and pods.. period. Not something to replace NRI or containerd events or otel or.. :-) Probably should only be one active registration per namespace unless reset due to connection/restart of kubelet/rancher etc..?

cri container create and cri container start also looks right..

the pod ones need work at least due to overloading with container events ( an artifact of crio overloading container/pod ids, one of the few differences of opinion on the api between containerd/crio )

the pod ones need work at least due to overloading with container events

I am not fully understanding the rationale, and asked it in the KEP PR (kubernetes/enhancements#3387 (comment)), so let's discuss there.

But anyway, if we want to separate sandbox events from container events, then a new CRI API GetSandboxEvents will be needed.

not do this message api as a broadcast-able filter-able by container id subscription for all event types

To make sure I understand correctly, you are suggesting that there should be only one caller/subscriber to the GetContainerEvents API, and the caller shoud not use any filters. Containerd will just return all events of k8s.io containers to this one and only subscriber.

I agree that the caller should just get all events of its namespace. We can continue this discussion around whether id/labels are needed in the KEP PR or CRI API PR.

Probably should only be one active registration

Kubelet will subscribe for sure, but on top of that what if user calls this API directly through crictl? As an implemntation of the API we should be able to handle the case of multiple subscribers even tho ideally this case should not happen, right?

great questions .. should probably take this to a zoom call..

I am not fully understanding the rationale, and asked it in the KEP PR (kubernetes/enhancements#3387 (comment)), so let's discuss there.

answered there

GetSandboxEvents will be needed.

agreed

To make sure I understand correctly, you are suggesting that there should be only one caller/subscriber to the GetContainerEvents API, and the caller shoud not use any filters. Containerd will just return all events of k8s.io containers to this one and only subscriber.

two issues.. someone "proxying" CRI (non-issue for single user); someone registering for kubelet's pleg events (what is the use case here? monitoring, some new trace? is it to be truly broadcast or sequence by subs? do we need n-subs for alpha?)

I agree that the caller should just get all events of its namespace. We can continue this discussion around whether id/labels are needed in the KEP PR or CRI API PR.

nod

Kubelet will subscribe for sure, but on top of that what if user calls this API directly through crictl? As an implemntation of the API we should be able to handle the case of multiple subscribers even tho ideally this case should not happen, right?

if we are not supporting two(to n) subscriptions for one namespace the crictl request would be able to validate that "alpha" limitation.., in alpha critest is more likely the use case and it would own the k8s.io ns

the logs/metrics/trace already have this information in them... these events are targeted I think for the orchestrator's cache updates, and are not guaranteed (thus requiring sync) this isn't some set of messages for a hook pattern or logs/metrics/trace.. ?

mikebrow

see comments

mikebrow · 2022-06-18T12:52:04Z

vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

+// GetEventsFilter is used to filter a list of events.
+// All those fields are combined with 'AND'
+message GetEventsFilter {
+    // ID of the container or sandbox.


see comments in kep pr.. this (combining container/sandbox ids) won't work...

would rather start with namespace as the filter "k8s.io" not a pod id or if nil a container id.. but that may only make sense to containerd users.. Maybe just leave it more abstract for now..

mikebrow · 2022-06-18T12:54:56Z

vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

+    // LabelSelector to select matches.
+    // Only api.MatchLabels is supported for now and the requirements
+    // are ANDed. MatchExpressions is not supported yet.
+    map<string, string> label_selector = 3;


is there a use case for this?

mikebrow · 2022-06-18T12:56:10Z

vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

+
+message ContainerEventResponse {
+    // ID of the container
+	string container_id = 1;


tabs broken..

see notes regarding not combining container and pod ids / events

mikebrow · 2022-06-18T13:09:02Z

pkg/cri/server/container_events.go

+)
+
+func (c *criService) GetContainerEvents(r *runtime.GetEventsRequest, s runtime.RuntimeService_GetContainerEventsServer) error {
+	// TODO: replace with a real implementation that broadcasts containerEventsChan to all subscribers.


a simple implementation to start I think for this POC would be ok I think, then maybe look at refactoring after if needed? I think he's reusing container exit/stopped off the cri events code correctly, I like how that looks below. I would rather not do this message api as a broadcast-able filter-able by container id subscription for all event types .. not in the alpha of the api anyhow. IMO this get msg subs api should be about the orchestrator asking for its PLEG messages for its namespace, it's owned containers and pods.. period. Not something to replace NRI or containerd events or otel or.. :-) Probably should only be one active registration per namespace unless reset due to connection/restart of kubelet/rancher etc..?

mikebrow · 2022-06-18T13:22:38Z

pkg/cri/server/container_events.go

+)
+
+func (c *criService) GetContainerEvents(r *runtime.GetEventsRequest, s runtime.RuntimeService_GetContainerEventsServer) error {
+	// TODO: replace with a real implementation that broadcasts containerEventsChan to all subscribers.


cri container create and cri container start also looks right..

the pod ones need work at least due to overloading with container events ( an artifact of crio overloading container/pod ids, one of the few differences of opinion on the api between containerd/crio )

ruiwen-zhao · 2022-08-12T21:39:32Z

Some CI checks failed. Most of the failures seem irrelevant:

go: gopkg.in/yaml.v2@v2.4.0: unrecognized import path "gopkg.in/yaml.v2": reading https://gopkg.in/yaml.v2?go-get=1: 502 Bad Gateway
	server response: Cannot obtain refs from GitHub: error reading from GitHub: net/http: request canceled (Client.Timeout exceeded while reading body)
make: *** [integration] Error 1
Makefile:[21](https://github.com/containerd/containerd/runs/7813786735?check_suite_focus=true#step:9:22)3: recipe for target 'integration' failed

One failure on the new test added TestContainerEvents:

        	Error Trace:	/home/runner/work/containerd/containerd/container_event_test.go:49
        	Error:      	Received unexpected error:
        	            	assertContainerEventResponse: wrong sandbox name. Expected sandbox_foobar2, got sandbox
        	Test:       	TestContainerEvents

Looking.

ruiwen-zhao · 2022-08-12T21:40:13Z

btw I am getting

+ run_crictl
+ /usr/local/bin/crictl --runtime-endpoint=unix:///run/containerd-test/containerd.sock info
FATA[0000] getting status of runtime failed: rpc error: code = Unimplemented desc = unknown service runtime.v1alpha2.RuntimeService

when running sudo make cri-integration. Looking into it as well.

Update: adding CONTAINERD_RUNTIME solves this issue ( sudo FOCUS=TestContainerEvents CONTAINERD_RUNTIME=io.containerd.runtime.v1.linux make cri-integration), but now I am seeing

time="2022-08-12T22:24:22.752372163Z" level=error msg="RunPodSandbox for &PodSandboxMetadata{Name:sandbox_foobar2,Uid:52fdfc072182654f163f5f0f9a621d729566c74d10037c4d7bbb0407d1e2c649,Namespace:events-81855ad8681d0d86d1e91e00167939cb6694d2c422acd208a0072939487f6999,Attempt:0,} failed, error" error="failed to create containerd task: cgroups: cgroup mountpoint does not exist: unknown"

Still looking

integration/remote/remote_runtime.go

samuelkarp · 2022-08-12T22:28:05Z

pkg/cri/server/container_events.go

+)
+
+func (c *criService) GetContainerEvents(r *runtime.GetEventsRequest, s runtime.RuntimeService_GetContainerEventsServer) error {
+	// TODO: replace with a real implementation that broadcasts containerEventsChan to all subscribers.


What's the status of the TODOs in this PR? Are you planning to address them prior to this PR being ready to merge or do you intend to track their completion somewhere else?

This todo is NOT a blocker of this PR because in most, if not all, use cases, there will be only one client calling GetContainerEvents.

I can create a separate containerd issue and reference that issue in the code if that's a better way to use TODOs?

Yes, please.

Done. 281b22f#diff-69fd78c71bbb81e8ad5ce2c58609d73579d8a285ccd90056d027e49e7c21120dR24

samuelkarp · 2022-08-12T22:48:39Z

Update: adding CONTAINERD_RUNTIME solves this issue ( sudo FOCUS=TestContainerEvents CONTAINERD_RUNTIME=io.containerd.runtime.v1.linux make cri-integration)

Can you try io.containerd.runc.v2 (this should be the default) and io.containerd.runc.v1 as well?

, but now I am seeing "failed to create containerd task: cgroups: cgroup mountpoint does not exist: unknown"

Are you perhaps using a machine with cgroups v2 configured? I believe the v1 shim depends on cgroups v1.

mikebrow

just some comments on the testing.. reviewing server code next

integration/container_event_test.go

mikebrow

See comments.. mostly I think we need to be tolerant of events for containers when the podsandbox is not in the store .. I think it should be possible to ignore that and continue vs leave the abandoned container un-stoppable/removable

pkg/cri/server/container_remove.go

pkg/cri/server/container_stop.go

pkg/cri/server/events.go

pkg/cri/server/helpers.go

mikebrow · 2022-12-08T18:54:11Z

waiting for confirmation that kubernetes/kubernetes#114351 is merged into a k/k v1.26 release

Signed-off-by: ruiwen-zhao <ruiwen@google.com>

ruiwen-zhao · 2022-12-08T19:33:09Z

Updated the code to skip sending events with nil PodSandboxStatus. See the TODO issue (#7785) for context and next step.

mikebrow

LGTM
As discussed we can't send the events with nil for the k/k v1.26.0 release because it would cause kubelet to throw a nil pointer deferrence exception. We will do the TODOs on both sides once kubernetes v1.26.1 has included support for ignoring nil and the service release has been around for a while

samuelkarp · 2022-12-08T22:10:16Z

samuelkarp requested review from mikebrow

I hit the wrong button there, sorry!

mikebrow

LGTM

np give it a good look ping if you have questions..

mikebrow · 2022-12-08T23:37:06Z

@fuweid and @samuelkarp expressed interest in reviewing

fuweid

LGTM

The CI issue can be handled in the follow-up

fuweid · 2022-12-09T06:56:46Z

integration/container_event_test.go

+
+const (
+	drainContainerEventChannelTimeout = 2 * time.Second
+	readContainerEventChannelTimeout  = 500 * time.Millisecond


IMO, we should increase it to [5, 10] seconds just in case that the CI action is under high pressure. Otherwise, it would be new flaky test case.

fuweid · 2022-12-09T07:22:28Z

pkg/cri/sbserver/helpers.go

+	}
+	containerStatuses, err := c.getContainerStatuses(ctx, sandboxID)
+	if err != nil {
+		logrus.Errorf("Failed to get container statuses for container event for sandboxID %q: %v", sandboxID, err)


[non-blocker] If there are 4 containers, only one fails. Currently, we sends empty container status. What if we send partial response?

Follow-up: containerd#7073 (comment) Signed-off-by: Wei Fu <fuweid89@gmail.com>

k8s-ci-robot added the needs-ok-to-test label Jun 17, 2022

ruiwen-zhao marked this pull request as draft June 17, 2022 18:25

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jun 17, 2022

ruiwen-zhao force-pushed the event branch from e32d89c to 324e840 Compare June 17, 2022 18:39

mxpv reviewed Jun 17, 2022

View reviewed changes

mikebrow reviewed Jun 18, 2022

View reviewed changes

ruiwen-zhao force-pushed the event branch from 324e840 to 0d93f77 Compare July 6, 2022 16:47

mikebrow mentioned this pull request Jul 7, 2022

NRI: add support for NRI with extended scope. #6019

Merged

ruiwen-zhao force-pushed the event branch 2 times, most recently from fa5e31f to 6cc59f1 Compare July 13, 2022 22:30

samuelkarp added the area/cri Container Runtime Interface (CRI) label Jul 18, 2022

ruiwen-zhao force-pushed the event branch from 6cc59f1 to 0fd23a5 Compare August 12, 2022 18:45

ruiwen-zhao changed the title ~~[WIP] Add container event support to containerd~~ Add container event support to containerd Aug 12, 2022

ruiwen-zhao force-pushed the event branch 4 times, most recently from 9304e91 to 93588a9 Compare August 12, 2022 19:24

ruiwen-zhao marked this pull request as ready for review August 12, 2022 19:25

ruiwen-zhao force-pushed the event branch 2 times, most recently from 6549a7a to b642888 Compare August 12, 2022 21:05

samuelkarp reviewed Aug 12, 2022

View reviewed changes

ruiwen-zhao force-pushed the event branch from b642888 to b9ab8ac Compare August 12, 2022 22:42

ruiwen-zhao force-pushed the event branch 2 times, most recently from 4f7af9e to 65cef12 Compare August 22, 2022 22:50

ruiwen-zhao force-pushed the event branch 2 times, most recently from 61cff81 to 28a0216 Compare December 1, 2022 03:11

mikebrow reviewed Dec 7, 2022

View reviewed changes

mikebrow requested changes Dec 7, 2022

View reviewed changes

ruiwen-zhao force-pushed the event branch 2 times, most recently from a20f1e8 to a911a6f Compare December 7, 2022 22:10

ruiwen-zhao mentioned this pull request Dec 7, 2022

[Evented PLEG] Ignore container events with nil PodSandboxStatus kubernetes/kubernetes#114351

Merged

ruiwen-zhao force-pushed the event branch from a911a6f to 06d2364 Compare December 7, 2022 23:47

ruiwen-zhao mentioned this pull request Dec 8, 2022

[Evented PLEG support] Do not filtering out container events with nil PodSandboxStatus #7785

Closed

ruiwen-zhao added 2 commits December 8, 2022 19:30

Add container event support to containerd

a338abc

Signed-off-by: ruiwen-zhao <ruiwen@google.com>

Add Evented PLEG support to sandbox server

a6929f9

Signed-off-by: ruiwen-zhao <ruiwen@google.com>

ruiwen-zhao force-pushed the event branch from 06d2364 to a6929f9 Compare December 8, 2022 19:31

mikebrow approved these changes Dec 8, 2022

View reviewed changes

samuelkarp requested review from mikebrow and samuelkarp December 8, 2022 22:08

mikebrow approved these changes Dec 8, 2022

View reviewed changes

mikebrow requested a review from fuweid December 8, 2022 23:35

samuelkarp approved these changes Dec 9, 2022

View reviewed changes

fuweid approved these changes Dec 9, 2022

View reviewed changes

fuweid merged commit f2cf411 into containerd:main Dec 9, 2022

fuweid added a commit to fuweid/containerd that referenced this pull request Dec 9, 2022

integration: increase timeout in container_event_test.go

423f438

Follow-up: containerd#7073 (comment) Signed-off-by: Wei Fu <fuweid89@gmail.com>

fuweid mentioned this pull request Dec 9, 2022

integration: increase timeout in container_event_test.go #7791

Merged

fangn2 pushed a commit to fangn2/containerd that referenced this pull request Dec 19, 2022

integration: increase timeout in container_event_test.go

791d5b2

Follow-up: containerd#7073 (comment) Signed-off-by: Wei Fu <fuweid89@gmail.com>

jsturtevant pushed a commit to jsturtevant/containerd that referenced this pull request Sep 21, 2023

integration: increase timeout in container_event_test.go

bf172bb

Follow-up: containerd#7073 (comment) Signed-off-by: Wei Fu <fuweid89@gmail.com>

kiashok pushed a commit to kiashok/containerd that referenced this pull request Oct 23, 2024

integration: increase timeout in container_event_test.go

2f3deed

Follow-up: containerd#7073 (comment) Signed-off-by: Wei Fu <fuweid89@gmail.com>

HirazawaUi mentioned this pull request Feb 19, 2025

Assign values for the containerStatuses and timestamp fields in the PodSandboxStatus interface #11408

Open

Add container event support to containerd #7073

Add container event support to containerd #7073

Uh oh!

Conversation

ruiwen-zhao commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 17, 2022

Uh oh!

samuelkarp commented Jun 17, 2022

Uh oh!

mxpv Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow Jun 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruiwen-zhao commented Aug 12, 2022

Uh oh!

ruiwen-zhao commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelkarp commented Aug 12, 2022

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikebrow commented Dec 8, 2022

Uh oh!

ruiwen-zhao commented Jun 17, 2022 •

edited

Loading

mxpv Jun 17, 2022 •

edited

Loading

mikebrow Jun 22, 2022 •

edited

Loading

mikebrow Jun 18, 2022 •

edited

Loading

ruiwen-zhao commented Aug 12, 2022 •

edited

Loading

samuelkarp commented Dec 8, 2022 •

edited

Loading

mikebrow left a comment •

edited

Loading