Skip to content

Conversation

@Random-Liu
Copy link
Member

@Random-Liu Random-Liu commented Feb 8, 2017

For #44.
Based on #81, #88 and #92.

Only the last commit is new.

This PR change the --log-monitor flag to --log-monitors. Multiple log monitor config separated by comma can be passed through --log-monitors, NPD will start a separate log monitor for each of them.

This makes it possible to use NPD to monitor different system log at the same time.

/cc @dchen1107 @kubernetes/node-problem-detector-reviewers


This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 8, 2017
@Random-Liu Random-Liu requested a review from dchen1107 February 8, 2017 23:24
@Random-Liu Random-Liu added this to the Kubernetes v1.6 milestone Feb 8, 2017
@Random-Liu Random-Liu mentioned this pull request Feb 8, 2017
11 tasks
@Random-Liu Random-Liu force-pushed the add-multiple-log-monitor-support branch 3 times, most recently from 3591c8a to 835e01f Compare February 15, 2017 01:56
@Random-Liu Random-Liu changed the title Add multiple log monitor support Add multiple system log monitor support Feb 15, 2017
@Random-Liu Random-Liu force-pushed the add-multiple-log-monitor-support branch from 835e01f to 6170b0c Compare February 15, 2017 21:15
@Random-Liu
Copy link
Member Author

@dchen1107 Rebased. PTAL.

@dchen1107
Copy link
Member

I only reviewed one commit (Add multiple log monitoring support) in this pr since all other should be covered / merged through different pr.

Two comments I have:

  1. can we have unittest for parsing --system-log-monitors? at least make sure the corresponding system-log-monitoring goroutine is invoked, and there is no leakage, etc.
  2. can we have a node / cluster e2e tests covering the new feature too?
@Random-Liu
Copy link
Member Author

can we have unittest for parsing --system-log-monitors? at least make sure the corresponding system-log-monitoring goroutine is invoked, and there is no leakage, etc.

Will do.

can we have a node / cluster e2e tests covering the new feature too?

I manually tested this, and this works. I'll add a test case in NPD e2e test for this feature.

@Random-Liu
Copy link
Member Author

@dchen1107 Add a new commit to test goroutine leak.

In fact, with the unit test, we are still not sure whether a failed log monitor will leak any other resources instead of goroutine. However, resource leaking is originally hard to capture in unit test.

@Random-Liu Random-Liu force-pushed the add-multiple-log-monitor-support branch from 51c1f60 to 889d9ef Compare February 16, 2017 08:09
@Random-Liu
Copy link
Member Author

@dchen1107 The PR is ready for another review. PTAL!

@dchen1107
Copy link
Member

LGTM

@dchen1107 dchen1107 merged commit 0f5db9e into kubernetes:master Feb 17, 2017
@Random-Liu Random-Liu deleted the add-multiple-log-monitor-support branch February 17, 2017 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.

3 participants