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

Jump to bottom

Add a metric that reports the VM snapshot create timestamp #12645

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add a metric that reports the VM snapshot create timestamp #12645

wants to merge 1 commit into from
285 −66

Conversation

avlitman
Copy link
Contributor

@avlitman avlitman commented Aug 21, 2024 •
edited
Loading

What this PR does

Add kubevirt_vmsnapshot_succeeded_timestamp_seconds metric, which reports the create timestamp per VM succeeded snapshot. In addition it also reports the "vm_name", "snapshot_name".

jira-ticket: https://issues.redhat.com/browse/CNV-46581

Why we need it and why it was done in this way

This metric will help users to identify VMs that didn't created a snapshot for a long time, also show the current state of vm's snapshots in a dashboard.

Release note

Add kubevirt_vmsnapshot_succeeded_timestamp_seconds metric

@kubevirt-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. sig/observability Denotes an issue or PR that relates to observability. sig/storage labels Aug 21, 2024
@avlitman avlitman force-pushed the add-snapshot-time-metric branch 2 times, most recently from 38e5671 to 33e736f Compare August 21, 2024 16:37
@avlitman avlitman force-pushed the add-snapshot-time-metric branch 2 times, most recently from a140a95 to 7d5437a Compare August 22, 2024 10:06
machadovilaca
machadovilaca suggested changes Aug 22, 2024
View reviewed changes
pkg/monitoring/metrics/virt-controller/vmsnapshot.go Show resolved Hide resolved
pkg/monitoring/metrics/virt-controller/vmsnapshot.go Outdated Show resolved Hide resolved
pkg/monitoring/metrics/virt-controller/vmsnapshot.go Outdated Show resolved Hide resolved
pkg/monitoring/metrics/virt-controller/vmsnapshot.go Outdated Show resolved Hide resolved
pkg/monitoring/metrics/virt-controller/vmsnapshot.go Outdated Show resolved Hide resolved
pkg/monitoring/metrics/virt-controller/vmsnapshot.go Outdated Show resolved Hide resolved
pkg/monitoring/metrics/virt-controller/vmsnapshot_test.go Show resolved Hide resolved
pkg/monitoring/metrics/virt-controller/vmsnapshot_test.go Outdated Show resolved Hide resolved
pkg/storage/snapshot/snapshot.go Outdated Show resolved Hide resolved
pkg/storage/snapshot/snapshot.go Outdated Show resolved Hide resolved
@avlitman
Copy link
Contributor Author

avlitman commented Aug 26, 2024 •
edited
Loading

Hi @machadovilaca I fixed most of your comments, the test failed with:

  [FAILED] Expected
      <float64>: 0
  to equal
      <float64>: 1.724677689e 09
  In [It] at: pkg/monitoring/metrics/virt-controller/vmsnapshot_test.go:52

Trying to understand why

@machadovilaca
Copy link
Member

Hi @machadovilaca I fixed most of your comments, the test failed with:

  [FAILED] Expected
      <float64>: 0
  to equal
      <float64>: 1.724677689e 09
  In [It] at: pkg/monitoring/metrics/virt-controller/vmsnapshot_test.go:52

Trying to understand why

can you push the new changes?

@sradco
Copy link
Contributor

sradco commented Aug 26, 2024

@avlitman can the VM namespace be different then the snapshot namespace?

ShellyKa13
ShellyKa13 reviewed Aug 27, 2024
View reviewed changes
pkg/monitoring/metrics/virt-controller/vmsnapshot.go Outdated Show resolved Hide resolved
pkg/storage/snapshot/snapshot.go Outdated Show resolved Hide resolved
pkg/storage/snapshot/snapshot.go Outdated Show resolved Hide resolved
pkg/monitoring/metrics/virt-controller/vmsnapshot.go Outdated Show resolved Hide resolved
@avlitman avlitman force-pushed the add-snapshot-time-metric branch 4 times, most recently from e70bedc to caf22c9 Compare August 29, 2024 09:26
@avlitman avlitman marked this pull request as ready for review August 29, 2024 09:26
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2024
@avlitman avlitman requested a review from sradco August 29, 2024 09:26
@avlitman avlitman force-pushed the add-snapshot-time-metric branch 16 times, most recently from a8693ce to 3e17eff Compare September 8, 2024 12:09
@machadovilaca
Copy link
Member

/retest

@avlitman
Add a metric that reports the VM snapshot create timestamp
e647361 
Add kubevirt_vmsnapshot_succeeded_timestamp_seconds metric, which
reports the create timestamp per VM succeeded snapshot. In addition it
also reports the "vm_name", "snapshot_name".

Signed-off-by: avlitman <[email protected]>
@avlitman
Copy link
Contributor Author

/retest

1 similar comment
@avlitman
Copy link
Contributor Author

/retest

avlitman
avlitman commented Sep 10, 2024
View reviewed changes
pkg/monitoring/metrics/virt-controller/vmsnapshot_test.go

metrics.NewVMSnapshotCreated(vmSnapshot)

metricTime, err := metrics.GetVmSnapshotSucceededTimestamp("vm-name", "snapshot-name", "namespace")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sradco vm-name it's not the label name it's just a name string for the test, here is the label name https://github.com/kubevirt/kubevirt/pull/12645/files#diff-1a38952b4e678a893790c3fe9ffd86cc9865aa1976fb163ad29ebd456eb47834R39

@kubevirt-bot
Copy link
Contributor

@avlitman: The following tests 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-check-tests-for-flakes e647361 link false /test pull-kubevirt-check-tests-for-flakes
pull-kubevirt-e2e-arm64 e647361 link false /test pull-kubevirt-e2e-arm64

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-sigs/prow repository. I understand the commands that are listed here.

@machadovilaca
Copy link
Member

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: machadovilaca
Once this PR has been reviewed and has the lgtm label, please ask for approval from enp0s3. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@enp0s3
Copy link
Contributor

enp0s3 commented Sep 10, 2024

/cc

@sradco
Copy link
Contributor

sradco commented Sep 11, 2024

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShellyKa13 ShellyKa13 ShellyKa13 approved these changes

@assafad assafad Awaiting requested review from assafad

@akalenyu akalenyu Awaiting requested review from akalenyu

@AlonaKaplan AlonaKaplan Awaiting requested review from AlonaKaplan

@sradco sradco Awaiting requested review from sradco

@machadovilaca machadovilaca Awaiting requested review from machadovilaca

@enp0s3 enp0s3 Awaiting requested review from enp0s3

Assignees

@sradco sradco

@machadovilaca machadovilaca

@enp0s3 enp0s3

@ShellyKa13 ShellyKa13

Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/observability Denotes an issue or PR that relates to observability. sig/storage size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@avlitman @kubevirt-bot @machadovilaca @sradco @ShellyKa13 @enp0s3