-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Add a proposal for high level volume metrics #809
Conversation
/sig storage |
@kubernetes/sig-storage-api-reviews |
cc @kubernetes/sig-instrumentation-misc |
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.
Just a small nit otherwise looks ok at first glance.
Similarly errors will be named: | ||
|
||
``` | ||
storage_volume_attach_errors { plugin = "aws-ebs" } |
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.
The suffix should be _errors_total
.
What about "pro-active" monitoring, i.e. performing reads on a volume while it is attatched, to identify issues at runtime? Is this in general interesting, and should this be a spearate issue? |
@fabiand can you elaborate more? What sort of metrics we are talking about? But it does sound like something out of scope for this proposal since this proposal is more about metric collection at controller level. |
Ah yes, you can capture the events at the controller level. I was thinking of i.e. doing active monitoring of attached storage, i.e. general connectivity checks, or read/write/seek latency checks while storage is attached, to ensure that the storage is not malfunctioning. |
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 skipped Implementation Detail part, as I'm not familiar with volume handling code. Please make sure that some will review this.
|
||
|
||
The metrics will be emitted using [Prometheus format](https://prometheus.io/docs/instrumenting/exposition_formats/) and available for collection | ||
from `/metrics` HTTP endpoint of kubelet, controller etc. All Kubernetes core components already emit |
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.
Can you please clarify which components will expose those metrics?
Will this be implemented in a common library?
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.
This would be available from both kubelet and controller-manager depending on component where particular operation was performed. This isn't implemented as a common library, more like hook into place where volume operations are executed.
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.
So please update this in the text.
from
/metrics
HTTP endpoint of kubelet, controller etc.
suggests that there is more
Any collector which can parse Prometheus metric format should be able to collect | ||
metrics from these endpoints. | ||
|
||
A more detailed description of monitoring pipeline can be found in [Monitoring architecture] (https://github.com/kubernetes/community/blob/master/contributors/design-proposals/monitoring_architecture.md#monitoring-pipeline) document. |
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.
Broken link. Remove space between ]
and (
.
emitting these metrics. | ||
|
||
We will be using `HistogramVec` type so as we can attach dimensions at runtime. Name of operation will become | ||
part of metric name and at minimum name of volume plugin will be emitted as a dimension. |
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.
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.
looking at the examples, maybe provider
would be clearer than plugin
?
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.
Why not having operation as label as well? @brancz @fabxc wdyt?
Because name of operation is already in the metric name.
Also a volume plugin is typically a third party service that provides actual volume services. Such as - EBS or GCE-PD or Openstack-Cinder etc. The idea behind labeling the metrics with plugin name is - typically in a cluster user may have more than one volume plugin configured. Using plugin name as a dimension allows them to isolate operation timing from one plugin to another.
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 wasn't questioning including it, rather the naming 🙂 .
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.
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 have renamed plugin
label to volume_plugin
- hopefully this would make it clearer.
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.
volume_plugin
sounds good to me as well
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.
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.
ack
## Motivation | ||
|
||
Currently we don't have high level metrics that captures time taken | ||
for operations like volume attach or volume mounting etc. |
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.
..and success/failure rates of these operations...
CC @kubernetes/sig-storage-feature-requests @kubernetes/sig-instrumentation-feature-requests |
|
||
|
||
``` | ||
storage_volume_attach_seconds { volume_plugin = "aws-ebs" } |
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.
Can we have operation as a parameter instead of in the metric name? That's more similar to what we did for cloud provider metrics.
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.
What advantage we are looking to gain from doing that? The reason we chose same name in cloudprovider metrics is because - we were interested in metrics such as, how many cloudprovider API calls kubernetes makes per minute in total. That metric is useful because it gives the user a good idea of whether he is within API quota or not.
Are we looking to do similar aggregation for these metrics? I think not and hence different metric name might be better. Is aggregating say volume_attach and mount_device metric useful in any sense?
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.
It makes it easier to add the metrics to any querying/display system, especially if we want to add more volume operations in the future. Then all the consumers of these metrics don't need to update every time. My understanding of the Prometheus format, is that you can filter by the labels, so you could implement a display by iterating through all the ops, instead of hardcoding every op.
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.
right, but moving the operation name to label introduces more cardinality to same metric. It can go either way tbh. There is another advantage of moving operation name to dimension - it makes code maintainence bit easier since each new metric (for an operation) has to be registered separately whereas using label means - we need to register just one metric.
By default if a metric has too many dimensions, some dimensions are elided in dashboards until you apply filters. lets ask what @brancz and @piosz think on this one.
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.
This is what I suggested in #809 (comment)
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.
As this is rather controlled information, I think this is generally fine to do. Where it's important to look out for these problems is when the label values can be completely arbitrary, let's say if you gave a request an ID, that would make the time-series created explode and naturally put high time-series churn on any tsdb. If I understand correctly the "operation" is controlled by us implementing said operations, so that generally sounds sane to me.
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.
ack, I will move the operation name to label. Wondering, what will be a good generic name for the metric itself then - "storage_operation_duration_seconds` is what I am thinking.
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.
storage_operation_duration_seconds
sounds good to me
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.
+1 but I'll let the other sig-storage folks to decide on the actual naming.
|
||
```go | ||
GenerateMountVolumeFunc(waitForAttachTimeout time.Duration, | ||
volumeToMount VolumeToMount, |
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.
Can the plugin name be returned in VolumeToMount instead?
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.
VolumeToMount
contains volume spec and at that point plugin name is kind of unknown. The resolution of which plugin will perform mounting or attaching or some other volume operation - usually happens inside GenXXX
functions of operationGenerator
module - https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util/operationexecutor/operation_generator.go#L360
So returning plugin that was chosen to perform certain operation avoids having to modify all these internal structs.
/lgtm |
/lgtm |
from metric design and Prometheus perspective this looks good to me, but can't comment on the implementation details |
storage_operation_duration_seconds { volume_plugin = "iscsi" , operation_name = "volume_unmount" } | ||
storage_operation_duration_seconds { volume_plugin = "aws-ebs", operation_name = "mount_device" } | ||
storage_operation_duration_seconds { volume_plugin = "aws-ebs", operation_name = "unmount_device" } | ||
storage_operation_duration_seconds { volume_plugin = "cinder" , operation_name = "verify_volume" } |
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.
there may be some troubles implementing metrics for VerifyVolumesAreAttached using the proposed method because it's done on a per-node basis, not per-plugin. Unless the plugin supports bulk verification, then it's fine.
anyway we can discuss this offline i.e. how verify_volume implementation detail may deviate from the others
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 plugin name is not available we can emit the volume_plugin dimension as <n/a>
if applicable. Usually, people don't mix the plugins so users would know which volume plugin is that, but we will at least have some metrics in that case. I will update the proposal
lgtm |
This document adds a proposal for gathering metrics at operation level for volume operations. This ensures metrics can be captured regardless of individual volume plugin implementation.
c8fd9e8
to
4a80e58
Compare
/lgtm |
Automatic merge from submit-queue Add volume operation metrics to operation executor and PV controller This PR implements the proposal for high level volume metrics kubernetes/community#809 **Special notes for your reviewer**: ~Differences from proposal:~ all resolved ~"verify_volume" is now "verify_volumes_are_attached" + "verify_volumes_are_attached_per_node" + "verify_controller_attached_volume." Which of them do we want?~ ~There is no "mount_device" metric because the MountVolume operation combines MountDevice and mount (plugin.Setup). Do we want to extract the mount_device metric or is it okay to keep mountvolume as one? For attachable volumes, MountDevice is the actual mount and Setup is a bindmount + setvolumeownership. For unattachable, mountDevice does not occur and Setup is an actual mount + setvolumeownership.~ ~PV controller metrics I did not implement following the proposal at all. I did not change goroutinemap nor scheduleOperation. Because provisionClaimOperation does not return an error, so it's impossible for the caller to know if there is actually a failure worth reporting. So I manually create a new metric inside the function according to some conditions.~ @gnufied I have tested the operationexecutor metrics but not provision & delete. Sample: ![screen shot 2017-08-02 at 15 01 08](https://user-images.githubusercontent.com/13111288/28889980-a7093526-7793-11e7-9aa9-ad7158be76fa.png) **Release note**: ```release-note Add error count and time-taken metrics for storage operations such as mount and attach, per-volume-plugin. ```
Add a proposal for high level volume metrics
This document adds a proposal for gathering metrics
at operation level for volume operations. This ensures
metrics can be captured regardless of individual volume
plugin implementation.
xref kubernetes/enhancements#349