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.

Sign up for GitHub

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

Jump to bottom

Update pluginwatcher doc #68562

Merged
merged 2 commits into from
Nov 2, 2018
Merged

Update pluginwatcher doc #68562

merged 2 commits into from
Nov 2, 2018

Conversation

RenaudWasTaken
Copy link
Contributor

@RenaudWasTaken RenaudWasTaken commented Sep 12, 2018 •
edited
Loading

What this PR does / why we need it: Adds more documentation about the pluginwatcher process and development

Which issue(s) this PR fixes See issue kubernetes/enhancements#595 and issue #65773

Special notes for your reviewer:
/sig node
/kind documentation
/cc @vikaschoudhary16 @figo @sbezverk @saad-ali @vladimirvivien @jiayingz

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 12, 2018
@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. kind/documentation Categorizes issue or PR as related to documentation. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubelet release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 12, 2018
vladimirvivien
vladimirvivien reviewed Sep 13, 2018
View reviewed changes
Copy link
Member

@vladimirvivien vladimirvivien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added couple of suggestions.

pkg/kubelet/util/pluginwatcher/README Outdated Show resolved Hide resolved
pkg/kubelet/util/pluginwatcher/README Outdated
- Implements the Registration service specified in
pkg/kubelet/apis/pluginregistration/v*/api.proto.
Here are the general rules that Kubelet plugin developers should follow:
- Run as 'root' user. Currently creating socket under PluginsDir, a root owned
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that Run plugin as root ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well your plugin should have the correct permissions to create and read any files in the plugin directory.

figo
figo reviewed Sep 13, 2018
View reviewed changes
pkg/kubelet/util/pluginwatcher/README Outdated

# Kubelet Internal API

For internal kubelet clients using the pluginwatcher module, you will need to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is "internal kubelet client" been defined somewhere? i feel it is more clear to say: For any kubelet components

pkg/kubelet/util/pluginwatcher/README Outdated
@@ -32,3 +42,25 @@ Here are the general rules that Kubelet plugin developers should follow:

- For an example plugin implementation, take a look at example_plugin.go
included in this directory.


# Kubelet Internal API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same question, is "Kubelet internal API" been defined somewhere? thanks

vladimirvivien
vladimirvivien reviewed Sep 21, 2018
View reviewed changes
Copy link
Member

@vladimirvivien vladimirvivien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename file README.md so mark down will render properly.

@vladimirvivien
Copy link
Member

/LGTM

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2018
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 21, 2018
@RenaudWasTaken
Copy link
Contributor Author

RenaudWasTaken commented Sep 21, 2018 •
edited
Loading

@vladimirvivien done!
Also split into two commits so that the diff is viewable

figo
figo reviewed Sep 21, 2018
View reviewed changes
pkg/kubelet/util/pluginwatcher/README.md Outdated
pluginwatcher module in kubelet.go by calling AddHandler(pluginType, handler).


The lifecycle follows a simple Validate -> Register -> DeRegister state machine.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ascii graph is off when render the file, add a extra blank line will work (find out by rendering a readme file in my local IDE).

@RenaudWasTaken
Copy link
Contributor Author

/retest

@RenaudWasTaken
Copy link
Contributor Author

Fixed @figo

@figo
Copy link
Contributor

figo commented Sep 24, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2018
@nikhita
Copy link
Member

nikhita commented Sep 29, 2018

/assign @yujuhong
for approval

@RenaudWasTaken
Copy link
Contributor Author

ping @vishh for approval

@vladimirvivien vladimirvivien mentioned this pull request Oct 18, 2018
Closed
@RenaudWasTaken
Copy link
Contributor Author

Maybe @saad-ali can help with approval on this docs PR :) ?

@vishh
Copy link
Contributor

vishh commented Nov 1, 2018

Appreciate taking time to write this readme @RenaudWasTaken. Can you also add this text to the k8s device plugin docs page, or at-least link to this doc?

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RenaudWasTaken, vishh

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2018
@RenaudWasTaken
Copy link
Contributor Author

Thanks @vishh !

Yep, I'll take care of the PR tomorrow!

@k8s-ci-robot k8s-ci-robot merged commit 3b66adb into kubernetes:master Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladimirvivien vladimirvivien vladimirvivien left review comments

@figo figo figo left review comments

@jiayingz jiayingz Awaiting requested review from jiayingz

@saad-ali saad-ali Awaiting requested review from saad-ali

@sbezverk sbezverk Awaiting requested review from sbezverk

@vikaschoudhary16 vikaschoudhary16 Awaiting requested review from vikaschoudhary16

Assignees

@figo figo

@vladimirvivien vladimirvivien

@vishh vishh

@yujuhong yujuhong

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@RenaudWasTaken @vladimirvivien @figo @nikhita @vishh @k8s-ci-robot @yujuhong