Add total VMs created metric #10418

machadovilaca · 2023-09-12T12:19:12Z

What this PR does / why we need it:

Track the total number of VMs created by virt-controller

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 #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note:

Add total VMs created metric

https://issues.redhat.com/browse/CNV-15536

/cc @sradco @enp0s3

fabiand · 2023-09-13T07:58:32Z

docs/metrics.md

@@ -81,6 +81,9 @@ The current available memory of the VM containers based on the rss. Type: Gauge.
 ### kubevirt_vm_container_free_memory_bytes_based_on_working_set_bytes
 The current available memory of the VM containers based on the working set. Type: Gauge.

+### kubevirt_vm_created_total
+Amount of VMs created, broken down by namespace. Type: Counter.


Created since when?
Since instal, update, controller pod restart, phase of the moon :) …

we can't obviously have the counter track VMs from previous versions, but other than that Prometheus takes care of summing all counter values.

maybe this is a good opportunity to think once again about more metrics metadata, release lifecycle, version when it was added, when they are deprecated, etc... (would also help with the discussion in https://groups.google.com/g/kubevirt-dev/c/7p3q5Lo71hs/m/jIJGNJpdAwAJ)

fabiand · 2023-09-13T07:58:52Z

docs/metrics.md

@@ -81,6 +81,9 @@ The current available memory of the VM containers based on the rss. Type: Gauge.
 ### kubevirt_vm_container_free_memory_bytes_based_on_working_set_bytes
 The current available memory of the VM containers based on the working set. Type: Gauge.

+### kubevirt_vm_created_total
+Amount of VMs created, broken down by namespace. Type: Counter.
+


And why do we need this metric?

providers using KubeVirt (through insights) and users would benefit from this because it would ease the process of understanding the VM usage through time (with absolute values and increase/delta/etc functions to understand trends)

The counter appears to be incremented whenever a VM is:

Created for the first time

Deleted and recreated

Reconciled the first time by a new instance of virt-controller

On a system with a lot of churn there will be a big drop when virt-controller is restarted. Maybe that's not a big deal? How is that handled by consumers?

Prometheus handles the counter restarting at 0, when virt-controller is restarted.

The only issue here would be point 3: "Reconciled the first time by a new instance of virt-controller". But with:

// VM is nil or already processed if vm == nil || len(vm.Status.Conditions) != 0 { return }

that I added in func NewVMCreated(vm *v1.VirtualMachine), since the VM would have the status conditions set, I would expect it not to happen, am I missing something?

akalenyu · 2023-10-19T10:49:18Z

pkg/monitoring/virt-controller/metrics/vm_metrics.go

+		[]string{"namespace"},
+	)
+
+	vmMap = map[string]bool{}


So we're essentially keeping state in the virt-controller here, which (correct me if I'm wrong) means that the created VM metric will start lying as soon as the virt-controller pod dies?

this state is to prevent "concurrent" requests when the VM is first created
because the vm.Status.Conditions are not set yet in these (which would make this function being skipped as I mentioned in #10418 (comment))

I mean, on a system with existing VMs, when the virt-controller dies because of maintenance or something,
the metric would reset to 0 (lie). Is that not the case?

That is expected with counters, Prometheus handles those counter resets

So we would be okay with this metric not reporting the correct amount of VMs after a restart?

I wouldn't put it in that way. We want the metric correctly report the number of VMs created by that instance of 'virt-controller'

So you can query it generally? so the sum of all instances that ever existed, including ones that died?

enp0s3

@machadovilaca @sradco Hi. My apologies but I find it hard to follow the motivation behind creating this metric. From my POV we can also add metric for VM deletion, VM modification. But what is the use-case we are trying to reach here? why is it important to track the creation rate?

machadovilaca · 2023-10-30T10:48:42Z

@machadovilaca @sradco Hi. My apologies but I find it hard to follow the motivation behind creating this metric. From my POV we can also add metric for VM deletion, VM modification. But what is the use-case we are trying to reach here? why is it important to track the creation rate?

@enp0s3
Currently we have no way of getting an historical information of how many VMs were created in a given cluster.
Yes we have some metrics tracking status change, but those are only good during the Prometheus retention time, after that those number are gone.
This metric would allows us to know the 'real' over-time usage of KubeVirt

enp0s3 · 2023-10-30T12:59:41Z

@machadovilaca @sradco Hi. My apologies but I find it hard to follow the motivation behind creating this metric. From my POV we can also add metric for VM deletion, VM modification. But what is the use-case we are trying to reach here? why is it important to track the creation rate?

@enp0s3 Currently we have no way of getting an historical information of how many VMs were created in a given cluster. Yes we have some metrics tracking status change, but those are only good during the Prometheus retention time, after that those number are gone. This metric would allows us to know the 'real' over-time usage of KubeVirt

@machadovilaca Thank you for the reply. I see this as an answer to what but not to why

sradco · 2023-10-31T12:07:07Z

The why is as @machadovilaca mentioned.
"This metric would allows us to know the 'real' over-time usage of KubeVirt".

xpivarc · 2023-11-08T15:15:33Z

Did we check if any existing Kubernetes metric could be used for this?

machadovilaca · 2023-11-08T15:33:27Z

Did we check if any existing Kubernetes metric could be used for this?

yes, we could track virt-launcher pods or even use KubeVirt VM status metric, the problem with those approaches is that we would lose information previous to retention date

machadovilaca · 2023-11-15T16:42:05Z

@xpivarc

xpivarc

I wonder why don't we implement this in the admitter? The admitter will process each VM exactly once(at least this hold for creation). It seems to me we will get the wrong numbers for the metric in the current implementation.

machadovilaca · 2023-11-20T12:26:32Z

I wonder why don't we implement this in the admitter? The admitter will process each VM exactly once(at least this hold for creation). It seems to me we will get the wrong numbers for the metric in the current implementation.

@xpivarc good suggesting, I updated the PR to push the metric on vms-admitter

xpivarc · 2023-11-20T13:06:34Z

pkg/monitoring/virt-controller/metrics/vm_metrics.go

+	)
+)
+
+func NewVMCreated(vm *v1.VirtualMachine) {


Can we reduce the vm to namespace?

since we are using a pointer I think we don't have any performance problems with copying the structure, and personally, I think it makes more sense that a function NewVMCreated receives a VM as an argument and not a Namespace. Even if in the future we want more labels or create new metrics in the function, it would be easier

xpivarc · 2023-11-20T13:08:35Z

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go

@@ -210,6 +211,10 @@ func (admitter *VMsAdmitter) Admit(ar *admissionv1.AdmissionReview) *admissionv1
 	reviewResponse := admissionv1.AdmissionResponse{}
 	reviewResponse.Allowed = true

+	if ar.Request.Operation == admissionv1.Create {
+		metrics.NewVMCreated(&vm)


I think vm might not have the namespace set. Could you check if we populate it?

I think a resource always comes with a namespace, even if it is not set, it gets the value of the current namespace by default

xpivarc · 2023-11-20T13:09:12Z

docs/metrics.md

@@ -66,6 +66,9 @@ The current available memory of the VM containers based on the rss. Type: Gauge.
 ### kubevirt_vm_container_free_memory_bytes_based_on_working_set_bytes
 The current available memory of the VM containers based on the working set. Type: Gauge.

+### kubevirt_vm_created_total
+Amount of VMs created, broken down by namespace. Type: Counter.


As @fabiand pointed out, you might want to elaborate on this.

xpivarc · 2023-11-28T13:17:17Z

/cc @fossedihelm

fossedihelm

Thanks @machadovilaca! few comments below

pkg/monitoring/virt-api/metrics/metrics.go

pkg/monitoring/virt-api/metrics/vm_metrics.go

fossedihelm · 2023-11-28T13:32:30Z

pkg/virt-api/api.go

+	// setup monitoring
+	err = metrics.SetupMetrics()
+	if err != nil {
+		return


is it correct to silently return?

I think you are correct, added a panic

fossedihelm · 2023-11-30T06:39:15Z

@machadovilaca IIUC the metric is reset every time the instance of the virt-api is restarted.
This means that there is the possibility that the reported value is not correct.
From this point of view, how can a metric that can report wrong values, be useful?
I mean, I think that the reported value could not be real, and as consequence, all the related statistics.
WDYT?
Thanks!

machadovilaca · 2023-11-30T11:01:12Z

@machadovilaca IIUC the metric is reset every time the instance of the virt-api is restarted. This means that there is the possibility that the reported value is not correct. From this point of view, how can a metric that can report wrong values, be useful? I mean, I think that the reported value could not be real, and as consequence, all the related statistics. WDYT? Thanks!

@fossedihelm that is the expected behavior and common in Prometheus. Since we are using a counter, which is monotonically increasing, Prometheus has mechanisms to handle when a value goes back to an inferior value (with the restart of the virt-api for example, where we would start counting from zero again), what they call a 'counter restart'

fossedihelm · 2023-12-04T10:51:52Z

/lgtm
Thank you!

xpivarc · 2023-12-04T11:32:13Z

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go

@@ -210,6 +211,10 @@ func (admitter *VMsAdmitter) Admit(ar *admissionv1.AdmissionReview) *admissionv1
 	reviewResponse := admissionv1.AdmissionResponse{}
 	reviewResponse.Allowed = true

+	if ar.Request.Operation == admissionv1.Create {


This might be a dry run, please add a check.

Please, check for nils as well

Signed-off-by: João Vilaça <jvilaca@redhat.com>

machadovilaca · 2023-12-04T13:19:29Z

/retest

xpivarc

/approve

xpivarc · 2023-12-04T13:22:09Z

@fossedihelm PTAL

kubevirt-bot · 2023-12-04T13:22:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

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:

~~OWNERS~~ [xpivarc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fossedihelm

Thanks

kubevirt-bot requested review from enp0s3 and sradco September 12, 2023 12:19

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L area/monitoring labels Sep 12, 2023

fabiand reviewed Sep 13, 2023

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2023

machadovilaca force-pushed the add-total-vm-created-metric branch from 5a0b77d to 10f6bf8 Compare October 9, 2023 11:49

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2023

machadovilaca force-pushed the add-total-vm-created-metric branch 2 times, most recently from 10f6bf8 to cc992c2 Compare October 9, 2023 14:38

akalenyu reviewed Oct 19, 2023

View reviewed changes

enp0s3 reviewed Oct 29, 2023

View reviewed changes

xpivarc reviewed Nov 16, 2023

View reviewed changes

machadovilaca force-pushed the add-total-vm-created-metric branch from 28bb15d to 903533e Compare November 20, 2023 12:25

kubevirt-bot added size/M and removed size/L labels Nov 20, 2023

machadovilaca force-pushed the add-total-vm-created-metric branch from 903533e to c2f83e5 Compare November 20, 2023 12:25

xpivarc reviewed Nov 20, 2023

View reviewed changes

machadovilaca force-pushed the add-total-vm-created-metric branch 2 times, most recently from e9704dd to 1787ffc Compare November 21, 2023 14:02

kubevirt-bot requested a review from fossedihelm November 28, 2023 13:17

fossedihelm reviewed Nov 28, 2023

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2023

machadovilaca force-pushed the add-total-vm-created-metric branch from 1787ffc to 8a8e125 Compare November 29, 2023 15:57

kubevirt-bot added size/L and removed size/M needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 29, 2023

kubevirt-bot assigned fossedihelm Dec 4, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023

xpivarc reviewed Dec 4, 2023

View reviewed changes

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023

Add total VMs created metric

bdddab0

Signed-off-by: João Vilaça <jvilaca@redhat.com>

machadovilaca force-pushed the add-total-vm-created-metric branch from 8d137f9 to bdddab0 Compare December 4, 2023 11:59

xpivarc reviewed Dec 4, 2023

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2023

fossedihelm approved these changes Dec 4, 2023

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023

kubevirt-bot merged commit a4ee72f into kubevirt:main Dec 5, 2023
39 checks passed

machadovilaca deleted the add-total-vm-created-metric branch December 5, 2023 10:08

chenrui333 mentioned this pull request Mar 6, 2024

virtctl 1.2.0 Homebrew/homebrew-core#165266

Merged

Add total VMs created metric #10418

Add total VMs created metric #10418

Conversation

machadovilaca commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalenyu Oct 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enp0s3 left a comment

Choose a reason for hiding this comment

machadovilaca commented Oct 30, 2023 • edited

enp0s3 commented Oct 30, 2023

sradco commented Oct 31, 2023 • edited

xpivarc commented Nov 8, 2023

machadovilaca commented Nov 8, 2023

machadovilaca commented Nov 15, 2023

xpivarc left a comment

Choose a reason for hiding this comment

machadovilaca commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpivarc commented Nov 28, 2023

fossedihelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fossedihelm commented Nov 30, 2023

machadovilaca commented Nov 30, 2023

fossedihelm commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machadovilaca commented Dec 4, 2023

xpivarc left a comment

Choose a reason for hiding this comment

xpivarc commented Dec 4, 2023

kubevirt-bot commented Dec 4, 2023

fossedihelm left a comment

Choose a reason for hiding this comment

akalenyu Oct 19, 2023 •

edited

machadovilaca commented Oct 30, 2023 •

edited

sradco commented Oct 31, 2023 •

edited