add support for running standalone #49

andyxning · 2016-12-14T14:51:44Z

Add support for running node-problem-detector standalone.

This PR adds two command line options

in-cluster-config configures whether we deploy NPD in cluster as a daemon set or on a bare metal standalone. Default to be true, i.e., running in cluster as a daemon set.
apiserver-addr is used to specify the apiserver address when in-cluster-config is set to false.

Finally, we can run NPD in standalone like this

node-problem-detector -in-cluster-config=false -apiserver-addr=APISERVER_ADDRESS -kernel-monitor=KERNEL_MONITOR_CONFIG_FILE

This change is

andyxning · 2016-12-14T15:00:10Z

@Random-Liu PTAL.

Random-Liu · 2016-12-15T18:58:18Z

@andyxning Thanks for the change! We also planned to provide a systemd service version of NPD, this would be quite useful!

Will review today.

Random-Liu · 2016-12-15T19:32:01Z

@andyxning I've got another idea. Can we import and reuse the heapster code https://github.com/kubernetes/heapster/blob/96d9dbf9ee20c200bb7703a8dff0b5ea7c1d1ba1/common/kubernetes/configs.go?

In this way, we can avoid writing the code twice, and keep the ux the same with heapster. :)

And fix #27 and #21 at the same time.

andyxning · 2016-12-16T02:24:41Z

@Random-Liu Yep. Indeed i just read the code in config.go and simply it. :)
I will refactor the code like to reuse package in heapster.

BTW, do you think that we need to port all the supporting parameters that heapster supports.

Random-Liu · 2016-12-16T02:41:17Z

@andyxning Yeah, I think we have the same requirement here. https://github.com/kubernetes/heapster/blob/96d9dbf9ee20c200bb7703a8dff0b5ea7c1d1ba1/docs/source-configuration.md

We may want to add a flag like:

--apiserver=http://APISERVER_IP:APISERVER_PORT?inClusterConfig=false&useServiceAccount=true&auth=

andyxning · 2016-12-16T03:44:35Z

@Random-Liu OK. Will refactor code ASAP.

Random-Liu · 2016-12-16T19:40:34Z

README.md

-detecting node problems and reporting them to apiserver. Now it is running as
+layers in cluster management stack.
+
+`node-problem-detector` can either run as a [DaemonSet](http://kubernetes.io/docs/admin/daemons/) or run standalone on bare metals.


s/node-problem-detector/it
or
s/node-problem-detector/node-problem-detector

Random-Liu · 2016-12-16T19:42:06Z

README.md

+
+`node-problem-detector` can either run as a [DaemonSet](http://kubernetes.io/docs/admin/daemons/) or run standalone on bare metals.
+
+`node-problem-detector` detects node problems and reporting them to apiserver.


Move this above, following "layers in cluster management stack".

"It is a daemon runs on each node, detects node problems and reports them to apiserver."

Random-Liu · 2016-12-16T19:47:20Z

README.md

+
+`node-problem-detector` detects node problems and reporting them to apiserver.
+
+Now it is running as


Extra new line.

Random-Liu · 2016-12-16T19:54:02Z

README.md

@@ -68,6 +79,9 @@ spec:
    spec:
      containers:
      - name: node-problem-detector
+        command:
+          - "/node-problem-detector"
+          - "-apiserver=http://APISERVER_IP:APISERVER_PORT?inClusterConfig=true&apiVersion=v1"


We should be able to use the default command.

Random-Liu · 2016-12-16T19:55:09Z

node_problem_detector.go

 )

+func validateCmdParams() {
+	if len(*apiServer) == 0 {


If apiserver-override is not set, we could just pass an empty url to heapster util, and it will return the default in cluster configuration.

Random-Liu · 2016-12-16T19:56:25Z

pkg/problemclient/problem_client.go

-	cfg, err := restclient.InClusterConfig()
+
+	// error is ignored because we have checked it after command line is parsed.:)
+	uri, _ := url.Parse(apiServer)


Because the function is called NewClientOrDie, I'm fine to panic or fatal here. :)

@Random-Liu Just as the comment said, the error is ignored. Because the apiServer has been validated after command line argument is parsed in node_problem_detector.go's validateCmdParams function. It will fatal in there if the format of apiServer URI is invalid.

This will it fail faster if some precondition is not meet.

Random-Liu · 2016-12-16T19:56:59Z

node_problem_detector.go

 )

 var (
 	kernelMonitorConfigPath = flag.String("kernel-monitor", "/config/kernel_monitor.json", "The path to the kernel monitor config file")
+	apiServer = flag.String("apiserver", "", "URI used to connect to Kubernetes ApiServer")


rename to apiserver-override.

Random-Liu · 2016-12-16T20:09:05Z

README.md

 a [Kubernetes Addon](https://github.com/kubernetes/kubernetes/tree/master/cluster/addons)
 enabled by default in the GCE cluster.

+# Command Line Arguments
+* `apiserver`  


How about we rename this to -apiserver-override?

Random-Liu · 2016-12-16T20:10:04Z

README.md

 a [Kubernetes Addon](https://github.com/kubernetes/kubernetes/tree/master/cluster/addons)
 enabled by default in the GCE cluster.

+# Command Line Arguments
+* `apiserver`  
+`apiserver` command line argument can customize how to generate a Kubernetes ApiServer client according to `inClusterConfig` URI parameter.


We should move this into Usage. Probably add a "Override Apiserver Client Configuration" section

Random-Liu · 2016-12-16T20:13:20Z

README.md

@@ -92,6 +106,9 @@ spec:
 * If needed, you can use [ConfigMap](http://kubernetes.io/docs/user-guide/configmap/)
 to overwrite the `config/`.

+## Start Standalone
+`node-problem-detector -apiserver=http://APISERVER_IP:APISERVER_PORT?inClusterConfig=false&apiVersion=v1`


Add a bit more explanation here.

To run node problem detector in standalone with insecure connection to apiserver:

node-problem-detector -apiserver=http://APISERVER_IP:APISERVER_INSECURE_PORT?inClusterConfig=false

And it would be better to add another example, possibly using serviceAccount or kubeconfig.

done.
@Random-Liu Examples about using serviceAccount and kubeconfig is out of mine knowledge now. :(.

Maybe others can do this. :)

Random-Liu · 2016-12-16T20:14:55Z

@andyxning Some changes are needed.
And it seems that this PR has some problem with Godep. Could you fix the Godep and update Godep in a separate commit?

If you encountered any problem, feel free to tell me. :)

andyxning · 2016-12-17T09:05:56Z

@Random-Liu I will make a separate PR for updating dependencies.

Random-Liu · 2017-01-05T19:27:59Z

node_problem_detector.go

 )

+func validateCmdParams() {
+	if _, err := url.Parse(*apiServerOverride); err != nil {
+		glog.Fatalf("apiserver-override %q is not a valid HTTP URI. %s", *apiServerOverride, err)


nit: "apiserver-override %q is not a valid HTTP URI: %v".

Random-Liu · 2017-01-05T19:31:06Z

node_problem_detector.go

@@ -21,15 +21,26 @@ import (

 	"k8s.io/node-problem-detector/pkg/kernelmonitor"
 	"k8s.io/node-problem-detector/pkg/problemdetector"
+	"github.com/golang/glog"


nit:

import ( "flag" "net/url" "k8s.io/node-problem-detector/pkg/kernelmonitor" "k8s.io/node-problem-detector/pkg/problemdetector" "github.com/golang/glog" )

done. Lost of gofmt. :)

Random-Liu · 2017-01-05T19:31:28Z

pkg/problemclient/problem_client.go

 	client "k8s.io/kubernetes/pkg/client/unversioned"
 	"k8s.io/kubernetes/pkg/types"
+	"k8s.io/kubernetes/pkg/util/clock"
+	"k8s.io/heapster/common/kubernetes"


ditto. Group same type of packages together.

Random-Liu · 2017-01-05T19:36:52Z

README.md

+It is a daemon runs on each node, detects node
+problems and reports them to apiserver.
+node-problem-detector can either run as a
+[DaemonSet](http://kubernetes.io/docs/admin/daemons/) or run standalone on bare metals.


Not necessarily on bare metals, right? We can still run it standalone on VM. :)

Yep. Added.

In fact, I mean removing "bare metals".

Random-Liu · 2017-01-05T19:38:58Z

README.md

@@ -48,6 +52,14 @@ List of supported problem daemons:
 | [KernelMonitor](https://github.com/kubernetes/node-problem-detector/tree/master/pkg/kernelmonitor) | KernelDeadlock | A problem daemon monitors kernel log and reports problem according to predefined rules. |

 # Usage
+## Override Apiserver Client Configuration
+* `-apiserver-override`  


Add a colon here, and do not add new line.

Random-Liu · 2017-01-05T19:41:14Z

README.md

@@ -48,6 +52,14 @@ List of supported problem daemons:
 | [KernelMonitor](https://github.com/kubernetes/node-problem-detector/tree/master/pkg/kernelmonitor) | KernelDeadlock | A problem daemon monitors kernel log and reports problem according to predefined rules. |

 # Usage
+## Override Apiserver Client Configuration


Change the section name to Flags, in the future we may add more flags. :)

Random-Liu · 2017-01-05T19:47:42Z

README.md

@@ -48,6 +52,14 @@ List of supported problem daemons:
 | [KernelMonitor](https://github.com/kubernetes/node-problem-detector/tree/master/pkg/kernelmonitor) | KernelDeadlock | A problem daemon monitors kernel log and reports problem according to predefined rules. |

 # Usage
+## Override Apiserver Client Configuration
+* `-apiserver-override`  
+`apiserver-override` command line argument can customize how to generate a Kubernetes ApiServer


Rephrase the description a bit.

An URI parameter used to customize how node-problem-detector connects the apiserver. The format is the same with the source flag of Heapster:

http://APISERVER_IP:APISERVER_PORT?inClusterConfig=false&userServiceAccount=false&auth=&insecure=

BTW, I add a link to source and Heapster.

Random-Liu · 2017-01-05T19:52:01Z

README.md

@@ -92,6 +104,10 @@ spec:
 * If needed, you can use [ConfigMap](http://kubernetes.io/docs/user-guide/configmap/)
 to overwrite the `config/`.

+## Start Standalone
+`inClusterConfig` should be set to `false`. To run node-problem-detector standalone with an insecure apiserver connection: 
+`node-problem-detector -apiserver-override=http://APISERVER_IP:APISERVER_PORT?inClusterConfig=false`


s/APISERVER_PORT/APISERVER_INSECURE_PORT

To run node-problem-detector standalone, you should set inClusterConfig to false and teach node-problem-detector how to access apiserver with apiserver-override.

To run node-problem-detector standalone with an insecure apiserver connection:

node-problem-detector -apiserver-override=http://APISERVER_IP:APISERVER_PORT?inClusterConfig=false

For more scenarios, see here

shyamjvs · 2017-01-05T23:25:26Z

@Random-Liu @andyxning I guess it would be helpful if we could also have the ServiceAccountFile overridable. As often in some images (like debian:jessie), we have the defaultServiceAccountFile on a read-only filesystem and hence overwriting the file is not possible when npd has to run in the stand-alone mode. One example where this would be useful is in kubemark where the node-problem-detector runs as a container inside the hollow-node pod. (Ref: https://github.com/kubernetes/kubernetes/blob/master/test/kubemark/resources/hollow-node_template.json#L147)

Random-Liu · 2017-01-06T00:24:34Z

Offline discussed with @shyamjvs, we could use either a service account file or the kube config file to do the authentication.

shyamjvs · 2017-01-06T00:29:23Z

Sorry about the confusion, my bad. Looks like what I suggested won't be needed as the npd runs in a separate container on which the path to the service account file is (of course) writable.
Was confusing this with the option of morphing the npd binary into the kubemark binary (just like the hollow-kubelet and hollow-proxy) instead of running separately.
Thanks for the clarification @Random-Liu :)

gmarek · 2017-01-06T18:22:34Z

@andyxning - do you think you can get it done soon-ish?

andyxning · 2017-01-07T02:22:53Z

@Random-Liu @gmarek All are done. PTAL.

Random-Liu

LGTM with final nits.

Random-Liu · 2017-01-07T08:29:38Z

README.md

+It is a daemon runs on each node, detects node
+problems and reports them to apiserver.
+node-problem-detector can either run as a
+[DaemonSet](http://kubernetes.io/docs/admin/daemons/) or run standalone on bare metals or VMs.


In fact, I mean to remove "on bare metals or VMs".

Misunderstanding. :)

Random-Liu · 2017-01-07T08:30:27Z

README.md

+connects the apiserver. The format is the same with the
+[`source`](https://github.com/kubernetes/heapster/blob/master/docs/source-configuration.md#kubernetes)
+flag of [Heapster](https://github.com/kubernetes/heapster):
+`http://APISERVER_IP:APISERVER_PORT?inClusterConfig=false&userServiceAccount=false&auth=&insecure=`.


Use "```" instead of "`".
See the difference:

http://APISERVER_IP:APISERVER_PORT?inClusterConfig=false&userServiceAccount=false&auth=&insecure=

and
http://APISERVER_IP:APISERVER_PORT?inClusterConfig=false&userServiceAccount=false&auth=&insecure=

Random-Liu · 2017-01-07T08:32:03Z

README.md

+teach node-problem-detector how to access apiserver with `apiserver-override`.
+
+To run node-problem-detector standalone with an insecure apiserver connection:
+`node-problem-detector -apiserver-override=http://APISERVER_IP:APISERVER_INSECURE_PORT?inClusterConfig=false`


andyxning · 2017-01-07T15:43:11Z

@Random-Liu PTAL.

Random-Liu · 2017-01-09T18:49:34Z

@andyxning LGTM. I'll manually verify this PR before merging it. :)

Random-Liu · 2017-01-10T07:35:11Z

@andyxning I've verified this PR. It works well. Thanks for the patch! :)

shyamjvs · 2017-01-10T14:06:09Z

@Random-Liu Seems like you haven't uploaded the new image to gcr.io/google_containers/node-problem-detector:v0.2. Can we do it now, and do we plan to bump the version up to v0.3?

gmarek · 2017-01-10T14:10:46Z

Yup, we should do it

shyamjvs · 2017-01-10T16:14:21Z

Done. Pushed to gcr.io/google_containers/node-problem-detector:v0.3.

However there are a couple of issues in the npd repo regarding this version update (I'll file an issue regarding this):

Changing version tag inside the Makefile.
Changing version tag inside the node-problem-detector.yaml.
Updating the Readme to indicate that v0.3 is the latest version and suggested for use?

@Random-Liu I hope this push doesn't disturb any plans you might be having for npd release schedule. If yes, it's not too late or difficult to revert this change. We just did it for use in kubemark.

andyxning · 2017-01-11T00:56:47Z

@gmarek @shyamjvs Maybe we should make a clear schedule about the release of node-problem-detector ASAP. Simply bump minor version for new binary maybe confusing for users. :)

This is on the planning of Kubernetes 1.6 release in #58 by @Random-Liu .

shyamjvs · 2017-01-11T01:20:09Z

@andyxning That sounds reasonable. However, our work on kubemark was kind of blocked due to not having a stand-alone npd mode. So we just experimentally pushed the v0.3 image for use right now. However, since we would (hopefully) be the only ones using this image or anyone who'd want to use this image would know that it is not part of the release, it is safe to just rewrite v0.3 to the one you actually plan to release later on.
Does that seem ok to you?

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 14, 2016

Random-Liu mentioned this pull request Dec 15, 2016

Allow reading a kubeconfig file for master / auth info #27

Closed

Random-Liu self-assigned this Dec 15, 2016

andyxning force-pushed the add_support_for_running_standalone branch 9 times, most recently from b7076d3 to 1848b91 Compare December 16, 2016 15:28

Random-Liu requested changes Dec 16, 2016

View reviewed changes

andyxning force-pushed the add_support_for_running_standalone branch 9 times, most recently from 3079f6a to b2f6ce9 Compare December 18, 2016 01:50

This was referenced Jan 5, 2017

Feature request for a "hollow"-node-problem-detector having an empty list of conditions and rules inside kernel monitor config #50

Closed

NodeProblemDetector in Kubemark doesn't work kubernetes/kubernetes#39245

Closed

Random-Liu requested changes Jan 5, 2017

View reviewed changes

Random-Liu mentioned this pull request Jan 7, 2017

NPD Kubernetes 1.6 Planning #58

Closed

11 tasks

andyxning force-pushed the add_support_for_running_standalone branch from 7e0dc46 to 0413bb8 Compare January 7, 2017 02:15

Random-Liu reviewed Jan 7, 2017

View reviewed changes

andyxning force-pushed the add_support_for_running_standalone branch from 0413bb8 to 7e55f8f Compare January 7, 2017 15:23

add support for running npd standalone

68b379c

andyxning force-pushed the add_support_for_running_standalone branch from 7e55f8f to 68b379c Compare January 7, 2017 15:49

andyxning mentioned this pull request Jan 8, 2017

does this tool work outside Google? #21

Closed

Random-Liu approved these changes Jan 9, 2017

View reviewed changes

Random-Liu merged commit 0cd7944 into kubernetes:master Jan 10, 2017

shyamjvs mentioned this pull request Jan 10, 2017

Bump the version to v0.3 in makefile, manifest and readme #60

Closed

Random-Liu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2017

andyxning deleted the add_support_for_running_standalone branch January 11, 2017 00:48

Random-Liu mentioned this pull request Jan 13, 2017

Standalone NPD Support #76

Closed

10 tasks

andyxning mentioned this pull request Jan 1, 2018

bump kubernetes to 1.9.3 kubernetes-retired/heapster#1849

Merged


		`node-problem-detector` can either run as a [DaemonSet](http://kubernetes.io/docs/admin/daemons/) or run standalone on bare metals.

		`node-problem-detector` detects node problems and reporting them to apiserver.


		`node-problem-detector` detects node problems and reporting them to apiserver.

		Now it is running as

add support for running standalone #49

add support for running standalone #49

Conversation

andyxning commented Dec 14, 2016 • edited Loading

andyxning commented Dec 14, 2016

Random-Liu commented Dec 15, 2016

Random-Liu commented Dec 15, 2016 • edited Loading

andyxning commented Dec 16, 2016 • edited Loading

Random-Liu commented Dec 16, 2016 • edited Loading

andyxning commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyxning Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Dec 16, 2016

andyxning commented Dec 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamjvs commented Jan 5, 2017

Random-Liu commented Jan 6, 2017

shyamjvs commented Jan 6, 2017

gmarek commented Jan 6, 2017

andyxning commented Jan 7, 2017 • edited Loading

Random-Liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyxning commented Jan 7, 2017

Random-Liu commented Jan 9, 2017

Random-Liu commented Jan 10, 2017

shyamjvs commented Jan 10, 2017

gmarek commented Jan 10, 2017

shyamjvs commented Jan 10, 2017

andyxning commented Jan 11, 2017 • edited Loading

shyamjvs commented Jan 11, 2017

andyxning commented Dec 14, 2016 •

edited

Loading

Random-Liu commented Dec 15, 2016 •

edited

Loading

andyxning commented Dec 16, 2016 •

edited

Loading

Random-Liu commented Dec 16, 2016 •

edited

Loading

andyxning Dec 21, 2016 •

edited

Loading

andyxning commented Jan 7, 2017 •

edited

Loading

andyxning commented Jan 11, 2017 •

edited

Loading