NETOBSERV-155: Added sampling field to console frontend configuration #264

OlivierCazade · 2023-02-13T13:22:37Z

Added sampling field to configuration.

To do that I needed to keep a reference to the FlowCollectorSpec object in the console object builder so I added a bit of refactoring to only keep this object and remove ConsolePlugin and Loki reference which are not needed anymore.

openshift-ci-robot · 2023-02-13T13:22:40Z

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

In response to this:

Added sampling field to configuration.

To do that I needed to keep a reference to the FlowCollectorSpec object in the console object builder so I added a bit of refactoring to only keep this object and remove ConsolePlugin and Loki reference which are not needed anymore.

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.

stleerh · 2023-02-15T19:55:55Z

controllers/consoleplugin/consoleplugin_objects.go

-	if b.desiredLoki != nil && b.desiredLoki.TLS.Enable && !b.desiredLoki.TLS.InsecureSkipVerify {
-		volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &b.desiredLoki.TLS, lokiCerts, b.cWatcher)
+	args := buildArgs(&b.desired.ConsolePlugin, &b.desired.Loki)
+	if b.desired != nil && b.desired.Loki.TLS.Enable && !b.desired.Loki.TLS.InsecureSkipVerify {


Do you still need to check b.desired.Loki != nil?

b.desiredLoki was a pointer while b.desired.Loki is not, we only need to check b.desired here

stleerh · 2023-02-15T20:39:17Z

controllers/consoleplugin/consoleplugin_test.go

@@ -110,13 +110,14 @@ func TestContainerUpdateCheck(t *testing.T) {
 	//equals specs
 	plugin := getPluginConfig()
 	loki := &flowsv1alpha1.FlowCollectorLoki{URL: "http://loki:3100/", TenantID: "netobserv"}
-	builder := newBuilder(testNamespace, testImage, &plugin, loki, &certWatcher)
+	spec := &flowsv1alpha1.FlowCollectorSpec{ConsolePlugin: plugin, Loki: *loki}
+	builder := newBuilder(testNamespace, testImage, spec, &certWatcher)


(Minor) For loki, there's no point in taking the address of (&) only to dereference it back (*) in the next line. Also, you might then want spec to be consistent.

loki := flowsv1alpha1.FlowCollectorLoki{URL: "http://loki:3100/", TenantID: "netobserv"} spec := flowsv1alpha1.FlowCollectorSpec{ConsolePlugin: plugin, Loki: loki} builder := newBuilder(testNamespace, testImage, &spec, &certWatcher)

If you do this, make sure you change it in all the other places in the code.

Fixed, thanks!

msherif1234 · 2023-02-15T21:54:11Z

api/v1alpha1/flowcollector_types.go

+		return int(*spec.Agent.EBPF.Sampling)
+	}
+	return int(spec.Agent.IPFIX.Sampling)
+}


why adding this util in the api types file, can you move to pkg/helpers ? it can be an utility api doesn't need to be a method right ?

Until now, helper directly related to the API were added here. Similar to UseEBPF, UseIPFIX...

It does not need to be a method but it improve code readability IMO.

I count 9 helper function, do you think we should move all of them in a new helper package?

I think so giving we will have different versions it will be a hassle, do we need to copy them to newer version or keep in the old one what if we need to delete the old api so additional Maintenance for no reason imo

@msherif1234 Personally I've found these methods quite nice to use and I don't think I'm alone - but for sure they have to stay trivial being just syntactic sugar, directly tied to nothing but the model itself, IMO.

I don't really get the comment on the maintenance hassle: if we use helpers, that would be the same, or even worse, as they still need to be duplicated per crd version? When creating a new version, we just copy from the existing one, update the package name and that's it. Am I missing something here?

I am not against the apis, my comment was about their location , they don't need to be in the api file I just suggested move them to helper pkg taht is it
as u know we have now v1alpha1, v1beta1 and at somepoint v1beta2 so are u gone copy them to all those apis header or keep them in v1alpha1, let me ask this what is the issue moving them out of the xxx_types.go to helper pkg ?

@jpinsonneau , I think Olivier's point is something like that: imagine I have a "UseMTLS" helper defined as:

func (tls *ClientTLS) UseMTLS() bool { return tls.UserCert.Name != "" }

Since ClientTLS is a struct embedded inside FlowCollector and used in different places, with your proposal we would have to duplicate this helper for each embedded case, such as:

func (dc *Desired) KafkaUseMTLS() bool { return dc.flowCollector.Spec.Kafka.TLS.UserCert.Name != "" } func (dc *Desired) LokiUseMTLS() bool { return dc.flowCollector.Spec.Loki.TLS.UserCert.Name != "" }

We already created another helper relying on functions for that situation. I'm not saying we should have only one solution; just adapt according to the needs.

If you guys are not confortable with these changes, feel free to rollback and let's take more time to see the impacts.

I think relying sometimes on external helpers and sometimes on a solution based on a Desired wrapper would only make things worse when writing code and looking to retrieve those helpers.

I'll give my +1 to the PR to not block it any further (too much time and energy spent here) but with the feeling that I don't think we've improved the code here

/lgtm

Epilogue: the topic triggered some discussion with the API team, having functions in api was not explicitly prohibited, but they said they will add that to their convention doc. My understanding is that our functions are not fundamentally bad but they need clear guidelines in order to ease their review process so they will prohibit that. So, given we want to follow those conventions (as much as possible), that's a win for the helper approach.

msherif1234 · 2023-02-15T21:56:07Z

controllers/consoleplugin/consoleplugin_objects.go

+	namespace string
+	labels    map[string]string
+	selector  map[string]string
+	desired   *flowsv1alpha1.FlowCollectorSpec


pls update your view we need to use flowslatest.FlowCollectorSpec here and everywhere v1alpha1 is referenced

Done, thanks!

jotak · 2023-02-22T09:41:16Z

api/v1alpha1/flowcollector_types.go

-func (spec *FlowCollectorSpec) UseEBPF() bool  { return spec.Agent.Type == AgentEBPF }
-func (spec *FlowCollectorSpec) UseIPFIX() bool { return spec.Agent.Type == AgentIPFIX }
-func (spec *FlowCollectorSpec) UseKafka() bool { return spec.DeploymentModel == DeploymentModelKafka }
-


(by the way, I don't think it was necessary to add the new helper to the v1alpha1 version, and maybe not doing so would have spared us from the controversy 😅)

Operator controller now point to v1beta1, this functions were not used anyway

OlivierCazade · 2023-02-22T10:45:30Z

/approve

openshift-ci · 2023-02-22T10:45:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: OlivierCazade

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

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 February 13, 2023 13:22

openshift-ci-robot added the jira/valid-reference label Feb 13, 2023

KalmanMeth pushed a commit to KalmanMeth/network-observability-operator that referenced this pull request Feb 13, 2023

vendor dependencies and speedup container build (netobserv#264)

d104d33

openshift-merge-robot added the needs-rebase label Feb 14, 2023

OlivierCazade force-pushed the console-sampling branch from 69b9e3b to 48780e3 Compare February 15, 2023 10:31

openshift-merge-robot removed the needs-rebase label Feb 15, 2023

stleerh reviewed Feb 15, 2023

View reviewed changes

msherif1234 reviewed Feb 15, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Feb 16, 2023

OlivierCazade added 2 commits February 17, 2023 15:36

Added sampling field to console frontend configuration

0e1aa23

Fix tests address management

2cbf593

OlivierCazade force-pushed the console-sampling branch from 48780e3 to 2cbf593 Compare February 17, 2023 14:39

openshift-merge-robot removed the needs-rebase label Feb 17, 2023

Refactoring to move API functions to helper section

69e9bd9

OlivierCazade force-pushed the console-sampling branch from b2d3c4f to 69e9bd9 Compare February 21, 2023 14:29

openshift-ci bot assigned jotak Feb 22, 2023

openshift-ci bot added the lgtm label Feb 22, 2023

jotak reviewed Feb 22, 2023

View reviewed changes

openshift-ci bot added the approved label Feb 22, 2023

openshift-merge-robot merged commit 7e7a9f8 into netobserv:main Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-155: Added sampling field to console frontend configuration #264

NETOBSERV-155: Added sampling field to console frontend configuration #264

OlivierCazade commented Feb 13, 2023

openshift-ci-robot commented Feb 13, 2023 •

edited by openshift-ci bot

Loading

stleerh Feb 15, 2023

OlivierCazade Feb 17, 2023

stleerh Feb 15, 2023

OlivierCazade Feb 17, 2023

msherif1234 Feb 15, 2023

OlivierCazade Feb 17, 2023

msherif1234 Feb 18, 2023

jotak Feb 20, 2023

msherif1234 Feb 20, 2023 •

edited

Loading

jotak Feb 22, 2023

jpinsonneau Feb 22, 2023

jpinsonneau Feb 22, 2023

jotak Feb 22, 2023

jotak Feb 22, 2023

msherif1234 Feb 15, 2023

OlivierCazade Feb 17, 2023

jotak Feb 22, 2023

OlivierCazade Feb 22, 2023

OlivierCazade commented Feb 22, 2023

openshift-ci bot commented Feb 22, 2023

NETOBSERV-155: Added sampling field to console frontend configuration #264

NETOBSERV-155: Added sampling field to console frontend configuration #264

Conversation

OlivierCazade commented Feb 13, 2023

openshift-ci-robot commented Feb 13, 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

msherif1234 Feb 20, 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

OlivierCazade commented Feb 22, 2023

openshift-ci bot commented Feb 22, 2023

openshift-ci-robot commented Feb 13, 2023 •

edited by openshift-ci bot

Loading

msherif1234 Feb 20, 2023 •

edited

Loading