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.

Sign up for GitHub

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

Proposal for adding PVC info to VolumeStats #930

Merged
merged 3 commits into from
Aug 30, 2017
Merged

Proposal for adding PVC info to VolumeStats #930

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7196a25
Proposal for adding PVC info to VolumeStats
Aug 17, 2017
8f90478
Address review feedback
Aug 28, 2017
94be197
Add PV use case
Aug 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions contributors/design-proposals/volume_stats_pvc_ref.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# Add PVC reference in Volume Stats

## Background
Pod volume stats tracked by kubelet do not currently include any information about the PVC (if the pod volume was referenced via a PVC)

This prevents exposing (and querying) volume metrics labeled by PVC name which is preferable for users, given that PVC is a top-level API object.

## Proposal

Modify ```VolumeStats``` tracked in Kubelet and populate with PVC info:

```
// VolumeStats contains data about Volume filesystem usage.
type VolumeStats struct {
// Embedded FsStats
FsStats
// Name is the name given to the Volume
// +optional
Name string `json:"name,omitempty"`
+ // PVCRef is a reference to the measured PVC.
+ // +optional
+ PVCRef PVCReference `json:"pvcRef"`
}

+// PVCReference contains enough information to describe the referenced PVC.
+type PVCReference struct {
+ Name string `json:"name"`
+ Namespace string `json:"namespace"`
+}
```

## Implementation
2 options are described below. Option 1 supports current requirements/requested use cases. Option 2 supports an additional use case that was being discussed and is called out for completeness/discussion/feedback.
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree use case of getting metrics by PV name is iffy, it could be left as the responsibility of the higher-level tool (heapster?) to do that, since it should be trivial for them to find the PVC to which PV is bound.

If not option 2 sounds good to me, the PV name is right there in volume spec, it just needs to be plumbed through GetMountedVolumesForPod.

Copy link
Contributor

Choose a reason for hiding this comment

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

PV name is not available in volume spec, I think. We have to retrieve through API server to get the PVC object first to get PV name, I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

from asw.attachedVolumes we can check attachedVolume.spec.PersistentVolume (attachedVolume.spec is volume.Spec https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/plugins.go#L277), so option #2 is possible w/o API call

so I think we have "option #1 + API get" or "option #2" if we want metrics indexed by PV name. I prefer option #2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed - given that we're already populating these caches via API calls - option #2 would be preferred rather than adding API calls in the volume stats collector.

Copy link
Contributor

Choose a reason for hiding this comment

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

you are right, I got confused. @vkamra I think we could add a separate function to get a list of PV name indexed by outerVolumeSpecName similarly to ListVolumesForPod. Then we can add the PV name information too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 - that's what I have in option 2. We should do that incrementally though if we need to return PV. For now, PVC seems good enough.


### Option 1
- Modify ```kubelet::server::stats::calcAndStoreStats()```
- If the pod volume is referenced via a PVC, populate ```PVCRef``` in VolumeStats using the Pod spec

- The Pod spec is already available in this method, so the changes are contained to this function.

- The limitation of this approach is that we're limited to reporting only what is available in the pod spec (Pod namespace and PVC claimname)

### Option 2
- Modify the ```volumemanager::GetMountedVolumesForPod()``` (or add a new function) to return additional volume information from the actual/desired state-of-world caches
- Use this to populate PVCRef in VolumeStats

- This allows us to get information not available in the Pod spec such as the PV name/UID which can be used to label metrics - enables exposing/querying volume metrics by PV name
- It's unclear whether this is a use case we need to/should support:
* Volume metrics are only refreshed for mounted volumes which implies a bound/available PVC
* We expect most user-storage interactions to be via the PVC
Copy link
Contributor

Choose a reason for hiding this comment

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

One use-case to call out is admins monitoring PVs so that they know when their users are running out of space and they can immediately find the PV's backing storage to resize

- Admins monitoring PVs (and not PVC's) so that they know when their users are running out of space or are over-provisioning would be a use case supporting adding PV information to
metrics