-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 kubernetes persistentvolume metrics #1932
Conversation
Is this still WIP? |
Yup, I did not take so much to work on it. I'll finish it now! |
0d7077d
to
16c4798
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.
Aggregation logic looks good to me. We need to update the metadata accordingly, and ideally add some lines to the text fixtures.
I'm wondering what the actually use case for kube_persistentvolumeclaim_resource_requests_storage_bytes
is, do we need the individual pvc ids, or could that be aggregated too?
@@ -72,7 +72,9 @@ def __init__(self, name, init_config, agentConfig, instances=None): | |||
'kube_node_status_capacity_nvidia_gpu_cards': 'node.gpu.cards_capacity', | |||
'kube_pod_container_status_terminated': 'container.terminated', | |||
'kube_pod_container_status_waiting': 'container.waiting', | |||
'kube_persistentvolume_status_phase': 'persistentvolume.status', |
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.
When a metric is handled with a method, it must not be listed in the metrics_mapper
, or the mapper will take precedence. Could you please remove this?
kubernetes_state/metadata.csv
Outdated
@@ -41,6 +41,10 @@ kubernetes_state.node.pods_capacity,gauge,,,,The total pod resources of the node | |||
kubernetes_state.node.gpu.cards_allocatable,gauge,,,,The GPU resources of a node that are available for scheduling,0,kubernetes,k8s_state.node.gpu.cards_allocatable | |||
kubernetes_state.node.gpu.cards_capacity,gauge,,,,The total GPU resources of the node,0,kubernetes,k8s_state.node.gpu.cards_capacity | |||
kubernetes_state.persistentvolumeclaim.status,gauge,,,,The phase the persistent volume claim is currently in,-1,kubernetes,k8s_state.persistentvolumeclaim.status | |||
kubernetes_state.persistentvolumeclaim.info,gauge,,,,The name and storage class of the persistent volume claim,-1,kubernetes,k8s_state.persistentvolumeclaim.info |
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.
These metadata should be updated too
'kube_persistentvolumeclaim_status_phase': 'persistentvolumeclaim.status', | ||
'kube_persistentvolumeclaim_resource_requests_storage_bytes': 'persistentvolumeclaim.request_storage_bytes', |
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'm relunctant to add yet another pvc-cardinality gauge (the existing persistentvolumeclaim.status
is an anomaly and should have been aggregated). What is the exact use case for it?
16c4798
to
ec64297
Compare
kubernetes_state/metadata.csv
Outdated
@@ -41,6 +41,8 @@ kubernetes_state.node.pods_capacity,gauge,,,,The total pod resources of the node | |||
kubernetes_state.node.gpu.cards_allocatable,gauge,,,,The GPU resources of a node that are available for scheduling,0,kubernetes,k8s_state.node.gpu.cards_allocatable | |||
kubernetes_state.node.gpu.cards_capacity,gauge,,,,The total GPU resources of the node,0,kubernetes,k8s_state.node.gpu.cards_capacity | |||
kubernetes_state.persistentvolumeclaim.status,gauge,,,,The phase the persistent volume claim is currently in,-1,kubernetes,k8s_state.persistentvolumeclaim.status | |||
kubernetes_state.persistentvolumeclaim.request_storage_bytes,gauge,,,,The amount of bytes requested by the persistent volume claim,-1,kubernetes,k8s_state.persistentvolumeclaim.request_storage_bytes | |||
kubernetes_state.persistentvolume.status,gauge,,,,The phase the persistent volume is currently in,-1,kubernetes,k8s_state.persistentvolume.status |
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.
Let's inspire ourselves from the nodes.by_condition
gauge, what about:
kubernetes_state.persistentvolumes.by_phase,gauge,,,,Number of persistent volume, to sum by phase and storageclass.,0,kubernetes,k8s_state.pv.by_phase
ec64297
to
5380f9b
Compare
Looks good, let's add tests and merge |
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.
LGTM !
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 does this PR do?
It adds the export of persistent volume and claims related metrics.
Motivation
The need to monitor those metrics.