Revert "Add timestamp to PodSandboxStatusResponse for kubernetes Evented PLEG"

[chrishenzie]

This reverts commit b529072.

Fixes: #11312

[k8s-ci-robot]

Hi @chrishenzie. 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-sigs/prow repository.

[samuelkarp]

/ok-to-test

But I'd like to understand how this relates to the original request to add the timestamp in #10728.

[djdongjin]

The original KEP mentioned there was a PodSandboxStatusRequest.includeContainers to indicate we should include both containerStatuses and timestamp in PodSandboxStatusResponse, but it seems that field is not added to CRI proto.

So it seems we should set both containerStatuses and timestamp (to fully support #10728 ), or set neither.

@hshiina is it the expectation that we should always include both containerStatuses/timestamp in PodSandboxStatusResponse to support PLEG? given there is no such a conditional variable PodSandboxStatusRequest.includeContainers.

[hshiina]

The original KEP mentioned there was a PodSandboxStatusRequest.includeContainers to indicate we should include both containerStatuses and timestamp in PodSandboxStatusResponse, but it seems that field is not added to CRI proto.

So it seems we should set both containerStatuses and timestamp (to fully support #10728 ), or set neither.

@hshiina is it the expectation that we should always include both containerStatuses/timestamp in PodSandboxStatusResponse to support PLEG? given there is no such a conditional variable PodSandboxStatusRequest.includeContainers.

I'm sorry I filed the issue only for timestamp just by looking at the code here:
https://github.com/kubernetes/kubernetes/blob/07cc2308c679f5402931ee46879a9edee36f8a4c/pkg/kubelet/kuberuntime/kuberuntime_manager.go#L1634-L1640

The condition for setting containerStatusesandtimestamp` is being reconsidered on kubernetes/kubernetes#129857. It seems better to revert the timestamp addition at the moment.

[chrishenzie]

Is there anything blocking or can we proceed?

[djdongjin]

Is there anything blocking or can we proceed?

I think we should proceed with the revert. And reopen #10728 with the updated info.

[chrishenzie]

/retest

[samuelkarp]

/retest-required

Thanks for the discussion @hshiina @djdongjin @chrishenzie. Reverting sounds good.

[samuelkarp]

LGTM on green CI.

[BenTheElder]

Two consecutive failures with E2eNode Suite: [It] [sig-node] ResourceMetricsAPI [Feature:ResourceMetrics] when querying /resource/metrics should report resource usage through the resource metrics api, is this failing on other PRs?

https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/containerd_containerd/11323/pull-containerd-node-e2e/1887204839893504000

The first run failed on some kubelet authz tests instead: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/containerd_containerd/11323/pull-containerd-node-e2e/1884671427139866624

https://prow.k8s.io/pr-history/?org=containerd&repo=containerd&pr=11323

[chrishenzie]

It looks like yeah, it's also failing on #11311 which was opened last week as well.

Job history shows it started failing recently due to timeouts

[chrishenzie]

/retest

[chrishenzie]

The failing test here does not appear related to this change and has been consistently failing the past couple days. Is it possible to proceed despite this?

[BenTheElder]

/retest
https://prow.k8s.io/job-history/gs/kubernetes-ci-logs/pr-logs/directory/pull-containerd-node-e2e is recently green

[BenTheElder]

All green now :-)

[chrishenzie]

Thanks! Although it looks like this is also being fixed on the k8s end (kubernetes/kubernetes#129990) so maybe we should hold off on making changes here?

[BenTheElder]

Perhaps so :-)

From that PR https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/129990/pull-kubernetes-e2e-kind-evented-pleg/1887124786673356800 passed on containerd 2.0.2 https://github.com/kubernetes-sigs/kind/blob/53ff1fb77a1e454cb577abe6e3403eefdd070b44/images/base/Dockerfile#L125

[HirazawaUi]

Regardless of whether kubernetes/kubernetes#129990 gets merged, the current PR remains necessary. As defined in original KEP), we should return the PodSandboxStatusResponse.Timestamp and PodSandboxStatusResponse.containerStatuses fields when PodSandboxStatusRequest.includeContainers is set to true.

Although we haven't added the PodSandboxStatusRequest.includeContainers field to CRI, I believe the PodSandboxStatusResponse.Timestamp and PodSandboxStatusResponse.containerStatuses fields should either both be assigned values simultaneously or neither should be assigned.

[mikebrow]

LGTM

[djdongjin]

/cherry-pick release/2.0

[k8s-infra-cherrypick-robot]

@djdongjin: new pull request created: #11403

In response to this:

/cherry-pick release/2.0

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.