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

Fix total_restored_common_templates metric update #559

Merged
merged 1 commit into from
Jun 13, 2023
Merged

Fix total_restored_common_templates metric update #559

merged 1 commit into from
Jun 13, 2023

Conversation

machadovilaca
Copy link
Member

What this PR does / why we need it:

SSPCommonTemplatesModificationReverted is being triggered during upgrades.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Release note:

 Fix total_restored_common_templates metric update

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 24, 2023
@openshift-ci
Copy link

openshift-ci bot commented May 24, 2023

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 the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label May 24, 2023
@machadovilaca
Copy link
Member Author

/test all

@kubevirt-bot
Copy link
Contributor

@machadovilaca: No presubmit jobs available for kubevirt/ssp-operator@master

In response to this:

/test all

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.

akrejcir
akrejcir reviewed May 25, 2023
View reviewed changes
tests/metrics_test.go Outdated Show resolved Hide resolved
@machadovilaca
Copy link
Member Author

/test all

@kubevirt-bot
Copy link
Contributor

@machadovilaca: No presubmit jobs available for kubevirt/ssp-operator@master

In response to this:

/test all

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.

@machadovilaca machadovilaca force-pushed the fix-wrong-template-reconcile-metrics branch from f1aead9 to 12658d1 Compare May 25, 2023 11:17
@machadovilaca machadovilaca marked this pull request as ready for review May 25, 2023 11:17
@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 May 25, 2023
@kubevirt-bot kubevirt-bot requested a review from 0xFelix May 25, 2023 11:18
@akrejcir
Copy link
Collaborator

Both commits change the same lines in file tests/metrics_test.go. Can you move changes in that file into a single commit?

@machadovilaca machadovilaca force-pushed the fix-wrong-template-reconcile-metrics branch 3 times, most recently from e6bd7df to 62ad95f Compare May 25, 2023 16:52
0xFelix
0xFelix reviewed May 30, 2023
View reviewed changes
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.

Can you please add some context into the commit descriptions?

tests/metrics_test.go Outdated Show resolved Hide resolved
akrejcir
akrejcir reviewed May 30, 2023
View reviewed changes
Copy link
Collaborator

@akrejcir akrejcir left a comment

Choose a reason for hiding this comment

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

Can you squash the commits? The fist commit is not correct without the second one.

internal/common/resource.go Outdated Show resolved Hide resolved
internal/common/resource.go Outdated Show resolved Hide resolved
internal/common/resource.go Outdated Show resolved Hide resolved
tests/metrics_test.go Outdated Show resolved Hide resolved
tests/metrics_test.go Outdated Show resolved Hide resolved
tests/metrics_test.go Outdated Show resolved Hide resolved
tests/metrics_test.go Outdated Show resolved Hide resolved
akrejcir
akrejcir reviewed May 30, 2023
View reviewed changes
Copy link
Collaborator

@akrejcir akrejcir left a comment

Choose a reason for hiding this comment

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

Can you squash the commits? The fist commit is not correct without the second one.

@machadovilaca machadovilaca force-pushed the fix-wrong-template-reconcile-metrics branch 2 times, most recently from 1ee67ef to 766c72c Compare May 31, 2023 09:16
@machadovilaca machadovilaca force-pushed the fix-wrong-template-reconcile-metrics branch 4 times, most recently from ded0a58 to f5e4371 Compare May 31, 2023 15:01
@machadovilaca machadovilaca force-pushed the fix-wrong-template-reconcile-metrics branch 3 times, most recently from 66c58fa to 9ec7e72 Compare May 31, 2023 17:50
akrejcir
akrejcir reviewed Jun 7, 2023
View reviewed changes
Copy link
Collaborator

@akrejcir akrejcir left a comment

Choose a reason for hiding this comment

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

Returning the existing resource from createOrUpdateWithImmutableSpec() seems hacky, but it's ok for this PR. We can improve the code later.

internal/operands/common-templates/reconcile_test.go Outdated Show resolved Hide resolved
internal/operands/common-templates/reconcile_test.go Outdated Show resolved Hide resolved
internal/operands/common-templates/reconcile_test.go Show resolved Hide resolved
internal/operands/common-templates/reconcile_test.go Outdated
CommonTemplatesRestored.Collect(ch)
close(ch)
m := <-ch
dto := &io_prometheus_client.Metric{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does dto mean? Can you use longer variable names?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is a commonly used name by Prometheus in these cases, but we can use metric for clarity

tests/metrics_test.go Show resolved Hide resolved
@machadovilaca
Fix total_restored_common_templates metric update
e6bfa8c 
Signed-off-by: João Vilaça <jvilaca@redhat.com>
@machadovilaca machadovilaca force-pushed the fix-wrong-template-reconcile-metrics branch from 9ec7e72 to e6bfa8c Compare June 9, 2023 14:50
@sonarcloud
Copy link

sonarcloud bot commented Jun 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

0xFelix
0xFelix reviewed Jun 12, 2023
View reviewed changes
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

@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 Jun 12, 2023
@akrejcir
Copy link
Collaborator

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 12, 2023
@akrejcir
Copy link
Collaborator

/retest

@kubevirt-bot kubevirt-bot merged commit 1aefd19 into kubevirt:master Jun 13, 2023
@machadovilaca
Copy link
Member Author

/cherry-pick release-v0.16

@kubevirt-bot
Copy link
Contributor

@machadovilaca: #559 failed to apply on top of branch "release-v0.16":

Applying: Fix total_restored_common_templates metric update
Using index info to reconstruct a base tree...
M	internal/common/resource.go
M	internal/operands/common-templates/reconcile.go
M	internal/operands/common-templates/reconcile_test.go
M	main.go
M	tests/metrics_test.go
Falling back to patching base and 3-way merge...
Auto-merging tests/metrics_test.go
CONFLICT (content): Merge conflict in tests/metrics_test.go
Auto-merging main.go
Auto-merging internal/operands/common-templates/reconcile_test.go
Auto-merging internal/operands/common-templates/reconcile.go
Auto-merging internal/common/resource.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix total_restored_common_templates metric update
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v0.16

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.

@machadovilaca machadovilaca mentioned this pull request Nov 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akrejcir akrejcir akrejcir left review comments

@0xFelix 0xFelix 0xFelix left review comments

@ksimon1 ksimon1 Awaiting requested review from ksimon1

Assignees

@akrejcir akrejcir

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 Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@machadovilaca @kubevirt-bot @akrejcir @0xFelix