kubelet: fix nil deref in volume type check#39493
Conversation
|
This change is |
|
@saad-ali note that my follow up PR for 1.6 removes this check completely |
|
Thanks for the quick fix @sjenning. LGTM. Please prepare a cherry-pick for this PR, as soon as it merges. 1.5.2 should go out on Tuesday, Jan 10. Let's make sure that this fix makes it in. Could you also add a release note to this PR. |
| if volume == nil { | ||
| continue | ||
| } | ||
| if (volume.EmptyDir == nil || volume.EmptyDir.Medium != v1.StorageMediumMemory) && |
There was a problem hiding this comment.
Do we still need this check below? Since if Volume field is nil - it would automatically mean we are dealing with temporal storage?
Also - it may be good idea to cover this with some test? Not sure how easy that will be though.
There was a problem hiding this comment.
We still need to check if the temporal storage is disk- or memory-backed. The idea here was that we wanted to skip removing disk-backed temporal storage until the pod is removed from the API server, but remove memory-backed volumes as soon as the pod is in a terminal state.
I have a follow-on PR #37228 for 1.6 that removes this check entirely.
There was a problem hiding this comment.
Do we still need this check below? Since if Volume field is nil - it would automatically mean we are dealing with temporal storage?
No, VolumeSource volumes could include both.
Also - it may be good idea to cover this with some test? Not sure how easy that will be though.
Always a good idea.
There was a problem hiding this comment.
Also - it may be good idea to cover this with some test? Not sure how easy that will be though.
I'll try to write a test as part of my follow-on PR.
|
LGTM |
|
@k8s-bot gce e2e test this |
|
@k8s-bot test this |
|
@k8s-bot gce etcd3 e2e test this |
|
Jenkins GCE etcd3 e2e failed for commit c4e6725. Full PR test history. The magic incantation to run this job again is DetailsInstructions 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. I understand the commands that are listed here. |
|
@k8s-bot gce etcd3 e2e test this |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue (batch tested with PRs 39493, 39496) |
Automatic merge from submit-queue kubelet: fix nil deref in volume type check Cherry pick #39493 @saad-ali @derekwaynecarr @grosskur @gnufied ```release-note fix nil dereference when doing a volume type check on persistent volumes ```
|
Commit found in the "release-1.5" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
An attempt to address memory exhaustion through a build up of terminated pods with memory backed volumes on the node in PR #36779 introduced this.
For the
VolumeSpec, either theVolumeorPersistentVolumefield is set, not both. This results in a situation where there is a nil deref on PVs. Since PVs are inherently not memory-backend, only local/temporal volumes should be considered.This needs to go into 1.5 as well.
Fixes #39480
@saad-ali @derekwaynecarr @grosskur @gnufied