Node lifecycle controller does not markPodsNotReady when the node Ready state changes from false to unknown

[xenv]

What happened?

When kubelet loses connect, the node goes into the unknown state. The node lifecycle controller marks the pod as not ready by the markPodsNotReady function because the health check status of the pod can not be obtained through kubelet. This feature is available only when node's Ready state transitions from true to unknown.

However, if the node is already in the fail state (such as a containerd failure), markPodsNotReady will not take effect if the node loses its connection at this time.

kubernetes/pkg/controller/nodelifecycle/node_lifecycle_controller.go

Lines 883 to 888 in cac5388

	case currentReadyCondition.Status != v1.ConditionTrue && observedReadyCondition.Status == v1.ConditionTrue:
	// Report node event only once when status changed.
	controllerutil.RecordNodeStatusChange(nc.recorder, node, "NodeNotReady")
	fallthrough
	case needsRetry && observedReadyCondition.Status != v1.ConditionTrue:
	if err = controllerutil.MarkPodsNotReady(ctx, nc.kubeClient, nc.recorder, pods, node.Name); err != nil {

In this case, the pod may accidentally remain ready, which may cause some network traffic to be accidentally forwarded to this node.

What did you expect to happen?

As long as the node loses its connection beyond grace time, MarkPodsNotReady should always work

How can we reproduce it (as minimally and precisely as possible)?

1. Stop containerd and wait for the node Ready state to false
2. Stop kubelet or shutdown the node and wait the node Ready state to unknown
3. The pods which not be evicted on this node would be always ready

Anything else we need to know?

In the node lifecycle controller logic,MarkPodsNotReady is just triggered when a node goes from true state to an unknown state. The correct way is to trigger when the node becomes unknown state regardless of whether the node state was previously true

Kubernetes version

$ kubectl version
Server Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.15", GitCommit:"1d79bc3bcccfba7466c44cc2055d6e7442e140ea", GitTreeState:"clean", BuildDate:"2022-09-22T06:03:36Z", GoVersion:"go1.16.15", Compiler:"gc", Platform:"linux/amd64"}

Cloud provider

OS version

# On Linux:
$ cat /etc/os-release

$ uname -a
5.4.119-1-tlinux4-0008 #1 SMP Fri Nov 26 11:17:45 CST 2021 x86_64 x86_64 x86_64 GNU/Linux

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

[answer1991]

/sig node

[bobbypage]

Thanks for the report and the simple repro steps. This sounds like a generalized duplicate of #109998

/triage accepted

[akankshakumari393]

/assign

[akankshakumari393]

Is this a good-first-issue. I would like to work on it?

[SergeyKanzhelev]

/triage accepted

[SergeyKanzhelev]

Is this a good-first-issue. I would like to work on it?

Yes, I believe the change required is very localized.

[akankshakumari393]

/good-first-issue

[k8s-ci-robot]

@akankshakumari393:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

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.

[MustkimKhatik]

Hey, I would like to work on this

[akankshakumari393]

@MustkimKhatik i am already working on it, and have raised a PR as well.🙂

[MustkimKhatik]

@MustkimKhatik i am already working on it, and have raised a PR as well.🙂

Oh alright

[answer1991]

Is this a good-first-issue. I would like to work on it?

@akankshakumari393 Hi, I think node-controller has some complex logic, it's not suitable for a new contributor. I have reviewed your PR and found there are some broken UT. Please close your PR, @xenv and me were trying to fix this issue.

/assign

[maan19]

/assign

not sure if this has been fixed already. looks like it is not.

[aojea]

there are 3 PRs targeting this bug, please coordinate with the reviewers to avoid duplicating efforts

[ashutosh887]

@aojea Does this need some work?

[lance5890]

I have found this problem in node_lifycycle_controller : when node is changed from ready to not ready, the ds pod is still in ready status, and will not be removed in the ep

[dsxing]

/assign

[utkarsh-singh1]

This issue, is still under consideration for good-first-issue, as it is reported here ⇾

Hi, I think node-controller has some complex logic, it's not suitable for a new contributor. I have reviewed your PR and found there are some broken UT. Please close your PR, @xenv and me were trying to fix this issue.

[yylt]

/assign

[yylt]

Hi all.

Although I know there have been many commits and I am aware of the earliest issue, it has not been fixed yet. I am truly sorry for the inconvenience.

Therefore, I have made a new commit in an attempt to fix the issue.