manager#syncPod should cover reconciliation #89155

tedyu · 2020-03-16T18:47:58Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Please see related discussion (around issue #80968): #88255 (comment)

@yujuhong 's question led me to thinking about whether reconciliation may miss some case.

For the following code in manager#syncBatch :

				delete(m.apiStatusVersions, syncedUID)
				updatedStatuses = append(updatedStatuses, podStatusSyncRequest{uid, status})

Please note that syncedUID and uid may carry different values.
With this additional log,

@@ -517,33 +521,37 @@ func (m *manager) syncBatch() {
                                        continue
                                }
                                syncedUID = mirrorUID
+                               klog.Infof("assigned syncedUID %q uid %q", syncedUID, uid)
                        }

Here is one example:

I0710 02:47:35.039903    9058 config.go:383] Receiving a new pod "kube-proxy-e2e-9e17d10a0f-a7d53-minion-group-32vz_kube-system(de2df129-bfbd-4c48-90b1-06d49400dad0)"
I0710 02:47:35.040140    9058 kubelet.go:1817] SyncLoop (ADD, "api"): "kube-proxy-e2e-9e17d10a0f-a7d53-minion-group-32vz_kube-system(de2df129-bfbd-4c48-90b1-06d49400dad0)"
...
I0710 02:47:42.007051    9058 status_manager.go:524] assigned syncedUID "de2df129-bfbd-4c48-90b1-06d49400dad0" uid "b31ab15bb4ea18d79cf1cfa40fe53c82"

Please note that, in current code, if m.needsUpdate returns false, syncPod() would return early.
However, this may not work with the reconciliation code path (the UID used to check presence in apiStatusVersions in syncPod may be different from the one whose entry is cleared in syncBatch).

This PR allows manager#syncPod to perform reconciliation. Without this change, reconciliation may be skipped (when syncedUID and uid differ).

See #88255 (comment) from @byxorna

Note: another potential fix is to remove uid from apiStatusVersions in manager#syncBatch but this is not as robust as the current fix.

Which issue(s) this PR fixes:
Fixes #

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

tedyu · 2020-03-16T18:51:02Z

/cc @sjenning @rphillips

tedyu · 2020-03-16T20:14:45Z

/retest

tedyu · 2020-03-16T21:04:51Z

/priority important-soon

tedyu · 2020-03-16T23:05:04Z

The current formation is a little smarter than the first patch @byxorna put in his cluster.

The checkNeedsUpdate flag only needs to be false when m.needsReconcile() says so.

tedyu · 2020-03-16T23:47:10Z

/test pull-kubernetes-e2e-gce

derekwaynecarr · 2020-03-17T22:28:32Z

/hold

derekwaynecarr · 2020-03-17T22:30:17Z

@smarterclayton i know you are also looking in similar areas.

lets have a broader discussion in sig-node meeting on areas we are investigating on kubelet reliability. right now a number of the changes are hard to follow in a coherent fashion without a broader story.

derekwaynecarr · 2020-03-17T22:44:26Z

/assign

tedyu · 2020-03-21T13:51:52Z

/cc @Random-Liu

tedyu · 2021-06-07T22:02:30Z

/remove-lifecycle stale

ehashman

Please don't fight the bot on stale PRs.

There are no accompanying test changes for this PR so we can't be sure it actually addresses the issue. We also recently refactored the PLEG so this change may no longer be necessary. Without demonstrated impact I am inclined to close this PR.

k8s-ci-robot · 2021-08-04T18:42:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tedyu
To complete the pull request process, please ask for approval from derekwaynecarr after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/kubelet/OWNERS

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

tedyu · 2021-08-04T19:49:58Z

I would suggest giving this PR some time to see if the problem still occurs in latest release.

tedyu · 2021-08-04T23:49:20Z

Which old releases did the PLEG refactoring go to ?

What about the releases without the refactoring ? Shouldn't those releases be covered (by this fix)?

tedyu · 2021-08-05T03:42:51Z

I took a look at recent checkins to pkg/kubelet/status/status_manager.go

As far as I can tell, the logic touched by the PR remains unchanged.

@ehashman
Can you point me to the commits which you referred to earlier ?

k8s-triage-robot · 2021-11-03T03:59:35Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

tedyu · 2021-11-03T08:01:53Z

/remove-lifecycle stale

dims · 2022-01-14T13:51:54Z

this PR has been on hold for about 2 weeks. Can you please see how to unblock this by reviewing comments etc? or please close this out (Trying to declutter our open list of PRs)

k8s-triage-robot · 2022-04-14T14:31:43Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

CormickKneey · 2022-04-15T07:24:10Z

Please don't fight the bot on stale PRs.

There are no accompanying test changes for this PR so we can't be sure it actually addresses the issue. We also recently refactored the PLEG so this change may no longer be necessary. Without demonstrated impact I am inclined to close this PR.

Any proposal doc or reference link can be followed?

tedyu · 2022-04-15T11:02:06Z

/remove-lifecycle stale

k8s-triage-robot · 2022-07-14T11:43:53Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

tedyu · 2022-07-14T15:48:45Z

/remove-lifecycle stale

k8s-triage-robot · 2022-10-12T16:24:37Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-11-11T16:53:53Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-12-11T17:24:13Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-12-11T17:24:18Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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/test-infra repository.

tedyu mentioned this pull request Mar 16, 2020

Update pod status if last update is too old #88255

Closed

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 16, 2020

k8s-ci-robot requested review from rphillips, sjenning and yifan-gu March 16, 2020 18:51

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 16, 2020

tedyu force-pushed the sync-pod-reconcile branch from 6c1850e to dfb0e62 Compare March 16, 2020 22:42

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 17, 2020

k8s-ci-robot assigned derekwaynecarr Mar 17, 2020

tedyu mentioned this pull request Mar 19, 2020

Pod ready condition is not always updated after readiness gates condition changes #86485

Closed

tedyu force-pushed the sync-pod-reconcile branch 2 times, most recently from 7d975a9 to 3354ac5 Compare March 20, 2020 15:11

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 7, 2021

ehashman suggested changes Aug 4, 2021

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 3, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 3, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 14, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 15, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 14, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 14, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 12, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 11, 2022

k8s-ci-robot closed this Dec 11, 2022

SIG Node PR Triage automation moved this from Waiting on Author to Done Dec 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

manager#syncPod should cover reconciliation #89155

manager#syncPod should cover reconciliation #89155

tedyu commented Mar 16, 2020 •

edited

tedyu commented Mar 16, 2020

tedyu commented Mar 16, 2020

tedyu commented Mar 16, 2020

tedyu commented Mar 16, 2020

tedyu commented Mar 16, 2020

derekwaynecarr commented Mar 17, 2020

derekwaynecarr commented Mar 17, 2020

derekwaynecarr commented Mar 17, 2020

tedyu commented Mar 21, 2020

tedyu commented Jun 7, 2021

ehashman left a comment

k8s-ci-robot commented Aug 4, 2021

tedyu commented Aug 4, 2021

tedyu commented Aug 4, 2021

tedyu commented Aug 5, 2021

k8s-triage-robot commented Nov 3, 2021

tedyu commented Nov 3, 2021

dims commented Jan 14, 2022

k8s-triage-robot commented Apr 14, 2022

CormickKneey commented Apr 15, 2022

tedyu commented Apr 15, 2022

k8s-triage-robot commented Jul 14, 2022

tedyu commented Jul 14, 2022

k8s-triage-robot commented Oct 12, 2022

k8s-triage-robot commented Nov 11, 2022

k8s-triage-robot commented Dec 11, 2022

k8s-ci-robot commented Dec 11, 2022

manager#syncPod should cover reconciliation #89155

manager#syncPod should cover reconciliation #89155

Conversation

tedyu commented Mar 16, 2020 • edited

tedyu commented Mar 16, 2020

tedyu commented Mar 16, 2020

tedyu commented Mar 16, 2020

tedyu commented Mar 16, 2020

tedyu commented Mar 16, 2020

derekwaynecarr commented Mar 17, 2020

derekwaynecarr commented Mar 17, 2020

derekwaynecarr commented Mar 17, 2020

tedyu commented Mar 21, 2020

tedyu commented Jun 7, 2021

ehashman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 4, 2021

tedyu commented Aug 4, 2021

tedyu commented Aug 4, 2021

tedyu commented Aug 5, 2021

k8s-triage-robot commented Nov 3, 2021

tedyu commented Nov 3, 2021

dims commented Jan 14, 2022

k8s-triage-robot commented Apr 14, 2022

CormickKneey commented Apr 15, 2022

tedyu commented Apr 15, 2022

k8s-triage-robot commented Jul 14, 2022

tedyu commented Jul 14, 2022

k8s-triage-robot commented Oct 12, 2022

k8s-triage-robot commented Nov 11, 2022

k8s-triage-robot commented Dec 11, 2022

k8s-ci-robot commented Dec 11, 2022

tedyu commented Mar 16, 2020 •

edited