Local PV health monitor #2

NickrenREN · 2018-04-08T05:17:38Z

Partly fix: #1

NickrenREN · 2018-04-08T05:18:43Z

/release-note-none

ddysher · 2018-04-08T05:45:27Z

do you have upstream PR? what's the relationship with the PR here?

NickrenREN · 2018-04-08T05:51:40Z

yeah, Upstream PR: kubernetes-retired/external-storage#528
They did the same work, but upstream PR is integrated into external-storage repo,
This one can be deployed separately.

ddysher

This seems to be highly coupled with local storage monitoring, what's the plan for generalizing it?

ddysher · 2018-04-08T06:26:08Z

src/local-pv-monitor/OWNERS

@@ -0,0 +1,4 @@
+approvers:


we don't need this anymore since we already have top-level one

I still suggest we can have our owners under each package just like kubernetes does.
Since it is likely that different storage driver monitors are authored by different people. If we put all these reviewers into the top-level file, bot may assign the wrong people to the issues and PRs.

ddysher · 2018-04-08T06:30:47Z

src/local-pv-monitor/cmd/main.go

@@ -20,24 +20,33 @@ import (
 	"flag"
 	"os"

+	lvmonitor "github.com/caicloud/kube-storage-monitor/src/local-pv-monitor/pkg/monitor"


what's the reason for this layout? src/local-pv-monitor/cmd/main.go.

ideally, we should just use cmd/main.go; here is our project template, FYI

├── .github │ ├── ISSUE_TEMPLATE │ └── PULL_REQUEST_TEMPLATE ├── .gitignore ├── .pre-commit-config.yaml ├── CHANGELOG.md ├── Makefile ├── CODEOWNERS ├── README.md ├── bin │ ├── admin │ └── controller ├── cmd │ ├── admin │ │ └── admin.go │ └── controller │ └── controller.go ├── build │ ├── admin │ │ ├── Dockerfile │ └── controller │ ├── Dockerfile ├── docs │ └── README.md ├── hack │ ├── README.md │ ├── deployment.yaml │ └── script.sh ├── pkg │ ├── utils │ │ └── net │ │ └── net.go │ └── version │ └── version.go ├── test │ └── README.md │ └── test_make.sh ├── third_party │ └── README.md └── vendor └── README.md

A brief description of the layout:

.github has two template files for creating PR and issue. Please see the files for more details.

.gitignore varies per project, but all projects need to ignore bin directory.

.pre-commit-config.yaml is for configuring pre-commit.

Makefile is used to build the project.

CHANGELOG.md contains auto-generated changelog information.

CODEOWNERS contains owners of the project.

README.md is a detailed description of the project.

bin is to hold build outputs.

cmd contains main packages, each subdirecoty of cmd is a main package.

build contains scripts, yaml files, dockerfiles, etc, to build and package the project.

docs for project documentations.

hack contains scripts used to manage this repository, e.g. codegen, installation, verification, etc.

pkg places most of project business logic.

test holds all tests (except unit tests), e.g. integration, e2e tests.

third_party for all third party libraries and tools, e.g. swagger ui, protocol buf, etc.

vendor contains all vendored code.

local pvs are kind of different from other storage drivers. Local PVs monitor must be deployed as Daemonset in order to watch local path on each node.
So i plan to make the local PV monitor separate from other monitors. Other storage driver PV monitors can be combined like you suggested.

NickrenREN · 2018-04-08T07:36:59Z

@ddysher Regarding the plan for generalizing this, i planned to implement the local PV monitor as well as other kind of storage PV monitors in this repo at first, since we are not sure we can merge all of these into kubernetes core repo.
BTW: We plan to just support local PV monitor in external-storage at the first stage, and add other kind of PV monitoring support if needed,but we can implement all of these monitors more faster here even if upstream does not want to do so. Because i think it is necessary and important for users to monitor their data.

And also, monitors in this repo can be deployed independently, if possible, we can put monitor CSI supporting code here later too.

ddysher · 2018-04-08T08:00:33Z

How about one binary for all use cases, that's more commonly seen from other projects,

for example,

kube-monitor --monit local-pv blabla

kube-monitor --monit ceph,glusterfs blabla

kube-monitor --monit ceph,glusterfs,local-pv blabla
ERROR!  can not run local-pv with other type of storage monitoring

or if you want to run only one binary at one time, use subcommand:

kube-monitor local-pv --blabla

kube-monitor ceph --blabla

I find it much easier to manage w.r.t deployment.

Implementation-wise, the plan sounds good to me.

NickrenREN · 2018-04-08T08:19:43Z

Sounds good, i will reconsider it

caicloud-bot · 2018-04-08T11:59:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: nickrenren

Assign the PR to them by writing /assign @nickrenren in a comment when ready.

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:

OWNERS

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

NickrenREN · 2018-04-08T12:01:17Z

How about this change for the layout ? @ddysher

├── cmd
│   ├── kube-storage-monitor
│   │   └── app
│   │       └──server.go
│   │   └── local-pv-monitor
│   │       └──server.go
│	└── main.go
├── pkg
│   ├── local-pv-monitor
│   │   └── config.go
│   │   └── localVolumeCache.go
│   │   └── monitor.go
│   └── util
│       └── util.go

ddysher · 2018-04-08T12:25:46Z

Much better.

you can "s/local-pv-monitor/local-pv", since you already have monitor in parent directory.

NickrenREN · 2018-04-08T12:33:34Z

seems not a big deal, done @ddysher

NickrenREN · 2018-04-08T12:35:00Z

will test this PR later

ddysher

briefly scanned the implementation

can u also add design, readme, makefile, etc (in later PRs)

ddysher · 2018-04-09T04:45:49Z