-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
[Sidecar Containers] Consider restartable init containers in eviction message #124947
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: toVersus 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 |
Welcome @toVersus! |
Hi @toVersus. 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. |
Signed-off-by: Tsubasa Nagasawa <toversus2357@gmail.com>
8d65e87
to
8da4a57
Compare
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.
I am not sure it is ok to add restartable init containers to the eviction messages without actually accounting them in eviction logic.
Checking the eviction behavior in the local Kubernetes 1.30.0 cluster, I confirmed that eviction occurs even when restartable init containers cause memory pressure on the node. cat <<EOF | kind create cluster --config -
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
kubeadmConfigPatches:
- |
kind: KubeletConfiguration
evictionHard:
memory.available: "1.5Gi"
nodes:
- role: control-plane
image: kindest/node:v1.30.0@sha256:047357ac0cfea04663786a612ba1eaba9702bef25227a794b52890dd8bcd692e
- role: worker
image: kindest/node:v1.30.0@sha256:047357ac0cfea04663786a612ba1eaba9702bef25227a794b52890dd8bcd692e
EOF
# Create Pod causing eviction by memory pressure
cat <<'EOF' | kubectl create -f -
apiVersion: v1
kind: Pod
metadata:
generateName: alloc-
spec:
terminationGracePeriodSeconds: 1
restartPolicy: Never
initContainers:
- name: el-sidecar1
image: registry.k8s.io/e2e-test-images/agnhost:2.52
args: ["stress", "--mem-alloc-size", "100Mi", "--mem-alloc-sleep", "1s", "--mem-total", "4000Mi"]
restartPolicy: Always
- name: el-sidecar2
image: registry.k8s.io/e2e-test-images/agnhost:2.52
command: ["sleep", "infinity"]
restartPolicy: Always
- name: el-sidecar3
image: registry.k8s.io/e2e-test-images/agnhost:2.52
command: ["sleep", "infinity"]
restartPolicy: Always
containers:
- name: el1
image: registry.k8s.io/e2e-test-images/agnhost:2.52
command: ["sleep", "infinity"]
- name: el2
image: registry.k8s.io/e2e-test-images/agnhost:2.52
command: ["sleep", "infinity"]
- name: el3
image: registry.k8s.io/e2e-test-images/agnhost:2.52
command: ["sleep", "infinity"]
EOF Details of the evicted Pod:
|
/ok-to-test can we add some e2e tests confirming eviction works for sidecars? Memory and storage would be good. A manually test is good but it would be ideal to make sure eviction is tested with sidecars. it can be a follow up if necessary. |
I agree that. I initially considered adding E2E tests, but I was reluctant because E2E tests related to Pod eviction are expensive (marked as Slow, Serial, and Disruptive). This is especially true since there is no special care in the eviction logic for sidecar containers. However, if necessary, I am trying to work on adding E2E tests, although I think it might be better to handle it in a separate PR. |
/triage accepted |
Signed-off-by: Tsubasa Nagasawa <toversus2357@gmail.com>
7d08a82
to
e68bde0
Compare
Non-restartable Init Containers are also not considered in the Pod Eviction messages, so I have modified it to take Init Containers into account. If it is acceptable to expand the scope of the changes, I will update the title and description of the PR. Steps to trigger Pod Eviction with Init Containers: cat <<EOF | kind create cluster --config -
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
kubeadmConfigPatches:
- |
kind: KubeletConfiguration
evictionHard:
memory.available: "1.5Gi"
nodes:
- role: control-plane
image: kindest/node:v1.30.0@sha256:047357ac0cfea04663786a612ba1eaba9702bef25227a794b52890dd8bcd692e
- role: worker
image: kindest/node:v1.30.0@sha256:047357ac0cfea04663786a612ba1eaba9702bef25227a794b52890dd8bcd692e
EOF
# Create Pod causing eviction by memory pressure
cat <<'EOF' | kubectl create -f -
apiVersion: v1
kind: Pod
metadata:
generateName: alloc-
spec:
terminationGracePeriodSeconds: 1
restartPolicy: Never
initContainers:
- name: el-init1
image: registry.k8s.io/e2e-test-images/agnhost:2.52
args: ["stress", "--mem-alloc-size", "100Mi", "--mem-alloc-sleep", "1s", "--mem-total", "4000Mi"]
containers:
- name: el1
image: registry.k8s.io/e2e-test-images/agnhost:2.52
command: ["sleep", "infinity"]
EOF Event before this PR: apiVersion: v1
count: 1
eventTime: null
firstTimestamp: "2024-05-29T09:20:56Z"
involvedObject:
apiVersion: v1
kind: Pod
name: alloc-v972z
namespace: default
resourceVersion: "573"
uid: a7e67c6e-4838-4ce8-a656-c31f9fb42831
kind: Event
lastTimestamp: "2024-05-29T09:20:56Z"
message: 'The node was low on resource: memory. Threshold quantity: 1536Mi, available:
1453492Ki. '
metadata:
annotations:
offending_containers: ""
offending_containers_usage: ""
starved_resource: memory
creationTimestamp: "2024-05-29T09:20:56Z"
name: alloc-v972z.17d3eb2bd8f92aca
namespace: default
resourceVersion: "655"
uid: b3aa8512-9383-4f8c-9794-89a77f1d6602
reason: Evicted
reportingComponent: kubelet
reportingInstance: kind-worker
source:
component: kubelet
host: kind-worker
type: Warning Event after this PR: apiVersion: v1
count: 1
eventTime: null
firstTimestamp: "2024-05-29T09:01:25Z"
involvedObject:
apiVersion: v1
kind: Pod
name: alloc-tlhbv
namespace: default
resourceVersion: "1673"
uid: 4bb36fc8-1dd7-4cc9-befd-9216b1fd7220
kind: Event
lastTimestamp: "2024-05-29T09:01:25Z"
message: 'The node was low on resource: memory. Threshold quantity: 1536Mi, available:
1186072Ki. Container el-init1 was using 4133704Ki, request is 0, has larger consumption
of memory. '
metadata:
annotations:
offending_containers: el-init1
offending_containers_usage: 4133704Ki
starved_resource: memory
creationTimestamp: "2024-05-29T09:01:25Z"
name: alloc-tlhbv.17d3ea1b1e10b7f6
namespace: default
resourceVersion: "1748"
uid: 8f0437ba-e380-47e3-8d07-2f67854d67d3
reason: Evicted
reportingComponent: kubelet
reportingInstance: kind-worker
source:
component: kubelet
host: kind-worker
type: Warning |
What type of PR is this?
/kind bug
What this PR does / why we need it:
When a Pod is evicted due to resources pressure on the node, the kubelet creates an eviction event and reports it to the user. The event message includes which containers within the Pod exceeded their resource requests, along with their actual usage. Similar information is also included in the event annotations.
Currently, the
evictionMessage
function, which generates the message and annotations for the eviction event, does not account for the restartable init containers (sidecar containers).This PR improves the event to account for restartable init containers as follows:
Which issue(s) this PR fixes:
Fixes #124938
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: