[stable/node-problem-detector] adds custom-plugin-monitor config #10549

Hashfyre · 2019-01-11T06:30:33Z

What this PR does / why we need it:

Adds --custom-plugin-monitors flag to the node-problem-detector daemonset run command.
Adds settings.custom_plugin_monitors so custom-monitors can be run with the correct flag.

Which issue this PR fixes

fixes [stable/node-problem-detector] missing --custom-plugin-monitors flag #10430

Special notes for your reviewer:

Does not fix the crashloopbackoff of NPD, but just adds the ability to add any monitor-config to be run under --custom-plugin-monitors

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md

Hashfyre · 2019-01-11T06:49:30Z

cc @max-rocket-internet Could you kindly review this PR. I believe I checklisted everything that needs to be done as a first time contributor. Let me know if I'm missing something.

max-rocket-internet · 2019-01-11T09:06:27Z

stable/node-problem-detector/README.md

@@ -31,6 +31,7 @@ $ helm delete my-release
 ## Configuration

 Custom System log monitor config files can be created, see [here](https://github.com/kubernetes/node-problem-detector/tree/master/config) for examples.
+Custom Plugin monitor config files have to be passed under `settings.custom_plugin_monitors`


Can you remove this line and put it in the table of parameters below?

max-rocket-internet · 2019-01-11T09:08:29Z

stable/node-problem-detector/templates/daemonset.yaml

@@ -31,7 +31,7 @@ spec:
          command:
            - "/bin/sh"
            - "-c"
-            - "exec /node-problem-detector --logtostderr --system-log-monitors={{- range $index, $monitor := .Values.settings.log_monitors }}{{if ne $index 0}},{{end}}{{ $monitor }}{{- end }}"
+            - "exec /node-problem-detector --logtostderr --system-log-monitors={{- range $index, $monitor := .Values.settings.log_monitors }}{{if ne $index 0}},{{end}}{{ $monitor }}{{- end }} --custom-plugin-monitors={{- range $index, $monitor := .Values.settings.custom_plugin_monitors }}{{if ne $index 0}},{{end}}{{ $monitor }}{{- end }}"


How is this going to work by default when settings. custom_plugin_monitors is just an empty map? Won't npd be run with the --custom-plugin-monitors= argument but with no value?

I have tried fixing this with:

{- if .Values.settings.custom_plugin_monitors }} ... {{- end}}

Please let me know if that is adequate.

@Hashfyre please avoid force pushing as it prevents us seeing the changes you make after the review. Can you also address the other comment I made about the README?

Yes, fixed that. Will keep the no force-push policy in mind.

stable/node-problem-detector/values.yaml

max-rocket-internet · 2019-01-14T17:03:25Z

@Hashfyre you still haven't fixed the comment about the README.

Again, please don't squash and force push as it means we can't easily see what's changing. This is covered in the guide here 🙂

max-rocket-internet · 2019-01-15T16:41:50Z

stable/node-problem-detector/README.md

+| `rbac.create`                     | RBAC                                       | `true`                                                       |
+| `resources`                       | Pod resource requests and limits           | `{}`                                                         |
+| `settings.log_monitors`           | System log monitor config files            | `/config/kernel-monitor.json`, `/config/docker-monitor.json` |
+| `settings.custom_plugin_monitors` | Custom plugin monitor config files         | `/config/kernel-monitor.json`, `/config/docker-monitor.json` |


I think you've copy and pasted this from above but not changed the default value. It should be []

Signed-off-by: Joy Bhattacherjee <[email protected]>

…lue for custompluginmonitor Signed-off-by: Joy Bhattacherjee <[email protected]>

Signed-off-by: Joy Bhattacherjee <[email protected]>

max-rocket-internet · 2019-01-15T16:56:44Z

/lgtm

k8s-ci-robot · 2019-01-15T16:57:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Hashfyre, max-rocket-internet

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:

~~stable/node-problem-detector/OWNERS~~ [max-rocket-internet]

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

…m#10549) * [stable/node-problem-detector] adds --custom-plugin-monitors flag Signed-off-by: Joy Bhattacherjee <[email protected]> * [stable/node-problem-detector] adds documentation custom_plugin_monitor Signed-off-by: Joy Bhattacherjee <[email protected]> * [stable/node-problem-detector] updates readme with correct default value for custompluginmonitor Signed-off-by: Joy Bhattacherjee <[email protected]> * [stable/node-problem-detector] updates chart version to 1.1.3 Signed-off-by: Joy Bhattacherjee <[email protected]>

helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 11, 2019

k8s-ci-robot requested a review from unguiculus January 11, 2019 06:30

Hashfyre force-pushed the feature/node-problem-detector/add-custom-plugin-monitor branch from f2b8adf to 683bd50 Compare January 11, 2019 06:33

helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 11, 2019

Hashfyre force-pushed the feature/node-problem-detector/add-custom-plugin-monitor branch from 4edd27c to 0b141d0 Compare January 11, 2019 06:38

helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 11, 2019

Hashfyre force-pushed the feature/node-problem-detector/add-custom-plugin-monitor branch from 0b141d0 to 26eb44f Compare January 11, 2019 06:45

helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 11, 2019

max-rocket-internet suggested changes Jan 11, 2019

View reviewed changes

Hashfyre force-pushed the feature/node-problem-detector/add-custom-plugin-monitor branch from 26eb44f to 4ea785b Compare January 14, 2019 12:59

helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 14, 2019

max-rocket-internet suggested changes Jan 14, 2019

View reviewed changes

stable/node-problem-detector/values.yaml Outdated Show resolved Hide resolved

Hashfyre force-pushed the feature/node-problem-detector/add-custom-plugin-monitor branch from 4ea785b to 10fcd30 Compare January 14, 2019 14:35

helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 14, 2019

Hashfyre force-pushed the feature/node-problem-detector/add-custom-plugin-monitor branch from 10fcd30 to 44a8be4 Compare January 15, 2019 05:54

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 15, 2019

max-rocket-internet suggested changes Jan 15, 2019

View reviewed changes

Joy Bhattacherjee added 2 commits January 15, 2019 22:19

[stable/node-problem-detector] adds --custom-plugin-monitors flag

60568b6

Signed-off-by: Joy Bhattacherjee <[email protected]>

[stable/node-problem-detector] adds documentation custom_plugin_monitor

4f3bbdb

Signed-off-by: Joy Bhattacherjee <[email protected]>

Hashfyre force-pushed the feature/node-problem-detector/add-custom-plugin-monitor branch from 44a8be4 to 4f3bbdb Compare January 15, 2019 16:49

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Jan 15, 2019

Joy Bhattacherjee added 2 commits January 15, 2019 22:23

[stable/node-problem-detector] updates readme with correct default va…

f57555b

…lue for custompluginmonitor Signed-off-by: Joy Bhattacherjee <[email protected]>

[stable/node-problem-detector] updates chart version to 1.1.3

b8eba38

Signed-off-by: Joy Bhattacherjee <[email protected]>

Hashfyre force-pushed the feature/node-problem-detector/add-custom-plugin-monitor branch from fbd1cda to b8eba38 Compare January 15, 2019 16:53

helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 15, 2019

k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 15, 2019

max-rocket-internet approved these changes Jan 15, 2019

View reviewed changes

k8s-ci-robot merged commit b3ef3a0 into helm:master Jan 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stable/node-problem-detector] adds custom-plugin-monitor config #10549

[stable/node-problem-detector] adds custom-plugin-monitor config #10549

Hashfyre commented Jan 11, 2019 •

edited

Loading

Hashfyre commented Jan 11, 2019

max-rocket-internet Jan 11, 2019

Hashfyre Jan 15, 2019

max-rocket-internet Jan 11, 2019

Hashfyre Jan 14, 2019

max-rocket-internet Jan 14, 2019

Hashfyre Jan 15, 2019

max-rocket-internet commented Jan 14, 2019

max-rocket-internet Jan 15, 2019

max-rocket-internet commented Jan 15, 2019

k8s-ci-robot commented Jan 15, 2019

[stable/node-problem-detector] adds custom-plugin-monitor config #10549

[stable/node-problem-detector] adds custom-plugin-monitor config #10549

Conversation

Hashfyre commented Jan 11, 2019 • edited Loading

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

Hashfyre commented Jan 11, 2019

max-rocket-internet Jan 11, 2019

Choose a reason for hiding this comment

Hashfyre Jan 15, 2019

Choose a reason for hiding this comment

max-rocket-internet Jan 11, 2019

Choose a reason for hiding this comment

Hashfyre Jan 14, 2019

Choose a reason for hiding this comment

max-rocket-internet Jan 14, 2019

Choose a reason for hiding this comment

Hashfyre Jan 15, 2019

Choose a reason for hiding this comment

max-rocket-internet commented Jan 14, 2019

max-rocket-internet Jan 15, 2019

Choose a reason for hiding this comment

max-rocket-internet commented Jan 15, 2019

k8s-ci-robot commented Jan 15, 2019

Hashfyre commented Jan 11, 2019 •

edited

Loading