Add VirtualMachineCRCErrors alert #663

machadovilaca · 2023-08-17T14:48:12Z

What this PR does / why we need it:

Following the initial work made in kubevirt/kubevirt#9741 and kubevirt/containerized-data-importer#2835, we need a way to alert users when they are using RBD volumes with VMs without the krbd:rxbounce option enabled because they may lose data and face severe performance degradation in the cluster.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Release note:

Add VirtualMachineCRCErrors alert

dominikholler · 2023-08-21T08:09:07Z

The ssp-operator is an "Operator that deploys additional KubeVirt resources". Does this PR fit to this expectation?

0xFelix · 2023-08-21T08:35:28Z

/hold Please let us discuss the necessity for this first.

akalenyu

Missing some SSP context but the PR looks great!

akalenyu · 2023-08-21T08:27:11Z

internal/operands/metrics/resources.go

@@ -164,6 +164,22 @@ func getAlertRules() ([]promv1.Rule, error) {
 				componentAlertLabelKey:    componentAlertLabelValue,
 			},
 		},
+		{
+			Alert: "VirtualMachineCRCErrors",
+			Expr:  intstr.FromString("kubevirt_ssp_vm_rbd_volume{volume_mode=\"Block\", rxbounce_enabled=\"false\"} > 0"),


Is the issue specific to Block PVCs?

adding source here for future ref kubevirt/kubevirt#9741 (comment)

akalenyu · 2023-08-21T08:33:06Z

controllers/vm_controller.go

+
+func (r *VmReconciler) setupController(mgr ctrl.Manager) error {
+	return ctrl.NewControllerManagedBy(mgr).
+		Named("vm-controller").


vmControllerName

akalenyu · 2023-08-21T08:34:45Z

controllers/vm_controller.go

+		}
+		pvc, err := r.getPVC(vm, volume.PersistentVolumeClaim.ClaimName)
+		if err != nil {
+			continue


I think we are better off requeueing in case of HTTP errors, we don't want to just lose that piece of information

and adding it to a list of errors, and continuing

akalenyu · 2023-08-21T08:35:12Z

controllers/vm_controller.go

+		}
+		pv, err := r.getPV(vm, pvc)
+		if err != nil {
+			continue


Same comment as above about requeueing

same as above

github-actions · 2023-08-22T10:58:02Z

/cc sradco

github-actions · 2023-08-22T11:38:37Z

/cc sradco

akalenyu

A couple of small concerns, but the controller makes sense to me

akalenyu · 2023-08-23T14:16:17Z

go.mod

@@ -6,15 +6,17 @@ require (
 	github.com/blang/semver/v4 v4.0.0
 	github.com/fsnotify/fsnotify v1.6.0
 	github.com/go-logr/logr v1.2.4
+	github.com/hashicorp/go-multierror v1.1.1


note kubevirt/kubevirt#10283

I think libraries will maintain MPL-2.0, only tools like terraform and vault will be under a Business Source License

akalenyu · 2023-08-23T14:23:37Z

internal/operands/metrics/resources.go

@@ -164,6 +164,22 @@ func getAlertRules() ([]promv1.Rule, error) {
 				componentAlertLabelKey:    componentAlertLabelValue,
 			},
 		},
+		{
+			Alert: "VirtualMachineCRCErrors",
+			Expr:  intstr.FromString("kubevirt_ssp_vm_rbd_volume{volume_mode=\"Block\", rxbounce_enabled=\"false\"} > 0"),


do you need or vector(0) or will this eval just fine?

this should be fine, no further actions are taken on the results of kubevirt_ssp_vm_rbd_volume

akalenyu · 2023-08-23T14:25:09Z

tests/monitoring_test.go

+		return fmt.Errorf("alert %s found", alertName)
+	}, env.Timeout(), time.Second).ShouldNot(HaveOccurred())
+
+	Consistently(func() error {


akalenyu · 2023-08-23T17:15:56Z

/lgtm

internal/operands/metrics/resources.go

0xFelix · 2023-08-25T12:56:32Z

Dockerfile

@@ -26,6 +26,7 @@ COPY main.go main.go
 COPY api/ api/
 COPY controllers/ controllers/
 COPY internal/ internal/
+COPY pkg/ pkg/


Can you place the controller in the internal path instead?

this is for the metrics, and not for the controller. But following kubevirt/community#219, we want to stay consistent in terms of monitoring between all KubeVirt components

controllers/vm_controller.go

pkg/monitoring/metrics/rbd_metrics.go

pkg/monitoring/metrics/rbd_metrics_test.go

pkg/monitoring/metrics/rbd_metrics.go

controllers/vm_controller.go

0xFelix · 2023-08-25T13:18:55Z

tests/monitoring_test.go

+}
+
+func alertShouldNotBeActive(alertName string) {
+	Eventually(func() error {


Can this Eventually be dropped and be replaced with the Consistently after?

not really, Consistently would not wait for the alert to disappear and would exit with error immediately

github-actions · 2023-08-25T17:21:12Z

/cc sradco

lyarwood · 2023-08-29T10:51:39Z

/approve

Several nits and supernits that can be fixed in a follow up once this lands in main but overall looks good enough to me.

@akrejcir @ksimon1 would either of you have time today to look at this and get it over the line while Felix is out?

kubevirt-bot · 2023-08-29T10:51:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lyarwood

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

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

pkg/monitoring/metrics/rbd_metrics_test.go

internal/common/scheme.go

lyarwood · 2023-08-29T10:35:15Z

pkg/monitoring/metrics/rbd_metrics.go

+	}
+
+	mounter, rxbounceEnabled := getAttributes(pv)
+	if mounter != "" && mounter != "rbd" {


nit - why check for ""?

if no value is set, the storage class defaults to rbd mounter

pkg/monitoring/metrics/metrics.go

lyarwood · 2023-08-29T10:41:43Z

config/rbac/role.yaml

+  verbs:
+  - get
+  - list
+  - watch


nit - appreciate they are all pretty much the same but do we need list and watch here?

So the ctrl runtime client will attempt to cache anything you GET once,
and for that, you need list and watch:
https://sdk.operatorframework.io/docs/faqs/#after-deploying-my-operator-i-see-errors-like-failed-to-watch-external-type

Ah apologies of course!

controllers/vm_controller.go

lyarwood · 2023-08-29T10:52:32Z

/lgtm cancel

Gah, github is such a useless platform sometimes.

akrejcir

I have mostly code style comments. Otherwise it looks good.

Can you add text to the commit messages explaining why the change is done?

tests/monitoring_test.go

controllers/vm_controller.go

pkg/monitoring/metrics/rbd_metrics.go

pkg/monitoring/metrics/rbd_metrics_test.go

pkg/monitoring/metrics/metrics_suite_test.go

github-actions · 2023-08-29T17:42:45Z

/cc sradco

github-actions · 2023-08-29T17:52:56Z

/cc sradco

github-actions · 2023-08-30T09:29:17Z

/cc sradco

akrejcir · 2023-08-30T09:57:06Z

Thanks!

/lgtm

lyarwood · 2023-08-30T10:04:15Z

/hold cancel

Signed-off-by: João Vilaça <[email protected]>

github-actions · 2023-08-30T10:46:14Z

/cc sradco

sonarcloud · 2023-08-30T10:46:22Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
1.1% Duplication

machadovilaca · 2023-08-30T10:48:14Z

Had to rebase due to some conflicts with a recent merged PR, on the RBAC related files

akrejcir · 2023-08-30T10:59:20Z

/lgtm

akrejcir · 2023-08-30T12:54:17Z

Should this be backported?

machadovilaca · 2023-08-30T13:09:04Z

yes, we should backport to release-v0.18

machadovilaca · 2023-08-30T13:09:11Z

/cherry-pick release-v0.18

kubevirt-bot · 2023-08-30T13:09:47Z

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

Applying: Add controller for kubevirt_ssp_vm_rbd_volume metrics
Using index info to reconstruct a base tree...
M	Makefile
M	config/rbac/role.yaml
M	controllers/setup.go
M	data/olm-catalog/ssp-operator.clusterserviceversion.yaml
M	docs/development.md
M	go.mod
M	go.sum
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Auto-merging docs/development.md
CONFLICT (content): Merge conflict in docs/development.md
Auto-merging data/olm-catalog/ssp-operator.clusterserviceversion.yaml
CONFLICT (content): Merge conflict in data/olm-catalog/ssp-operator.clusterserviceversion.yaml
Auto-merging controllers/setup.go
Auto-merging config/rbac/role.yaml
CONFLICT (content): Merge conflict in config/rbac/role.yaml
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add controller for kubevirt_ssp_vm_rbd_volume metrics
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.18

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.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 17, 2023

kubevirt-bot requested review from akrejcir and ksimon1 August 17, 2023 14:48

kubevirt-bot added the size/XXL label Aug 17, 2023

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2023

akalenyu reviewed Aug 21, 2023

View reviewed changes

machadovilaca force-pushed the add-VirtualMachineCRCErrors-alert branch from 58e061b to ffe6c8a Compare August 22, 2023 10:57

kubevirt-bot requested a review from sradco August 22, 2023 10:58

machadovilaca force-pushed the add-VirtualMachineCRCErrors-alert branch from ffe6c8a to 8cbf8e9 Compare August 22, 2023 11:38

akalenyu reviewed Aug 23, 2023

View reviewed changes

kubevirt-bot assigned akalenyu Aug 23, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2023

0xFelix reviewed Aug 25, 2023

View reviewed changes

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2023

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2023

lyarwood approved these changes Aug 29, 2023

View reviewed changes

kubevirt-bot assigned lyarwood Aug 29, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2023

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2023

akrejcir reviewed Aug 29, 2023

View reviewed changes

machadovilaca force-pushed the add-VirtualMachineCRCErrors-alert branch from af7c9b5 to 447c74d Compare August 29, 2023 17:42

machadovilaca force-pushed the add-VirtualMachineCRCErrors-alert branch from 447c74d to 134b92d Compare August 29, 2023 17:52

machadovilaca force-pushed the add-VirtualMachineCRCErrors-alert branch from 134b92d to f9a4cd5 Compare August 30, 2023 09:29

kubevirt-bot assigned akrejcir Aug 30, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2023

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2023

machadovilaca added 2 commits August 30, 2023 11:44

Add controller for kubevirt_ssp_vm_rbd_volume metrics

5cfa692

Signed-off-by: João Vilaça <[email protected]>

Add VirtualMachineCRCErrors alert

d138f6e

Signed-off-by: João Vilaça <[email protected]>

machadovilaca force-pushed the add-VirtualMachineCRCErrors-alert branch from f9a4cd5 to d138f6e Compare August 30, 2023 10:45

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2023

kubevirt-bot merged commit 0b32d61 into kubevirt:main Aug 30, 2023
5 checks passed

machadovilaca deleted the add-VirtualMachineCRCErrors-alert branch August 30, 2023 12:37

This was referenced Aug 30, 2023

[release-v0.18] Add VirtualMachineCRCErrors alert #673

Merged

[release-v0.17] Add VirtualMachineCRCErrors alert #681

Closed

akrejcir mentioned this pull request Sep 7, 2023

Operator crashes when kubevirt is not installed #682

Closed

Add VirtualMachineCRCErrors alert #663

Add VirtualMachineCRCErrors alert #663

Conversation

machadovilaca commented Aug 17, 2023

dominikholler commented Aug 21, 2023

0xFelix commented Aug 21, 2023

akalenyu 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

github-actions bot commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

akalenyu 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

akalenyu commented Aug 23, 2023

Choose a reason for hiding this comment

machadovilaca Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 25, 2023

lyarwood commented Aug 29, 2023

kubevirt-bot commented Aug 29, 2023

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

lyarwood commented Aug 29, 2023

akrejcir left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 29, 2023

github-actions bot commented Aug 29, 2023

github-actions bot commented Aug 30, 2023

akrejcir commented Aug 30, 2023

lyarwood commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

sonarcloud bot commented Aug 30, 2023

machadovilaca commented Aug 30, 2023

akrejcir commented Aug 30, 2023

akrejcir commented Aug 30, 2023

machadovilaca commented Aug 30, 2023

machadovilaca commented Aug 30, 2023

kubevirt-bot commented Aug 30, 2023

machadovilaca Aug 25, 2023 •

edited

Loading