NETOBSERV-555 NETOBSERV-552 CRD updates - many breaking changes #169

jotak · 2022-09-21T15:01:18Z

Main items:

NETOBSERV-555: "spec.flowlogsPipeline.kind" is removed, assuming
DaemonSet
processor.replicas/hpa are now
kafka.consumerReplicas/consumerAutoscaler (hpa renamed autoscaler,
also for console)
NETOBSERV-552: "ovn-kubernetes" and "cno" options moved into IPFIX section
IPFIX documented as a legacy option
NETOBSERV-544: remove kafka.enable option. Instead, use a top-level
enum for deployment mode: DIRECT or KAFKA
FLP reconciler refactoring (see first commit if you want to review that separately)

Smaller changes:

"flp.prometheus" option renamed to "flp.metricsServer", trying to be
more agnositc (it's actually openmetrics/opentelemetry standard) and
not make user thinks it actually communicates with prometheus
Refactored the main controller tests to adapt to new deployment
options
Make "FetchAll" logs less verbose

jotak · 2022-09-21T15:16:26Z

You can check just the first commit (9599fd4) to review separately the FLP reconciler refactoring.

mariomac

Overall, great job! I have few comments

mariomac · 2022-09-21T15:21:27Z

config/samples/flows_v1alpha1_flowcollector.yaml

@@ -4,12 +4,19 @@ metadata:
  name: cluster
 spec:
  namespace: netobserv
+  deploymentType: DIRECT


The name deploymentType sounds like a bit ambiguous to me. Maybe communication?

I'm not fan of deploymentType, but maybe not communication either .. let's get more ideas :)

what about optionnalComponents where we can list Kafka but also Prometheus and even Loki in the future if it become optional ?
It will be very flexible and would allow any new options.

If we really want to be explicit, we can simply use components with values like Agent-Processor and Agent-Kafka-Processor

Some ideas: wiring, connect, messaging, linking...

transfer...

mariomac · 2022-09-21T15:26:48Z

config/samples/flows_v1alpha1_flowcollector.yaml

@@ -4,12 +4,19 @@ metadata:
  name: cluster
 spec:
  namespace: netobserv
+  deploymentType: DIRECT
  agent:
    type: EBPF


Not 100% related to this task, but I was thinking in the case where a customer would like to override some properties of the eBPF agent, which is the only supported agent for downstream.

They should provide a yaml fragment like:

agent: ebpf: sampling: 1

Being ebpf the only section they should use, wouldn't it be cleaner just to:

remove agent.type

move all the ebpf stuff as properties of the agent section

try to find a "hidden" place for ipfix section that would allow overriding the eBPF agent without making our users to "pay" the price of a discriminated union.

Another option could be to move as properties of agent these properties that are common to all the agents (sampling, cacheMaxLenght, cacheMaxTimeout...) and keep some implementation details as subsections of ebpf (like the image, the image pull policy, etc...)

(Commenting here for the third time because I am reading this too fast...)

My understanding is we are finally keeping both agent downstream but we are going to document clearly that only the ebpf agent is officially supported.

But may be we could move to the agent section the fields that are relevent to both agent:

agent: sampling: 1 ebpf: ... ipfix: ...

they are not shared fields, and I don't think we want make them shared, for instance because we want to have different default values (eBPF supports higher sampling rate than IPFIX for instance)

OlivierCazade

The feedback we had during the CRD API review was also to not use boolean in the API, are we going to change this part?

I only reviewed the CRD change not the impacted code. I will continue the review tomorrow.

OlivierCazade · 2022-09-21T15:35:23Z

api/v1alpha1/flowcollector_types.go

+	//+kubebuilder:default:=1
+	// consumerReplicas defines the number of replicas (pods) to start for flowlogs-pipeline-transformer, which consumes Kafka messages.
+	// This setting is ignored when Kafka is disabled.
+	ConsumerReplicas int32 `json:"consumerReplicas,omitempty"`


I understand the logic of moving the field here, but I think it would be better to let it in the FLP section:

while this fields only apply when kafka is enabled, this field apply to FLP. May be we could name it better in FLP like "KafkaConsumerReplicas"

we would be more consitant with the console plugin section which also has this kind of field

OlivierCazade · 2022-09-21T15:35:36Z

api/v1alpha1/flowcollector_types.go

+	// consumerAutoscaler spec of a horizontal pod autoscaler to set up for flowlogs-pipeline-transformer, which consumes Kafka messages.
+	// This setting is ignored when Kafka is disabled.
+	// +optional
+	ConsumerAutoscaler *FlowCollectorHPA `json:"consumerAutoscaler,omitempty"`


Same comment here

stleerh · 2022-09-22T02:53:24Z

api/v1alpha1/flowcollector_types.go

+	AgentIPFIX           = "IPFIX"
+	AgentEBPF            = "EBPF"
+	DeploymentTypeDirect = "DIRECT"
+	DeploymentTypeKafka  = "KAFKA"


Maybe call it DeploymentModelSimple and DeploymentModelKafka?

I think we will need to organize a vote, because everyone has different ideas :)

stleerh · 2022-09-22T03:03:00Z

api/v1alpha1/flowcollector_types.go

+	//+kubebuilder:default:=1
+	// consumerReplicas defines the number of replicas (pods) to start for flowlogs-pipeline-transformer, which consumes Kafka messages.
+	// This setting is ignored when Kafka is disabled.
+	ConsumerReplicas int32 `json:"consumerReplicas,omitempty"`


Suggest FLPConsumerReplicas

stleerh · 2022-09-22T03:11:32Z

api/v1alpha1/flowcollector_types.go

+	// consumerAutoscaler spec of a horizontal pod autoscaler to set up for flowlogs-pipeline-transformer, which consumes Kafka messages.
+	// This setting is ignored when Kafka is disabled.
+	// +optional
+	ConsumerAutoscaler *FlowCollectorHPA `json:"consumerAutoscaler,omitempty"`


Are you sure you want to change HPA to Autoscaler everywhere? There are VPA (Vertical Pod Autoscaler) so HPA makes it more clear.

I can rollback it, but it was to avoid acronyms that maybe not everybody knows. and the full size "horizontalPodAutoscaler" is.. very long :/

stleerh · 2022-09-22T03:22:16Z

config/crd/bases/flows.netobserv.io_flowcollectors.yaml

+                      cases as it offers better performances and should work regardless
+                      of the CNI installed on the cluster. "IPFIX" works with OVN-Kubernetes
+                      CNI (other CNIs could work if they support exporting IPFIX,
+                      but they would require manual configuration).
                    enum:
                    - IPFIX
                    - EBPF


If this is the order in the UI dropdown, then put EBPF first.

stleerh · 2022-09-22T03:24:18Z

config/crd/bases/flows.netobserv.io_flowcollectors.yaml

+                      to start for flowlogs-pipeline-transformer, which consumes Kafka
+                      messages. This setting is ignored when Kafka is disabled.
+                    format: int32
+                    minimum: 0


Shouldn't this be 1?

stleerh · 2022-09-22T03:27:04Z

config/crd/bases/flows.netobserv.io_flowcollectors.yaml

+                      enable:
+                        default: false
+                        description: enable TLS
+                        type: boolean


Looks like TLS has a similar situation with "enable" as Kafka. Might be okay though.

i'll check the remaining booleans in a follow-up

jotak · 2022-09-22T14:46:14Z

I'll review the boolean fields as @OlivierCazade and @stleerh pointed out

Decoupling the inner "single reconcilers" into different files and structs: - Main entry point is still flp_reconciler.go, which now only initialises and calls inner reconcilers - ingest/transfo/monolith inner reconcilers do their own stuff in a decoupled way, with their own "*_objects" files - "flp_common_objects" is like a common lib for the specialized objects files

Main items: - NETOBSERV-555: "spec.flowlogsPipeline.kind" is removed, assuming DaemonSet - processor.replicas/hpa are now kafka.consumerReplicas/consumerAutoscaler (hpa renamed autoscaler, also for console) - NETOBSERV-552: "ovn-kubernetes" and "cno" options moved into IPFIX section - IPFIX documented as a legacy option - NETOBSERV-544: remove kafka.enable option. Instead, use a top-level enum for deployment mode: DIRECT or KAFKA Smaller changes: - "flp.prometheus" option renamed to "flp.metricsServer", trying to be more agnositc (it's actually openmetrics/opentelemetry standard) and not make user thinks it actually communicates with prometheus - Refactored the main controller tests to adapt to new deployment options - Make "FetchAll" logs less verbose

- move consumer replicas/hpa to FLP section rather than kafka - s/DeploymentType/DeploymentModel

mariomac

LGTM. A minor change suggestion

api/v1alpha1/flowcollector_types.go

Co-authored-by: Mario Macias <mmaciasl@redhat.com>

OlivierCazade

LGTM, thanks!

Amoghrd · 2022-09-27T13:58:30Z

/ok-to-test

github-actions · 2022-09-27T14:01:04Z

New image: ["quay.io/netobserv/network-observability-operator:eeb83b9"]. It will expire after two weeks.

openshift-ci · 2022-09-28T11:15:08Z

New changes are detected. LGTM label has been removed.

jotak · 2022-09-28T11:20:01Z

/approve

openshift-ci · 2022-09-28T11:20:07Z

[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

update cli title

jotak added the breaking-change This pull request has breaking changes. They should be described in PR description. label Sep 21, 2022

jotak requested review from mariomac, OlivierCazade and jpinsonneau September 21, 2022 15:01

mariomac reviewed Sep 21, 2022

View reviewed changes

OlivierCazade reviewed Sep 21, 2022

View reviewed changes

stleerh reviewed Sep 22, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase label Sep 22, 2022

jotak added 3 commits September 26, 2022 16:15

Feedback addressed

b106ca9

- move consumer replicas/hpa to FLP section rather than kafka - s/DeploymentType/DeploymentModel

jotak force-pushed the flp-deployment-opts branch from 71d42f7 to b106ca9 Compare September 26, 2022 14:47

openshift-merge-robot removed the needs-rebase label Sep 26, 2022

mariomac approved these changes Sep 27, 2022

View reviewed changes

api/v1alpha1/flowcollector_types.go Outdated Show resolved Hide resolved

openshift-ci bot assigned mariomac Sep 27, 2022

openshift-ci bot added the lgtm label Sep 27, 2022

Update api/v1alpha1/flowcollector_types.go

5d663e0

Co-authored-by: Mario Macias <mmaciasl@redhat.com>

openshift-ci bot removed the lgtm label Sep 27, 2022

OlivierCazade approved these changes Sep 27, 2022

View reviewed changes

openshift-ci bot assigned OlivierCazade Sep 27, 2022

openshift-ci bot added the lgtm label Sep 27, 2022

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

mariomac approved these changes Sep 28, 2022

View reviewed changes

Update CLI info

be5bd3f

openshift-ci bot removed the lgtm label Sep 28, 2022

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

openshift-ci bot added the approved label Sep 28, 2022

jotak merged commit 919f6a0 into netobserv:main Sep 28, 2022

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

Merge pull request netobserv#169 from eranra/fix-cli-title

0bc0398

update cli title

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-555 NETOBSERV-552 CRD updates - many breaking changes #169

NETOBSERV-555 NETOBSERV-552 CRD updates - many breaking changes #169

jotak commented Sep 21, 2022 •

edited

Loading

jotak commented Sep 21, 2022

mariomac left a comment

mariomac Sep 21, 2022

jotak Sep 21, 2022

jpinsonneau Sep 22, 2022

mariomac Sep 22, 2022

mariomac Sep 22, 2022

mariomac Sep 21, 2022

OlivierCazade Sep 21, 2022 •

edited

Loading

jotak Sep 21, 2022

OlivierCazade left a comment

OlivierCazade Sep 21, 2022

jotak Sep 26, 2022

OlivierCazade Sep 21, 2022

stleerh Sep 22, 2022

jotak Sep 22, 2022

stleerh Sep 22, 2022

stleerh Sep 22, 2022

jotak Sep 22, 2022

stleerh Sep 22, 2022

stleerh Sep 22, 2022

stleerh Sep 22, 2022

jotak Sep 26, 2022

jotak commented Sep 22, 2022

mariomac left a comment

OlivierCazade left a comment

Amoghrd commented Sep 27, 2022

github-actions bot commented Sep 27, 2022

openshift-ci bot commented Sep 28, 2022

jotak commented Sep 28, 2022

openshift-ci bot commented Sep 28, 2022

NETOBSERV-555 NETOBSERV-552 CRD updates - many breaking changes #169

NETOBSERV-555 NETOBSERV-552 CRD updates - many breaking changes #169

Conversation

jotak commented Sep 21, 2022 • edited Loading

jotak commented Sep 21, 2022

mariomac 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

OlivierCazade Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlivierCazade 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

jotak commented Sep 22, 2022

mariomac left a comment

Choose a reason for hiding this comment

OlivierCazade left a comment

Choose a reason for hiding this comment

Amoghrd commented Sep 27, 2022

github-actions bot commented Sep 27, 2022

openshift-ci bot commented Sep 28, 2022

jotak commented Sep 28, 2022

openshift-ci bot commented Sep 28, 2022

jotak commented Sep 21, 2022 •

edited

Loading

OlivierCazade Sep 21, 2022 •

edited

Loading