Remove lingering nb/sb ctl calls from the code base #2697

astoycos · 2021-12-08T17:58:47Z

Removes/ converts all (that I'm aware of) nb/sbctl calls and converts them to use libovsdb. (except for #2574) This is mostly completed by adding helper functions to libovsdbops package. Its also moves the libovsdbops package to go-controller/pkg since its used by many packages in that directory, not just the OVN package

TODO:

Test on a downstream merge to ensure we didn't break the Hybrid Overlay feature
Remove final nb/sbctl util functions for verifying tables exist server side when model: Connecting to DBs that don't contain tables that are in the model should not be an error. libovsdb#235 is fixed
Decide if we want to remove nb/sbctl calls from the ovn-kube-util readiness probe binary (I left it for now)

coveralls · 2021-12-08T19:20:25Z

Coverage decreased (-0.3%) to 49.824% when pulling cd445c5 on astoycos:remove-old-clients into ae6172e on ovn-org:master.

jcaamano

Just some minor things.

go-controller/vendor/k8s.io/api/certificates/v1/doc.go~merged

go-controller/pkg/util/ovs.go

go-controller/pkg/ovn/libovsdbops/encap.go

go-controller/pkg/node/node.go

go-controller/hybrid-overlay/pkg/controller/node_linux.go

go-controller/pkg/node/node.go

go-controller/pkg/ovn/libovsdbops/meter.go

astoycos · 2021-12-09T22:04:15Z

NOTE: I had an all green CI run on by local fork here -> https://github.com/astoycos/ovn-kubernetes/actions/runs/1560092606

flavio-fernandes

Good stuff! Just some small-ish comment, Andrew.

flavio-fernandes · 2021-12-09T22:10:42Z

go-controller/pkg/ovn/libovsdbops/meter.go

+			},
+		},
+		{
+			Model: meter,


in the unlikely scenario that meter already had a meterBand, consider making this a mutateInsert opeartion?
Otherwise maybe we should call this fcn as ... 'CreateMeterAndSetMeterBand' ?!? Maybe not, that is ugly :P

Well we assume if we're in the function the meter didn't exist, and we're also assuming that if the meter didn't exist that the meterBand doesn't exist either (since there's no way to lookup the meterband without looking up the meter)

go-controller/pkg/ovn/libovsdbops/model.go

go-controller/pkg/ovn/libovsdbops/logical_switch_port.go

go-controller/pkg/node/node.go

go-controller/pkg/ovn/libovsdbops/encap.go

go-controller/pkg/ovn/controller/unidling/ovn_event.go

trozet

In general worried about making libovsdb clients on the node code. It will for sure break upgrades because node will upgrade first, try to connect to an older schema and fail to validate the client's schema expectation. I think we should figure out if we want libovsdb clients from the nodes and if the answer is yes, then we will need something like ovn-org/libovsdb#257

go-controller/cmd/ovnkube/ovnkube.go

go-controller/pkg/node/node.go

go-controller/hybrid-overlay/pkg/controller/node_windows.go

go-controller/hybrid-overlay/cmd/hybrid-overlay-node/hybrid-overlay-node.go

go-controller/pkg/node/node.go

go-controller/pkg/ovn/controller/unidling/ovn_event.go

trozet · 2021-12-10T15:30:01Z

go-controller/hybrid-overlay/pkg/controller/master.go

@@ -374,7 +375,7 @@ func (m *MasterController) setupHybridLRPolicySharedGw(nodeSubnets []*net.IPNet,

 		if len(logicalRouterPolicyRes) == 0 {
 			logicalPort := ovntypes.RouterToSwitchPrefix + nodeName
-			if err := util.CreateMACBinding(logicalPort, ovntypes.OVNClusterRouter, portMac, drIP); err != nil {
+			if err := util.CreateMACBinding(m.sbClient, logicalPort, ovntypes.OVNClusterRouter, portMac, drIP); err != nil {


note to self: Remove final sbctl calls from Hybrid Overlay commit looks good

go-controller/pkg/ovn/ovn.go

astoycos · 2021-12-10T18:06:31Z

Waiting a bit for any more comments here before taking out the node process libovsdb commits :(

dcbw · 2021-12-10T19:50:02Z

@astoycos so, yeah there are some OVN calls in the HO Linux... I think it's fine to pass an NBClient into NewNode/newNodeController and use that in the Linux code, because we never run standalone HO binary on Linux. Windows wouldn't use it anyway since it doesn't talk to OVN at all, so we can just pass nil in there or something.

astoycos · 2021-12-13T16:21:04Z

I've taken out the bits where I started making libovsdbclients per node, specifically in the HO node controller and ovnkube-node code, where we still will use nb/sbctl

however it would be nice if we could run the libovsdb-clients in a no-cache mode so that we could be consistent in it's use across the codebase

astoycos · 2021-12-13T18:54:14Z

Here's a quick summary of where we are still using nb/sbctl commands in the code base following this PR

ovn-kube-util binary

In the SBDBReadiness function
In the NBDBReadiness function

hybrid-overlay-node binary

nbctl command in the getLocalNodeSubnet function

ovnkube-node binary

sbctl commands setting up encaps in setupOVNNode()
sbctl command to tell if the server understands ovn-installed within the getOVNIFUpCheckMode() function

ovnkube-master binary

nbctl command to check if load_balancer_groups are enabled by the server in StartClusterMaster

The util functions to support the above raw nb/sbctl calls are found in the github.com/ovn-org/ovn-kubernetes/go-controller/util package

go-controller/cmd/ovnkube/ovnkube.go

trozet · 2021-12-14T23:12:27Z

go-controller/cmd/ovnkube/ovnkube.go

@@ -286,7 +290,6 @@ func runOvnKube(ctx *cli.Context) error {
 	// start the prometheus server to serve OVN Metrics (default port: 9476)
 	// Note: for ovnkube node mode dpu-host no ovn metrics is required as ovn is not running on the node.
 	if config.OvnKubeNode.Mode != types.NodeModeDPUHost && config.Kubernetes.OVNMetricsBindAddress != "" {
-		metrics.RegisterOvnMetrics(ovnClientset.KubeClient, node)


@girishmg iirc you put the metrics in the node because in upstream/nvidia you run OVN DB on its own node so running it on master isnt feasible. Is my memory correct? Maybe we should put the ovn metrics into ovn-dbchecker?

@trozet that is correct. To enable rolling updates, this is the split we have:

|-------------+-----------------------------+-----------| | Kind | Pod | Component | |-------------+-----------------------------+-----------| | Statefulset | ovn-nbdb | OVN | | Statefulset | ovn-sbdb | OVN | | Daemonset | ovn-host | OVN | | Deployment | ovn-northd (replicas=3) | | |-------------+-----------------------------+-----------| | Deployment | ovnkube-master (replicas=3) | OVN-K8s | | Daemonset | ovnkube-node | OVN-K8s | |-------------+-----------------------------+-----------|

There was also requirement from both RH and NVDA that we reduce the number of TCP Ports from which we expose prometheus metrics. If we have more TCP Ports, then we need equal instances of rbac-proxy to terminate TLS. So, we moved all the OVN/OVS/Ovnkube-node metric collection to ovnkube-node.

Ovnkube-master still continues to export it's metric

@girishmg what do you think about putting the metrics on ovndbchecker?

Convert the creation/ configuration of the OVN meter and meter_band objects for the acl-logging feature to libovdb Signed-off-by: astoycos <[email protected]>

Set the `controller_event` option on the nb_global table to `true` with libovsdb Signed-off-by: astoycos <[email protected]>

Convert all remaining nbctl calls from the hybridOverlay package Update unit test accordingly Signed-off-by: astoycos <[email protected]>

Remove the final sbctl calls regarding mac_bindings from the hybrid overlay code and convert to libovsdb Ensure we index mac_binding by both possible options, the `logical_port` OR `ip` columns Signed-off-by: astoycos <[email protected]>

Remove some erronous fexec calls Remove the NBtxn struct and associated helper functions Signed-off-by: astoycos <[email protected]>

Remove all of the unused nb/sb ctl helper functions from the util package and add notes on when the rest can be removed Signed-off-by: astoycos <[email protected]>

Move the libovsdbops package out of the `go-controller/pkg/ovn` directory and into `go-controller/pkg` since its used by many packages in the directory. Signed-off-by: astoycos <[email protected]>

astoycos · 2021-12-15T15:40:19Z

For now until we decide what to do with the ovn_db metrics I removed the commit from this PR and made an issue to track that work item

flavio-fernandes · 2021-12-15T16:24:21Z

@astoycos if possible, please paste a link to the tracking issue created here, so we can located from this pr.

astoycos · 2021-12-15T16:26:00Z

#2713

trozet

/lgtm

astoycos force-pushed the remove-old-clients branch 2 times, most recently from 0ecbe76 to 40ad9df Compare December 8, 2021 18:31

astoycos force-pushed the remove-old-clients branch 2 times, most recently from e9ade69 to ea871b8 Compare December 8, 2021 21:07

jcaamano requested changes Dec 9, 2021

View reviewed changes

astoycos force-pushed the remove-old-clients branch 2 times, most recently from b8b0007 to f16f210 Compare December 9, 2021 18:16

flavio-fernandes self-requested a review December 9, 2021 18:46

flavio-fernandes requested changes Dec 9, 2021

View reviewed changes

trozet requested changes Dec 10, 2021

View reviewed changes

astoycos mentioned this pull request Dec 10, 2021

Remove nbctl daemon and fix upgrade tests #2707

Merged

astoycos force-pushed the remove-old-clients branch from f16f210 to 0458314 Compare December 13, 2021 16:19

astoycos force-pushed the remove-old-clients branch from 0458314 to ddb344e Compare December 13, 2021 16:43

astoycos force-pushed the remove-old-clients branch 2 times, most recently from 9ca22b5 to a0b1625 Compare December 14, 2021 21:03

trozet requested changes Dec 14, 2021

View reviewed changes

go-controller/cmd/ovnkube/ovnkube.go Outdated Show resolved Hide resolved

astoycos force-pushed the remove-old-clients branch from a0b1625 to fad115e Compare December 14, 2021 23:02

trozet reviewed Dec 14, 2021

View reviewed changes

astoycos added 6 commits December 15, 2021 10:07

Convert OVN Meter + Meter_Band ops to libovsdb

9e12506

Convert the creation/ configuration of the OVN meter and meter_band objects for the acl-logging feature to libovdb Signed-off-by: astoycos <[email protected]>

set NB_Global options with libovsdb

95729a6

Set the `controller_event` option on the nb_global table to `true` with libovsdb Signed-off-by: astoycos <[email protected]>

Remove final nbctl calls from HO code

d47a3fc

Convert all remaining nbctl calls from the hybridOverlay package Update unit test accordingly Signed-off-by: astoycos <[email protected]>

Remove final sbctl calls from Hybrid Overlay

1b94cbb

Remove the final sbctl calls regarding mac_bindings from the hybrid overlay code and convert to libovsdb Ensure we index mac_binding by both possible options, the `logical_port` OR `ip` columns Signed-off-by: astoycos <[email protected]>

Cleanup OVN package

c3472e1

Remove some erronous fexec calls Remove the NBtxn struct and associated helper functions Signed-off-by: astoycos <[email protected]>

Remove some nb/sb ctl helper functions

c8c1899

Remove all of the unused nb/sb ctl helper functions from the util package and add notes on when the rest can be removed Signed-off-by: astoycos <[email protected]>

Move libovsdbops package

cd445c5

Move the libovsdbops package out of the `go-controller/pkg/ovn` directory and into `go-controller/pkg` since its used by many packages in the directory. Signed-off-by: astoycos <[email protected]>

astoycos mentioned this pull request Dec 15, 2021

Move OVN_DB metrics to ovnkube-db-checker #2713

Closed

astoycos force-pushed the remove-old-clients branch from fad115e to cd445c5 Compare December 15, 2021 15:39

astoycos mentioned this pull request Dec 15, 2021

Metrics: Start OVN DB/Northd metrics only on OVNKube master #2714

Closed

trozet approved these changes Dec 15, 2021

View reviewed changes

trozet merged commit 8b849a3 into ovn-org:master Dec 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove lingering nb/sb ctl calls from the code base #2697

Remove lingering nb/sb ctl calls from the code base #2697

astoycos commented Dec 8, 2021 •

edited

Loading

coveralls commented Dec 8, 2021 •

edited

Loading

jcaamano left a comment

astoycos commented Dec 9, 2021 •

edited

Loading

flavio-fernandes left a comment

flavio-fernandes Dec 9, 2021

astoycos Dec 10, 2021

trozet left a comment

trozet Dec 10, 2021

astoycos commented Dec 10, 2021

dcbw commented Dec 10, 2021

astoycos commented Dec 13, 2021 •

edited

Loading

astoycos commented Dec 13, 2021 •

edited

Loading

trozet Dec 14, 2021

girishmg Dec 14, 2021

trozet Dec 15, 2021

astoycos commented Dec 15, 2021

flavio-fernandes commented Dec 15, 2021

astoycos commented Dec 15, 2021

trozet left a comment

Remove lingering nb/sb ctl calls from the code base #2697

Remove lingering nb/sb ctl calls from the code base #2697

Conversation

astoycos commented Dec 8, 2021 • edited Loading

coveralls commented Dec 8, 2021 • edited Loading

jcaamano left a comment

Choose a reason for hiding this comment

astoycos commented Dec 9, 2021 • edited Loading

flavio-fernandes left a comment

Choose a reason for hiding this comment

flavio-fernandes Dec 9, 2021

Choose a reason for hiding this comment

astoycos Dec 10, 2021

Choose a reason for hiding this comment

trozet left a comment

Choose a reason for hiding this comment

trozet Dec 10, 2021

Choose a reason for hiding this comment

astoycos commented Dec 10, 2021

dcbw commented Dec 10, 2021

astoycos commented Dec 13, 2021 • edited Loading

astoycos commented Dec 13, 2021 • edited Loading

trozet Dec 14, 2021

Choose a reason for hiding this comment

girishmg Dec 14, 2021

Choose a reason for hiding this comment

trozet Dec 15, 2021

Choose a reason for hiding this comment

astoycos commented Dec 15, 2021

flavio-fernandes commented Dec 15, 2021

astoycos commented Dec 15, 2021

trozet left a comment

Choose a reason for hiding this comment

astoycos commented Dec 8, 2021 •

edited

Loading

coveralls commented Dec 8, 2021 •

edited

Loading

astoycos commented Dec 9, 2021 •

edited

Loading

astoycos commented Dec 13, 2021 •

edited

Loading

astoycos commented Dec 13, 2021 •

edited

Loading