Add VMI filesystem usage metrics #7814

machadovilaca · 2022-05-27T11:06:29Z

Currently, in the dashboard, under Virtualization -> Overview, the filesystem
report uses a metric about the pod filesystem usage. This PR adds new metrics
with values corresponding to VM filesystem usage so they can be used in the
dashboard.

What this PR does / why we need it:

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:

Release note:

Add VMI filesystem usage metrics

machadovilaca · 2022-05-27T15:04:25Z

/retest

machadovilaca · 2022-05-29T18:39:59Z

/retest

enp0s3 · 2022-06-06T16:15:30Z

/cc

enp0s3

Hi @machadovilaca Thank you for the PR
I have a small question below

enp0s3 · 2022-07-07T07:31:34Z

pkg/monitoring/domainstats/prometheus/prometheus.go

+		metrics.pushCustomMetric(
+			"kubevirt_vmi_filesystem_total_bytes",
+			"Total VM filesystem capacity in bytes.",
+			prometheus.GaugeValue,


Why have you decided to work with Gauge metric instead of Counter?

Per my understanding, for some types of filesystem, it is possible to change the total capacity, so this value might both go up and down, so I think we should use the Gauge here

enp0s3 · 2022-07-07T07:31:42Z

pkg/monitoring/domainstats/prometheus/prometheus.go

+		metrics.pushCustomMetric(
+			"kubevirt_vmi_filesystem_used_bytes",
+			"Used VM filesystem capacity in bytes.",
+			prometheus.GaugeValue,


Here, this value might go up and down arbitrarily, so I think we should use the Gauge

enp0s3

@machadovilaca Thank you! overall looks good, please see my comments below

enp0s3 · 2022-07-18T16:04:38Z

pkg/monitoring/domainstats/prometheus/prometheus.go

+	fsLabels := []string{"disk"}
+
+	for _, fsStat := range vmFSStats.Items {
+		fsLabelValues := []string{fsStat.DiskName}


@machadovilaca Following is the struct:

type VirtualMachineInstanceFileSystem struct { DiskName string `json:"diskName"` MountPoint string `json:"mountPoint"` FileSystemType string `json:"fileSystemType"` UsedBytes int `json:"usedBytes"` TotalBytes int `json:"totalBytes"` }

I see that you don't scrap the MountPoint and the FileSystemType values, is that in purpose?

not really, will add

enp0s3 · 2022-07-18T16:27:35Z

pkg/monitoring/domainstats/prometheus/prometheus.go

+		return
+	}
+
+	// GetDomainStats() or  may hang for a long time.


@machadovilaca Sorry I didn't understand the comment, can you please elaborate?

misadded "or "

enp0s3 · 2022-07-18T16:30:00Z

pkg/monitoring/domainstats/prometheus/prometheus.go

@@ -536,17 +562,23 @@ func (ps *prometheusScraper) Scrape(socketFile string, vmi *k6tv1.VirtualMachine
 	}
 	defer cli.Close()

-	vmStats, exists, err := cli.GetDomainStats()
+	vmDomainStats, exists, err := cli.GetDomainStats()


@machadovilaca Can you please create a separate commit for the refactoring of the vmStats name? please add in the commit message the reason why the new name is better

Does this still make sense after https://github.com/kubevirt/kubevirt/pull/7814/files#r923806175 ?

enp0s3 · 2022-07-18T20:00:43Z

pkg/monitoring/domainstats/prometheus/prometheus.go

 }

-func (ps *prometheusScraper) Report(socketFile string, vmi *k6tv1.VirtualMachineInstance, vmStats *stats.DomainStats) {
-	// statsMaxAge is an estimation - and there is not better way to do that. So it is possible that
+func (ps *prometheusScraper) Report(socketFile string, vmi *k6tv1.VirtualMachineInstance, vmDomainStats *stats.DomainStats, vmFSStats k6tv1.VirtualMachineInstanceFileSystemList) {


@machadovilaca I think it could be better if you will create a type vmStats struct and you will be able to encapsulate the vmDomainStats and vmFSStats there

👍 great idea.
This way even the vmStats name can stay, only the type would change.
In the long term, it would keep our function signatures small, therefore more readable and easily changable.

I like it, changed

enp0s3 · 2022-07-18T20:02:41Z

pkg/monitoring/domainstats/prometheus/prometheus_test.go

@@ -69,7 +73,7 @@ var _ = Describe("Prometheus", func() {

 			ps := prometheusScraper{ch: ch}

-			vmStats := &stats.DomainStats{
+			vmDomainStats := &stats.DomainStats{


@machadovilaca same here about a dedicated commit for refactoring

Does this still make sense after https://github.com/kubevirt/kubevirt/pull/7814/files#r923806175 ?

enp0s3 · 2022-07-18T20:04:11Z

pkg/monitoring/domainstats/prometheus/prometheus_test.go

@@ -99,7 +103,7 @@ var _ = Describe("Prometheus", func() {
 				},
 			}
 			vmi := k6tv1.VirtualMachineInstance{}
-			ps.Report("test", &vmi, vmStats)
+			ps.Report("test", &vmi, vmStats, emptyVmFSStats)


@machadovilaca Here you've left the vmStats although you've refactored its name in previous places

changed when using the suggested struct

iholder101

Thanks @machadovilaca! Good work!

iholder101 · 2022-07-19T09:17:48Z

pkg/monitoring/domainstats/prometheus/prometheus.go

+	fsLabels := []string{"disk"}
+
+	for _, fsStat := range vmFSStats.Items {


nit: Maybe return if len(vmFSStats.Items) == 0? This would prevent allocating fsLabels := []string{"disk"} for these scenarios.

iholder101 · 2022-07-19T09:19:34Z

pkg/monitoring/domainstats/prometheus/prometheus.go

+	for _, fsStat := range vmFSStats.Items {
+		fsLabelValues := []string{fsStat.DiskName}
+
+		metrics.pushCustomMetric(


I'm wondering - why not publish all data we have? e.g. MountPoint and FileSystemType? Maybe as labels?

I think it is what @enp0s3 suggested in https://github.com/kubevirt/kubevirt/pull/7814/files#r923548381

Sorry but this link just leads me to "Files" tab :)

Oh you meant #7814 (comment). Got it. Thanks

iholder101 · 2022-07-19T09:23:37Z

pkg/monitoring/domainstats/prometheus/prometheus_test.go

+var emptyVmFSStats = k6tv1.VirtualMachineInstanceFileSystemList{
+	Items: []k6tv1.VirtualMachineInstanceFileSystem{},
+}
+


Having global mutabale variables for tests is a bad practice. I think you should choose one of the two options:

Declare the variable globally but initialize it in a BeforeEach clause

Replace the variable with a function that returns an empty VMFSStats

Option 2 is safer and better imho, it isolates tests from one another and more readable

Also, this relates to @enp0s3's suggestion about vmStats struct. If this was the call, all you would need to change here is to initialize another field in the struct.

iholder101 · 2022-07-19T09:25:51Z

pkg/monitoring/domainstats/prometheus/prometheus_test.go

+			Expect(result.Desc().String()).To(ContainSubstring("kubevirt_vmi_filesystem_total_bytes"))
+			result = <-ch
+			Expect(result).ToNot(BeNil())
+			Expect(result.Desc().String()).To(ContainSubstring("kubevirt_vmi_filesystem_used_bytes"))


Maybe expect the channel to be empty here?

machadovilaca · 2022-07-20T12:22:07Z

/test pull-kubevirt-e2e-k8s-1.22-sig-compute

iholder101

Thanks @machadovilaca! Great work!
/lgtm

left small nit

iholder101 · 2022-07-20T13:50:35Z

pkg/monitoring/domainstats/prometheus/prometheus.go

@@ -543,6 +573,11 @@ type prometheusScraper struct {
 	ch chan<- prometheus.Metric
 }

+type VirtualMachineStats struct {


nit: Maybe VirtualMachineInstanceStats is more accurate?

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

machadovilaca · 2022-07-21T09:11:27Z

/retest

machadovilaca · 2022-07-21T14:40:40Z

/test pull-kubevirt-e2e-kind-1.22-sriov

machadovilaca · 2022-07-22T10:14:43Z

/test pull-kubevirt-e2e-kind-1.22-sriov

machadovilaca · 2022-07-25T09:26:46Z

/test pull-kubevirt-e2e-kind-1.22-sriov

stu-gott

/approve

kubevirt-bot · 2022-07-25T16:19:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stu-gott

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~~ [stu-gott]

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

machadovilaca · 2022-07-26T04:03:09Z

/retest

machadovilaca · 2022-07-26T09:15:05Z

/retest

kubevirt-bot · 2022-07-26T12:14:17Z

@machadovilaca: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-fossa	`5621389`	link	false	`/test pull-kubevirt-fossa`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

machadovilaca · 2022-07-26T13:04:12Z

/test pull-kubevirt-e2e-k8s-1.22-operator

machadovilaca · 2022-07-26T20:49:31Z

/test pull-kubevirt-unit-test

machadovilaca · 2022-10-18T13:10:45Z

/cherry-pick release-0.53

kubevirt-bot · 2022-10-18T13:11:25Z

@machadovilaca: new pull request created: #8621

In response to this:

/cherry-pick release-0.53

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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 May 27, 2022

kubevirt-bot requested review from EdDev and yuhaohaoyu May 27, 2022 11:06

kubevirt-bot requested a review from enp0s3 June 6, 2022 16:15

enp0s3 reviewed Jul 7, 2022

View reviewed changes

enp0s3 reviewed Jul 18, 2022

View reviewed changes

iholder101 reviewed Jul 19, 2022

View reviewed changes

machadovilaca force-pushed the add-kubevirt-filesystem-usage-metric branch 3 times, most recently from 30844a0 to c23fb96 Compare July 20, 2022 10:50

iholder101 reviewed Jul 20, 2022

View reviewed changes

kubevirt-bot assigned iholder101 Jul 20, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2022

machadovilaca added 3 commits July 20, 2022 15:10

Add VMI filesystem usage metrics

8e02996

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

Add unit tests for VMI filesystem usage metrics

d22bd98

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

Add documentation for VMI filesystem usage metrics

5621389

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

machadovilaca force-pushed the add-kubevirt-filesystem-usage-metric branch from c23fb96 to 5621389 Compare July 20, 2022 14:10

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2022

iholder101 approved these changes Jul 20, 2022

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2022

stu-gott reviewed Jul 25, 2022

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2022

kubevirt-bot merged commit cdb4032 into kubevirt:main Jul 27, 2022

kubevirt-bot mentioned this pull request Aug 4, 2022

Allow live-migration for pod network in bridge mode #7768

Closed

machadovilaca deleted the add-kubevirt-filesystem-usage-metric branch August 11, 2022 12:43

kubevirt-bot mentioned this pull request Oct 18, 2022

[release-0.53] Add VMI filesystem usage metrics #8621

Merged

		fsLabels := []string{"disk"}

		for _, fsStat := range vmFSStats.Items {

Add VMI filesystem usage metrics #7814

Add VMI filesystem usage metrics #7814

Conversation

machadovilaca commented May 27, 2022

machadovilaca commented May 27, 2022

machadovilaca commented May 29, 2022

enp0s3 commented Jun 6, 2022

enp0s3 left a 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

enp0s3 left a 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

machadovilaca Jul 19, 2022 • edited

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

iholder101 left a 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

iholder101 Jul 19, 2022 • edited

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

machadovilaca commented Jul 20, 2022

iholder101 left a 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

machadovilaca commented Jul 21, 2022

machadovilaca commented Jul 21, 2022

machadovilaca commented Jul 22, 2022

machadovilaca commented Jul 25, 2022

stu-gott left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Jul 25, 2022

machadovilaca commented Jul 26, 2022

machadovilaca commented Jul 26, 2022

kubevirt-bot commented Jul 26, 2022 • edited

machadovilaca commented Jul 26, 2022

machadovilaca commented Jul 26, 2022

machadovilaca commented Oct 18, 2022

kubevirt-bot commented Oct 18, 2022

machadovilaca Jul 19, 2022 •

edited

iholder101 Jul 19, 2022 •

edited

kubevirt-bot commented Jul 26, 2022 •

edited