NETOBSERV-1182 added cluster name to flp configuration #386

KalmanMeth · 2023-06-29T12:16:18Z

Added transform_filter to flp pipeline to add a clusterName field to flow logs.
clusterName is taken from config file. If blank, take clusterID from clusterVersion struct.

codecov · 2023-06-29T13:05:06Z

Codecov Report

Merging #386 (6b70f0e) into main (0d6ebe8) will increase coverage by 2.34%.
Report is 3 commits behind head on main.
The diff coverage is 85.40%.

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
+ Coverage   53.67%   56.02%   +2.34%     
==========================================
  Files          44       45       +1     
  Lines        5559     5865     +306     
==========================================
+ Hits         2984     3286     +302     
- Misses       2359     2362       +3     
- Partials      216      217       +1

Flag	Coverage Δ
unittests	`56.02% <85.40%> (+2.34%)`	⬆️

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

Files Changed	Coverage Δ
api/v1alpha1/zz_generated.conversion.go	`0.25% <0.00%> (-0.01%)`	⬇️
api/v1beta1/flowcollector_types.go	`100.00% <ø> (ø)`
controllers/flowlogspipeline/flp_reconciler.go	`67.85% <ø> (-5.32%)`	⬇️
controllers/flowcollector_controller.go	`55.55% <20.00%> (+0.42%)`	⬆️
...ntrollers/flowlogspipeline/flp_monolith_objects.go	`88.46% <75.00%> (ø)`
...ontrollers/flowlogspipeline/flp_transfo_objects.go	`93.68% <75.00%> (ø)`
controllers/flowcollector_objects.go	`46.91% <77.77%> (+9.62%)`	⬆️
controllers/flowlogspipeline/flp_common_objects.go	`82.01% <88.18%> (+0.24%)`	⬆️
pkg/helper/dashboard.go	`96.49% <96.49%> (ø)`
api/v1beta1/zz_generated.deepcopy.go	`55.36% <100.00%> (+1.58%)`	⬆️
... and 5 more

... and 5 files with indirect coverage changes

KalmanMeth · 2023-07-02T08:25:33Z

https://issues.redhat.com/browse/NETOBSERV-245
https://issues.redhat.com/browse/OCPSTRAT-641

jpinsonneau · 2023-07-10T07:50:54Z

api/v1beta1/flowcollector_types.go

+	//+kubebuilder:default:="defaultCluster"
+	// +optional
+	// `clusterName` is the name of the cluster.
+	ClusterName string `json:"clusterName,omitempty"`
+


For Openshift we can automatically retreive ClusterID from api
https://pkg.go.dev/github.com/openshift/api@v0.0.0-20230707160225-81d582da354b/config/v1#ClusterID

https://access.redhat.com/solutions/5280291

Suggested change

//+kubebuilder:default:="defaultCluster"

// +optional

// `clusterName` is the name of the cluster.

ClusterName string `json:"clusterName,omitempty"`

//+kubebuilder:default:=""

// +optional

// `clusterName` is the name of the cluster.

ClusterName string `json:"clusterName,omitempty"`

I would suggest to set default as empty to avoid FLP extra stage for single cluster usage. WDYT ?

If we automatically retrieve clusterID from openshift when clusterName is blank, we really cannot distinguish between single cluster vs multi-cluster. To distinguish between them, we would have to add an explicit parameter to the flowcollector config file.

Default set to empty.

jpinsonneau · 2023-07-10T07:52:53Z

controllers/flowlogspipeline/flp_common_objects.go

@@ -45,6 +45,7 @@ const (
 	conntrackTerminatingTimeout = 5 * time.Second
 	conntrackEndTimeout         = 10 * time.Second
 	conntrackHeartbeatInterval  = 30 * time.Second
+	clusterNameDefault          = "defaultCluster"


Suggested change

clusterNameDefault = "defaultCluster"

following previous comment

Changed default to blank, but in that case we take the clusterName from Openshift.

jpinsonneau · 2023-07-10T07:53:28Z

controllers/flowlogspipeline/flp_common_objects.go

@@ -310,15 +311,29 @@ func (b *builder) addTransformStages(stage *config.PipelineBuilderStage) (*corev
 	lastStage := *stage
 	indexFields := constants.LokiIndexFields

+	clusterName := clusterNameDefault


Suggested change

clusterName := clusterNameDefault

clusterName := ""

following previous comment

jpinsonneau · 2023-07-13T08:26:19Z

bundle/manifests/netobserv-operator.clusterserviceversion.yaml

@@ -351,7 +351,7 @@ metadata:
    capabilities: Seamless Upgrades
    categories: Monitoring
    console.openshift.io/plugins: '["netobserv-plugin"]'
-    containerImage: quay.io/netobserv/network-observability-operator:1.0.3
+    containerImage: quay.io/meth/network-observability-operator:main


you can run make update-bundle to avoid this 😸

network-observability-operator/Makefile

Lines 325 to 327 in 84c7fe8

.PHONY: update-bundle

update-bundle: IMG=$(IMAGE_TAG_BASE):$(OPERATOR_VERSION)

update-bundle: bundle ## Prepare a clean bundle to be commited

config/samples/flows_v1beta1_flowcollector.yaml

jotak · 2023-07-19T05:57:06Z

controllers/flowlogspipeline/flp_common_objects.go

+		//take clustername from openshift
+		clusterName = string(constants.DefaultClusterID)
+	}
+	transformFilterRules := []api.TransformFilterRule{


ClusterName can be empty (e.g. if not setup in CR and not running on openshift) => in that case, I think we should not create this rule

jotak · 2023-07-19T06:02:09Z

api/v1beta1/flowcollector_types.go

@@ -436,6 +436,11 @@ type FlowCollectorFLP struct {
 	// `conversationTerminatingTimeout` is the time to wait from detected FIN flag to end a conversation. Only relevant for TCP flows.
 	ConversationTerminatingTimeout *metav1.Duration `json:"conversationTerminatingTimeout,omitempty"`

+	//+kubebuilder:default:=""
+	// +optional
+	// `clusterName` is the name of the cluster.


Since this is a user-facing doc, it deserves some explanation, example:

Suggested change

// `clusterName` is the name of the cluster.

// `clusterName` is the name of the cluster to appear in the flows data. This is useful in a multi-cluster context. When using OpenShift, leave empty to make it automatically determined.

jotak · 2023-07-19T06:23:36Z

controllers/flowcollector_controller.go

+	cversion := &configv1.ClusterVersion{}
+	key := client.ObjectKey{Name: "version"}
+	if err := r.Client.Get(ctx, key, cversion); err != nil {
+		log.Error(err, "unable to obtain cluster ID")
+	} else {
+		constants.DefaultClusterID = cversion.Spec.ClusterID
+	}


This block should be run only once, and only for openshift. To do so you can wrap it in a block if r.permissions.Vendor(ctx) == discover.VendorOpenShift && constants.DefaultClusterID == "" {

Also it is unexpected to update a "constant". I would move DefaultClusterID somewhere else. For instance, it could be part of the FlowCollectorReconciler struct, and passed down to reconcilers via newCommonInfo; or if you prefer to keep a global access, maybe create a new file globals.go in package reconcilers ?

In order to pass down the clusterName through the series of function calls it would have required many changes and would have made the code ugly. I therefore made it into a global, as suggested.

That's ok
The "CommonInfo" is already done in that purpose, to be passed to the reconcilers, so you wouldn't have to change all the function signatures, just adding it to a struct that is already spread across reconcilers.
But that's ok with the global approach - as long as not everything ends up like that, which would denote a lack of modularity and makes sometimes testing more difficult

jotak · 2023-07-19T06:37:23Z

controllers/flowlogspipeline/flp_common_objects.go

+	}
+	transformFilterRules := []api.TransformFilterRule{
+		{
+			Input: "clusterName",


We should use the same pattern as the other fields, ie:

Suggested change

Input: "clusterName",

Input: "K8S_ClusterName",

github-actions · 2023-07-20T13:43:57Z

New images:

quay.io/netobserv/network-observability-operator:30c2fdc
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-30c2fdc
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-30c2fdc

They will expire after two weeks.

To deploy this build:

# Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:30c2fdc make deploy

# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-30c2fdc

Or as a 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-30c2fdc
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

jotak · 2023-07-20T14:19:35Z

/lgtm
thanks @KalmanMeth !
There's one pending question, that we can answer later: should we define K8S_ClusterName as a Loki index? I think we should, since it will become a common aggregating key for queries in a multi-cluster scenario. We can do that later.

PS : looks like the linter is unhappy due to high cyclomatic ...

KalmanMeth · 2023-07-24T10:44:22Z

PS : looks like the linter is unhappy due to high cyclomatic ...

I refactored a bit to make the linter happy.

jotak · 2023-07-24T16:45:08Z

Thanks !
/lgtm

mffiedler · 2023-07-25T17:25:49Z

@KalmanMeth @jotak Can we re-add ok-to-test?
nm - i see tests in progress

github-actions · 2023-07-26T10:25:37Z

New images:

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

They will expire after two weeks.

To deploy this build:

# Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:d93080e make deploy

# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-d93080e

Or as a 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-d93080e
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

KalmanMeth · 2023-08-03T06:54:58Z

performed rebase

github-actions · 2023-08-03T07:55:45Z

New images:

quay.io/netobserv/network-observability-operator:1a0da20
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-1a0da20
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-1a0da20

They will expire after two weeks.

To deploy this build:

# Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:1a0da20 make deploy

# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-1a0da20

Or as a 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-1a0da20
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

jotak · 2023-08-03T16:00:25Z

/lgtm

mffiedler · 2023-08-03T23:31:04Z

/label qe-approved

Basic testing looks good, can continue QE post-merge

jpinsonneau · 2023-08-04T07:46:46Z

/approve

openshift-ci · 2023-08-04T07:46:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpinsonneau

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

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

KalmanMeth requested review from jotak and jpinsonneau July 3, 2023 09:29

jpinsonneau changed the title ~~added cluster name to flp configuration~~ NETOBSERV-1182 added cluster name to flp configuration Jul 10, 2023

jpinsonneau reviewed Jul 10, 2023

View reviewed changes

jpinsonneau reviewed Jul 13, 2023

View reviewed changes

config/samples/flows_v1beta1_flowcollector.yaml Outdated Show resolved Hide resolved

jotak reviewed Jul 19, 2023

View reviewed changes

jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 20, 2023

openshift-ci bot assigned jotak Jul 20, 2023

openshift-ci bot added lgtm and removed lgtm labels Jul 20, 2023

openshift-merge-robot added the needs-rebase label Jul 24, 2023

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 24, 2023

KalmanMeth force-pushed the add-cluster-name branch from 2dbc9af to 1179cc9 Compare July 24, 2023 10:37

openshift-merge-robot removed the needs-rebase label Jul 24, 2023

KalmanMeth requested a review from jotak July 24, 2023 10:44

openshift-ci bot added the lgtm label Jul 24, 2023

This was referenced Jul 25, 2023

NETOBSERV-939: Detect which dashboards to show based on ocp version #399

Closed

initial support for labels netobserv/flowlogs-pipeline#447

Merged

KalmanMeth added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 26, 2023

openshift-merge-robot added the needs-rebase label Aug 1, 2023

add cluster ID to flow logs

6b70f0e

KalmanMeth force-pushed the add-cluster-name branch from 1179cc9 to 6b70f0e Compare August 3, 2023 06:54

openshift-ci bot removed the lgtm label Aug 3, 2023

openshift-merge-robot removed the needs-rebase label Aug 3, 2023

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 3, 2023

KalmanMeth added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 3, 2023

openshift-ci bot added the lgtm label Aug 3, 2023

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

openshift-ci bot added the approved label Aug 4, 2023

openshift-merge-robot merged commit 9d208dd into netobserv:main Aug 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-1182 added cluster name to flp configuration #386

NETOBSERV-1182 added cluster name to flp configuration #386

KalmanMeth commented Jun 29, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading

KalmanMeth commented Jul 2, 2023 •

edited

Loading

jpinsonneau Jul 10, 2023

jpinsonneau Jul 10, 2023

KalmanMeth Jul 13, 2023

KalmanMeth Jul 16, 2023

jpinsonneau Jul 10, 2023 •

edited

Loading

KalmanMeth Jul 13, 2023

jpinsonneau Jul 10, 2023

jpinsonneau Jul 13, 2023

KalmanMeth Jul 13, 2023

jotak Jul 19, 2023

KalmanMeth Jul 20, 2023

jotak Jul 19, 2023

KalmanMeth Jul 20, 2023

jotak Jul 19, 2023

jotak Jul 19, 2023

KalmanMeth Jul 20, 2023

jotak Jul 20, 2023

jotak Jul 19, 2023

KalmanMeth Jul 20, 2023

github-actions bot commented Jul 20, 2023

jotak commented Jul 20, 2023

KalmanMeth commented Jul 24, 2023

jotak commented Jul 24, 2023

mffiedler commented Jul 25, 2023 •

edited

Loading

github-actions bot commented Jul 26, 2023

KalmanMeth commented Aug 3, 2023

github-actions bot commented Aug 3, 2023

jotak commented Aug 3, 2023

mffiedler commented Aug 3, 2023

jpinsonneau commented Aug 4, 2023

openshift-ci bot commented Aug 4, 2023

	.PHONY: update-bundle
	update-bundle: IMG=$(IMAGE_TAG_BASE):$(OPERATOR_VERSION)
	update-bundle: bundle ## Prepare a clean bundle to be commited

	// `clusterName` is the name of the cluster.
	// `clusterName` is the name of the cluster to appear in the flows data. This is useful in a multi-cluster context. When using OpenShift, leave empty to make it automatically determined.

NETOBSERV-1182 added cluster name to flp configuration #386

NETOBSERV-1182 added cluster name to flp configuration #386

Conversation

KalmanMeth commented Jun 29, 2023 • edited Loading

codecov bot commented Jun 29, 2023 • edited Loading

Codecov Report

KalmanMeth commented Jul 2, 2023 • edited 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

jpinsonneau Jul 10, 2023 • edited 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

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 Jul 20, 2023

jotak commented Jul 20, 2023

KalmanMeth commented Jul 24, 2023

jotak commented Jul 24, 2023

mffiedler commented Jul 25, 2023 • edited Loading

github-actions bot commented Jul 26, 2023

KalmanMeth commented Aug 3, 2023

github-actions bot commented Aug 3, 2023

jotak commented Aug 3, 2023

mffiedler commented Aug 3, 2023

jpinsonneau commented Aug 4, 2023

openshift-ci bot commented Aug 4, 2023

KalmanMeth commented Jun 29, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading

KalmanMeth commented Jul 2, 2023 •

edited

Loading

jpinsonneau Jul 10, 2023 •

edited

Loading

mffiedler commented Jul 25, 2023 •

edited

Loading