-
Notifications
You must be signed in to change notification settings - Fork 778
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
Volume Health Monitoring #1495
Volume Health Monitoring #1495
Conversation
9cfe524
to
b6bed59
Compare
b6bed59
to
666eb54
Compare
/hold Known caveats: Using non EKS-D sidecar until their team can build the external health monitor sidecar as well. We should consider what the appropriate timing is for health checks, the default is once per minute per volume. |
666eb54
to
b63814f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Largely lgtm besides a few suggestions and really minor nits.
Let's add a test case for the volume-health-monitoring
flag in TestControllerOptions
:
func TestControllerOptions(t *testing.T) { |
{
name: "lookup volume-health-monitoring",
flag: "volume-health-monitoring",
found: true,
},
@@ -89,6 +89,17 @@ sidecars: | |||
securityContext: | |||
readOnlyRootFilesystem: true | |||
allowPrivilegeEscalation: false | |||
healthMonitor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider allowing users to specify their own monitoring interval via the monitor-interval
flag: https://github.com/kubernetes-csi/external-health-monitor/blob/f11edf7690317fbad79ac0506a166eef3d78bf18/cmd/csi-external-health-monitor-controller/main.go#L55
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this open until we decide what the default for monitor-interval
will be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every configuration option represents a failure of the software to do the right thing automatically. Every configuration option needs to be documented and protected by unit tests, thereby increasing the cognitive load of user and developer alike. Sometimes they are necessary, but only as a last resort.
"Action": [ | ||
"ec2:DescribeVolumeStatus" | ||
], | ||
"Resource": "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about restricting to just volumes created by the CSI driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think documenting how to do that is better left to #1340. Right now, our default example IAM policy gives the driver global permissions for several APIs like DescribeVolumes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going to add to the entropy that #1340 has to clean up, at least leave a note (if not a new rev) over there.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rdpsin 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 |
b63814f
to
ba7e7c1
Compare
Fixed most of the review comments, ready for re-review on anything marked resolved. Thinking about also using this PR to implement |
ba7e7c1
to
da6201f
Compare
Signed-off-by: Connor Catlett <conncatl@amazon.com>
Signed-off-by: Connor Catlett <conncatl@amazon.com>
da6201f
to
893d8e8
Compare
Signed-off-by: Connor Catlett <conncatl@amazon.com>
Signed-off-by: Connor Catlett <conncatl@amazon.com>
Signed-off-by: Connor Catlett <conncatl@amazon.com>
893d8e8
to
d160e75
Compare
Closing this for now because it's stale and will likely be reworked |
Is this a bug fix or adding new feature?
Feature
What is this PR about? / Why do we need it?
Implements https://kubernetes.io/docs/concepts/storage/volume-health-monitoring/
What testing is done?
Manually tested, as well as significant additional tests in-driver