Skip to content

order sandbox by attempt or create time #130551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+8 −3
Open

order sandbox by attempt or create time #130551

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions pkg/kubelet/kuberuntime/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,14 @@ func (b containersByID) Less(i, j int) bool { return b[i].ID.ID < b[j].ID.ID }
// Newest first.
type podSandboxByCreated []*runtimeapi.PodSandbox

func (p podSandboxByCreated) Len() int { return len(p) }
func (p podSandboxByCreated) Swap(i, j int) { p[i], p[j] = p[j], p[i] }
func (p podSandboxByCreated) Less(i, j int) bool { return p[i].CreatedAt > p[j].CreatedAt }
func (p podSandboxByCreated) Len() int { return len(p) }
func (p podSandboxByCreated) Swap(i, j int) { p[i], p[j] = p[j], p[i] }
func (p podSandboxByCreated) Less(i, j int) bool {
if p[i].Metadata == nil || p[j].Metadata == nil {
return p[i].CreatedAt > p[j].CreatedAt
}
return p[i].Metadata.Attempt > p[j].Metadata.Attempt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue appears to be caused by containerd, and this PR aims to implement some defensive programming in kubelet to address the bug in containerd.

At first glance, this seems like a positive change: when the physical clock is unreliable, it replaces the absolute time order with a logical incremental relationship.

I would like to raise a question here:

Both #126514 and containerd/containerd#9459 seem to be atypical issues.

That is, when an exception occurs here and reports error="failed to reserve sandbox name", there are already issues elsewhere before this point.

Could this fix potentially mask the real underlying problems? If it does mask the true errors, will users still have other means to troubleshoot this exception after we've obscured it?

I also have another question: Can this issue be reproduced on CRIO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YES, also happend on CRIO

This is a system time issue. If NTP synchronizes the wrong time, causing time rollback, this problem will occur

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned earlier, the repeated creation of sandbox containers by kubelet is atypical behavior. This could be caused by:

  1. Race conditions in kubelet and rapid pod lifecycle changes leading to duplicate sandbox creation (this would be a bug).

  2. Containerd failing to create sandbox containers due to resource constraints, or sandbox containers entering erroneous states for other reasons.

For 2, we have already implemented some defensive programming in kubelet. We now sort the list of retrieved sandbox containers by creation time and select the first one.

Currently, kubelet strictly increments the Attempt value each time it tries to create a new sandbox for the same Pod (starting at 0, incrementing to 1 on retry after failure, etc.). This Attempt value is persisted and retained even across node reboots or containerd crashes, and continues to increment. Its value does not depend on system time, it is solely tied to the actual number of sandbox creation attempts.

From a defensive programming perspective, there appears to be no reason to reject this PR. In distributed systems, logical increment-based ordering is more reliable than absolute time order. Using the Attempt value as the sorting criterion is therefore justified.

cc @yujuhong @SergeyKanzhelev @tallclair sig-node maintainers for more accurate and comprehensive insights.

}

type containerStatusByCreated []*kubecontainer.Status

Expand Down