Emit CSV metric on startup #2216

josefkarasek · 2021-06-28T12:59:48Z

csv_succeeded metric is lost between pod restarts.
This is because this metric is only emitted when CSV.Status is changed.

Description of the change:
Emit csv_succeeded/csv_abnormal metric during OLM startup

Motivation for the change:
csv_succeeded metric is lost between pod restarts.

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 /doc
Commit messages sensible and descriptive

openshift-ci · 2021-06-28T12:59:56Z

Hi @josefkarasek. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 · 2021-06-28T12:59:58Z

@josefkarasek: This pull request references Bugzilla bug 1952576, which is valid. 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.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jianzhangbjz

In response to this:

Bug 1952576: Emit CSV metric on startup

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 · 2021-06-28T13:00:00Z

@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: jianzhangbjz.

Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@josefkarasek: This pull request references Bugzilla bug 1952576, which is valid. 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.9.0) matches configured target release for branch (4.9.0)

bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jianzhangbjz

In response to this:

Bug 1952576: Emit CSV metric on startup

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.

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.

josefkarasek · 2021-06-28T13:03:07Z

This is an alternative solution along with #2213

anik120

/ok-to-test

cmd/olm/main.go

test/e2e/metrics_e2e_test.go

anik120

/lgtm

dinhxuanvu

/lgtm

cmd/olm/main.go

ecordell · 2021-07-13T16:24:35Z

/hold

if we can make this change in the operator package it will be much easier to backport

we can un-hold if there's a reason it needs to be in main, but it's not clear to me from the existing discussion why it would need to be

estroz · 2021-08-19T20:32:13Z

test/e2e/metrics_e2e_test.go

@@ -116,6 +117,45 @@ var _ = Describe("Metrics are generated for OLM managed resources", func() {
 				})
 			})
 		})
+
+		When("the OLM pod restars", func() {


Suggested change

When("the OLM pod restars", func() {

When("the OLM pod restarts", func() {

The setup of the test feels a little unnatural to me. I would expect a When block inside this Context. And the layout ideally would something be:

Context("Metrics emitted by objects during operator installation", func() { When("A CSV is created", func() { BeforeEach(func() { // create CSV } It("emits a CSV metrics", func() { // check for CSV metrics } When("The OLM pod restarts", func() { BeforeEach(func(){ // kill the deployment and wait for it to come up again } It("The csv metrics are preserved" func(){ // check for CSV metrics again }) }) }

This leaves room for extending this portion of the test suite in the future

estroz · 2021-08-19T20:45:24Z

I don't see necessarily a "better" place for the EnsureCSVMetrics call since queueinformer.Operator.Run can't easily be wrapped, so this looks fine to me.

If the spelling nit gets addressed, I'll re-lgtm this.

/lgtm

anik120 · 2021-08-19T21:26:14Z

test/e2e/metrics_e2e_test.go

@@ -116,6 +117,45 @@ var _ = Describe("Metrics are generated for OLM managed resources", func() {
 				})
 			})
 		})
+
+		When("the OLM pod restars", func() {


The setup of the test feels a little unnatural to me. I would expect a When block inside this Context. And the layout ideally would something be:

Context("Metrics emitted by objects during operator installation", func() { When("A CSV is created", func() { BeforeEach(func() { // create CSV } It("emits a CSV metrics", func() { // check for CSV metrics } When("The OLM pod restarts", func() { BeforeEach(func(){ // kill the deployment and wait for it to come up again } It("The csv metrics are preserved" func(){ // check for CSV metrics again }) }) }

This leaves room for extending this portion of the test suite in the future

anik120 · 2021-08-20T19:44:40Z

This looks good to me. If Evan's concerns have been assuaged (I don't see any conversation here that suggests that they have been yet), we can remove the hold and lgtm it.

josefkarasek · 2021-08-23T07:44:40Z

/retest

josefkarasek · 2021-08-23T11:43:17Z

/retest

Signed-off-by: Josef Karasek <jkarasek@redhat.com>

josefkarasek · 2021-08-24T15:05:26Z

/cc @anik120

dinhxuanvu

/lgtm

openshift-ci · 2021-09-03T15:11:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, josefkarasek, kevinrizza

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:

~~OWNERS~~ [dinhxuanvu,kevinrizza]

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

kevinrizza · 2021-09-03T15:20:10Z

/hold cancel

Signed-off-by: Josef Karasek <jkarasek@redhat.com>

* Update getMetricsFromPort to infer port number Problem: The getMetricsFromPod function assumes that metrics are exposed on port 8080. This function fails to retrieve metrics from the olm or catalog operator when the port is changed. Solution: Name the port in each of the deployments and update the getMetricsFromPod function to infer the port number from the deployments. Signed-off-by: Alexander Greene <greene.al1991@gmail.com> * Emit CSV metric on startup (#2216) Signed-off-by: Josef Karasek <jkarasek@redhat.com> * fix e2e CSV metric is preserved failure (#2530) Signed-off-by: akihikokuroda <akihikokuroda2020@gmail.com> Co-authored-by: Alexander Greene <greene.al1991@gmail.com> Co-authored-by: Josef Karasek <jkarasek@redhat.com> Co-authored-by: Akihiko (Aki) Kuroda <16141898+akihikokuroda@users.noreply.github.com>

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 28, 2021

openshift-ci bot requested review from exdx and hasbro17 June 28, 2021 12:59

openshift-ci bot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Jun 28, 2021

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jun 28, 2021

anik120 requested changes Jun 28, 2021

View reviewed changes

cmd/olm/main.go Outdated Show resolved Hide resolved

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 28, 2021

anik120 reviewed Jul 12, 2021

View reviewed changes

cmd/olm/main.go Outdated Show resolved Hide resolved

anik120 reviewed Jul 12, 2021

View reviewed changes

cmd/olm/main.go Outdated Show resolved Hide resolved

anik120 reviewed Jul 12, 2021

View reviewed changes

test/e2e/metrics_e2e_test.go Outdated Show resolved Hide resolved

josefkarasek force-pushed the csv-metrics branch from 2a70697 to fc58998 Compare July 12, 2021 14:35

anik120 approved these changes Jul 12, 2021

View reviewed changes

openshift-ci bot assigned anik120 Jul 12, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2021

josefkarasek mentioned this pull request Jul 12, 2021

Bug 1952576: csv_succeeded metric not present #2213

Closed

5 tasks

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2021

josefkarasek force-pushed the csv-metrics branch from cf95b83 to 927d684 Compare July 12, 2021 16:00

dinhxuanvu approved these changes Jul 12, 2021

View reviewed changes

openshift-ci bot assigned dinhxuanvu Jul 12, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2021

ecordell reviewed Jul 13, 2021

View reviewed changes

cmd/olm/main.go Outdated Show resolved Hide resolved

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2021

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2021

josefkarasek requested a review from ecordell July 20, 2021 16:34

estroz reviewed Aug 19, 2021

View reviewed changes

openshift-ci bot assigned estroz Aug 19, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2021

anik120 requested changes Aug 19, 2021

View reviewed changes

josefkarasek force-pushed the csv-metrics branch from 776c9af to cb549b6 Compare August 20, 2021 11:51

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2021

Emit CSV metric on startup

969ed27

Signed-off-by: Josef Karasek <jkarasek@redhat.com>

josefkarasek force-pushed the csv-metrics branch from cb549b6 to 969ed27 Compare August 24, 2021 13:51

openshift-ci bot requested a review from anik120 August 24, 2021 15:05

josefkarasek requested a review from estroz August 25, 2021 07:58

dinhxuanvu approved these changes Sep 3, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2021

openshift-merge-robot merged commit de4bebe into operator-framework:master Sep 3, 2021

ankitathomas mentioned this pull request Sep 14, 2021

Upstream sync openshift/operator-framework-olm#187

Closed

ankitathomas mentioned this pull request Sep 23, 2021

Upstream sync openshift/operator-framework-olm#184

Closed

timflannagan mentioned this pull request Oct 4, 2021

Flake: Metrics are generated for OLM managed resources/a CSV is created/the OLM pod restarts #2390

Closed

timflannagan mentioned this pull request Nov 9, 2021

"Metrics are generated for OLM managed resources Given an OperatorGroup that supports all namespaces when a CSV is created when the OLM pod restarts CSV metric is preserved" #2441

Closed

anik120 mentioned this pull request Dec 9, 2021

Sync olm 12-10-21 openshift/operator-framework-olm#223

Merged

openshift-ci bot mentioned this pull request Jan 11, 2022

Bug 1952576: Emit CSV metric on startup openshift/operator-framework-olm#239

Merged

anik120 pushed a commit to anik120/operator-lifecycle-manager that referenced this pull request May 3, 2022

Emit CSV metric on startup (operator-framework#2216)

131ecfa

Signed-off-by: Josef Karasek <jkarasek@redhat.com>

anik120 pushed a commit to anik120/operator-lifecycle-manager that referenced this pull request May 3, 2022

Emit CSV metric on startup (operator-framework#2216)

239c027

Signed-off-by: Josef Karasek <jkarasek@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit CSV metric on startup #2216

Emit CSV metric on startup #2216

josefkarasek commented Jun 28, 2021

openshift-ci bot commented Jun 28, 2021

openshift-ci bot commented Jun 28, 2021

openshift-ci bot commented Jun 28, 2021

josefkarasek commented Jun 28, 2021

anik120 left a comment

anik120 left a comment

dinhxuanvu left a comment

ecordell commented Jul 13, 2021

estroz Aug 19, 2021

anik120 Aug 19, 2021

estroz commented Aug 19, 2021 •

edited

Loading

anik120 Aug 19, 2021

anik120 commented Aug 20, 2021

josefkarasek commented Aug 23, 2021

josefkarasek commented Aug 23, 2021

josefkarasek commented Aug 24, 2021

dinhxuanvu left a comment

openshift-ci bot commented Sep 3, 2021

kevinrizza commented Sep 3, 2021

	When("the OLM pod restars", func() {
	When("the OLM pod restarts", func() {

Emit CSV metric on startup #2216

Emit CSV metric on startup #2216

Conversation

josefkarasek commented Jun 28, 2021

openshift-ci bot commented Jun 28, 2021

openshift-ci bot commented Jun 28, 2021

openshift-ci bot commented Jun 28, 2021

josefkarasek commented Jun 28, 2021

anik120 left a comment

Choose a reason for hiding this comment

anik120 left a comment

Choose a reason for hiding this comment

dinhxuanvu left a comment

Choose a reason for hiding this comment

ecordell commented Jul 13, 2021

estroz Aug 19, 2021

Choose a reason for hiding this comment

anik120 Aug 19, 2021

Choose a reason for hiding this comment

estroz commented Aug 19, 2021 • edited Loading

anik120 Aug 19, 2021

Choose a reason for hiding this comment

anik120 commented Aug 20, 2021

josefkarasek commented Aug 23, 2021

josefkarasek commented Aug 23, 2021

josefkarasek commented Aug 24, 2021

dinhxuanvu left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 3, 2021

kevinrizza commented Sep 3, 2021

estroz commented Aug 19, 2021 •

edited

Loading