WIP: Add pod and container statuses to CRI events #112555
Conversation
Signed-off-by: Harshal Patil <harpatil@redhat.com>
k8s-ci-robot
added
release-note-none
|
@harche: 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/test-infra repository. |
k8s-ci-robot
added
needs-priority
|
/sig node |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: harche 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 |
| @@ -1589,6 +1589,12 @@ message ContainerEventResponse { | |||
|
|
|||
| // ID of the sandbox container | |||
| PodSandboxMetadata pod_sandbox_metadata = 4; | |||
There was a problem hiding this comment.
With the status added do we still need pod_sandbox_metadata?
There was a problem hiding this comment.
@ruiwen-zhao we end up using the function determinePodSandboxIPs from kuberuntime_sandbox.go at,
which uses Name and Namespace fields (although only for logging)
There was a problem hiding this comment.
gotcha, yeah in this case I guess it should be fine to keep it, as long as kubelet doesn't need to get pod status after receiving the events.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| @@ -1589,6 +1589,12 @@ message ContainerEventResponse { | |||
|
|
|||
| // ID of the sandbox container | |||
| PodSandboxMetadata pod_sandbox_metadata = 4; | |||
|
|
|||
| // Sandbox statuses | |||
| repeated PodSandboxStatus pod_sandbox_statuses = 5; | |||
There was a problem hiding this comment.
how will we ensure that kubelet will work OK with the older CRI not supporting these statuses in the response?
There was a problem hiding this comment.
In general, if CRI does not support GetContainerEvents but kubelet enables evented pleg, then kubelet will update the pod status using only generic pleg but with super long relist interval.
More specifically, if CRI somehow supports GetContainerEvents but just misses PodSandboxStatus (which I dont think will happen given that GetContainerEvents is pretty new), then it will depend on the error handling of evented pleg. IMO event_pleg should just error out if PodSandboxStatus is missing, and discard that event. In this case, all events will be discarded, and kubelet will rely only on generic pleg.
k8s-ci-robot
added
the
needs-rebase
|
@harche: PR needs rebase. 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. |
|
If you still need this PR then please rebase, if not, please close the PR |
|
/close #111384 got the cri events support. |
Signed-off-by: Harshal Patil harpatil@redhat.com
What type of PR is this?
/kind api-change
What this PR does / why we need it:
While developing the Evente PLEG feature we realized that after receiving the CRI event, we don't have to make another call to the runtime to get the pod status if we package the pod status within the CRI event object while sending it from the runtime to the kubelet.
More discussion - https://kubernetes.slack.com/archives/C03HA69L6R1/p1663194480554129?thread_ts=1663111556.951889&cid=C03HA69L6R1
The PoC of the kubelet and runtime (crio) have been tested with this CRI change.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: