[addons] Introduce NodeProblemDetector #11381

dntosas · 2021-05-04T13:56:09Z

Node Problem Detector aims to make various node problems visible to
the upstream layers in the cluster management stack. It is a daemon
that runs on each node, detects node problems and reports them to apiserver
so to avoid scheduling new pods on bad nodes and also easily identify
which are the problems on underlying nodes.

Project Home: https://github.com/kubernetes/node-problem-detector

Signed-off-by: dntosas ntosas@gmail.com

k8s-ci-robot · 2021-05-04T13:56:17Z

Hi @dntosas. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

olemarkus · 2021-05-07T07:08:55Z

/ok-to-test

dntosas · 2021-05-22T19:55:02Z

/assign @mikesplain

dntosas · 2021-05-22T19:55:41Z

/assign @olemarkus

docs/addons.md

upup/pkg/fi/cloudup/bootstrapchannelbuilder/bootstrapchannelbuilder.go

hakman

A few more questions if you don't mind:

Why not also add the limits together with the requests?
Why not use the tolerations from the original manifest:

        - effect: NoSchedule
          operator: Exists
        - effect: NoExecute
          operator: Exists

upup/models/cloudup/resources/addons/node-problem-detector.addons.k8s.io/k8s-1.17.yaml.template

dntosas · 2021-06-10T10:37:35Z

A few more questions if you don't mind:

Why not also add the limits together with the requests?

Why not use the tolerations from the original manifest:
        - effect: NoSchedule
          operator: Exists
        - effect: NoExecute
          operator: Exists

I thought it was like a "common practice" for Kops addons to avoid limits and unnecessary OOMkills and user friction. I can add them ofc if you believe they are needed in here. WDYT? ^^
Done

olemarkus · 2021-06-10T10:53:11Z

We a) avoid setting any default limits. b) don't add options for setting limits where limits can cause harm. Such as CNIs OOMing over a bad limit. For addons such as this, I think exposing limits is fine.

hakman · 2021-06-10T16:01:07Z

pkg/apis/kops/componentconfig.go

+	// Default: false
+	Enabled *bool `json:"enabled,omitempty"`
+	// Image is the NodeProblemDetector docker container used
+	// Default: k8s.gcr.io/node-problem-detector/node-problem-detector:v0.8.8


I think we can remove this line. Seems too much effort to maintain it.

Suggested change

// Default: k8s.gcr.io/node-problem-detector/node-problem-detector:v0.8.8

I still see the line here and in a few other places.

upup/pkg/fi/cloudup/bootstrapchannelbuilder/bootstrapchannelbuilder.go

hakman · 2021-06-16T15:14:34Z

@dntosas Any chance you have time to address the remaining nits?

dntosas · 2021-06-16T15:30:32Z

@dntosas Any chance you have time to address the remaining nits?

y sure, pushed ^^ adding the limits also

hakman

Let's keep the limits same as in the source template, unless there is a reason for them to differ.

hakman · 2021-06-16T16:24:02Z

pkg/model/components/nodeproblemdetector.go

+	}
+
+	if npd.MemoryRequest == nil {
+		defaultMemoryRequest := resource.MustParse("32Mi")


https://github.com/kubernetes/node-problem-detector/blob/v0.8.8/deployment/node-problem-detector.yaml#L33-L39

Suggested change

defaultMemoryRequest := resource.MustParse("32Mi")

defaultMemoryRequest := resource.MustParse("80Mi")

hakman · 2021-06-16T16:24:17Z

pkg/model/components/nodeproblemdetector.go

+	}
+
+	if npd.CPULimit == nil {
+		defaultCPULimit := resource.MustParse("100m")


https://github.com/kubernetes/node-problem-detector/blob/v0.8.8/deployment/node-problem-detector.yaml#L33-L39

Suggested change

defaultCPULimit := resource.MustParse("100m")

defaultCPULimit := resource.MustParse("10m")

hakman · 2021-06-16T16:24:32Z

pkg/model/components/nodeproblemdetector.go

+	}
+
+	if npd.MemoryLimit == nil {
+		defaultMemoryLimit := resource.MustParse("128Mi")


https://github.com/kubernetes/node-problem-detector/blob/v0.8.8/deployment/node-problem-detector.yaml#L33-L39

Suggested change

defaultMemoryLimit := resource.MustParse("128Mi")

defaultMemoryLimit := resource.MustParse("80Mi")

hakman · 2021-06-16T16:28:35Z

pkg/apis/kops/componentconfig.go

+	// Default: false
+	Enabled *bool `json:"enabled,omitempty"`
+	// Image is the NodeProblemDetector docker container used
+	// Default: k8s.gcr.io/node-problem-detector/node-problem-detector:v0.8.8


I still see the line here and in a few other places.

Node Problem Detector aims to make various node problems visible to the upstream layers in the cluster management stack. It is a daemon that runs on each node, detects node problems and reports them to apiserver so to avoid scheduling new pods on bad nodes and also easily identify which are the problems on underlying nodes. Project Home: https://github.com/kubernetes/node-problem-detector Signed-off-by: dntosas <ntosas@gmail.com>

hakman

Thanks @dntosas!
👍

k8s-ci-robot · 2021-06-16T19:45:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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 files:

~~OWNERS~~ [hakman]

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

fejta-bot · 2021-06-16T21:47:30Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-06-17T00:14:06Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-06-17T03:23:07Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

johngmyers · 2021-06-17T05:20:43Z

/retest

johngmyers · 2021-06-17T06:10:55Z

/retest

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 4, 2021

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 4, 2021

k8s-ci-robot requested review from KashifSaadat and olemarkus May 4, 2021 13:56

k8s-ci-robot added area/addons area/api labels May 4, 2021

dntosas force-pushed the addons-add-npd branch 2 times, most recently from 2ec87e3 to 0a8268c Compare May 4, 2021 14:01

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2021

dntosas force-pushed the addons-add-npd branch 2 times, most recently from 92ec890 to 73899ee Compare May 4, 2021 14:23

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 7, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2021

dntosas force-pushed the addons-add-npd branch from 73899ee to c766fb9 Compare May 22, 2021 19:36

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2021

dntosas force-pushed the addons-add-npd branch from c766fb9 to e3bb75a Compare May 22, 2021 19:44

k8s-ci-robot added the area/documentation label May 22, 2021

k8s-ci-robot assigned mikesplain May 22, 2021

k8s-ci-robot assigned olemarkus May 22, 2021

johngmyers reviewed May 22, 2021

View reviewed changes

docs/addons.md Outdated Show resolved Hide resolved

johngmyers requested changes May 22, 2021

View reviewed changes

docs/addons.md Outdated Show resolved Hide resolved

upup/pkg/fi/cloudup/bootstrapchannelbuilder/bootstrapchannelbuilder.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned johngmyers May 22, 2021

dntosas force-pushed the addons-add-npd branch from e3bb75a to 55e55d7 Compare May 23, 2021 09:43

dntosas requested a review from hakman June 3, 2021 20:57

hakman requested changes Jun 10, 2021

View reviewed changes

upup/models/cloudup/resources/addons/node-problem-detector.addons.k8s.io/k8s-1.17.yaml.template Outdated Show resolved Hide resolved

k8s-ci-robot assigned hakman Jun 10, 2021

dntosas force-pushed the addons-add-npd branch from 8c2cfb2 to 9b7a60f Compare June 10, 2021 10:35

hakman reviewed Jun 10, 2021

View reviewed changes

upup/pkg/fi/cloudup/bootstrapchannelbuilder/bootstrapchannelbuilder.go Outdated Show resolved Hide resolved

dntosas force-pushed the addons-add-npd branch from 9b7a60f to 50acd39 Compare June 16, 2021 15:29

dntosas force-pushed the addons-add-npd branch 2 times, most recently from aec26c6 to 340a5eb Compare June 16, 2021 15:55

dntosas requested a review from hakman June 16, 2021 15:56

hakman requested changes Jun 16, 2021

View reviewed changes

dntosas force-pushed the addons-add-npd branch from 340a5eb to 20124d3 Compare June 16, 2021 18:00

hakman approved these changes Jun 16, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2021

k8s-ci-robot merged commit 559b57e into kubernetes:master Jun 17, 2021

k8s-ci-robot added this to the v1.22 milestone Jun 17, 2021

dntosas mentioned this pull request Oct 7, 2022

REQUEST: New membership for dntosas kubernetes/org#3753

Closed

9 tasks

hakman mentioned this pull request Mar 6, 2024

Improve release process kubernetes/node-problem-detector#874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[addons] Introduce NodeProblemDetector #11381

[addons] Introduce NodeProblemDetector #11381

dntosas commented May 4, 2021 •

edited

Loading

k8s-ci-robot commented May 4, 2021

olemarkus commented May 7, 2021

dntosas commented May 22, 2021

dntosas commented May 22, 2021

hakman left a comment

dntosas commented Jun 10, 2021

olemarkus commented Jun 10, 2021

hakman Jun 10, 2021

hakman Jun 16, 2021

hakman commented Jun 16, 2021

dntosas commented Jun 16, 2021

hakman left a comment

hakman Jun 16, 2021

hakman Jun 16, 2021

hakman Jun 16, 2021

hakman Jun 16, 2021

hakman left a comment

k8s-ci-robot commented Jun 16, 2021

fejta-bot commented Jun 16, 2021

fejta-bot commented Jun 17, 2021

fejta-bot commented Jun 17, 2021

johngmyers commented Jun 17, 2021

johngmyers commented Jun 17, 2021

	defaultMemoryRequest := resource.MustParse("32Mi")
	defaultMemoryRequest := resource.MustParse("80Mi")

	defaultCPULimit := resource.MustParse("100m")
	defaultCPULimit := resource.MustParse("10m")

	defaultMemoryLimit := resource.MustParse("128Mi")
	defaultMemoryLimit := resource.MustParse("80Mi")

[addons] Introduce NodeProblemDetector #11381

[addons] Introduce NodeProblemDetector #11381

Conversation

dntosas commented May 4, 2021 • edited Loading

k8s-ci-robot commented May 4, 2021

olemarkus commented May 7, 2021

dntosas commented May 22, 2021

dntosas commented May 22, 2021

hakman left a comment

Choose a reason for hiding this comment

dntosas commented Jun 10, 2021

olemarkus commented Jun 10, 2021

hakman Jun 10, 2021

Choose a reason for hiding this comment

hakman Jun 16, 2021

Choose a reason for hiding this comment

hakman commented Jun 16, 2021

dntosas commented Jun 16, 2021

hakman left a comment

Choose a reason for hiding this comment

hakman Jun 16, 2021

Choose a reason for hiding this comment

hakman Jun 16, 2021

Choose a reason for hiding this comment

hakman Jun 16, 2021

Choose a reason for hiding this comment

hakman Jun 16, 2021

Choose a reason for hiding this comment

hakman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 16, 2021

fejta-bot commented Jun 16, 2021

fejta-bot commented Jun 17, 2021

fejta-bot commented Jun 17, 2021

johngmyers commented Jun 17, 2021

johngmyers commented Jun 17, 2021

dntosas commented May 4, 2021 •

edited

Loading