Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Dec 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions go-controller/hybrid-overlay/pkg/controller/master.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ import (
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/informer"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/kube"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/libovsdbops"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/libovsdbops"

"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/subnetallocator"
ovntypes "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
Expand All @@ -32,6 +33,7 @@ type MasterController struct {
nodeEventHandler informer.EventHandler
modelClient libovsdbops.ModelClient
nbClient client.Client
sbClient client.Client
}

// NewMaster a new master controller that listens for node events
Expand Down Expand Up @@ -227,13 +229,11 @@ func (m *MasterController) handleOverlayPort(node *kapi.Node, annotator kube.Ann
if !lspOK {
klog.Infof("Creating / updating node %s hybrid overlay port with mac %s", node.Name, portMAC.String())

var stderr string
// create / update lsps
_, stderr, err = util.RunOVNNbctl("--", "--may-exist", "lsp-add", node.Name, portName,
"--", "lsp-set-addresses", portName, portMAC.String())
err := libovsdbops.CreateLSPOrMutateMac(m.nbClient, node.Name, portName, portMAC.String())
if err != nil {
return fmt.Errorf("failed to add hybrid overlay port for node %s"+
", stderr:%s: %v", node.Name, stderr, err)
", err: %v", node.Name, err)
}
for _, subnet := range subnets {
if err := util.UpdateNodeSwitchExcludeIPs(m.nbClient, node.Name, subnet); err != nil {
Expand All @@ -255,7 +255,9 @@ func (m *MasterController) handleOverlayPort(node *kapi.Node, annotator kube.Ann
func (m *MasterController) deleteOverlayPort(node *kapi.Node) {
klog.Infof("Removing node %s hybrid overlay port", node.Name)
portName := util.GetHybridOverlayPortName(node.Name)
_, _, _ = util.RunOVNNbctl("--", "--if-exists", "lsp-del", portName)
if err := libovsdbops.LSPDelete(m.nbClient, portName); err != nil {
klog.Errorf("Failed deleting hybrind overlay port for node %s err: %v", node.Name, err)
}
}

// AddNode handles node additions
Expand Down Expand Up @@ -374,7 +376,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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return fmt.Errorf("failed to create MAC Binding for hybrid overlay: %v", err)
}
}
Expand Down
89 changes: 48 additions & 41 deletions go-controller/hybrid-overlay/pkg/controller/master_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ import (
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/informer"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/kube"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/libovsdbops"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb"

ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing"
libovsdbtest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
Expand Down Expand Up @@ -193,17 +195,11 @@ var _ = Describe("Hybrid SDN Master Operations", func() {
Name: types.OVNClusterRouter,
Policies: []string{"reroute-policy-UUID"},
},
&nbdb.LogicalSwitchPort{
Name: types.HybridOverlayPrefix + nodeName,
UUID: types.HybridOverlayPrefix + nodeName + "-UUID",
},
}

// Pre-add the HO port until the ovn-nbctl lsp-add commands are converted to libovsdb
nodeSwitch := &nbdb.LogicalSwitch{
Name: nodeName,
UUID: nodeName + "-UUID",
Ports: []string{types.HybridOverlayPrefix + nodeName + "-UUID"},
Name: nodeName,
UUID: nodeName + "-UUID",
}

initialExpectedDB := append(expectedDatabaseState, nodeSwitch)
Expand All @@ -226,20 +222,14 @@ var _ = Describe("Hybrid SDN Master Operations", func() {
)
Expect(err).NotTo(HaveOccurred())

// #1 node add
fexec.AddFakeCmdsNoOutputNoError([]string{
// Setting the mac on the lsp
"ovn-nbctl --timeout=15 -- " +
"--may-exist lsp-add node1 int-node1 -- " +
"lsp-set-addresses int-node1 " + nodeHOMAC,
})
// #2 comes because we set the ho dr gw mac annotation in #1
fexec.AddFakeCmdsNoOutputNoError([]string{
// Setting the mac on the lsp
"ovn-nbctl --timeout=15 -- " +
"--may-exist lsp-add node1 int-node1 -- " +
"lsp-set-addresses int-node1 " + nodeHOMAC,
})
// Make the expected LSP is created and added to the node
expectedLSP := &nbdb.LogicalSwitchPort{
UUID: libovsdbops.BuildNamedUUID(),
Name: types.HybridOverlayPrefix + nodeName,
Addresses: []string{nodeHOMAC},
}

nodeSwitch.Ports = []string{expectedLSP.UUID}

f.Start(stopChan)
wg.Add(1)
Expand All @@ -259,29 +249,22 @@ var _ = Describe("Hybrid SDN Master Operations", func() {

nodeSwitch.OtherConfig = map[string]string{"exclude_ips": "10.1.2.2"}

expectedDatabaseState = append(expectedDatabaseState, nodeSwitch)
expectedDatabaseState = append(expectedDatabaseState, nodeSwitch, expectedLSP)

Eventually(fexec.CalledMatchesExpected, 2).Should(BeTrue(), fexec.ErrorDesc)
Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveDataIgnoringUUIDs(expectedDatabaseState))

// Test that deleting the node cleans up the OVN objects
fexec.AddFakeCmdsNoOutputNoError([]string{
"ovn-nbctl --timeout=15 -- --if-exists lsp-del int-node1",
})

err = fakeClient.CoreV1().Nodes().Delete(context.TODO(), nodeName, *metav1.NewDeleteOptions(0))
Expect(err).NotTo(HaveOccurred())

Eventually(fexec.CalledMatchesExpected, 2).Should(BeTrue(), fexec.ErrorDesc)

// In a real db, deleting the LSP would remove the reference here, but in our testing ovsdb server it does not
// nodeSwitch.Ports = []string{}
expectedDatabaseState = []libovsdbtest.TestData{
&nbdb.LogicalRouter{
Name: types.OVNClusterRouter,
},
// This will be deleted once the nbctl commands for lsps are converted
&nbdb.LogicalSwitchPort{
Name: types.HybridOverlayPrefix + nodeName,
UUID: types.HybridOverlayPrefix + nodeName + "-uuid",
},
nodeSwitch,
}
Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveDataIgnoringUUIDs(expectedDatabaseState))
Expand Down Expand Up @@ -405,7 +388,8 @@ var _ = Describe("Hybrid SDN Master Operations", func() {
f := informers.NewSharedInformerFactory(fakeClient, informer.DefaultResyncInterval)

dynAdd := nodeHOMAC + " " + nodeHOIP
expectedDatabaseState := []libovsdbtest.TestData{
lspUUID := libovsdbops.BuildNamedUUID()
initialDatabaseState := []libovsdbtest.TestData{
&nbdb.LogicalRouterPolicy{
Priority: 1002,
ExternalIDs: map[string]string{
Expand All @@ -421,17 +405,19 @@ var _ = Describe("Hybrid SDN Master Operations", func() {
Policies: []string{"reroute-policy-UUID"},
},
&nbdb.LogicalSwitchPort{
UUID: lspUUID,
Name: "int-" + nodeName,
Addresses: []string{nodeHOMAC, nodeHOIP},
DynamicAddresses: &dynAdd,
},
&nbdb.LogicalSwitch{
Name: nodeName,
UUID: nodeName + "-UUID",
Name: nodeName,
UUID: nodeName + "-UUID",
Ports: []string{lspUUID},
},
}
dbSetup := libovsdbtest.TestSetup{
NBData: expectedDatabaseState,
NBData: initialDatabaseState,
}
var libovsdbOvnNBClient libovsdbclient.Client
libovsdbOvnNBClient, libovsdbCleanup, err = libovsdbtest.NewNBTestHarness(dbSetup, nil)
Expand All @@ -456,11 +442,8 @@ var _ = Describe("Hybrid SDN Master Operations", func() {
f.WaitForCacheSync(stopChan)

Eventually(fexec.CalledMatchesExpected, 2).Should(BeTrue(), fexec.ErrorDesc)
Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveDataIgnoringUUIDs(expectedDatabaseState))
Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveDataIgnoringUUIDs(initialDatabaseState))

fexec.AddFakeCmdsNoOutputNoError([]string{
"ovn-nbctl --timeout=15 -- --if-exists lsp-del int-node1",
})
k := &kube.Kube{KClient: fakeClient}
updatedNode, err := k.GetNode(nodeName)
Expect(err).NotTo(HaveOccurred())
Expand All @@ -478,6 +461,30 @@ var _ = Describe("Hybrid SDN Master Operations", func() {
return updatedNode.Annotations, nil
}, 5).ShouldNot(HaveKey(hotypes.HybridOverlayDRMAC))

expectedDatabaseState := []libovsdbtest.TestData{
&nbdb.LogicalRouterPolicy{
Priority: 1002,
ExternalIDs: map[string]string{
"name": "hybrid-subnet-node1",
},
Action: nbdb.LogicalRouterPolicyActionReroute,
Nexthops: []string{nodeHOIP},
Match: "inport == \"rtos-node1\" && ip4.dst == 11.1.0.0/16",
UUID: "reroute-policy-UUID",
},
&nbdb.LogicalRouter{
Name: types.OVNClusterRouter,
Policies: []string{"reroute-policy-UUID"},
},
&nbdb.LogicalSwitch{
Name: nodeName,
UUID: nodeName + "-UUID",
// CI server doesn't clean this up even though the LSP has been deleted, will
// be garbage collected in real scenario
Ports: []string{lspUUID},
},
}

Eventually(fexec.CalledMatchesExpected, 2).Should(BeTrue(), fexec.ErrorDesc)
Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveDataIgnoringUUIDs(expectedDatabaseState))
return nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
44 changes: 44 additions & 0 deletions go-controller/pkg/libovsdbops/datapath.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package libovsdbops

import (
"context"
"fmt"

libovsdbclient "github.com/ovn-org/libovsdb/client"

"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/sbdb"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
)

func findDatapathByPredicate(sbClient libovsdbclient.Client, lookupFcn func(dp *sbdb.DatapathBinding) bool) (*sbdb.DatapathBinding, error) {
ctx, cancel := context.WithTimeout(context.Background(), types.OVSDBTimeout)
defer cancel()
datapathBindings := []sbdb.DatapathBinding{}
err := sbClient.WhereCache(lookupFcn).List(ctx, &datapathBindings)
if err != nil {
return nil, err
}

if len(datapathBindings) != 1 {
return nil, fmt.Errorf("multiple datapath bindings found for a given lookup function")
}

return &datapathBindings[0], nil
}

func FindDatapathByExternalIDs(sbClient libovsdbclient.Client, externalIDs map[string]string) (*sbdb.DatapathBinding, error) {
datapathLookupFcn := func(dp *sbdb.DatapathBinding) bool {
datapathMatch := false
for k, v := range externalIDs {
if itemVal, ok := dp.ExternalIDs[k]; ok {
if itemVal == v {
datapathMatch = true
}
}
}

return datapathMatch
}

return findDatapathByPredicate(sbClient, datapathLookupFcn)
}
73 changes: 73 additions & 0 deletions go-controller/pkg/libovsdbops/logical_switch_port.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package libovsdbops

import (
"fmt"

libovsdbclient "github.com/ovn-org/libovsdb/client"

"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb"
)

// CreateLSPOrMutateMac creates a given LSP or mutates its MAC address and adds it to a logical Switch
func CreateLSPOrMutateMac(nbClient libovsdbclient.Client, nodeName, portName, portMAC string) error {
existingNodeSwitch := &nbdb.LogicalSwitch{
Name: nodeName,
}

existingLSP := []nbdb.LogicalSwitchPort{}

newLSP := &nbdb.LogicalSwitchPort{
Name: portName,
Addresses: []string{portMAC},
}

opModels := []OperationModel{
// For LSP's Name is a valid index, so no predicate is needed
{
Model: newLSP,
ExistingResult: &existingLSP,
OnModelMutations: []interface{}{
newLSP.Addresses,
},
DoAfter: func() {
// Bulkop is false, so we won't get here if len existingLSP > 1
if len(existingLSP) == 1 {
existingNodeSwitch.Ports = []string{existingLSP[0].UUID}
} else {
existingNodeSwitch.Ports = []string{newLSP.UUID}
}
},
},
{
Model: existingNodeSwitch,
ModelPredicate: func(ls *nbdb.LogicalSwitch) bool { return ls.Name == nodeName },
OnModelMutations: []interface{}{
&existingNodeSwitch.Ports,
},
ErrNotFound: true,
},
}

m := NewModelClient(nbClient)
if _, err := m.CreateOrUpdate(opModels...); err != nil {
return fmt.Errorf("failed to create or Mutate LSP %v and add it to Node %s , error: %v", newLSP, nodeName, err)
}

return nil
}

// LSPDelete performs an idepotent delete of the LSP provided by it's name
func LSPDelete(nbClient libovsdbclient.Client, LSPName string) error {
opModel := OperationModel{
Model: &nbdb.LogicalSwitchPort{
Name: LSPName,
},
}

m := NewModelClient(nbClient)
if err := m.Delete(opModel); err != nil {
return fmt.Errorf("failed to delete LSP %s, error: %v", LSPName, err)
}

return nil
}
29 changes: 29 additions & 0 deletions go-controller/pkg/libovsdbops/mac_binding.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package libovsdbops

import (
libovsdbclient "github.com/ovn-org/libovsdb/client"

"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/sbdb"
)

// Create or UpdateMacBinding either create a new mac binding or ensure the existing one has the
// correct datapath, logical_port, ip, and mac column entries
func CreateOrUpdateMacBinding(sbClient libovsdbclient.Client, mb *sbdb.MACBinding) error {
opModel := OperationModel{
// no predicate needed, ip and logical_port columns are indexes
Model: mb,
OnModelUpdates: []interface{}{
&mb.Datapath,
&mb.LogicalPort,
&mb.IP,
&mb.MAC,
},
}

m := NewModelClient(sbClient)
if _, err := m.CreateOrUpdate(opModel); err != nil {
return err
}

return nil
}
Loading