NETOBSERV-881: Added field to disable alerts creation #295

OlivierCazade · 2023-03-14T09:04:41Z

Added disable list to not create alerts. Each alert listed in this field would not be created.

openshift-ci-robot · 2023-03-14T09:04:45Z

@OlivierCazade: This pull request references NETOBSERV-881 which is a valid jira issue.

In response to this:

Added disable list to not create alerts. Each alert listed in this field would not be created.

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.

jotak · 2023-03-14T09:25:57Z

api/v1alpha1/flowcollector_webhook.go

@@ -53,6 +53,10 @@ func (r *FlowCollector) ConvertTo(dstRaw conversion.Hub) error {
 		dst.Spec.Processor.ConnectionEndTimeout = restored.Spec.Processor.ConnectionEndTimeout
 	}

+	if restored.Spec.Processor.Metrics.DisableAlerts != nil {


out of curiosity, did you have "something" telling you that you had something to do here, or was it something you had to keep in mind yourself? (trying to understand how error prone it can be if we forget about the conversion)

After adding the new field in v1beta1 running make generate was triggering some warning because of the new fields.

But then I mostly looked at previous PR to understand how to do it.

jotak · 2023-03-14T09:32:26Z

api/v1beta1/flowcollector_types.go

@@ -280,6 +280,10 @@ type FLPMetrics struct {
 	// ignoreTags is a list of tags to specify which metrics to ignore
 	//+kubebuilder:default:={"egress","packets"}
 	IgnoreTags []string `json:"ignoreTags,omitempty"`
+
+	// disableAlerts is a list of alerts related to FLP that should not be created


I am not sure what is the best is terms of usage here, a deny-list or an allow-list. Thoughts?
In any case we should document here what are the acceptable values, ie NetObservNoFlows and NetObservLokiError at the moment

I guess it's good to use a deny-list as we want alerts, in general, being set by default

I think as a default behavior we want to enable alerts. Using an allow list would make upgrade with new alert a bit more trickier since we would not know if it is still the default list and if we should add the new alert to the upgrade.

I will improve the documentation to mention the possibilities.

Also, I would remove the FLP mention, as it might not always be clear to the users when it's related to FLP or not (they don't necessarily know well our architecture)

Suggested change

// disableAlerts is a list of alerts related to FLP that should not be created

// disableAlerts is a list of alerts that should not be created. Possible values are: `NetObservNoFlows`, which is triggered when no flows are being observed for a certain period, and `NetObservLokiError`, which is triggered when flows are being dropped due to Loki errors.

cc @skrthomas could you please also review the CRD doc added here?
Thanks

I added your change but also moved to an enum so we can control the potential values.

codecov · 2023-03-14T11:04:22Z

Codecov Report

Merging #295 (70b7d52) into main (8a4db3c) will decrease coverage by 0.04%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##             main     #295      +/-   ##
==========================================
- Coverage   47.61%   47.57%   -0.04%     
==========================================
  Files          43       43              
  Lines        5015     5036      +21     
==========================================
+ Hits         2388     2396       +8     
- Misses       2416     2427      +11     
- Partials      211      213       +2

Flag	Coverage Δ
unittests	`47.57% <63.63%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/v1alpha1/flowcollector_webhook.go	`0.00% <0.00%> (ø)`
api/v1alpha1/zz_generated.conversion.go	`0.27% <0.00%> (+<0.01%)`	⬆️
api/v1beta1/flowcollector_types.go	`100.00% <ø> (ø)`
api/v1beta1/zz_generated.deepcopy.go	`42.33% <0.00%> (-0.60%)`	⬇️
controllers/flowlogspipeline/flp_common_objects.go	`84.95% <89.74%> (-0.37%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jotak · 2023-03-14T13:10:43Z

api/v1beta1/flowcollector_types.go

+)
+
+// Name of a processor alert
+// Possible values are: `NetObservNoFlows`, which is triggered when no flows are being observed for a certain period, and `NetObservLokiError`, which is triggered when flows are being dropped due to Loki errors.disableAlerts is a list of alerts related to FLP that should not be created


The end of the comment seems wrong here: disableAlerts is a list of alerts related to FLP that should not be created

I agree. The bit "related to FLP" could be unpacked a little. Could it be a list of alerts related to the health of flows in the FLP? 🤔 Also , a space is needed after the period and before disableAlerts.

skrthomas

I left a few comments. Mostly a couple of punctuation suggestions and also wondering about the same phrase Joel was asking about.

skrthomas · 2023-03-14T13:20:59Z

api/v1beta1/flowcollector_types.go

+)
+
+// Name of a processor alert
+// Possible values are: `NetObservNoFlows`, which is triggered when no flows are being observed for a certain period, and `NetObservLokiError`, which is triggered when flows are being dropped due to Loki errors.disableAlerts is a list of alerts related to FLP that should not be created


Suggested change

// Possible values are: `NetObservNoFlows`, which is triggered when no flows are being observed for a certain period, and `NetObservLokiError`, which is triggered when flows are being dropped due to Loki errors.disableAlerts is a list of alerts related to FLP that should not be created

// Possible values are: `NetObservNoFlows`, which is triggered when no flows are being observed for a certain period, and `NetObservLokiError`, which is triggered when flows are being dropped due to Loki errors. disableAlerts is a list of alerts related to FLP that should not be created

skrthomas · 2023-03-14T13:23:46Z

api/v1beta1/flowcollector_types.go

+)
+
+// Name of a processor alert
+// Possible values are: `NetObservNoFlows`, which is triggered when no flows are being observed for a certain period, and `NetObservLokiError`, which is triggered when flows are being dropped due to Loki errors.disableAlerts is a list of alerts related to FLP that should not be created


I agree. The bit "related to FLP" could be unpacked a little. Could it be a list of alerts related to the health of flows in the FLP? 🤔 Also , a space is needed after the period and before disableAlerts.

skrthomas · 2023-03-14T13:25:56Z

bundle/manifests/flows.netobserv.io_flowcollectors.yaml

+                        description: disableAlerts is a list of alerts that should
+                          not be created.
+                        items:
+                          description: 'Name of a processor alert Possible values


Suggested change

description: 'Name of a processor alert Possible values

description: 'Name of a processor alert. Possible values

skrthomas · 2023-03-14T13:26:27Z

api/v1beta1/flowcollector_types.go

+	AlertLokiError = "NetObservLokiError"
+)
+
+// Name of a processor alert


Suggested change

// Name of a processor alert

// Name of a processor alert.

Same as my comment below. A period is needed here.

OlivierCazade · 2023-03-14T16:18:08Z

@Amoghrd this PR is ready for testing, using the new field you should be able to see that the alert rule is not set.

To see alert rule go to CustomResourceDefinition -> prometheusRules, there should be an instance called flowlogs-pipeline-alert and in the yaml of this instance you should have a list of the alerts under spec -> groups

Amoghrd · 2023-03-14T18:00:27Z

/ok-to-test

github-actions · 2023-03-14T18:06:18Z

New images:

quay.io/netobserv/network-observability-operator:27d0843
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-27d0843
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-27d0843

They will expire after two weeks.

Catalog source:

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: netobserv-dev
  namespace: openshift-marketplace
spec:
  sourceType: grpc
  image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-27d0843
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

jotak

lgtm

jotak · 2023-03-16T08:24:52Z

@skrthomas since there is both this PR and also Julien's one that require regenerating the doc, what I can do is once they are merged I'll open a new one with just the doc update to have all at once

skrthomas

Sounds good, @jotak. lgtm!

Amoghrd · 2023-03-16T14:57:52Z

/label qe-approved

jotak · 2023-03-16T15:56:22Z

@OlivierCazade I'm merging this, I have a pending PR that conflicts with this one
/approve

openshift-ci · 2023-03-16T15:56:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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

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

OlivierCazade requested review from jotak and jpinsonneau March 14, 2023 09:04

openshift-ci-robot added the jira/valid-reference label Mar 14, 2023

jotak reviewed Mar 14, 2023

View reviewed changes

OlivierCazade added 2 commits March 14, 2023 13:04

Added field to disable alerts creation

45fe96d

Added enum for FLP alerts name

316d507

OlivierCazade force-pushed the disable-alerts branch from 4b89c94 to 316d507 Compare March 14, 2023 12:06

jotak reviewed Mar 14, 2023

View reviewed changes

skrthomas reviewed Mar 14, 2023

View reviewed changes

OlivierCazade force-pushed the disable-alerts branch from 7776ef4 to 59d9061 Compare March 14, 2023 13:51

DisableAlerts field wording.

70b7d52

OlivierCazade force-pushed the disable-alerts branch from 59d9061 to 70b7d52 Compare March 14, 2023 14:11

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 14, 2023

jotak approved these changes Mar 16, 2023

View reviewed changes

openshift-ci bot assigned jotak Mar 16, 2023

openshift-ci bot added the lgtm label Mar 16, 2023

skrthomas approved these changes Mar 16, 2023

View reviewed changes

openshift-ci bot assigned skrthomas Mar 16, 2023

openshift-ci bot added the qe-approved QE has approved this pull request label Mar 16, 2023

openshift-ci bot added the approved label Mar 16, 2023

jotak merged commit 88c67a6 into netobserv:main Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-881: Added field to disable alerts creation #295

NETOBSERV-881: Added field to disable alerts creation #295

OlivierCazade commented Mar 14, 2023

openshift-ci-robot commented Mar 14, 2023 •

edited by openshift-ci bot

Loading

jotak Mar 14, 2023

OlivierCazade Mar 14, 2023

jotak Mar 14, 2023

jotak Mar 14, 2023

OlivierCazade Mar 14, 2023

jotak Mar 14, 2023

jotak Mar 14, 2023

OlivierCazade Mar 14, 2023

codecov bot commented Mar 14, 2023 •

edited

Loading

jotak Mar 14, 2023

skrthomas Mar 14, 2023

skrthomas left a comment

skrthomas Mar 14, 2023

skrthomas Mar 14, 2023

skrthomas Mar 14, 2023

skrthomas Mar 14, 2023

skrthomas Mar 14, 2023

OlivierCazade commented Mar 14, 2023

Amoghrd commented Mar 14, 2023

github-actions bot commented Mar 14, 2023

jotak left a comment

jotak commented Mar 16, 2023

skrthomas left a comment

Amoghrd commented Mar 16, 2023

jotak commented Mar 16, 2023

openshift-ci bot commented Mar 16, 2023

	// disableAlerts is a list of alerts related to FLP that should not be created
	// disableAlerts is a list of alerts that should not be created. Possible values are: `NetObservNoFlows`, which is triggered when no flows are being observed for a certain period, and `NetObservLokiError`, which is triggered when flows are being dropped due to Loki errors.

	// Possible values are: `NetObservNoFlows`, which is triggered when no flows are being observed for a certain period, and `NetObservLokiError`, which is triggered when flows are being dropped due to Loki errors.disableAlerts is a list of alerts related to FLP that should not be created
	// Possible values are: `NetObservNoFlows`, which is triggered when no flows are being observed for a certain period, and `NetObservLokiError`, which is triggered when flows are being dropped due to Loki errors. disableAlerts is a list of alerts related to FLP that should not be created

	description: 'Name of a processor alert Possible values
	description: 'Name of a processor alert. Possible values

NETOBSERV-881: Added field to disable alerts creation #295

NETOBSERV-881: Added field to disable alerts creation #295

Conversation

OlivierCazade commented Mar 14, 2023

openshift-ci-robot commented Mar 14, 2023 • edited by openshift-ci bot Loading

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

codecov bot commented Mar 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skrthomas 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

OlivierCazade commented Mar 14, 2023

Amoghrd commented Mar 14, 2023

github-actions bot commented Mar 14, 2023

jotak left a comment

Choose a reason for hiding this comment

jotak commented Mar 16, 2023

skrthomas left a comment

Choose a reason for hiding this comment

Amoghrd commented Mar 16, 2023

jotak commented Mar 16, 2023

openshift-ci bot commented Mar 16, 2023

openshift-ci-robot commented Mar 14, 2023 •

edited by openshift-ci bot

Loading

codecov bot commented Mar 14, 2023 •

edited

Loading