[release-0.65] Add alert to notify of nmstate removal #1302

rhrazdil · 2022-03-24T16:00:56Z

kubernetes-nmstate is removed in the next CNAO version.
This change adds an alert that notifies users who have
knmstate deployed with CNAO, and point them to runbook,
that explains that standalone kubernetes-nmstate operator
should be installed.

Signed-off-by: Radim Hrazdil rhrazdil@redhat.com

What this PR does / why we need it:

Special notes for your reviewer:

Release note:

rhrazdil · 2022-03-24T16:01:13Z

/cc @RamLavi

RamLavi · 2022-03-29T08:02:28Z

@sradco can you also review this ?

RamLavi

Can you also add an e2e to alerts_tests?

RamLavi · 2022-03-29T08:04:30Z

test/e2e/workflow/deployment_test.go

@@ -408,12 +408,33 @@ var _ = Describe("NetworkAddonsConfig", func() {
 		var (
 			configSpec cnao.NetworkAddonsConfigSpec
 		)
+		checkMetricValues := func(expectedMetricValueMap map[string]string) {


we recently moved endpoints tests to here. can you do the same?

It also means adding the monitoring lane, right?

yaacov · 2022-03-30T08:47:36Z

data/monitoring/prom-rule.yaml

+          labels:
+            severity: warning
+            kubernetes_operator_part_of: kubevirt
+            kubernetes_operator_component: cluster-network-addons-operator


we want to highlight this alert in the UI virtualization overview page.

can we add a label that will tell the UI to show this alert on the top of the virtualization overview page for better visibility ?

for exampe:
kubevirt_ui: highlighted
does that makes sense?

do we have a naming convention for alert labels?

I don't think we have, but @sradco may confirm that.

I would suggest

kubevirt_console_scope: [cluster,virtualmachine]

But IIUC, we need to provide more info in case an alert is cluster scoped (that's not case of the alert in this PR, just to make the mechanism general).

@yaacov Can you please explain what is the reasoning to highlight this specific alert?
I believe alerts should be ordered based on their severity.

The documentation for the label should be part of OCP metrics documentation.
Im not sure if we should have it as mandatory as the severity.
The issue is that the label naming impacts also OCP and all other operators.
We should propose this in the monitoring forum and get their opinion.

I think we should keep it as priority.

makes sense to me too

Im not sure if we should have it as mandatory as the severity.

IMHO it should not be mandatory (default to medium if missing)
my reasoning is that unlike severity that is clear to define by the rule maintainer, priority may depend on things outside the scope of the alert rules maintainer, the cases where the rule maintainer wish to raise/lower priority are the odd cases.

I created openshift/enhancements#1077. Please review.

from openshift/enhancements#1077 is looks like "priority: high"

@rhrazdil can you add the "priority" label?

Sorry, I've missed your comment.
Added the priority: high label in a new commit

yaacov · 2022-03-30T13:52:19Z

data/monitoring/prom-rule.yaml

+          labels:
+            severity: warning
+            kubernetes_operator_part_of: kubevirt
+            kubernetes_operator_component: cluster-network-addons-operator
        - alert: CnaoDown
          annotations:
            summary: CNAO pod is down.


we may want to be more verbose if we highlight this alert.
maybe cluster network addons operator pod is not found, the operator deploy additional networking components required by kubevirt?

We want to highlight the CnaoNmstateMigration alert, but yes, I get your point

Adding a new e2e test module and lane for monitoring. Signed-off-by: Ram Lavi <ralavi@redhat.com>

rhrazdil · 2022-04-07T08:05:09Z

/test pull-e2e-cnao-nmstate-functests-release-0.65

phoracek · 2022-04-22T13:03:15Z

Adding explicit hold so we don't merge this preemptively

/hold

sradco · 2022-05-09T08:44:46Z

data/monitoring/prom-rule.yaml

@@ -22,6 +22,7 @@ spec:
          expr: sum(kubevirt_cnao_nmstate_deployed or vector(0)) > 0 and sum(kubevirt_nmstate_operator_deployments or vector(0)) == 0
          for: 5m
          labels:
+            priority: high


@rhrazdil Please move the Priority label below the severity.

@sradco I have nothing against moving it, but could you explain, what is the reasoning?
p is before s alphabetically

IMHO semantically, the required fields should go before the optional ones,
using that way of thinking, severity that is required label (and more important) will go before the optional priority.
I have nothing against going with alphabetical order if it's the common practice for labels, so that is also good option, don't know what is best.

@rhrazdil I agree with Kobi.

Okay, I take it. Updated

rhrazdil · 2022-05-16T09:05:11Z

/hold cancel

RamLavi

Please check if we really need to define kubevirt_cnao_nmstate_deployed as gauge, if you can define it with pure prometheus given metrics (like it seems you already have succeeded), then we can remove the gauges, which will allow us to remove some of the cherry picks which will make things simpler IMO)

RamLavi · 2022-05-16T12:37:40Z

pkg/monitoring/metrics.go

@@ -33,10 +33,15 @@ var (
 			Name: "kubevirt_cnao_cr_kubemacpool_deployed",
 			Help: "Kubemacpool is deployed by Cnao CR",
 		})
+	nmstateHandlerDeployed = prometheus.NewGauge(
+		prometheus.GaugeOpts{
+			Name: "kubevirt_cnao_nmstate_deployed",


hmmm.. I don't get something..
if you already defined kubevirt_cnao_nmstate_deployed with kube_daemonset_labels, i.e. without the use of a GAUGE variable, why do you need to define one here? isn't this redundant?

Removed the variable in this force-push.

rhrazdil · 2022-05-16T14:14:48Z

Please check if we really need to define kubevirt_cnao_nmstate_deployed as gauge, if you can define it with pure prometheus given metrics (like it seems you already have succeeded), then we can remove the gauges, which will allow us to remove some of the cherry picks which will make things simpler IMO)

Updated (removed the redundant variable).
To the cherry-picks, the monitoring lane adds infra for testing alerts, we could add the infra alone without adding the lane, but it seems easier from maintenance perspective to cherry-pick the whole commit.
The following two commits:
monitoring, e2e, Move endpoints test to monitoring lane
tests, metrics: Fix potential flakes on metrics scraping test

Are not really neccessary, we could keep the tests in workflow/deployment, if you prefer

kubernetes-nmstate is removed in the next CNAO version. This change adds an alert that notifies users who have knmstate deployed with CNAO, and point them to runbook, that explains that standalone kubernetes-nmstate operator should be installed. Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

sonarcloud · 2022-05-17T06:07:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

No Coverage information
0.0% Duplication

RamLavi · 2022-05-17T09:12:06Z

/retest

RamLavi

/lgtm
/approve

kubevirt-bot · 2022-05-17T09:14:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RamLavi

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~~ [RamLavi]

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

RamLavi · 2022-05-17T09:14:41Z

/release-note-none

yaacov · 2022-05-23T09:47:45Z

@rhrazdil hi,
should we merge the alert in the main branch too ?
it makes sense to me to have fixes in main when possible ?

@RamLavi WDYT ?

cc:// @sradco @phoracek

rhrazdil · 2022-05-23T10:06:26Z

Hello @yaacov,
we decided to only throw the alert in the release-0.65 branch, because cnao in newer release deletes kubernetes-nmstate, if it's installed.
So the alert couldn't fire in newer branch anyway.

sradco · 2022-05-23T10:27:53Z

@rhrazdil I think it is important to add this alerts also to main branch. In case the user upgrade directly to the next major version and not to the next minor z-stream.

rhrazdil · 2022-05-23T11:14:20Z

We're blocking an upgrade when user upgrades from CNV 4.10 to CNV 4.11 and has kubernetes-nmstate deployed
via CNAO.
Hence, if user has CNV 4.11 (and thus also ocp 4.11) and knmstate deployed, we can safely assume, that it's deployed via Kubernetes Nmstate Operator. CNAO would delete the knmstate deployment within a minute.

So this alert has no meaning on 4.11

kubevirt-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 24, 2022

kubevirt-bot requested review from alonSadan and RamLavi March 24, 2022 16:01

kubevirt-bot added the size/M label Mar 24, 2022

rhrazdil force-pushed the add_alert_nmstate_migration branch from 9ec9af3 to ebeab21 Compare March 25, 2022 14:09

rhrazdil mentioned this pull request Mar 25, 2022

Add runbook explaining CnaoNmstateMigration alert kubevirt/monitoring#82

Merged

RamLavi requested a review from sradco March 29, 2022 08:02

RamLavi requested changes Mar 29, 2022

View reviewed changes

kubevirt-bot assigned RamLavi Mar 29, 2022

rhrazdil force-pushed the add_alert_nmstate_migration branch from ebeab21 to 1835e5b Compare March 29, 2022 14:36

kubevirt-bot added size/XXL and removed size/M labels Mar 29, 2022

yaacov reviewed Mar 30, 2022

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2022

yaacov reviewed Mar 30, 2022

View reviewed changes

e2e/monitoring, Add monitring e2e test lane (kubevirt#1250)

fdc41cc

Adding a new e2e test module and lane for monitoring. Signed-off-by: Ram Lavi <ralavi@redhat.com>

rhrazdil force-pushed the add_alert_nmstate_migration branch from 1835e5b to 04ceb17 Compare April 4, 2022 11:21

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2022

rhrazdil changed the title ~~Add alert to notify of nmstate removal~~ [release-0.65] Add alert to notify of nmstate removal Apr 7, 2022

rhrazdil force-pushed the add_alert_nmstate_migration branch from 0cf3891 to 5567456 Compare April 7, 2022 06:29

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2022

sradco reviewed May 9, 2022

View reviewed changes

rhrazdil force-pushed the add_alert_nmstate_migration branch from 46cadb7 to faf97e0 Compare May 12, 2022 12:55

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 16, 2022

RamLavi requested changes May 16, 2022

View reviewed changes

rhrazdil force-pushed the add_alert_nmstate_migration branch from faf97e0 to 7cc8bcd Compare May 16, 2022 14:08

rhrazdil force-pushed the add_alert_nmstate_migration branch from 7cc8bcd to 79db7a4 Compare May 16, 2022 14:36

Radim Hrazdil added 4 commits May 17, 2022 07:53

e2e, monitoring: test CnaoNmstateMigration alert

62f8e40

Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

go mod vendor

eb0e354

Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

Add priority: high label to alert CnaoNmstateMigration

511b995

Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

rhrazdil force-pushed the add_alert_nmstate_migration branch from 79db7a4 to 511b995 Compare May 17, 2022 06:06

RamLavi approved these changes May 17, 2022

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2022

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 17, 2022

kubevirt-bot merged commit ba15d4a into kubevirt:release-0.65 May 17, 2022

[release-0.65] Add alert to notify of nmstate removal #1302

[release-0.65] Add alert to notify of nmstate removal #1302

Conversation

rhrazdil commented Mar 24, 2022

rhrazdil commented Mar 24, 2022

RamLavi commented Mar 29, 2022

RamLavi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhrazdil commented Apr 7, 2022

phoracek commented Apr 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhrazdil commented May 16, 2022

RamLavi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhrazdil commented May 16, 2022

sonarcloud bot commented May 17, 2022

RamLavi commented May 17, 2022

RamLavi left a comment

Choose a reason for hiding this comment

kubevirt-bot commented May 17, 2022

RamLavi commented May 17, 2022

yaacov commented May 23, 2022

rhrazdil commented May 23, 2022

sradco commented May 23, 2022 • edited

rhrazdil commented May 23, 2022

sradco commented May 23, 2022 •

edited