NETOBSERV-220 implement ovn-kubernetes reconciler #97

jotak · 2022-05-06T15:33:18Z

It's creating a new reconciler for upstream ovn-k daemonset config

openshift-ci · 2022-05-06T15:33:20Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

jotak · 2022-05-06T15:33:51Z

cc @eranra

eranra · 2022-05-08T09:00:27Z

@jotak let me know if and how this needs to be connected to #65 . I ended up removing the kind script from #65 and used the simulator to generate flow logs.

I believe that providing upstream operator with kind support is important (if this is doable for us) .

mariomac · 2022-05-12T07:35:07Z

controllers/flowlogspipeline/flp_objects.go

+			HostNetwork:        hostNetwork,
+			DNSPolicy:          corev1.DNSClusterFirst,


As already commented in another thread:

You don't need to use the host network. I could confirm in a manual deployment that (1) the FLP Daemonset can be deployed without hostNetwork: true and (2) the agent can anyway use the HostIP + FLP Port to only send flows to its own host.

If anyway you want to use hostNetwork: true by any other reason I'm not aware of, you need to set DNSPolicy: corev1.DNSClusterFirstWithHostNet to allow FLP to access K8s services by its name, such as loki while keeping the host network.

jotak · 2022-05-17T17:16:19Z

controllers/flowcollector_controller.go

-	if err := ovsConfigController.Reconcile(ctx, desired, gfReconciler.GetServiceName(desired.Spec.Kafka)); err != nil {
-		return ctrl.Result{},
-			fmt.Errorf("failed to reconcile ovs-flows-config ConfigMap: %w", err)
+	if desired.Spec.ClusterNetworkOperator.Namespace != "" {


maybe I should replace the namespace check with something more explicit like a new field Spec.ClusterNetworkOperator.Enabled to switch between ovn-k and CNO

To avoid user intervention by means of configuration, in some other parts of the we try to discover if components are enabled by fetching concrete ApiService, e.g:

https://github.com/netobserv/network-observability-operator/blob/main/pkg/discover/discover.go#L32-L43

https://github.com/netobserv/network-observability-operator/blob/main/controllers/flowcollector_controller.go#L216-L224

I don't know if this could be useful also in this case.

Actually I see that you are already using discovery here.

Can we assume that any OpenShift cluster with OVN-K will have the CNO already installed?

Off-topic: as a future task, it would be nice to discover OVN-K or another CNI and then deploy IPFIX or eBPF by default.

+1 let me rework that

So I'm using the discovery client to check for presence of the CNO now.
I haven't done the ebpf switch as you suggest although I agree it will be nice

mariomac

Overall looks fine. ATM I have only tested it in downstream OpenShift, to verify that we don't break anything.

mariomac · 2022-05-18T08:23:05Z

README.md

@@ -110,26 +110,17 @@ Note that the `FlowCollector` resource must be unique and must be named `cluster

 ## Enabling OVS IPFIX export

-If you use OpenShift 4.10, you don't have anything to do: the operator will configure OVS *via* the Cluster Network Operator. Else, some manual steps are still required:
+If you use OpenShift 4.10 or the upstream ovn-kubernetes without OpenShift, you don't have anything to do: the operator will configure OVS *via* OpenShift Cluster Network Operator, or the ovn-kubernetes layer directly.


I'd create a new JIRA issue to document eBPF as an alternative to IPFIX that is not restricted to OVN and might work out of the box with OpenShift 4.8/4.9

oh yes exactly, I wanted to ask you if you had that planned already :)

Created JIRA issue: https://issues.redhat.com/browse/NETOBSERV-370

mariomac · 2022-05-18T08:58:49Z

controllers/flowcollector_controller.go

-	if err := ovsConfigController.Reconcile(ctx, desired, gfReconciler.GetServiceName(desired.Spec.Kafka)); err != nil {
-		return ctrl.Result{},
-			fmt.Errorf("failed to reconcile ovs-flows-config ConfigMap: %w", err)
+	if desired.Spec.ClusterNetworkOperator.Namespace != "" {


To avoid user intervention by means of configuration, in some other parts of the we try to discover if components are enabled by fetching concrete ApiService, e.g:

https://github.com/netobserv/network-observability-operator/blob/main/pkg/discover/discover.go#L32-L43

https://github.com/netobserv/network-observability-operator/blob/main/controllers/flowcollector_controller.go#L216-L224

I don't know if this could be useful also in this case.

mariomac · 2022-05-18T09:03:05Z

controllers/flowcollector_controller.go

-	if err := ovsConfigController.Reconcile(ctx, desired, gfReconciler.GetServiceName(desired.Spec.Kafka)); err != nil {
-		return ctrl.Result{},
-			fmt.Errorf("failed to reconcile ovs-flows-config ConfigMap: %w", err)
+	if desired.Spec.ClusterNetworkOperator.Namespace != "" {


Actually I see that you are already using discovery here.

Can we assume that any OpenShift cluster with OVN-K will have the CNO already installed?

Off-topic: as a future task, it would be nice to discover OVN-K or another CNI and then deploy IPFIX or eBPF by default.

mariomac · 2022-05-18T09:05:16Z

controllers/ovs/flowsconfig_ovnk_reconciler.go

+)
+
+const (
+	// Make configurable?


I think we should find a way, with the difficulty that the default value would be openshift-ovn-kubernetes for downstream OpenShift and ovn-kubernetes for upstream OVN-K.

But I don't think it's something mandatory right now.

done, there's a new entry in CR now to define upstream ovnk setup
I don't think it's very useful to handle the upstream ovnk case with openshift, I guess we can assume that the CNO will be used in that case.

mariomac · 2022-05-18T09:06:12Z

controllers/ovs/flowsconfig_ovnk_reconciler.go

+		return err
+	}
+
+	ovnkubeNode := reconcilers.FindContainer(&current.Spec.Template.Spec, "ovnkube-node")


instead of the literal, shall you use the constant ovnkDSName?

I actually introduced it as a config

Distinct case openshift / other for hostnetwork - With Openshift use its SCC API - Else use pod's spec hostnetwork Update documentation

- Configure ovnk (namespace, daemonset, container name). Defaults should work in most cases. - Detect CNO presence to switch between CNO or upstream ovnk - Merge the existing Console detection in a new "AvailableAPIs" mechanism in the existing dicover package

mariomac · 2022-05-19T08:25:25Z

pkg/discover/apis.go

@@ -0,0 +1,51 @@
+package discover


Nice! In a future patch I'll merge here the Openshift/Vanilla kubernetes

yeah I wasn't sure if your permissions code could be merged or if there was some additional logic

mariomac · 2022-05-19T08:26:42Z

controllers/flowcollector_controller.go

+	if !apis.HasCNO() {
+		log.Info("CNO not detected: using ovnKubernetes config and reconciler")
 	}


Is the intention here only to log a message?

yes, I find it useful while checking the logs :)

jotak · 2022-05-19T08:36:27Z

thanks @mariomac !
/approve

openshift-ci · 2022-05-19T08:36:32Z

[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

openshift-ci bot added the do-not-merge/work-in-progress label May 6, 2022

jotak changed the title ~~WIP make it work with kind~~ WIP make it work better with kind May 6, 2022

jotak force-pushed the kind-wip branch from afb9d4e to 9711b44 Compare May 10, 2022 08:48

jotak changed the title ~~WIP make it work better with kind~~ NETOBSERV-220 implement ovn-kubernetes reconciler May 10, 2022

jotak mentioned this pull request May 10, 2022

NETOBSERV-255: FLP multiple deployments #78

Merged

jotak force-pushed the kind-wip branch from 9711b44 to ad248cf Compare May 11, 2022 07:40

jotak mentioned this pull request May 11, 2022

add development_kind #65

Merged

mariomac reviewed May 12, 2022

View reviewed changes

jotak force-pushed the kind-wip branch from ad248cf to 2f7444c Compare May 17, 2022 16:25

jotak marked this pull request as ready for review May 17, 2022 16:36

openshift-ci bot removed the do-not-merge/work-in-progress label May 17, 2022

jotak requested a review from mariomac May 17, 2022 17:14

jotak commented May 17, 2022

View reviewed changes

jotak force-pushed the kind-wip branch from 2f7444c to 5d92d95 Compare May 18, 2022 00:19

mariomac reviewed May 18, 2022

View reviewed changes

mariomac approved these changes May 18, 2022

View reviewed changes

openshift-ci bot assigned mariomac May 18, 2022

openshift-ci bot added the lgtm label May 18, 2022

Create ovn-k reconciler that directly acts on ovnk daemonset config

ee72bf8

Distinct case openshift / other for hostnetwork - With Openshift use its SCC API - Else use pod's spec hostnetwork Update documentation

jotak force-pushed the kind-wip branch from 5d92d95 to 9d74d07 Compare May 18, 2022 15:19

openshift-ci bot removed the lgtm label May 18, 2022

jotak force-pushed the kind-wip branch from 9d74d07 to c781ed3 Compare May 18, 2022 16:42

jotak force-pushed the kind-wip branch from c781ed3 to 4940b24 Compare May 18, 2022 16:49

Add finalizer to cleanup ovn-kube env

8b02c82

mariomac reviewed May 19, 2022

View reviewed changes

mariomac approved these changes May 19, 2022

View reviewed changes

openshift-ci bot added the lgtm label May 19, 2022

openshift-ci bot added the approved label May 19, 2022

openshift-merge-robot merged commit 64b0d2e into netobserv:main May 19, 2022

jotak deleted the kind-wip branch December 7, 2022 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-220 implement ovn-kubernetes reconciler #97

NETOBSERV-220 implement ovn-kubernetes reconciler #97

jotak commented May 6, 2022 •

edited

Loading

openshift-ci bot commented May 6, 2022

jotak commented May 6, 2022

eranra commented May 8, 2022

mariomac May 12, 2022

jotak May 17, 2022

mariomac May 18, 2022

mariomac May 18, 2022

jotak May 18, 2022

jotak May 18, 2022

mariomac left a comment

mariomac May 18, 2022

jotak May 18, 2022

mariomac May 18, 2022

mariomac May 18, 2022

mariomac May 18, 2022

mariomac May 18, 2022

jotak May 18, 2022

mariomac May 18, 2022

jotak May 18, 2022

mariomac May 19, 2022

jotak May 19, 2022

mariomac May 19, 2022

jotak May 19, 2022

jotak commented May 19, 2022

openshift-ci bot commented May 19, 2022

NETOBSERV-220 implement ovn-kubernetes reconciler #97

NETOBSERV-220 implement ovn-kubernetes reconciler #97

Conversation

jotak commented May 6, 2022 • edited Loading

openshift-ci bot commented May 6, 2022

jotak commented May 6, 2022

eranra commented May 8, 2022

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

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

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 May 19, 2022

openshift-ci bot commented May 19, 2022

jotak commented May 6, 2022 •

edited

Loading