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.

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 VolumeAttachment collector #946

Merged
merged 1 commit into from
Oct 17, 2019

Conversation

JensErat
Copy link
Contributor

What this PR does / why we need it:

Kubernetes has a new resource type: VolumeAttachments. They provide helpful information on where a volume is attached and to alert on unexpected attachment status (for example, differences between information scraped from node-exporter and kube-state-metrics).

The collector adds a bunch of new metrics. Each VolumeAttachment (ie., each CSI-attached volume) will have one of each, so we do not overly pollute the metrics space. Most metrics are rather unsurprising.

  • kube_volumeattachment_status_attachment_metadata: provides a label-like export of the attachment metadata map. Generalizing the label-conversion function slightly helps at providing this metric.
  • kube_volumeattachment_created: as VolumeAttachments are automatically created and we already suffered from duplicate VolumeAttachments, this can be invaluable for debugging
    misattachments.
  • kube_volumeattachment_spec_source_persistentvolume: will only be generated when the volume source is of PersistentVolume type. The other type inlineVolumeSpec is still alpha-level and hard to map to metrics.

No end-to-end test manifest was added, as VolumeAttachments are automatically generated when mounting volumes.

Since kube-state-metrics 1.8.0 wants to support Kubernetes 1.11 onwards, I went for storagev1beta1; the API is GA since Kubernetes 1.13 ( so still some time to go, now matter what version this gets released under).

Important note: I'm on vacation soon. My coworker @sbueringer will take over regarding any issues as soon as I'm gone.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #872 -- mostly follows the ideas discussed in this feature request; I had to deviate in some metrics to follow the other collectors and also properly take specifics of VolumeAttachments into account.

@k8s-ci-robot
Copy link
Contributor

Welcome @JensErat!

It looks like this is your first PR to kubernetes/kube-state-metrics 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kube-state-metrics has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 11, 2019

| Metric name| Metric type | Labels/tags | Status |
| ---------- | ----------- | ----------- | ----------- |
| kube_volumeattachment_info | Gauge | `volumeattachment`=&lt;volumeattachment-name&gt; <br> `attacher`=&lt;attacher-name&gt; <br> `nodeName`=&lt;node-name&gt; | STABLE |
Copy link
Contributor

Choose a reason for hiding this comment

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

These metrics should be EXPERIMENTAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll switch to EXPERIMENTAL; just to understand the semantics better: I thought it was about stability of the Kubernetes API, or is it also about "being new to kube-state-metrics"?

@@ -34,6 +34,7 @@ import (
extensions "k8s.io/api/extensions/v1beta1"
policy "k8s.io/api/policy/v1beta1"
storagev1 "k8s.io/api/storage/v1"
storagev1beta1 "k8s.io/api/storage/v1beta1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use storagev1 for VolumeAttachment. It has been promoted to v1 since 1.13 if I am not mistaken.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should even support the watching and listing of VolumeAttachments created under v1beta1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it graduated to GA in 1.13 (as also stated in the PR). I started with storagev1 originally, but it failed in a Kubernetes 1.14 cluster -- kube-state-metrics got v1beta1 objects returned and paniced.

On the other hand, following the compatibility chart from the README we need to support Kubernetes 1.11? In this case, only storagev1beta1 is available.

Kubernetes has a new resource type: `VolumeAttachments`. They provide
helpful information on where a volume is attached and to alert on
unexpected attachment status (for example, differences between
information scraped from node-exporter and kube-state-metrics).

The collector adds a bunch of new metrics. Each VolumeAttachment (ie.,
each CSI-attached volume) will have one of each, so we do not overly
pollute the metrics space. Most metrics are rather unsurprising.

- `kube_volumeattachment_status_attachment_metadata`: provides a
  label-like export of the attachment metadata map. Generalizing the
  label-conversion function slightly helps at providing this metric.
- `kube_volumeattachment_created`: as VolumeAttachments are
  automatically created and we already suffered from duplicate
  `VolumeAttachments`, this can be invaluable for debugging
  misattachments.
- `kube_volumeattachment_spec_source_persistentvolume`: will only be
  generated when the volume source is of `PersistentVolume` type. The
  other type `inlineVolumeSpec` is still alpha-level and hard to map to
  metrics.

No end-to-end test manifest was added, as `VolumeAttachment`s are
automatically generated when mounting volumes.

Signed-off-by: Jens Erat <email@jenserat.de>
@JensErat
Copy link
Contributor Author

Fixed documentation (STABLE to EXPERIMENTAL, missing created metrics documentation), added manifest for e2e-tests and squashed.

As already mentioned, I'm not sure about going to storage/v1; it didn't work out for me against Kubernetes 1.14 and kube-state-metrics even wants to support Kubernetes 1.11 (or 1.12 with the next release) as I understand, and those generally only support storage/v1beta1 for VolumeAttachments.

Copy link
Contributor

@tariq1890 tariq1890 left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

Thank you very much for your contribution @JensErat :).

Deferring to @lilic or @brancz for final approval.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2019
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JensErat, tariq1890

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 Oct 16, 2019
@brancz
Copy link
Member

brancz commented Oct 17, 2019

lgtm

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 17, 2019
@k8s-ci-robot k8s-ci-robot merged commit 99fbbea into kubernetes:master Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: export VolumeAttachment metrics
4 participants