{"action":"created","issue":{"url":"https://api.github.com/repos/kubernetes/kubernetes/issues/64610","repository_url":"https://api.github.com/repos/kubernetes/kubernetes","labels_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/64610/labels{/name}","comments_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/64610/comments","events_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/64610/events","html_url":"https://github.com/kubernetes/kubernetes/pull/64610","id":328475026,"node_id":"MDExOlB1bGxSZXF1ZXN0MTkyMDAyNDU5","number":64610,"title":"Don't re-add terminating pods into DesiredStateOfWorld.","user":{"login":"cofyc","id":91730,"node_id":"MDQ6VXNlcjkxNzMw","avatar_url":"https://avatars3.githubusercontent.com/u/91730?v=4","gravatar_id":"","url":"https://api.github.com/users/cofyc","html_url":"https://github.com/cofyc","followers_url":"https://api.github.com/users/cofyc/followers","following_url":"https://api.github.com/users/cofyc/following{/other_user}","gists_url":"https://api.github.com/users/cofyc/gists{/gist_id}","starred_url":"https://api.github.com/users/cofyc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cofyc/subscriptions","organizations_url":"https://api.github.com/users/cofyc/orgs","repos_url":"https://api.github.com/users/cofyc/repos","events_url":"https://api.github.com/users/cofyc/events{/privacy}","received_events_url":"https://api.github.com/users/cofyc/received_events","type":"User","site_admin":false},"labels":[{"id":477397086,"node_id":"MDU6TGFiZWw0NzczOTcwODY=","url":"https://api.github.com/repos/kubernetes/kubernetes/labels/cncf-cla:%20yes","name":"cncf-cla: yes","color":"bfe5bf","default":false},{"id":349530249,"node_id":"MDU6TGFiZWwzNDk1MzAyNDk=","url":"https://api.github.com/repos/kubernetes/kubernetes/labels/release-note-none","name":"release-note-none","color":"c2e0c6","default":false},{"id":253450793,"node_id":"MDU6TGFiZWwyNTM0NTA3OTM=","url":"https://api.github.com/repos/kubernetes/kubernetes/labels/size/XS","name":"size/XS","color":"009900","default":false}],"state":"open","locked":false,"assignee":{"login":"jingxu97","id":19172805,"node_id":"MDQ6VXNlcjE5MTcyODA1","avatar_url":"https://avatars3.githubusercontent.com/u/19172805?v=4","gravatar_id":"","url":"https://api.github.com/users/jingxu97","html_url":"https://github.com/jingxu97","followers_url":"https://api.github.com/users/jingxu97/followers","following_url":"https://api.github.com/users/jingxu97/following{/other_user}","gists_url":"https://api.github.com/users/jingxu97/gists{/gist_id}","starred_url":"https://api.github.com/users/jingxu97/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jingxu97/subscriptions","organizations_url":"https://api.github.com/users/jingxu97/orgs","repos_url":"https://api.github.com/users/jingxu97/repos","events_url":"https://api.github.com/users/jingxu97/events{/privacy}","received_events_url":"https://api.github.com/users/jingxu97/received_events","type":"User","site_admin":false},"assignees":[{"login":"jingxu97","id":19172805,"node_id":"MDQ6VXNlcjE5MTcyODA1","avatar_url":"https://avatars3.githubusercontent.com/u/19172805?v=4","gravatar_id":"","url":"https://api.github.com/users/jingxu97","html_url":"https://github.com/jingxu97","followers_url":"https://api.github.com/users/jingxu97/followers","following_url":"https://api.github.com/users/jingxu97/following{/other_user}","gists_url":"https://api.github.com/users/jingxu97/gists{/gist_id}","starred_url":"https://api.github.com/users/jingxu97/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jingxu97/subscriptions","organizations_url":"https://api.github.com/users/jingxu97/orgs","repos_url":"https://api.github.com/users/jingxu97/repos","events_url":"https://api.github.com/users/jingxu97/events{/privacy}","received_events_url":"https://api.github.com/users/jingxu97/received_events","type":"User","site_admin":false},{"login":"msau42","id":24448061,"node_id":"MDQ6VXNlcjI0NDQ4MDYx","avatar_url":"https://avatars3.githubusercontent.com/u/24448061?v=4","gravatar_id":"","url":"https://api.github.com/users/msau42","html_url":"https://github.com/msau42","followers_url":"https://api.github.com/users/msau42/followers","following_url":"https://api.github.com/users/msau42/following{/other_user}","gists_url":"https://api.github.com/users/msau42/gists{/gist_id}","starred_url":"https://api.github.com/users/msau42/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/msau42/subscriptions","organizations_url":"https://api.github.com/users/msau42/orgs","repos_url":"https://api.github.com/users/msau42/repos","events_url":"https://api.github.com/users/msau42/events{/privacy}","received_events_url":"https://api.github.com/users/msau42/received_events","type":"User","site_admin":false}],"milestone":null,"comments":5,"created_at":"2018-06-01T11:02:59Z","updated_at":"2018-06-08T16:49:14Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/kubernetes/kubernetes/pulls/64610","html_url":"https://github.com/kubernetes/kubernetes/pull/64610","diff_url":"https://github.com/kubernetes/kubernetes/pull/64610.diff","patch_url":"https://github.com/kubernetes/kubernetes/pull/64610.patch"},"body":"<!--  Thanks for sending a pull request!  Here are some tips for you:\r\n1. If this is your first time, read our contributor guidelines https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide\r\n2. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews\r\n3. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md\r\n4. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests\r\n-->\r\n\r\n**What this PR does / why we need it**:\r\n\r\nEven if a pod is deleted from apiserver by kubelet, DesiredStateOfWorld may get terminating pods from `podManager.GetPods()` and will re-added volumes again. This causes unnecessary volume mounting in kubelet, and will conflict if another pod is created to use same volume (e.g. local volume will not set FS group if volume is mounted by other pods).\r\n\r\nThis PR ignore terminating pods in addition to terminated ones in `desiredStateOfWorldPopulator.findAndAddNewPods`.\r\n\r\n**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:\r\nFixes #64474\r\n\r\n**Special notes for your reviewer**:\r\n\r\n**Release note**:\r\n<!--  Write your release note:\r\n1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string \"action required\".\r\n2. If no release note is required, just write \"NONE\".\r\n-->\r\n```release-note\r\nNONE\r\n```"},"comment":{"url":"https://api.github.com/repos/kubernetes/kubernetes/issues/comments/395820346","html_url":"https://github.com/kubernetes/kubernetes/pull/64610#issuecomment-395820346","issue_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/64610","id":395820346,"node_id":"MDEyOklzc3VlQ29tbWVudDM5NTgyMDM0Ng==","user":{"login":"cofyc","id":91730,"node_id":"MDQ6VXNlcjkxNzMw","avatar_url":"https://avatars3.githubusercontent.com/u/91730?v=4","gravatar_id":"","url":"https://api.github.com/users/cofyc","html_url":"https://github.com/cofyc","followers_url":"https://api.github.com/users/cofyc/followers","following_url":"https://api.github.com/users/cofyc/following{/other_user}","gists_url":"https://api.github.com/users/cofyc/gists{/gist_id}","starred_url":"https://api.github.com/users/cofyc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cofyc/subscriptions","organizations_url":"https://api.github.com/users/cofyc/orgs","repos_url":"https://api.github.com/users/cofyc/repos","events_url":"https://api.github.com/users/cofyc/events{/privacy}","received_events_url":"https://api.github.com/users/cofyc/received_events","type":"User","site_admin":false},"created_at":"2018-06-08T16:49:14Z","updated_at":"2018-06-08T16:49:14Z","author_association":"CONTRIBUTOR","body":"I found [a way to reproduce this issue](https://github.com/kubernetes/kubernetes/compare/15cd3552819367...cofyc:reproduc64474) locally and verify that if we delay deletion of pod status from statusMananger after pod is removed from podMananger will fix this issue.\r\n\r\n## Process of pod terminating\r\n\r\n- Terminating: pod.DeletionTimestamp != nil\r\n- Running/NotRunning/Missing: containers status is running or not running or missing\r\n\r\n### Phase 0: kubelet received graceful deletion event\r\n\r\npodManager.status: Terminating & Running\r\nstatusManager.status: Terminating & Running\r\ndswp: Mounted\r\n\r\n### Phase 1. kubelet begins to stop containers\r\n\r\nafter all containers are stopped, statusManager.status will be Terminating & NotRunning and report it to apiserver, dwsp will begin to remove volumes from cache.\r\n\r\npodManager.status: Terminating & Running\r\nstatusManager.status: Terminating & NotRunning\r\ndswp: none (start to unmount volumes)\r\n\r\n### Phase 2.1 kubelet.HandlePodCleanups clean up pod resources\r\n\r\nBecause pod status from statusManager is Terminating & NotRunning which means pod is terminated, kubelet.HandlePodCleanups will clean up pod resources (cgroup configs, etc). It will succeed when volumes are mounted (see `kubelet.cleanupOrphanedPodCgroup`).\r\n\r\nAfter pod resources are reclaimed, `statusMananger.canBeDeleted` will return true, status cache will be deleted from statusMananger.\r\n\r\n### Phase 2.2 kubelet receive update pod event (Terminating & Not Running)\r\n\r\nBecause pod spec does not change, this update pod event will trigger a RECONCILE sync operation. This operation will update pod status in podMananger.\r\n\r\n(case 1 ) If phase 2.2 happened before phase 2.1, status will be:\r\n\r\n```\r\npodManager.status: Terminating & NotRunning\r\nstatusManager.status: Terminating & Missing\r\n```\r\n\r\ndswp will do nothing\r\n\r\n(case 2) if there is a delay in phase 2.2 (e.g. api, delay), status will be in follow state for a while:\r\n\r\n```\r\npodManager.status: Terminating & Running\r\nstatusManager.status: Terminating & Missing\r\n```\r\n\r\n**dswp will remount volumes again here**, because it cannot get status from statusManager, it will use stale status from podManager.\r\n\r\n## How to fix this\r\n\r\nStatusManager is source of truth for kubelet pod status, kubelet (dswp and kubelet.HandlePodCleanups) get pod status from statusManager first, if not found then use status in podManager. Status in statusMananger should only be deleted when all its consumers are up to date, otherwise kubelet may get stale status, e.g.\r\n\r\n|statusManager | podManager | status for kubelet | |\r\n| -- | -- | -- | --|\r\n| running | running | running | ok | \r\n| not running | running | not running | ok |\r\n| missing | running | running | bad |\r\n\r\nSo, if we delay deletion of status of statusManager after pod is removed from podManager, kubelet will always get up to date status.\r\n\r\n| statusManager | podManager | status for kubelet | |\r\n| -- | -- | -- | -- |\r\n| running | running | running | ok | \r\n| not running | running | not running | ok |\r\n| not running | not running | not running | ok |\r\n| not running | missing | missing (pod not found) | ok |\r\n| missing | missing | missing | ok |\r\n\r\nPossible ways I found to delay deletion of status of statusManager.\r\n\r\n- leave it to `statusManager.RemoveOrphanedStatuses`, orphan pod statuses will be cleaned up in housekeep sync \r\n- when pod is deleted in podManager, delete status from statusManager immediately\r\n\r\nThis pr has been updated to use method 1. I don't know if there will be other issues, please take a look. @jingxu97 @msau42 @dashpole "}}
{"actor":{"display_login":"cofyc"}}