PV monitoring proposal #1484

NickrenREN · 2017-12-11T04:51:33Z

Add a proposal for PV monitoring.
We may focus on local storage PV monitoring at the first phase.

/assign @msau42
/cc @ddysher @jingxu97
/sig storage

k8s-ci-robot · 2017-12-11T04:51:35Z

@NickrenREN: GitHub didn't allow me to request PR reviews from the following users: ddysher.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

Add a proposal for PV monitoring.
We may focus on local storage PV monitoring at the first phase.

/assign @msau42
/cc @ddysher @jingxu97
/sig storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

NickrenREN · 2017-12-11T07:10:25Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+Take local storage as an example, implementation may be like this:
+
+```
+ func (mon *minitor) CheckStatus(spec *v1.PersistentVolumeSpec) (string, error) {


This needs further discussion

redbaron · 2017-12-17T11:40:22Z

It is not clear how apps can react to bad news and recover from failure. I'd be very hesitant to allow Statefulset to delete PVC should there be failure detected:

there is no guarantee that new PV created will be healthy
removing PVC removes data, so if PV has a chance to become healthy later on I'd much rather wait
usually PV failure is tied with storage failure, that means that all PVs in that failure domain are going to report bad health state and therefore deleted. This is catastrophic

gnufied · 2017-12-18T17:42:37Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+
+### Monitoring controller: 
+
+Like PV controller, monitoring controller should check PVs’ health condition periodically and taint them if PVs are unhealthy.


What happens when PVs are tainted? Currently we do not have ability to taint PVs

Yeah, at the first phase, do not need to change PV struct, we will mark PV by adding annotations instead.

gnufied · 2017-12-18T17:44:01Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+
+We can separate the proposal into two parts:
+
+* monitoring PVs and marking them if they have problems


What exactly will it monitor btw? Are you talking about bad blocks or fsck? Please add some examples.

It will monitor if volumes still exist or if their status are ok.
e.g.
For local PV: If its Path( directory) is deleted by mistake or node breaks down... this will cause data loss, kubernetes needs to know that.

It would be good to explicitly write down some specific error conditions somewhere.

msau42 · 2017-12-20T00:21:01Z

@redbaron agree. That is why we should clearly document here exactly what use cases we are trying to solve. And also this reaction part needs to be opt-in by the application, so that only those that can recover from these scenarios can benefit.

From a workloads point of view, we want to target distributed applications like Cassandra, that replica their data across multiple instances for redundancy.

From a platform point of view, the infrastructure failures I'm thinking of are:

local disks in cloud providers, where if the VM is deleted, then your local disk is really gone forever.
local disks in on-prem, and the disk hardware has failed (open issue on how to actually detect that). If you don't have them in a RAID config, then the data is also gone.

NickrenREN · 2017-12-20T02:55:52Z

@redbaron sorry for the delay. yeah, if we want to delete the PVC and pods directly and reschedule them to a new node, the application needs to have data backup and can restore it or can tolerate data loss

msau42 · 2018-01-30T17:33:06Z

cc @vkamra

msau42 · 2018-02-08T01:03:39Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+                return ...
+        }
+
+        // check volume health condition depending on device type


Can you outline here what kind of checks you want to do? What are some general checks, and what are some specific checks?

general checks: volume accessing check, volume existing check ...
specific checks: mountpoint check, local path check,...
will update

msau42 · 2018-02-08T01:04:06Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+
+At the first phase, we just consider the statefulset reaction.
+
+statefulset reaction: check the annotation timestamp, if the PV can recover within the predefined time interval, we will do nothing, 


How does this controller know which StatefulSets to monitor?

by monitoring relatedPVs

How do we mark which PVs we should monitor? For example, not all StatefulSets may want this behavior. So we need a way to be able to say which StatefulSets/PVs should be handled by the reaction controller.

BTW, @jingxu97 just showed me some cool work related to a Snapshot policy controller using the metacontroller framework. A CRD is defined that specifies:

How often a snapshot should be taken

Label selector to choose which PVCs should follow this policy.

I think we could do something similar for this controller, and have a CRD to define a policy with:

What conditions to react to

Time thresholds before reacting

What should the action be for each condition

Label selector for which StatefulSets to monitor

msau42 · 2018-02-08T01:04:26Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+
+}
+```
+If monitor finds that one PV is unhealthy, it will mark the PV by adding annotations including timestamp. 


Can you define the annotation key/value?

yeah, sure, defined in demo PR: kubernetes-retired/external-storage#528
will update this proposal

Can you define the annotation format and syntax in this design doc, and also put some of the examples here too?

msau42 · 2018-02-08T01:07:27Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+which is responsible for creating informers, watching Node and PV events and calling each plugin’s monitor functions. 
+And each volume plugin will create its own monitor to check its volumes’ status.
+
+#### For local storage:


I would be very interested in seeing an outline for monitoring node failures, which is a failure mode that local volumes are especially prone to compared to other volume types.

yeah, will do

msau42 · 2018-02-08T01:10:53Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+
+We can separate the proposal into two parts:
+
+* monitoring PVs and marking them if they have problems


It would be good to explicitly write down some specific error conditions somewhere.

msau42 · 2018-02-08T01:12:41Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+## User Experience
+### Use Cases
+
+* Users create many local PVs manually and want to monitor their health condition;


Since all of the use cases here are for local PVs, should we just have this document be specific to local PVs for now? We can consider extending this to other PV types in the future if the need arises.

yeah, we want to focus on local storage at the first stage, and then move to other storage drivers if needed.

msau42 · 2018-02-08T01:17:29Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+* Fill PV cache from etcd;
+* Watch for PV update events;
+* Resync and populate periodically;
+* Delete related PVC and pods if needed (just for statefulsets and reclaim PV depending on reclaim policy);


BTW, what application are you planning to use with this? It may be good to mention in the beginning use cases what kinds of applications this is suitable for.

if we want to delete the PV directly, the application should have data backup and can restore it or can tolerate data loss.

NickrenREN · 2018-03-28T12:53:38Z

@ddysher @msau42 update the use cases here, and will modify the rest accordingly.
simple example is here: kubernetes-retired/external-storage#528

msau42 · 2018-04-20T22:35:03Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+
+At the first phase, we just consider the statefulset reaction.
+
+statefulset reaction: check the annotation timestamp, if the PV can recover within the predefined time interval, we will do nothing, 


How do we mark which PVs we should monitor? For example, not all StatefulSets may want this behavior. So we need a way to be able to say which StatefulSets/PVs should be handled by the reaction controller.

BTW, @jingxu97 just showed me some cool work related to a Snapshot policy controller using the metacontroller framework. A CRD is defined that specifies:

How often a snapshot should be taken

Label selector to choose which PVCs should follow this policy.

I think we could do something similar for this controller, and have a CRD to define a policy with:

What conditions to react to

Time thresholds before reacting

What should the action be for each condition

Label selector for which StatefulSets to monitor

msau42 · 2018-04-20T22:35:55Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+
+}
+```
+If monitor finds that one PV is unhealthy, it will mark the PV by adding annotations including timestamp. 


Can you define the annotation format and syntax in this design doc, and also put some of the examples here too?

msau42 · 2018-04-20T22:39:30Z

contributors/design-proposals/storage/pv-monitoring-proposal.md

+## User Experience
+### Use Cases
+
+* If the local PV path is deleted, users should know that and the local PV should be marked and deleted;


It seems like there's two different reaction controllers being proposed here?

One for StatefulSets that are using these PVs

One for local PVs

Could you try to clarify this in the doc and separate out which behaviors will be handled by which controllers? I think one of the confusing things here is there's multiple levels/layers of reaction and it's not clear how they will all interact with each other.

Or maybe this design should just focus on the local PV monitoring part. And we can leave StatefulSet handling to a different design.

yes, reaction needs more discussion, so i will move it to next stage.

k8s-ci-robot · 2018-04-23T04:03:06Z

[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: childsb

Assign the PR to them by writing /assign @childsb 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:

contributors/design-proposals/storage/OWNERS

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

ddysher · 2018-05-13T12:05:42Z

@NickrenREN @msau42 what's the status of the proposal :)

NickrenREN · 2018-05-14T02:44:18Z

@ddysher I am prototyping PV monitor here in https://github.com/caicloud/kube-storage-monitor
And will update this proposal soon. I am planning to add local volume PV monitor support in Q2 or Q3,
WDYT, @msau42 ?

msau42 · 2018-05-14T18:16:19Z

I probably won't have time to thoroughly review this until Q3 timeframe.

But it would help if in this design proposal, you could summarize things into a table with the following columns:

Failure type
Reaction (and by who)

Also it would be good to think about some of these questions:

Can reaction be configurable per PV/PVC? Reaction should only be opt-in because it could have dangerous consequences for various workloads.
What happens if the failure is only temporary? Can we have configurable timeouts before reacting?

I really like the idea of having CRDs where you can define your reaction policies, and then using something like the metacontroller to write a reaction controller. I would sync up with @jingxu97 on the work she did for snapshot policies.

msau42 · 2018-06-18T17:16:01Z

I opened up kubernetes-retired/external-storage#817 to discuss ideas for handling the node deletion scenario.

fejta-bot · 2018-09-16T17:43:12Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

NickrenREN · 2018-09-17T07:14:17Z

/remove-lifecycle stale

fejta-bot · 2019-01-20T07:31:42Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2019-02-19T07:49:10Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

sarjeet2013 · 2019-02-21T20:37:22Z

/remove-lifecycle rotten

fejta-bot · 2019-05-22T20:47:33Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

NickrenREN · 2019-06-03T08:28:46Z

Closing, in favor of kubernetes/enhancements#1077

k8s-ci-robot assigned msau42 Dec 11, 2017

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 11, 2017

k8s-ci-robot requested a review from jingxu97 December 11, 2017 04:51

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 11, 2017

NickrenREN force-pushed the pv-monitor-proposal branch 2 times, most recently from 4b7a149 to 484ad79 Compare December 11, 2017 04:59

NickrenREN commented Dec 11, 2017

View reviewed changes

gnufied reviewed Dec 18, 2017

View reviewed changes

NickrenREN force-pushed the pv-monitor-proposal branch 2 times, most recently from a3b2291 to 908ad48 Compare December 20, 2017 02:51

NickrenREN force-pushed the pv-monitor-proposal branch 2 times, most recently from 269479f to 9393e1a Compare December 20, 2017 08:55

This was referenced Dec 20, 2017

Where can we place the local storage monitor ? kubernetes-retired/external-storage#517

Closed

[WIP]: Monitor local pv kubernetes-retired/external-storage#528

Closed

k8s-github-robot added the kind/design Categorizes issue or PR as related to design. label Feb 6, 2018

msau42 reviewed Feb 8, 2018

View reviewed changes

msau42 mentioned this pull request Mar 26, 2018

Pod's PVCs using feature "Local Volume Scheduling" maybe not support re-scheduling kubernetes/kubernetes#61620

Closed

NickrenREN force-pushed the pv-monitor-proposal branch from 9393e1a to d082a85 Compare March 28, 2018 12:50

msau42 reviewed Apr 20, 2018

View reviewed changes

NickrenREN force-pushed the pv-monitor-proposal branch from d082a85 to 9c6eb40 Compare April 23, 2018 04:02

pv monitoring proposal

8e0a47a

NickrenREN force-pushed the pv-monitor-proposal branch from 9c6eb40 to 8e0a47a Compare April 23, 2018 04:14

NickrenREN mentioned this pull request May 14, 2018

PV health monitor kubernetes/enhancements#568

Closed

msau42 mentioned this pull request Jun 18, 2018

[local-volume] New controller to handle node deletion kubernetes-retired/external-storage#817

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 16, 2018

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 17, 2018

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2019

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 19, 2019

msau42 mentioned this pull request Feb 21, 2019

Health check by CSI driver kubernetes/kubernetes#74359

Closed

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Feb 21, 2019

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 22, 2019

NickrenREN closed this Jun 3, 2019

NickrenREN deleted the pv-monitor-proposal branch June 3, 2019 08:29

NickrenREN mentioned this pull request Jun 18, 2019

PV health monitor KEP kubernetes/enhancements#1077

Merged


		### Monitoring controller:

		Like PV controller, monitoring controller should check PVs’ health condition periodically and taint them if PVs are unhealthy.


		We can separate the proposal into two parts:

		* monitoring PVs and marking them if they have problems


		At the first phase, we just consider the statefulset reaction.

		statefulset reaction: check the annotation timestamp, if the PV can recover within the predefined time interval, we will do nothing,

PV monitoring proposal #1484

PV monitoring proposal #1484

Conversation

NickrenREN commented Dec 11, 2017

k8s-ci-robot commented Dec 11, 2017

NickrenREN Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

redbaron commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Dec 20, 2017

NickrenREN commented Dec 20, 2017 • edited Loading

msau42 commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

NickrenREN commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 23, 2018

ddysher commented May 13, 2018

NickrenREN commented May 14, 2018 • edited Loading

msau42 commented May 14, 2018

msau42 commented Jun 18, 2018

fejta-bot commented Sep 16, 2018

NickrenREN commented Sep 17, 2018

fejta-bot commented Jan 20, 2019

fejta-bot commented Feb 19, 2019

sarjeet2013 commented Feb 21, 2019

fejta-bot commented May 22, 2019

NickrenREN commented Jun 3, 2019

NickrenREN Dec 11, 2017 •

edited

Loading

NickrenREN Dec 20, 2017 •

edited

Loading

NickrenREN commented Dec 20, 2017 •

edited

Loading

NickrenREN Mar 28, 2018 •

edited

Loading

NickrenREN commented May 14, 2018 •

edited

Loading