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

chore(route-monitor-operator): add console monitor [OSD-5499] #736

Merged

Conversation

georgettica
Copy link
Contributor

@georgettica georgettica commented Mar 4, 2021

I have 2 questions for this PR:

  • who should be the owners
  • what should the .spec.targetAvailabilityPercent be?

@georgettica georgettica changed the title chore(route-monitor-operator): add console monitor chore(route-monitor-operator): add console monitor [OSD-5499] Mar 4, 2021
@RiRa12621
Copy link
Contributor

/assign @jeremyeder

@RiRa12621
Copy link
Contributor

It doesn't need a separate owner.
You can't merge without the gods' approval to prod anyway
Availability seems fine, needs revisit after a while to see if we can raise it

@RiRa12621
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2021
@openshift-ci-robot openshift-ci-robot added do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 4, 2021
@georgettica
Copy link
Contributor Author

/hold
I need to add a SOP for it

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2021
@cblecker
Copy link
Member

cblecker commented Mar 4, 2021

It doesn't need a separate owner.

There should be an owners file with a reviewers section detailing who should be point of contact for reviews. You can look at the other folders under deploy/ for examples

what should the .spec.targetAvailabilityPercent be?

Do we have a process for selecting these? Will alerts be raised based off this, or just a metric?

cc @dofinn

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Mar 4, 2021
@RiRa12621
Copy link
Contributor

Who needs that if it can't go in without "approved" anyway?

Yes alerts is what route monitor operator does

@cblecker
Copy link
Member

cblecker commented Mar 4, 2021

Who needs that if it can't go in without "approved" anyway?

Because change to MCC should be reviewed by subject matter experts in addition to leads.

@RiRa12621
Copy link
Contributor

RiRa12621 commented Mar 4, 2021

So you need me to tell you it's ok, so you can approve? Not very efficient imho
Let SMEs approve if they're (rightfully) supposed to evaluate and review changes.
Then the owners file makes more sense too

@dofinn
Copy link
Contributor

dofinn commented Mar 8, 2021

We have nothing in stone that helps us define initial values for SLOs.
Lets make sure we have some dedicated actions to follow this up to analyze if we can increase this SLO based on current data.

@NautiluX
Copy link
Contributor

NautiluX commented Mar 8, 2021

/hold need to rework alerts in RMO, they are currently based on http_requests_total metrics

@georgettica
Copy link
Contributor Author

/unhold
changes that @NautiluX requested are in and SOP is also in

@RiRa12621 can you give a final /lgtm?
not sure if @dofinn has additional notes on this PR aside of what was raised

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 15, 2021
@RiRa12621
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2021
@georgettica georgettica requested a review from RiRa12621 March 17, 2021 09:57
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2021
@georgettica
Copy link
Contributor Author

as @NautiluX suggeste, changed NS and need a lgtm becuase of it

@georgettica
Copy link
Contributor Author

tested on stage (I can replace my routemonitor with this one, but outcome should be similar)

@georgettica
Copy link
Contributor Author

changing to the current CR still works and is scraped, only future concern is the message naming, which should be solved in the RMO code itself

(the value of the breach is 1 and not a good indication to how long the alert is firing)

@@ -8,4 +8,4 @@ spec:
name: console
namespace: openshift-console
slo:
targetAvailabilityPercent: "99.5"
targetAvailabilityPercent: "99.95"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why now 99.95? According to the SLO doc it should be 99.5? Probably I miss some discussion here...

Copy link
Contributor

Choose a reason for hiding this comment

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

turns out this was confused with the API server SLA. We'll change it back to 99.5.

@NautiluX NautiluX added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 23, 2021
@wgordon17
Copy link
Contributor

Will this automatically merge once #771 is resolved?

@georgettica
Copy link
Contributor Author

@wgordon17 If I get a +1 for this then yes

@wgordon17
Copy link
Contributor

@georgettica Consider this a ➕ 1 from PM if it helps :) Not sure if you need plus one's from others ¯_(ツ)_/¯

@cblecker
Copy link
Member

cblecker commented Apr 7, 2021

@georgettica LGTM, but as previously commented, needs an OWNERS file that looks something like:

reviewers:
- georgettica
- RiRa12621

@cblecker
Copy link
Member

cblecker commented Apr 7, 2021

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, georgettica, RiRa12621

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 Apr 7, 2021
@openshift-merge-robot openshift-merge-robot merged commit 51c5b63 into openshift:master Apr 7, 2021
@georgettica georgettica deleted the georgettica/add-rmo-files branch April 7, 2021 18:28
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. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants