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

Bug 1822396: Delete subscription metric when an operator is uninstalled #1519

Merged
merged 1 commit into from
Jun 19, 2020
Merged

Bug 1822396: Delete subscription metric when an operator is uninstalled #1519

merged 1 commit into from
Jun 19, 2020

Conversation

anik120
Copy link
Contributor

@anik120 anik120 commented May 13, 2020

When an operator was subscribed to using a Subscription Object, the subscription_sync_total
metric was emitted whenever the Subscription Object was created/updated/deleted. This PR
updates that behaviour to emit the metric only when the Subscription object is created/updated,
and deletes the time series for that particular subscription when the subscription object is
deleted.

Description of the change:

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label May 13, 2020
@openshift-ci-robot
Copy link
Collaborator

@anik120: This pull request references Bugzilla bug 1822396, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1822396: Delete subscription metric when an operator is uninstalled

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.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 13, 2020
awgreene
awgreene requested changes May 14, 2020
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

Great work on this @anik120 - I have a few nits.

pkg/controller/operators/catalog/subscription/syncer.go Outdated Show resolved Hide resolved
pkg/controller/operators/catalog/subscription/syncer.go Outdated Show resolved Hide resolved
pkg/controller/operators/catalog/subscription/syncer.go Outdated Show resolved Hide resolved
pkg/controller/operators/catalog/subscription/syncer.go Outdated
"reconciling": fmt.Sprintf("%T", res),
"selflink": res.GetSelfLink(),
"reconciling": fmt.Sprintf("%T", subs),
"selflink": subs.GetSelfLink(),
Copy link
Member

Choose a reason for hiding this comment

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

TIL that this is a Human readable alternative to UID.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, selflink is deprecated now.

pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Show resolved Hide resolved
@anik120 anik120 force-pushed the subs_sync_metric_fix branch from 5eaf92a to 2f2cdf6 Compare May 14, 2020 20:35
@anik120
Copy link
Contributor Author

anik120 commented May 14, 2020

@awgreene thanks for the review! I have addressed the nit comments, and I'm waiting for #1443 to merge before I write the test you suggested.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2020
@anik120 anik120 force-pushed the subs_sync_metric_fix branch from 2f2cdf6 to b3025a7 Compare May 27, 2020 13:50
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2020
@anik120 anik120 force-pushed the subs_sync_metric_fix branch 5 times, most recently from 0c33430 to 1240be1 Compare May 27, 2020 18:12
@anik120
Copy link
Contributor Author

anik120 commented May 27, 2020

@awgreene thanks for the review! I have addressed the nit comments, and I'm waiting for #1443 to merge before I write the test you suggested.

Since #1443 is not merged yet, adding to the existing portfolio that'll need to be incorporated into #1443

awgreene
awgreene requested changes May 27, 2020
View reviewed changes
pkg/metrics/metrics.go Show resolved Hide resolved
test/e2e/metrics_e2e_test.go Outdated Show resolved Hide resolved
@anik120 anik120 force-pushed the subs_sync_metric_fix branch from 1240be1 to 1440311 Compare May 27, 2020 20:38
@anik120 anik120 force-pushed the subs_sync_metric_fix branch 3 times, most recently from de728d3 to 9b56643 Compare May 27, 2020 21:25
@anik120 anik120 force-pushed the subs_sync_metric_fix branch from 9b56643 to ece5805 Compare May 27, 2020 23:34
awgreene
awgreene reviewed May 27, 2020
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 27, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2020
@anik120
Copy link
Contributor Author

anik120 commented Jun 18, 2020

/test e2e-aws-olm

njhale
njhale approved these changes Jun 18, 2020
View reviewed changes
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anik120, njhale

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2020
@anik120
Copy link
Contributor Author

anik120 commented Jun 18, 2020

/test e2e-aws-olm

2 similar comments
@anik120
Copy link
Contributor Author

anik120 commented Jun 18, 2020

/test e2e-aws-olm

@kevinrizza
Copy link
Member

/test e2e-aws-olm

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

9 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit a90b83a into operator-framework:master Jun 19, 2020
@openshift-ci-robot
Copy link
Collaborator

@anik120: All pull requests linked via external trackers have merged: operator-framework/operator-lifecycle-manager#1519. Bugzilla bug 1822396 has been moved to the MODIFIED state.

In response to this:

Bug 1822396: Delete subscription metric when an operator is uninstalled

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.

@anik120
Copy link
Contributor Author

anik120 commented Jun 22, 2020

/cherry-pick release-4.5

@openshift-cherrypick-robot
Copy link

@anik120: #1519 failed to apply on top of branch "release-4.5":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	test/e2e/metrics_e2e_test.go
M	test/e2e/subscription_e2e_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/subscription_e2e_test.go
Auto-merging test/e2e/metrics_e2e_test.go
CONFLICT (content): Merge conflict in test/e2e/metrics_e2e_test.go
Patch failed at 0001 Bug 1822396: Delete subscription metric when an operator is uninstalled

In response to this:

/cherry-pick release-4.5

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.

anik120 added a commit to anik120/operator-lifecycle-manager that referenced this pull request Jun 25, 2020
@anik120
Bug 1822396: Update Subscription metric when Subscription object is u…
0b1805e 
…pdated

In operator-framework#1519, the subscription_sync_total metric was updated when the
related subscription object was updated. However, the PR had a bug,
in which the map used to store metric related information was not
being updated when the metric was udpated. As a result of which any
update after the first update was not removing the old metric. This
PR fixes that bug.
anik120 added a commit to anik120/operator-lifecycle-manager that referenced this pull request Jun 25, 2020
@anik120
Bug 1822396: Update metric when Subscription is updated
2583f2c 
In operator-framework#1519, the subscription_sync_total metric was updated when the
related subscription object was updated. However, the PR had a bug,
in which the map used to store metric related information was not
being updated when the metric was udpated. As a result of which any
update after the first update was not removing the old metric. This
PR fixes that bug.
@anik120 anik120 mentioned this pull request Jun 25, 2020
Merged
5 tasks
anik120 added a commit to anik120/operator-lifecycle-manager that referenced this pull request Jun 29, 2020
@anik120
Bug 1822396: Update metric when Subscription is updated
447c11b 
In operator-framework#1519, the subscription_sync_total metric was updated when the
related subscription object was updated. However, the PR had a bug,
in which the map used to store metric related information was not
being updated when the metric was udpated. As a result of which any
update after the first update was not removing the old metric. This
PR fixes that bug.
anik120 added a commit to anik120/operator-lifecycle-manager that referenced this pull request Jun 29, 2020
@anik120
Bug 1822396: Update metric when Subscription is updated
c6e37ba 
In operator-framework#1519, the subscription_sync_total metric was updated when the
related subscription object was updated. However, the PR had a bug,
in which the map used to store metric related information was not
being updated when the metric was udpated. As a result of which any
update after the first update was not removing the old metric. This
PR fixes that bug.
anik120 added a commit to anik120/operator-lifecycle-manager that referenced this pull request Jun 30, 2020
@anik120
Bug 1822396: Update metric when Subscription is updated
2b658ed 
In operator-framework#1519, the subscription_sync_total metric was updated when the
related subscription object was updated. However, the PR had a bug,
in which the map used to store metric related information was not
being updated when the metric was udpated. As a result of which any
update after the first update was not removing the old metric. This
PR fixes that bug.
@anik120 anik120 mentioned this pull request Sep 3, 2020
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awgreene awgreene awgreene requested changes

@ecordell ecordell ecordell left review comments

@njhale njhale njhale approved these changes

@Bowenislandsong Bowenislandsong Awaiting requested review from Bowenislandsong

@dinhxuanvu dinhxuanvu Awaiting requested review from dinhxuanvu

Assignees

@ecordell ecordell

@awgreene awgreene

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@anik120 @openshift-ci-robot @Bowenislandsong @ecordell @kevinrizza @openshift-bot @openshift-cherrypick-robot @njhale @awgreene @openshift-merge-robot