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

Refactor kubevirt_vmi_number_of_outdated #11538

Merged
merged 1 commit into from Apr 10, 2024
Merged

Refactor kubevirt_vmi_number_of_outdated #11538

merged 1 commit into from Apr 10, 2024
+67 −30

Conversation

machadovilaca
Copy link
Member

What this PR does

Before this PR:

kubevirt_vmi_number_of_outdated was not refactored with the rest of the metrics

After this PR:

kubevirt_vmi_number_of_outdated is also refactored

Fixes #

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

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

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

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.

Release note

none

@kubevirt-bot kubevirt-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 18, 2024
@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Mar 18, 2024
@kubevirt-bot kubevirt-bot added area/monitoring sig/buildsystem Denotes an issue or PR that relates to changes in the build system. size/M labels Mar 18, 2024
@machadovilaca
Copy link
Member Author

/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

@machadovilaca
Refactor kubevirt_vmi_number_of_outdated
44b3f54 
Signed-off-by: João Vilaça <jvilaca@redhat.com>
@machadovilaca machadovilaca force-pushed the refactor-kubevirt_vmi_number_of_outdated branch from 23bdbff to 44b3f54 Compare March 19, 2024 13:28
@machadovilaca
Copy link
Member Author

/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

@avlitman
Copy link
Contributor

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2024
avlitman
avlitman reviewed Mar 19, 2024
View reviewed changes
pkg/virt-controller/watch/workload-updater/workload-updater.go
@@ -31,6 +30,7 @@ import (
"kubevirt.io/client-go/log"

"kubevirt.io/kubevirt/pkg/controller"
metrics "kubevirt.io/kubevirt/pkg/monitoring/metrics/virt-controller"
Copy link
Contributor

@avlitman avlitman Mar 19, 2024 •
edited

Choose a reason for hiding this comment

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

nit: I think we can remove the empty lines (line 25, 27, 31) since all is kubevirt.io

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch

although "kubevirt.io/client-go/... imports are usually separated, but yes, a few others out of place

anyway I think that is not very related to this PR, maybe could be done in a separate one
by you maybe :D

@avlitman avlitman mentioned this pull request Apr 8, 2024
Closed
sradco
sradco reviewed Apr 8, 2024
View reviewed changes
Copy link
Contributor

@sradco sradco left a comment

Choose a reason for hiding this comment

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

/lgtm

@sradco
Copy link
Contributor

sradco commented Apr 8, 2024

/approve

enp0s3
enp0s3 reviewed Apr 9, 2024
View reviewed changes
Copy link
Contributor

@enp0s3 enp0s3 left a comment

Choose a reason for hiding this comment

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

@machadovilaca @sradco Hi. To me it looks like it will be more accurate to unit test the virt_controller package you've created in the metrics directory, because currently each controller tests these metrics in its own suite (hopefully).

enp0s3
enp0s3 reviewed Apr 9, 2024
View reviewed changes
Copy link
Contributor

@enp0s3 enp0s3 left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enp0s3, sradco

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2024
@kubevirt-commenter-bot
Copy link

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

3 similar comments
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit eaab797 into kubevirt:main Apr 10, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enp0s3 enp0s3 enp0s3 left review comments

@avlitman avlitman avlitman left review comments

@sradco sradco sradco approved these changes

@brianmcarey brianmcarey Awaiting requested review from brianmcarey

@mhenriks mhenriks Awaiting requested review from mhenriks

Assignees

@sradco sradco

@avlitman avlitman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/monitoring 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-none Denotes a PR that doesn't merit a release note. sig/buildsystem Denotes an issue or PR that relates to changes in the build system. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@machadovilaca @avlitman @sradco @kubevirt-bot @kubevirt-commenter-bot @enp0s3