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: Increase reconcile metric only if SSP CR was not changed #950

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

akrejcir
Copy link
Collaborator

@akrejcir akrejcir commented Apr 9, 2024

What this PR does / why we need it:
The metric kubevirt_ssp_common_templates_restored_total should be increased if user changes a template and the operator restores it back to what it should be.

If the SSP CR is changed, for example one of the labels that are propagated to the templates are changed, then the reconciliation should not increase the metric.

Fixes: https://issues.redhat.com/browse/CNV-39928

Release note:

None

The metric "kubevirt_ssp_common_templates_restored_total" should
be increased if user changes a template and the operator restores
it back to what it should be.

If the SSP CR is changed, for example one of the labels
that are propagated to the templates are changed,
then the reconciliation should not increase the metric.

Signed-off-by: Andrej Krejcir <[email protected]>
@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Apr 9, 2024
@kubevirt-bot kubevirt-bot requested review from jcanocan and ksimon1 April 9, 2024 09:58
@akrejcir
Copy link
Collaborator Author

akrejcir commented Apr 9, 2024

/cc @0xFelix @machadovilaca @ksimon1

Copy link

sonarqubecloud bot commented Apr 9, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

/approve

Looks good to me. But allow me a question, is there really a benefit to exposing this metric at all?

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

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
@akrejcir
Copy link
Collaborator Author

akrejcir commented Apr 9, 2024

/approve

Looks good to me. But allow me a question, is there really a benefit to exposing this metric at all?

I think it is a question for @machadovilaca or @sradco .

@sradco
Copy link
Collaborator

sradco commented Apr 9, 2024

/approve

Looks good to me. But allow me a question, is there really a benefit to exposing this metric at all?

I think it is a question for @machadovilaca or @sradco .

This metric is used for an alert.

@0xFelix
Copy link
Member

0xFelix commented Apr 9, 2024

/approve
Looks good to me. But allow me a question, is there really a benefit to exposing this metric at all?

I think it is a question for @machadovilaca or @sradco .

This metric is used for an alert.

Let me rephrase, is there a benefit to having that alert?

@sradco
Copy link
Collaborator

sradco commented Apr 9, 2024

/approve
Looks good to me. But allow me a question, is there really a benefit to exposing this metric at all?

I think it is a question for @machadovilaca or @sradco .

This metric is used for an alert.

Let me rephrase, is there a benefit to having that alert?

This is not for me or @machadovilaca to decide.
It up to your team.

From what I understand the alert is educational, But I see it has "warning" severity.
Is that the correct severity for it?

I think it should be "info" if you keep it, since it doesn't impact the SSP operator functionality or even puts it at risk of losing functionality. https://github.com/kubevirt/monitoring/blob/main/docs/runbooks/SSPCommonTemplatesModificationReverted.md

@0xFelix
Copy link
Member

0xFelix commented Apr 9, 2024

@sradco Thanks for the clarification. I don't see a benefit in this alert to be honest. We should reevaluate the need for it. Also, you are right, it should be of severity info.

@akrejcir
Copy link
Collaborator Author

I've opened a new issue for us to reevaluate which metrics are needed: #956

@akrejcir
Copy link
Collaborator Author

akrejcir commented Apr 15, 2024

Can you please take a look, when you have a moment?

/cc @ksimon1 @jcanocan

@jcanocan
Copy link
Contributor

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2024
@kubevirt-bot kubevirt-bot merged commit c1637a1 into kubevirt:main Apr 15, 2024
12 checks passed
@akrejcir akrejcir deleted the fix-template-metric branch April 15, 2024 12:52
@akrejcir
Copy link
Collaborator Author

/cherry-pick release-v0.19

@kubevirt-bot
Copy link
Contributor

@akrejcir: new pull request created: #959

In response to this:

/cherry-pick release-v0.19

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants