Kubelet status manager sync the status of local Pods #57106

JulienBalestra · 2017-12-12T18:08:49Z

What this PR does / why we need it:

In the kubelet, when using --pod-manifest-path the kubelet creates static pods but doesn't update the status accordingly in the PodList.

This PR fixes the incorrect status of each Pod in the kubelet's PodList.

This is the setup used to reproduce the issue:

manifest:

cat ~/kube/staticpod.yaml

apiVersion: v1
kind: Pod
metadata:
  labels:
    app: nginx
  name: nginx
  namespace: default
spec:
  hostNetwork: true
  containers:
  - name: nginx
    image: nginx:latest
    imagePullPolicy: IfNotPresent
    volumeMounts:
    - name: os-release
      mountPath: /usr/share/nginx/html/index.html
      readOnly: true

  volumes:
  - name: os-release
    hostPath:
      path: /etc/os-release

kubelet:

~/go/src/k8s.io/kubernetes/_output/bin/kubelet --pod-manifest-path ~/kube/ --cloud-provider="" --register-node --kubeconfig kubeconfig.yaml

You can observe this by querying the kubelet API /pods:

curl -s 127.0.0.1:10255/pods | jq .

{
  "kind": "PodList",
  "apiVersion": "v1",
  "metadata": {},
  "items": [
    {
      "metadata": {
        "name": "nginx-nodeName",
        "namespace": "default",
        "selfLink": "/api/v1/namespaces/default/pods/nginx-nodeName",
        "uid": "0fdfa64c73d9de39a9e5c05ef7967e72",
        "creationTimestamp": null,
        "labels": {
          "app": "nginx"
        },
        "annotations": {
          "kubernetes.io/config.hash": "0fdfa64c73d9de39a9e5c05ef7967e72",
          "kubernetes.io/config.seen": "2017-12-12T18:42:46.088157195+01:00",
          "kubernetes.io/config.source": "file"
        }
      },
      "spec": {
        "volumes": [
          {
            "name": "os-release",
            "hostPath": {
              "path": "/etc/os-release",
              "type": ""
            }
          }
        ],
        "containers": [
          {
            "name": "nginx",
            "image": "nginx:latest",
            "resources": {},
            "volumeMounts": [
              {
                "name": "os-release",
                "readOnly": true,
                "mountPath": "/usr/share/nginx/html/index.html"
              }
            ],
            "terminationMessagePath": "/dev/termination-log",
            "terminationMessagePolicy": "File",
            "imagePullPolicy": "IfNotPresent"
          }
        ],
        "restartPolicy": "Always",
        "terminationGracePeriodSeconds": 30,
        "dnsPolicy": "ClusterFirst",
        "nodeName": "nodeName",
        "hostNetwork": true,
        "securityContext": {},
        "schedulerName": "default-scheduler",
        "tolerations": [
          {
            "operator": "Exists",
            "effect": "NoExecute"
          }
        ]
      },
      "status": {
        "phase": "Pending",
        "conditions": [
          {
            "type": "PodScheduled",
            "status": "True",
            "lastProbeTime": null,
            "lastTransitionTime": "2017-12-12T17:42:51Z"
          }
        ]
      }
    }
  ]
}

The status of the nginx Pod will remain in Pending state phase.

curl -I 127.0.0.1
HTTP/1.1 200 OK

It's reported as expected on the apiserver side:

kubectl get po --all-namespaces -w

NAMESPACE   NAME                    READY     STATUS    RESTARTS   AGE
default     nginx-nodeName   0/1       Pending   0          0s
default     nginx-nodeName   1/1       Running   0          2s

It doesn't work either with a standalone kubelet:

~/go/src/k8s.io/kubernetes/_output/bin/kubelet --pod-manifest-path ~/kube/ --cloud-provider="" --register-node false

Special notes for your reviewer:

Release note:

NONE

dashpole · 2017-12-12T18:12:58Z

/ok-to-test

feiskyer · 2017-12-13T09:05:22Z

LGTM

/assign @Random-Liu

@Random-Liu Could you take another look at this?

feiskyer · 2017-12-13T09:05:39Z

/sig node

vikaschoudhary16 · 2017-12-14T15:31:50Z

LGTM

dashpole · 2017-12-14T17:49:43Z

Another option is to call kl.statusManager.GetPodStatus(podUID) when we are handling calls to the /pods endpoint. I think we would want the /pods endpoint to be correct regardless of whether there is a remote client.

JulienBalestra · 2017-12-16T12:31:45Z

@dashpole I tried this approach and it works too.

I draft something like:

diff --git a/pkg/kubelet/kubelet_getters.go b/pkg/kubelet/kubelet_getters.go
index fefd8aac14..d4a3b174e6 100644
--- a/pkg/kubelet/kubelet_getters.go
+++ b/pkg/kubelet/kubelet_getters.go
@@ -142,7 +142,13 @@ func (kl *Kubelet) getPodContainerDir(podUID types.UID, ctrName string) string {
 // GetPods returns all pods bound to the kubelet and their spec, and the mirror
 // pods.
 func (kl *Kubelet) GetPods() []*v1.Pod {
-       return kl.podManager.GetPods()
+       pods := kl.podManager.GetPods()
+       for _, po := range pods {
+               if status, ok := kl.statusManager.GetPodStatus(po.UID); ok {
+                       po.Status = status
+               }
+       }
+       return pods
 }

Personally I may prefer updating the pods statuses in the status/status_manager.go scope.
What do you think ?

dashpole · 2017-12-20T19:13:20Z

pkg/kubelet/status/status_manager.go

 	// on the master, where the kubelet is responsible for bootstrapping the pods
 	// of the master components.
 	if m.kubeClient == nil {
-		glog.Infof("Kubernetes client is nil, not starting status manager.")
+		glog.Infof("Kubernetes client is nil, sync pod status locally")
+		go wait.Forever(func() {


I think you can just do for syncRequest := range m.podStatusChannel

@dashpole of course, I did the changes.

Random-Liu · 2018-01-02T23:18:01Z

@JulienBalestra @dashpole I think we should keep pod manager reflect newest pod from apiserver, which is the assumption we've always being make.

The GetPods change proposed in #57106 (comment) makes sense to me! :)

Random-Liu · 2018-01-02T23:21:03Z

pkg/kubelet/status/status_manager.go

+// updateLocalPod updates the kubelet pod in the podManager if this pod isn't from an apiserver
+// The common use-case is providing a manifest-path containing Pod resources to bootstrap the Master role
+func (m *manager) updateLocalPod(uid types.UID, status versionedPodStatus) {
+	pod, podOk := m.podManager.GetPodByUID(uid)


There is still a race that the pod is updated by kubelet syncloop after this point but before UpdatePod underlying.
In that case, the pod spec might still be changed accidentally.

JulienBalestra · 2018-01-03T10:37:58Z

@Random-Liu thank you for your feedback !

In the meantime I caught a missing field: .status.hostIP

This is the PodList of a node running a single static Pod from a file manifest with both implementations (the initial one and the comment one) :

{
  "kind": "PodList",
  "apiVersion": "v1",
  "metadata": {},
  "items": [
    {
      "metadata": {
        "name": "nginx-192.168.128.135",
        "namespace": "default",
        "selfLink": "/api/v1/namespaces/default/pods/nginx-192.168.128.135",
        "uid": "d00b60dec4eb99d4e04099e43459a100",
        "creationTimestamp": null,
        "labels": {
          "app": "nginx"
        },
        "annotations": {
          "kubernetes.io/config.hash": "d00b60dec4eb99d4e04099e43459a100",
          "kubernetes.io/config.seen": "2018-01-03T11:01:26.642008795+01:00",
          "kubernetes.io/config.source": "file"
        }
      },
      "spec": {
        "containers": [
          {
            "name": "nginx",
            "image": "nginx:latest",
            "resources": {},
            "terminationMessagePath": "/dev/termination-log",
            "terminationMessagePolicy": "File",
            "imagePullPolicy": "IfNotPresent"
          }
        ],
        "restartPolicy": "Always",
        "terminationGracePeriodSeconds": 30,
        "dnsPolicy": "ClusterFirst",
        "nodeName": "192.168.128.135",
        "securityContext": {},
        "schedulerName": "default-scheduler",
        "tolerations": [
          {
            "operator": "Exists",
            "effect": "NoExecute"
          }
        ]
      },
      "status": {
        "phase": "Running",
        "conditions": [
          {
            "type": "Initialized",
            "status": "True",
            "lastProbeTime": null,
            "lastTransitionTime": "2018-01-03T10:01:31Z"
          },
          {
            "type": "Ready",
            "status": "True",
            "lastProbeTime": null,
            "lastTransitionTime": "2018-01-03T10:01:45Z"
          },
          {
            "type": "PodScheduled",
            "status": "True",
            "lastProbeTime": null,
            "lastTransitionTime": "2018-01-03T10:01:31Z"
          }
        ],
        "podIP": "172.17.0.2",
        "startTime": "2018-01-03T10:01:31Z",
        "containerStatuses": [
          {
            "name": "nginx",
            "state": {
              "running": {
                "startedAt": "2018-01-03T10:01:45Z"
              }
            },
            "lastState": {},
            "ready": true,
            "restartCount": 0,
            "image": "nginx:latest",
            "imageID": "docker-pullable://nginx@sha256:cf8d5726fc897486a4f628d3b93483e3f391a76ea4897de0500ef1f9abcd69a1",
            "containerID": "docker://38027962ea06462f28ca390d9ccc3f494e221ba00efeb2c1b806f59196842682"
          }
        ],
        "qosClass": "BestEffort"
      }
    }
  ]
}

To fulfill this field, I created the following implementation:

diff --git a/pkg/kubelet/kubelet_getters.go b/pkg/kubelet/kubelet_getters.go
index fefd8aac14..d10a48b56f 100644
--- a/pkg/kubelet/kubelet_getters.go
+++ b/pkg/kubelet/kubelet_getters.go
@@ -139,10 +139,36 @@ func (kl *Kubelet) getPodContainerDir(podUID types.UID, ctrName string) string {
        return filepath.Join(kl.getPodDir(podUID), config.DefaultKubeletContainersDirName, ctrName)
 }
 
+// setStatusHostIP ensure the field status.hostIP is properly set when the status Phase is Running
+func (kl *Kubelet) setStatusHostIP(status *v1.PodStatus) error {
+       if status.Phase != v1.PodRunning {
+               return nil
+       }
+       if status.HostIP != "" {
+               return nil
+       }
+       ip, err := kl.GetHostIP()
+       if err != nil {
+               return err
+       }
+       status.HostIP = ip.String()
+       return nil
+}
+
 // GetPods returns all pods bound to the kubelet and their spec, and the mirror
 // pods.
 func (kl *Kubelet) GetPods() []*v1.Pod {
-       return kl.podManager.GetPods()
+       pods := kl.podManager.GetPods()
+       for _, p := range pods {
+               if status, ok := kl.statusManager.GetPodStatus(p.UID); ok {
+                       p.Status = status
+                       err := kl.setStatusHostIP(&p.Status)
+                       if err != nil {
+                               glog.Warningf("fail to update the status.hostIP for Pod %s: %s", p.Name, err)
+                       }
+               }
+       }
+       return pods
 }

Another option could be to create a new getter like:

diff --git a/pkg/kubelet/kubelet_getters.go b/pkg/kubelet/kubelet_getters.go
index fefd8aac14..f6b1686f2a 100644
--- a/pkg/kubelet/kubelet_getters.go
+++ b/pkg/kubelet/kubelet_getters.go
@@ -139,10 +139,41 @@ func (kl *Kubelet) getPodContainerDir(podUID types.UID, ctrName string) string {
        return filepath.Join(kl.getPodDir(podUID), config.DefaultKubeletContainersDirName, ctrName)
 }
 
+// getUpdatedPodStatus get an updated status of the Pod and ensure the field status.hostIP is properly set
+// when the status Phase is Running
+func (kl *Kubelet) getUpdatedPodStatus(podUID types.UID) (v1.PodStatus, error) {
+       var status v1.PodStatus
+       status, ok := kl.statusManager.GetPodStatus(podUID)
+       if !ok {
+               return status, fmt.Errorf("fail to get PodStatus for Pod %s", podUID)
+       }
+       if status.Phase != v1.PodRunning {
+               return status, nil
+       }
+       if status.HostIP != "" {
+               return status, nil
+       }
+       ip, err := kl.GetHostIP()
+       if err != nil {
+               return status, err
+       }
+       status.HostIP = ip.String()
+       return status, nil
+}
+
 // GetPods returns all pods bound to the kubelet and their spec, and the mirror
 // pods.
 func (kl *Kubelet) GetPods() []*v1.Pod {
-       return kl.podManager.GetPods()
+       pods := kl.podManager.GetPods()
+       for _, p := range pods {
+               status, err := kl.getUpdatedPodStatus(p.UID)
+               if err != nil {
+                       glog.Warningf("fail to update the status for Pod %s: %s", p.Name, err)
+                       continue
+               }
+               p.Status = status
+       }
+       return pods
 }

But I'm not 💯 sure it's the most appropriate place to do that.
Because we'll fill this field each time we'll call the GetPods() method.

Also, as you said, the Pod manager is dedicated to work with the apiserver only, so the interface created here isn't adapted too.

dashpole · 2018-01-03T20:37:50Z

WRT HostIP, it looks like we intentionally dont set HostIP when we dont have a kubeClient
It might be better to change that block of code, but I am not sure what the implication of that would be.

dashpole · 2018-01-03T23:09:39Z

Looks like that check was added in #9722, which appears to be because at that point the kubelet would only get the node object from the API server.
I think it should be safe to remove that check. @JulienBalestra can you try that out?

JulienBalestra · 2018-01-04T22:06:25Z

Thanks for the relevant feedback @dashpole !
I made the changes.

dashpole · 2018-01-19T21:09:04Z

/lgtm

dashpole · 2018-01-19T21:11:07Z

pkg/kubelet/kubelet_getters.go

@@ -142,7 +142,13 @@ func (kl *Kubelet) getPodContainerDir(podUID types.UID, ctrName string) string {
 // GetPods returns all pods bound to the kubelet and their spec, and the mirror
 // pods.
 func (kl *Kubelet) GetPods() []*v1.Pod {
-	return kl.podManager.GetPods()
+	pods := kl.podManager.GetPods()
+	for _, p := range pods {


nit: can you add a comment explaining that this is required to get the status of static pods when there is no apiserver?

dashpole · 2018-02-07T17:39:18Z

/lgtm

JulienBalestra · 2018-02-08T09:39:42Z

/test pull-kubernetes-unit

BenTheElder · 2018-02-12T17:35:33Z

/test pull-kubernetes-unit

JulienBalestra · 2018-02-12T19:00:32Z

/test pull-kubernetes-e2e-kops-aws

Random-Liu · 2018-02-13T23:20:54Z

/approve

@JulienBalestra Thanks for the fix!
@dashpole Thanks for the review!

k8s-ci-robot · 2018-02-13T23:21:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, JulienBalestra, Random-Liu

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 OWNERS Files:

~~pkg/kubelet/OWNERS~~ [Random-Liu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-02-14T00:02:59Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-14T00:46:32Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

Random-Liu · 2018-02-14T21:23:03Z

@JulienBalestra @dashpole I think this PR broke the reboot test. https://k8s-testgrid.appspot.com/google-gce#gci-gce-reboot

I guess it is related to the kubeClient change.

Let's revert or fix.

dashpole · 2018-02-14T21:34:54Z

@Random-Liu sgtm

Automatic merge from submit-queue (batch tested with PRs 59877, 59886, 59892). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. kubelet: revert the status HostIP behavior **What this PR does / why we need it**: This PR partially revert #57106 to fix #59889. The PR #57106 changed the behavior of `generateAPIPodStatus` when a **kubeClient** is nil. **Release note**: ```release-note NONE ```

@dashpole

…t-pod-status Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. kubelet: revert the get pod with updated status **What this PR does / why we need it**: Following kubernetes#59892 this PR finish to revert kubernetes#57106 The first revert didn't solve the reboot test in the [test grid](https://k8s-testgrid.appspot.com/google-gce#gci-gce-reboot). **Special notes for your reviewer**: cc @dashpole @Random-Liu **Release note**: ```release-note None ```

k8s-github-robot assigned feiskyer and dashpole Dec 12, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 12, 2017

k8s-ci-robot assigned Random-Liu Dec 13, 2017

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Dec 13, 2017

dashpole reviewed Dec 20, 2017

View reviewed changes

JulienBalestra force-pushed the kubelet-update-local-pods branch from 5309d55 to c044f8d Compare December 21, 2017 15:46

Random-Liu reviewed Jan 2, 2018

View reviewed changes

JulienBalestra force-pushed the kubelet-update-local-pods branch from c044f8d to e22ce76 Compare January 4, 2018 10:17

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2018

dashpole reviewed Jan 19, 2018

View reviewed changes

JulienBalestra force-pushed the kubelet-update-local-pods branch from e22ce76 to 0ce0a87 Compare January 20, 2018 10:56

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2018

JulienBalestra force-pushed the kubelet-update-local-pods branch from 0ce0a87 to dbbbef5 Compare January 20, 2018 10:58

Kubelet provides an updated and complete status of local-static Pods

4c28901

JulienBalestra force-pushed the kubelet-update-local-pods branch from dbbbef5 to 4c28901 Compare January 20, 2018 11:02

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2018

JulienBalestra mentioned this pull request Feb 12, 2018

Unable to load build details kubernetes/test-infra#6781

Closed

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2018

k8s-github-robot merged commit 7b678dc into kubernetes:master Feb 14, 2018

roberthbailey mentioned this pull request Feb 14, 2018

[e2e test failure] each node by dropping all outbound packets for a while and ensure they function afterwards #59889

Closed

JulienBalestra mentioned this pull request Feb 14, 2018

kubelet: revert the status HostIP behavior #59892

Merged

JulienBalestra mentioned this pull request Feb 15, 2018

kubelet: revert the get pod with updated status #59948

Merged

JulienBalestra mentioned this pull request Mar 26, 2018

Kubelet doesn't update static pod status #61717

Closed

hkaj mentioned this pull request Jan 31, 2019

Monitor static pods DataDog/datadog-agent#2970

Merged

mfpierre mentioned this pull request May 9, 2019

Kubelet status manager sync the status of local Pods #77661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubelet status manager sync the status of local Pods #57106

Kubelet status manager sync the status of local Pods #57106

JulienBalestra commented Dec 12, 2017

dashpole commented Dec 12, 2017

feiskyer commented Dec 13, 2017

feiskyer commented Dec 13, 2017

vikaschoudhary16 commented Dec 14, 2017

dashpole commented Dec 14, 2017 •

edited

JulienBalestra commented Dec 16, 2017 •

edited

dashpole Dec 20, 2017

JulienBalestra Dec 21, 2017

Random-Liu commented Jan 2, 2018 •

edited

Random-Liu Jan 2, 2018 •

edited

JulienBalestra commented Jan 3, 2018 •

edited

dashpole commented Jan 3, 2018

dashpole commented Jan 3, 2018

JulienBalestra commented Jan 4, 2018

dashpole commented Jan 19, 2018

dashpole Jan 19, 2018

dashpole commented Feb 7, 2018

JulienBalestra commented Feb 8, 2018

BenTheElder commented Feb 12, 2018

JulienBalestra commented Feb 12, 2018

Random-Liu commented Feb 13, 2018

k8s-ci-robot commented Feb 13, 2018

k8s-github-robot commented Feb 14, 2018

k8s-github-robot commented Feb 14, 2018

Random-Liu commented Feb 14, 2018 •

edited

dashpole commented Feb 14, 2018

Kubelet status manager sync the status of local Pods #57106

Kubelet status manager sync the status of local Pods #57106

Conversation

JulienBalestra commented Dec 12, 2017

dashpole commented Dec 12, 2017

feiskyer commented Dec 13, 2017

feiskyer commented Dec 13, 2017

vikaschoudhary16 commented Dec 14, 2017

dashpole commented Dec 14, 2017 • edited

JulienBalestra commented Dec 16, 2017 • edited

dashpole Dec 20, 2017

Choose a reason for hiding this comment

JulienBalestra Dec 21, 2017

Choose a reason for hiding this comment

Random-Liu commented Jan 2, 2018 • edited

Random-Liu Jan 2, 2018 • edited

Choose a reason for hiding this comment

JulienBalestra commented Jan 3, 2018 • edited

dashpole commented Jan 3, 2018

dashpole commented Jan 3, 2018

JulienBalestra commented Jan 4, 2018

dashpole commented Jan 19, 2018

dashpole Jan 19, 2018

Choose a reason for hiding this comment

dashpole commented Feb 7, 2018

JulienBalestra commented Feb 8, 2018

BenTheElder commented Feb 12, 2018

JulienBalestra commented Feb 12, 2018

Random-Liu commented Feb 13, 2018

k8s-ci-robot commented Feb 13, 2018

k8s-github-robot commented Feb 14, 2018

k8s-github-robot commented Feb 14, 2018

Random-Liu commented Feb 14, 2018 • edited

dashpole commented Feb 14, 2018

dashpole commented Dec 14, 2017 •

edited

JulienBalestra commented Dec 16, 2017 •

edited

Random-Liu commented Jan 2, 2018 •

edited

Random-Liu Jan 2, 2018 •

edited

JulienBalestra commented Jan 3, 2018 •

edited

Random-Liu commented Feb 14, 2018 •

edited