From 4c1d7585d3caa9d42ab51dcd632556b6cd628f7d Mon Sep 17 00:00:00 2001 From: astoycos Date: Wed, 8 Dec 2021 11:03:28 -0500 Subject: [PATCH] Cleanup OVN package Remove some erronous fexec calls Remove the NBtxn struct and associated helper functions Signed-off-by: astoycos --- go-controller/pkg/ovn/egressfirewall_test.go | 7 ----- go-controller/pkg/ovn/libovsdbops/chassis.go | 2 +- go-controller/pkg/ovn/master_test.go | 24 ++++------------ go-controller/pkg/ovn/ovn.go | 30 ++++---------------- 4 files changed, 13 insertions(+), 50 deletions(-) diff --git a/go-controller/pkg/ovn/egressfirewall_test.go b/go-controller/pkg/ovn/egressfirewall_test.go index db30bfb68b2..d91a8d2da0f 100644 --- a/go-controller/pkg/ovn/egressfirewall_test.go +++ b/go-controller/pkg/ovn/egressfirewall_test.go @@ -378,13 +378,6 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", }, } - //fExec.AddFakeCmdsNoOutputNoError([]string{ - // fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%d priority>=%d", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - // fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%d priority>=%d direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - // "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ((udp && ( udp.dst == 100 ))) && ip4.dst != 10.128.0.0/14\" action=drop external-ids:egressFirewall=namespace1", - // "ovn-nbctl --timeout=15 --id=@node1-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ((udp && ( udp.dst == 100 ))) && ip4.dst != 10.128.0.0/14\" action=drop external-ids:egressFirewall=namespace1 -- add logical_switch " + node1Name + " acls @node1-10000", - //}) - namespace1 := *newNamespace("namespace1") egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ { diff --git a/go-controller/pkg/ovn/libovsdbops/chassis.go b/go-controller/pkg/ovn/libovsdbops/chassis.go index 5789d902b8f..b9321cdb957 100644 --- a/go-controller/pkg/ovn/libovsdbops/chassis.go +++ b/go-controller/pkg/ovn/libovsdbops/chassis.go @@ -10,7 +10,7 @@ import ( "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" ) -// findChassis returns all the logical chassis +// ListChassis returns all the logical chassis func ListChassis(sbClient libovsdbclient.Client) ([]sbdb.Chassis, error) { ctx, cancel := context.WithTimeout(context.Background(), types.OVSDBTimeout) defer cancel() diff --git a/go-controller/pkg/ovn/master_test.go b/go-controller/pkg/ovn/master_test.go index 0858bd28d16..6b108a81847 100644 --- a/go-controller/pkg/ovn/master_test.go +++ b/go-controller/pkg/ovn/master_test.go @@ -21,7 +21,6 @@ import ( addressset "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/address_set" lsm "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/logical_switch_manager" ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing" - "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb" libovsdbtest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util" @@ -64,6 +63,7 @@ type tNode struct { DnatSnatIP string } +/* func cleanupGateway(fexec *ovntest.FakeExec, nodeName string, nodeSubnet string, clusterCIDR string, nextHop string) { const ( node1RouteUUID string = "0cac12cf-3e0f-4682-b028-5ea2e0001962" @@ -88,7 +88,6 @@ func cleanupGateway(fexec *ovntest.FakeExec, nodeName string, nodeSubnet string, }) } -/* func defaultFakeExec(nodeSubnet, nodeName string, sctpSupport bool) *ovntest.FakeExec { const ( mgmtMAC string = "01:02:03:04:05:06" @@ -240,7 +239,7 @@ func addNodeportLBs(fexec *ovntest.FakeExec, nodeName, tcpLBUUID, udpLBUUID, sct } */ -func addNodeLogicalFlows(testData []libovsdb.TestData, expectedOVNClusterRouter *nbdb.LogicalRouter, expectedNodeSwitch *nbdb.LogicalSwitch, expectedClusterRouterPortGroup, expectedClusterPortGroup *nbdb.PortGroup, node *tNode, clusterCIDR string, enableIPv6 bool) []libovsdb.TestData { +func addNodeLogicalFlows(testData []libovsdbtest.TestData, expectedOVNClusterRouter *nbdb.LogicalRouter, expectedNodeSwitch *nbdb.LogicalSwitch, expectedClusterRouterPortGroup, expectedClusterPortGroup *nbdb.PortGroup, node *tNode, clusterCIDR string, enableIPv6 bool) []libovsdbtest.TestData { lrpName := types.RouterToSwitchPrefix + node.Name chassisName := node.SystemID @@ -891,11 +890,7 @@ var _ = ginkgo.Describe("Gateway Init Operations", func() { EgressFirewallClient: egressFirewallFakeClient, } - fexec := ovntest.NewLooseCompareFakeExec() - err := util.SetExec(fexec) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - - _, err = config.InitConfig(ctx, fexec, nil) + _, err := config.InitConfig(ctx, nil, nil) gomega.Expect(err).NotTo(gomega.HaveOccurred()) config.Kubernetes.HostNetworkNamespace = "" nodeAnnotator := kube.NewNodeAnnotator(&kube.Kube{kubeFakeClient, egressIPFakeClient, egressFirewallFakeClient}, testNode.Name) @@ -978,7 +973,7 @@ var _ = ginkgo.Describe("Gateway Init Operations", func() { libovsdbOvnNBClient, libovsdbOvnSBClient, libovsdbCleanup, err = libovsdbtest.NewNBSBTestHarness(dbSetup) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - expectedDatabaseState := []libovsdb.TestData{} + expectedDatabaseState := []libovsdbtest.TestData{} expectedDatabaseState = addNodeLogicalFlows(expectedDatabaseState, expectedOVNClusterRouter, expectedNodeSwitch, expectedClusterRouterPortGroup, expectedClusterPortGroup, &node1, clusterCIDR, config.IPv6Mode) clusterController := NewOvnController(fakeClient, f, stopChan, addressset.NewFakeAddressSetFactory(), @@ -1020,9 +1015,6 @@ var _ = ginkgo.Describe("Gateway Init Operations", func() { expectedDatabaseState = generateGatewayInitExpectedNB(expectedDatabaseState, expectedOVNClusterRouter, expectedNodeSwitch, node1.Name, clusterSubnets, []*net.IPNet{subnet}, l3Config, []*net.IPNet{joinLRPIPs}, []*net.IPNet{dLRPIPs}, skipSnat, node1.NodeMgmtPortIP) gomega.Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) - // Make sure remaining fexec calls are still correct - gomega.Expect(fexec.CalledMatchesExpected()).To(gomega.BeTrue(), fexec.ErrorDesc) - return nil } @@ -1086,11 +1078,7 @@ var _ = ginkgo.Describe("Gateway Init Operations", func() { EgressFirewallClient: egressFirewallFakeClient, } - fexec := ovntest.NewLooseCompareFakeExec() - err := util.SetExec(fexec) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - - _, err = config.InitConfig(ctx, fexec, nil) + _, err := config.InitConfig(ctx, nil, nil) gomega.Expect(err).NotTo(gomega.HaveOccurred()) config.Kubernetes.HostNetworkNamespace = "" nodeAnnotator := kube.NewNodeAnnotator(&kube.Kube{kubeFakeClient, egressIPFakeClient, egressFirewallFakeClient}, testNode.Name) @@ -1173,7 +1161,7 @@ var _ = ginkgo.Describe("Gateway Init Operations", func() { libovsdbOvnNBClient, libovsdbOvnSBClient, libovsdbCleanup, err = libovsdbtest.NewNBSBTestHarness(dbSetup) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - expectedDatabaseState := []libovsdb.TestData{} + expectedDatabaseState := []libovsdbtest.TestData{} expectedDatabaseState = addNodeLogicalFlows(expectedDatabaseState, expectedOVNClusterRouter, expectedNodeSwitch, expectedClusterRouterPortGroup, expectedClusterPortGroup, &node1, clusterCIDR, config.IPv6Mode) clusterController := NewOvnController(fakeClient, f, stopChan, addressset.NewFakeAddressSetFactory(), diff --git a/go-controller/pkg/ovn/ovn.go b/go-controller/pkg/ovn/ovn.go index d20068bbfb6..9ccd06ac2b8 100644 --- a/go-controller/pkg/ovn/ovn.go +++ b/go-controller/pkg/ovn/ovn.go @@ -742,21 +742,14 @@ func (oc *Controller) WatchEgressFirewall() *factory.Handler { return oc.watchFactory.AddEgressFirewallHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { egressFirewall := obj.(*egressfirewall.EgressFirewall).DeepCopy() - txn := util.NewNBTxn() addErrors := oc.addEgressFirewall(egressFirewall) if addErrors != nil { klog.Error(addErrors) egressFirewall.Status.Status = egressFirewallAddError - } else { - _, stderr, err := txn.Commit() - if err != nil { - klog.Errorf("Failed to commit db changes for egressFirewall in namespace %s stderr: %q, err: %+v", egressFirewall.Namespace, stderr, err) - egressFirewall.Status.Status = egressFirewallAddError - } else { - egressFirewall.Status.Status = egressFirewallAppliedCorrectly - } } + egressFirewall.Status.Status = egressFirewallAppliedCorrectly + err := oc.updateEgressFirewallWithRetry(egressFirewall) if err != nil { klog.Error(err) @@ -768,21 +761,14 @@ func (oc *Controller) WatchEgressFirewall() *factory.Handler { newEgressFirewall := newer.(*egressfirewall.EgressFirewall).DeepCopy() oldEgressFirewall := old.(*egressfirewall.EgressFirewall) if !reflect.DeepEqual(oldEgressFirewall.Spec, newEgressFirewall.Spec) { - txn := util.NewNBTxn() errList := oc.updateEgressFirewall(oldEgressFirewall, newEgressFirewall) if errList != nil { newEgressFirewall.Status.Status = egressFirewallUpdateError klog.Error(errList) - } else { - _, stderr, err := txn.Commit() - if err != nil { - klog.Errorf("Failed to commit db changes for egressFirewall in namespace %s stderr: %q, err: %+v", newEgressFirewall.Namespace, stderr, err) - newEgressFirewall.Status.Status = egressFirewallUpdateError - - } else { - newEgressFirewall.Status.Status = egressFirewallAppliedCorrectly - } } + + newEgressFirewall.Status.Status = egressFirewallAppliedCorrectly + err := oc.updateEgressFirewallWithRetry(newEgressFirewall) if err != nil { klog.Error(err) @@ -792,16 +778,12 @@ func (oc *Controller) WatchEgressFirewall() *factory.Handler { }, DeleteFunc: func(obj interface{}) { egressFirewall := obj.(*egressfirewall.EgressFirewall) - txn := util.NewNBTxn() deleteErrors := oc.deleteEgressFirewall(egressFirewall) if deleteErrors != nil { klog.Error(deleteErrors) return } - stdout, stderr, err := txn.Commit() - if err != nil { - klog.Errorf("Failed to commit db changes for egressFirewall in namespace %s stdout: %q, stderr: %q, err: %+v", egressFirewall.Namespace, stdout, stderr, err) - } + metrics.UpdateEgressFirewallRuleCount(float64(-len(egressFirewall.Spec.Egress))) metrics.DecrementEgressFirewallCount() },