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

Allow problem daemon plugins to define their command line options #348

Open
wants to merge 1 commit into
base: master
from

Conversation

@xueweiz
Copy link
Contributor

xueweiz commented Sep 13, 2019

This is to keep the interface for problem daemon plugins and exporter plugins in sync.

A little background:
We want to make all problem daemons (system-log, system-stats, custom-plugin) and all exporters (k8s, prometheus, stackdriver) into "plugins". By "plugin", what I really mean is this:

  1. NPD main code does not directly import these libraries, and does not use these libraries directly.

  2. Each plugin should be able to be disabled at compile time.

The challenge comes from the existing exporters (k8s, prometheus). They have been existing for a while, and is configured via command-line flags (e.g. --apiserver-override, --apiserver-wait-timeout). So to refactor them into plugins, we had to allow plugins to define their own command-line options.

This flexibility (defining command-line options) is implemented in #335 as part of #346 . So now we face another question:

Do we also change the code for problem daemon plugins so that they follow the same pattern? (i.e. allowing each plugin to define their own command-line interfaces)

  • The benefit we gain is a more unified code base.
  • The down side is that problem daemons already has a pretty unified command-line interfaces (--config.xxx), and does not really need this flexibility. So it's not that crucial.
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Sep 13, 2019

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/check-cla

@xueweiz xueweiz force-pushed the xueweiz:clo branch from f35bae4 to cd87317 Sep 14, 2019
@k8s-ci-robot k8s-ci-robot added size/XL and removed size/XXL labels Sep 14, 2019
@xueweiz

This comment has been minimized.

Copy link
Contributor Author

xueweiz commented Sep 16, 2019

/assign @wangzhen127

@wangzhen127

This comment has been minimized.

Copy link
Member

wangzhen127 commented Sep 16, 2019

/assign @Random-Liu
I don't have a strong opinion on this. Deferring to @Random-Liu to take a look and see if other OSS projects did similar things.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 18, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xueweiz
To complete the pull request process, please assign random-liu
You can assign the PR to them by writing /assign @random-liu in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@xueweiz xueweiz force-pushed the xueweiz:clo branch from aaac4e1 to 35048f1 Nov 5, 2019
@xueweiz

This comment has been minimized.

Copy link
Contributor Author

xueweiz commented Nov 5, 2019

Hi @Random-Liu , could you help take a look for this PR when you have time? Thanks :)
(No rush, this is not blocking anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.