Skip to content
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.

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

Add multiple system log monitor support #94

Merged

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 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants