-
Notifications
You must be signed in to change notification settings - Fork 40.9k
[PodLevelResources] Pod Level Resources Eviction Manager #132277
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.
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
base: master
Are you sure you want to change the base?
[PodLevelResources] Pod Level Resources Eviction Manager #132277
Conversation
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-sigs/prow repository. |
Hi @KevinTMtz. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
f0a8d0f
to
4a2fd75
Compare
/assign @ndixita |
/ok-to-test |
pkg/kubelet/eviction/helpers.go
Outdated
|
||
switch resourceToReclaim { | ||
case v1.ResourceMemory: | ||
podUsage = memoryUsage(podStats.Memory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be cases where stats cannot be collected via the Kubelet Summary API, so it might be better to perform a nil check for podStats.Memory
here, just like with container-level resource requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you
pkg/kubelet/eviction/helpers.go
Outdated
podUsage = resource.NewQuantity(int64(*podStats.EphemeralStorage.UsedBytes), resource.BinarySI) | ||
} | ||
|
||
message += fmt.Sprintf("Pod %s was using %s, request is %s, has larger consumption of %v. ", pod.Name, podUsage.String(), podRequest.String(), resourceToReclaim) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It might be better to extract the format of this message into a variable (e.g. podMessageFmt
), similar to containerMessageFmt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you
pkg/kubelet/eviction/helpers.go
Outdated
podUsage = resource.NewQuantity(int64(*podStats.EphemeralStorage.UsedBytes), resource.BinarySI) | ||
} | ||
|
||
message += fmt.Sprintf("Pod %s was using %s, request is %s, has larger consumption of %v. ", pod.Name, podUsage.String(), podRequest.String(), resourceToReclaim) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought. Consider a Pod that uses pod-level resources and contains three containers.
apiVersion: v1
kind: Pod
metadata:
name: evicted
spec:
resources:
requests:
memory: 500Mi
initContainers:
- name: sidecar1
image: registry.k8s.io/e2e-test-images/agnhost:2.55
command: ["sleep", "infinity"]
- name: sidecar2
image: registry.k8s.io/e2e-test-images/agnhost:2.55
command: ["sleep", "infinity"]
containers:
- name: regular
image: registry.k8s.io/e2e-test-images/agnhost:2.55
command: ["sleep", "infinity"]
resources:
requests:
memory: 300Mi
If an eviction occurs due to memory usage from sidecar1
or sidecar2
, I feel like the current event and annotation alone doesn’t clearly indicate which container actually caused it. If pod-level resources are specified and none of the containers that have memory requests exceed their requested memory, it might be helpful to record the memory usage of the container without a memory request that used the most memory. That way, we can at least get a hint about which container likely triggered the eviction. Though, maybe this is overkill and not really necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After getting the pod level resource stats, the function still continues and iterates all containers (like it used to do), so the user would get something like this:
Warning Evicted 49s kubelet The node was low on resource: memory. Threshold quantity: 80996576029, available: 75619264Ki. Pod stressor-sidecar-pod was using 12981880Ki, request is 1G, has larger consumption of memory. Container sidecar-stressor-container was using 10141636Ki, request is 0, has larger consumption of memory.
Normal Killing 49s kubelet Stopping container sidecar-stressor-container
It shows the comparison between the whole pod usage against its pod level request, along the usage and request of each container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I didn't take into account that when requests isn't specified, it always shows the actual usage along with a message saying it exceeds zero.
4a2fd75
to
dec7b9b
Compare
@@ -39,16 +42,23 @@ func GetResourceRequestQuantity(pod *v1.Pod, resourceName v1.ResourceName) resou | |||
requestQuantity = resource.Quantity{Format: resource.DecimalSI} | |||
} | |||
|
|||
for _, container := range pod.Spec.Containers { | |||
if rQuantity, ok := container.Resources.Requests[resourceName]; ok { | |||
// Supported pod level resources will be used instead of container level ones when available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetResourceRequestQuantity is called in
kubernetes/pkg/api/v1/resource/helpers.go
Line 73 in 74210dd
requestQuantity := GetResourceRequestQuantity(pod, resource) |
which is used in premption and allocation manager related files. Can we please add the e2e coverage for preemption logic as well along with eviction manager related e2e...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: KevinTMtz 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 |
@@ -147,6 +147,66 @@ var _ = SIGDescribe("CriticalPod", framework.WithSerial(), framework.WithDisrupt | |||
}) | |||
}) | |||
|
|||
var _ = SIGDescribe("CriticalPodWithPodLevelResources", framework.WithSerial(), framework.WithDisruptive(), feature.PodLevelResources, func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is the first addition of Node E2E tests related to Pod Level Resources feature, kubernetes/test-infra#35061 seems necessary to pass presubmit job (pull-kubernetes-node-e2e-containerd-alpha-features
).
What type of PR is this?
What this PR does / why we need it:
This PR implements Pod Level Resources Eviction Manager that require following changes:
Which issue(s) this PR is related to:
Fixes #132448
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: