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

Fix bug in pv_hourly_cost metric still emitting even after PV deletion #2355

Merged
merged 3 commits into from Dec 4, 2023

Conversation

thomasvn
Copy link
Contributor

@thomasvn thomasvn commented Dec 1, 2023

What does this PR change?

  • Deletes pv_hourly_cost metric from the PersistentVolumePriceRecorder once the PV no longer exists.
// Setting the label values for pv_hourly_cost.
cmme.PersistentVolumePriceRecorder.WithLabelValues(pv.Name, pv.Name, cacPv.ProviderID).Set(c)

// Deleting the pv_hourly_cost metric which corresponds with these label values
// The labels here *must* include values for [ pv.Name, pv.Name, cacPv.ProviderID ]
// Note that I found cacPV.ProviderID to always be an empty string, but we still need to key on this label regardless
cmme.PersistentVolumePriceRecorder.DeleteLabelValues(labels...)

Does this PR relate to any other PRs?

How will this PR impact users?

  • BEFORE. Users are seeing metrics emitted for pv_hourly_cost even after the deletion of the PV. No issues observed with other PV-related metrics.
  • AFTER. Users will no longer see metrics emitted for pv_hourly_cost once the PV is deleted.

Does this PR address any GitHub or Zendesk issues?

How was this PR tested?

  • Locally tested by validating that pv_hourly_cost metric was no longer being emitted

Old behavior:

$ curl 'localhost:9003/metrics' | rg -i "pvc-e424dc1b-eec0-4d83-9793-f693ba96d124"
kube_persistentvolume_capacity_bytes{persistentvolume="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124"} 1.073741824e+09
kube_persistentvolume_status_phase{persistentvolume="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124",phase="Available"} 0
kube_persistentvolume_status_phase{persistentvolume="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124",phase="Bound"} 1
kube_persistentvolume_status_phase{persistentvolume="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124",phase="Failed"} 0
kube_persistentvolume_status_phase{persistentvolume="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124",phase="Pending"} 0
kube_persistentvolume_status_phase{persistentvolume="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124",phase="Released"} 0
kube_persistentvolumeclaim_info{namespace="kubecost-nightly",persistentvolumeclaim="mypvc",storageclass="standard-rwo",volumename="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124"} 1
kubecost_pv_info{persistentvolume="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124",provider_id="projects/guestbook-227502/zones/us-east1-d/disks/pvc-e424dc1b-eec0-4d83-9793-f693ba96d124",storageclass="standard-rwo"} 1
pod_pvc_allocation{namespace="kubecost-nightly",persistentvolume="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124",persistentvolumeclaim="mypvc",pod="mypod"} 1.073741824e+09
pv_hourly_cost{persistentvolume="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124",provider_id="",volumename="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124"} 5.479452e-05

# After deleting the PVC
$ curl 'localhost:9003/metrics' | rg -i "pvc-e424dc1b-eec0-4d83-9793-f693ba96d124"
pv_hourly_cost{persistentvolume="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124",provider_id="",volumename="pvc-e424dc1b-eec0-4d83-9793-f693ba96d124"} 5.479452e-05

New behavior:

$ curl 'localhost:9003/metrics' | rg -i "pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce"
kube_persistentvolume_capacity_bytes{persistentvolume="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce"} 1.073741824e+09
kube_persistentvolume_status_phase{persistentvolume="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce",phase="Available"} 0
kube_persistentvolume_status_phase{persistentvolume="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce",phase="Bound"} 1
kube_persistentvolume_status_phase{persistentvolume="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce",phase="Failed"} 0
kube_persistentvolume_status_phase{persistentvolume="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce",phase="Pending"} 0
kube_persistentvolume_status_phase{persistentvolume="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce",phase="Released"} 0
kube_persistentvolumeclaim_info{namespace="kubecost-nightly",persistentvolumeclaim="mypvc",storageclass="standard-rwo",volumename="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce"} 1
kubecost_pv_info{persistentvolume="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce",provider_id="projects/guestbook-227502/zones/us-east1-d/disks/pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce",storageclass="standard-rwo"} 1
pod_pvc_allocation{namespace="kubecost-nightly",persistentvolume="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce",persistentvolumeclaim="mypvc",pod="mypod"} 1.073741824e+09
pv_hourly_cost{persistentvolume="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce",provider_id="",volumename="pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce"} 5.479452e-05

# After deleting the PVC, no responses returned which match this PVC
$ curl 'localhost:9003/metrics' | rg -i "pvc-5b39b6f6-ba0c-43a0-adbb-6bf22f0ebfce"

Does this PR require changes to documentation?

  • No, but I will write some nightly integration tests to ensure no regressions of this bug.

Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next OpenCost release? If not, why not?

  • v1.108?

to correctly delete the Prometheus Metric using the Recorder.

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>
Copy link

vercel bot commented Dec 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
opencost ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 2, 2023 0:25am

@thomasvn thomasvn changed the title Fix bug in pv_hourly_cost_metric still emitting even after PV deletion Fix bug in pv_hourly_cost metric still emitting even after PV deletion Dec 1, 2023
@thomasvn thomasvn added the v1.108 label Dec 2, 2023
Signed-off-by: thomasvn <thomasnguyen96@gmail.com>
@thomasvn
Copy link
Contributor Author

thomasvn commented Dec 2, 2023

@AjayTripathy I think this PR is ready for review! I've removed all debug logs, and have written up the tests I've performed. Only thing remaining is to write some nightly integration tests.

@mattray mattray merged commit 75d9465 into opencost:develop Dec 4, 2023
6 checks passed
@mattray
Copy link
Collaborator

mattray commented Dec 4, 2023

Merged, thanks @thomasvn!

@thomasvn thomasvn deleted the thomasn/ghost-pvhourlycost branch December 18, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants