kubelet: return mirror pod in GetActivePods() #74222
Conversation
k8s-ci-robot
added
release-note
|
/retest |
|
Master
…On Mon, Feb 18, 2019, 3:15 PM Wei Huang ***@***.*** wrote:
*What type of PR is this?*
/kind bug
*What this PR does / why we need it*:
For static pod, kubelet maintains a pair of {static pod, mirror pod}
internally. Mirror pod reflects the real status of the pod, and also
persisted in etcd/apiserver, audited by admission plugins, etc.
But when kubelet make eviction decisions, it fetches regular pods + static
pods. It's problematic b/c "static pod" is just a snapshot of the yaml
file, hence it doesn't carry the real info like "spec.priority" (usually
only "spec.priorityClassName" is fined in yaml file). This is what issue
#73572 <#73572> reported.
*Which issue(s) this PR fixes*:
Fixes #73572 <#73572>
*Special notes for your reviewer*:
*Does this PR introduce a user-facing change?*:
Kubelet doesn't evict a static pod if Priority feature is enabled.
/sig node
/sig scheduling
------------------------------
You can view, comment on, or merge this pull request online at:
#74222
Commit Summary
- kubelet: return mirror pod in GetActivePods()
File Changes
- *M* pkg/kubelet/kubelet_pods.go
<https://github.com/kubernetes/kubernetes/pull/74222/files#diff-0>
(14)
Patch Links:
- https://github.com/kubernetes/kubernetes/pull/74222.patch
- https://github.com/kubernetes/kubernetes/pull/74222.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#74222>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/Atif5VNOcAsdHykPrW4KxtYCw-_Kpf4-ks5vOwn9gaJpZM4bBi7h>
.
|
|
cc/ @yujuhong as you seem to be most recent author on mirror pod related logic. Ignore if I'm wrong. /priority important-soon |
k8s-ci-robot
added
priority/important-soon
|
/retest |
|
|
|
/retest |
1 similar comment
|
/retest |
|
/assign @yujuhong for reviewing. |
|
is there a e2e or unit test we can write around eviction manager to confirm this does not regress again? |
|
@derekwaynecarr I will try to compose a test. |
k8s-ci-robot
added
size/L
Huang-Wei
force-pushed
the
kubelet-mirrorpod
branch
from
7e1256c to
76174b3
Compare
|
/retest |
|
@derekparker e2e test has been added, PTAL. I verified locally that it fails on codebase without my change, and passes with my change. PS: the local test is running as PSS: the e2e test gets skipped due to keyword "slow/serial/disruptive tests". Let me know if you'd like to enable it in ci pre-commit temporarily. |
|
I will verify locally for now, thanks for test. |
| @@ -88,7 +88,19 @@ func (kl *Kubelet) listPodsFromDisk() ([]types.UID, error) { | |||
|
|
|||
| // GetActivePods returns non-terminal pods | |||
| func (kl *Kubelet) GetActivePods() []*v1.Pod { | |||
| allPods := kl.podManager.GetPods() | |||
| allPods, mirrorPods := kl.podManager.GetPodsAndMirrorPods() | |||
There was a problem hiding this comment.
Why can't we make this change within GetPodsAndMirrorPods() fn? I don't see any other place in the code where this is being used.
There was a problem hiding this comment.
GetPodsAndMirrorPods() is defined as:
// GetPodsAndMirrorPods returns the both regular and mirror pods.
So IMO "regular pods" literally means all pods including "real regular" and "static" pods. I prefer to think of current function as a consistent behavior with the comment.
There was a problem hiding this comment.
I am fine with the change, just that I am not sure, if there is a need of using static pods somewhere else in the code base instead of using mirror pods. If we have some use then it makes sense to keep the function as is.
|
I verified this test locally on kube master branch, it is working as expected but I see the following the logs even when the eviction is not happening.
|
|
Kindly ping @derekwaynecarr as 1.14 code freeze is approaching. |
|
thank you for adding the test case. /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, Huang-Wei The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest |
|
this patch does not work. see #73572 (comment) |
| @@ -88,7 +88,19 @@ func (kl *Kubelet) listPodsFromDisk() ([]types.UID, error) { | |||
|
|
|||
| // GetActivePods returns non-terminal pods | |||
| func (kl *Kubelet) GetActivePods() []*v1.Pod { | |||
| allPods := kl.podManager.GetPods() | |||
| allPods, mirrorPods := kl.podManager.GetPodsAndMirrorPods() | |||
There was a problem hiding this comment.
Looking at the other usage of mirrorPods in removeOrphanedPodStatuses, the mirrorPods can be in the Map structure shown below (and not affecting functionality).
I wonder if GetPodsAndMirrorPods should return mirror pods in Map.
What type of PR is this?
/kind bug
What this PR does / why we need it:
For static pod, kubelet maintains a pair of {static pod, mirror pod} internally. Mirror pod reflects the real status of the pod, and also persisted in etcd/apiserver, audited by admission plugins, etc.
But when kubelet makes eviction decisions, it fetches regular pods + static pods. It's problematic b/c "static pod" is just a snapshot of the yaml file, hence it doesn't carry the real info like "spec.priority" (usually only "spec.priorityClassName" is defined in yaml file and hence "spec.priority" is nil b/c it's updated by admission plugin). This is what issue #73572 reported.
Which issue(s) this PR fixes:
Fixes #73572
Special notes for your reviewer:
This PR is based on the assumption that static pod don't need or can't be updated. Otherwise I can come up with optimal solutions.
Does this PR introduce a user-facing change?:
/sig node
/sig scheduling