Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
xenv opened this issue Sep 26, 2022 · 26 comments · May be fixed by #131283
Open

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

xenv opened this issue Sep 26, 2022 · 26 comments · May be fixed by #131283
Assignees
@answer1991 @yylt @akankshakumari393
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. sig/node Categorizes an issue or PR as relevant to SIG Node. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@xenv
Copy link

xenv commented Sep 26, 2022 •
edited
Loading

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)
@xenv xenv added the kind/bug Categorizes issue or PR as related to a bug. label Sep 26, 2022
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 26, 2022
@xenv xenv changed the title Node lifecycle controller does not markPodsNotReady when the node Ready state changes from fail to unknown Node lifecycle controller does not markPodsNotReady when the node Ready state changes from false to unknown Sep 26, 2022
@answer1991
Copy link
Contributor

/sig node

@k8s-ci-robot k8s-ci-robot added 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 Sep 26, 2022
@bobbypage
Copy link
Member

bobbypage commented Sep 26, 2022 •
edited
Loading

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

/triage accepted

@bobbypage bobbypage mentioned this issue Sep 26, 2022
Open
@akankshakumari393
Copy link
Member

/assign

@akankshakumari393
Copy link
Member

akankshakumari393 commented Sep 28, 2022 •
edited
Loading

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

@SergeyKanzhelev
Copy link
Member

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 28, 2022
@SergeyKanzhelev
Copy link
Member

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

Yes, I believe the change required is very localized.

@akankshakumari393
Copy link
Member

/good-first-issue

@k8s-ci-robot
Copy link
Contributor

@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.

@k8s-ci-robot k8s-ci-robot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Sep 29, 2022
@akankshakumari393 akankshakumari393 mentioned this issue Sep 29, 2022
Closed
@MustkimKhatik
Copy link

Hey, I would like to work on this

@akankshakumari393
Copy link
Member

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

@MustkimKhatik
Copy link

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

Oh alright

@answer1991
Copy link
Contributor

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

@xenv xenv mentioned this issue Oct 8, 2022
Closed
@jag678 jag678 mentioned this issue Nov 30, 2022
Closed
@jag678
Copy link

jag678 commented Nov 30, 2022 •
edited
Loading

/assign

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

@aojea
Copy link
Member

aojea commented Dec 2, 2022

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

@ashutosh887
Copy link

@aojea Does this need some work?

@lance5890
Copy link
Contributor

lance5890 commented May 6, 2023 •
edited
Loading

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

This was referenced May 17, 2023
Closed
Closed
@dsxing dsxing removed their assignment May 24, 2023
@dsxing
Copy link

dsxing commented May 24, 2023

/assign

@pradeeptosarkar pradeeptosarkar removed their assignment Oct 9, 2023
@zxh326 zxh326 mentioned this issue Oct 10, 2023
Closed
@utkarsh-singh1
Copy link

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 yylt mentioned this issue May 10, 2024
Open
@yylt
Copy link
Contributor

yylt commented May 10, 2024

/assign

@yylt
Copy link
Contributor

yylt commented May 10, 2024

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.

@tuibeovince
Copy link

Curious, I am trying to think why this was initially implemented this way.

Was it because it was assumed that if node's Ready being false is already expected to have pods at makePodsNotReady ? In effect, shifting from false to unknown didn't expect to make a new change in pod readiness?

@kannon92 kannon92 moved this to Triaged in SIG Node Bugs Jul 22, 2024
@dsxing dsxing removed their assignment Aug 6, 2024
@hitsub2
Copy link

hitsub2 commented Nov 21, 2024

for daemonset pod, it seems that the DaemonSet controller automatically adds a set of tolerations to DaemonSet Pods, and these tolerations can not be overwrite even the user has claimed in the daemonset yaml like this:

apiVersion: apps/v1
kind: DaemonSet
metadata:
  name: fluentd-elasticsearch
  namespace: kube-system
  labels:
    k8s-app: fluentd-logging
spec:
  selector:
    matchLabels:
      name: fluentd-elasticsearch
  template:
    metadata:
      labels:
        name: fluentd-elasticsearch
    spec:
      tolerations:
      # these tolerations are to have the daemonset runnable on control plane nodes
      # remove them if your control plane nodes should not run pods
      - key: node-role.kubernetes.io/control-plane
        operator: Exists
        effect: NoSchedule
      - key: node-role.kubernetes.io/master
        operator: Exists
        effect: NoSchedule
      - effect: NoExecute
        key: node.kubernetes.io/not-ready
        operator: Exists
        tolerationSeconds: 30
      - effect: NoExecute
        key: node.kubernetes.io/unreachable
        operator: Exists
        tolerationSeconds: 30

When check the pod, the tolerationSeconds: 30 is still missing. So the overwrite is not working.

@jzlyy
Copy link

jzlyy commented Mar 13, 2025

I’d like to work on this

@chiru0631
Copy link

Hey! I’m new to open-source
I’d love to work on this issue.
Can you assign it to me?

@utkarsh-singh1
Copy link

@jzlyy @chiru0631 you can take this issue both by work independently or work together.

@utkarsh-singh1
Copy link

utkarsh-singh1 commented Mar 23, 2025 •
edited
Loading

You can use /assign to take assign issue.

@chiru0631 chiru0631 linked a pull request Apr 14, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@answer1991 answer1991

@yylt yylt

@akankshakumari393 akankshakumari393

Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. sig/node Categorizes an issue or PR as relevant to SIG Node. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node Bugs
Status: Triaged
Milestone
No milestone