Skip to content

Commit

Permalink
port groups: idempotency, bulk updates
Browse files Browse the repository at this point in the history 
Updates mocks to reflect change in go-ovn, updates test cases.

Update helper functions to allow for bulk operations.

Signed-off-by: Casey Callendrello <cdc@redhat.com>
  • Loading branch information
@squeed
squeed committed Jun 7, 2021
1 parent cd9924c commit ced8a1f
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 27 deletions.
71 changes: 54 additions & 17 deletions go-controller/pkg/ovn/policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"net"
"reflect"
"strconv"
"strings"
"sync"

goovn "github.com/ebay/go-ovn"
Expand Down Expand Up @@ -222,33 +223,69 @@ func deleteACLPortGroup(portGroupName, direction, priority, match, action string
return nil
}

func addToPortGroup(ovnNBClient goovn.Client, portGroupName string, portInfo *lpInfo) error {
cmd, err := ovnNBClient.PortGroupAddPort(portGroupName, portInfo.uuid)
if err == nil {
if err = ovnNBClient.Execute(cmd); err != nil {
return fmt.Errorf("execute error adding port %s to port group %s (%v)", portInfo.name, portGroupName, err)
func addToPortGroup(ovnNBClient goovn.Client, portGroupName string, ports ...*lpInfo) error {
cmds := make([]*goovn.OvnCommand, 0, len(ports))
for _, portInfo := range ports {
cmd, err := ovnNBClient.PortGroupAddPort(portGroupName, portInfo.uuid)
if err != nil {
if err != goovn.ErrorExist {
return fmt.Errorf("error preparing adding port %v to port group %s (%v)", portInfo.name, portGroupName, err)
}
}
} else if err != goovn.ErrorExist {
// Ignore goovn.ErrorExist to implement port "--may-exist" behavior
// i.e., if the port is already there, it's okay.
return fmt.Errorf("error adding port %s to port group %s (%v)", portInfo.name, portGroupName, err)
cmds = append(cmds, cmd)
}

if err := ovnNBClient.Execute(cmds...); err != nil {
names := []string{}
for _, portInfo := range ports {
names = append(names, portInfo.name)
}
return fmt.Errorf("error committing adding ports (%v) to port group %s (%v)", strings.Join(names, ","), portGroupName, err)
}
return nil
}

func deleteFromPortGroup(ovnNBClient goovn.Client, portGroupName string, portInfo *lpInfo) error {
cmd, err := ovnNBClient.PortGroupRemovePort(portGroupName, portInfo.uuid)
if err == nil {
if err = ovnNBClient.Execute(cmd); err != nil {
return fmt.Errorf("execute error removing port %s from port group %s (%v)", portInfo.name, portGroupName, err)
func deleteFromPortGroup(ovnNBClient goovn.Client, portGroupName string, ports ...*lpInfo) error {
cmds := make([]*goovn.OvnCommand, 0, len(ports))
for _, portInfo := range ports {
cmd, err := ovnNBClient.PortGroupRemovePort(portGroupName, portInfo.uuid)
if err != nil {
// PortGroup already deleted...
if err == goovn.ErrorNotFound {
return nil
} else {
return fmt.Errorf("error preparing removing port %v from port group %s (%v)", portInfo.name, portGroupName, err)
}
}
cmds = append(cmds, cmd)
}

if err := ovnNBClient.Execute(cmds...); err != nil {
names := []string{}
for _, portInfo := range ports {
names = append(names, portInfo.name)
}
} else if err != goovn.ErrorNotFound {
// Ignore goovn.ErrorNotFound to implement "--if-exists" behavior
return fmt.Errorf("error removing port %s from port group %s (%v)", portInfo.name, portGroupName, err)
return fmt.Errorf("error committing removing ports (%v) from port group %s (%v)", strings.Join(names, ","), portGroupName, err)
}
return nil
}

// setPortGroup declaratively sets a port group to have a specific set of ports.
func setPortGroup(ovnNBClient goovn.Client, portGroupName string, ports ...*lpInfo) error {

uuids := make([]string, 0, len(ports))
for _, port := range ports {
uuids = append(uuids, port.uuid)
}

cmd, err := ovnNBClient.PortGroupUpdate(portGroupName, uuids, nil)
if err != nil {
return err
}

return ovnNBClient.Execute(cmd)
}

func defaultDenyPortGroup(namespace, gressSuffix string) string {
return hashedPortGroup(namespace) + "_" + gressSuffix
}
Expand Down
17 changes: 10 additions & 7 deletions go-controller/pkg/ovn/port_group_unit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,21 +225,24 @@ func TestAddToPortGroup(t *testing.T) {
},
},
{
desc: "port exists",
desc: "positive idempotency",
name: pgName,
portInfo: &lpInfo{name: lspName, uuid: lspUUID},
errMatch: nil,
onRetArgMockGoOvnNBClient: []ovntest.TestifyMockHelper{
{
OnCallMethodName: "PortGroupAddPort", OnCallMethodArgType: []string{"string", "string"}, RetArgList: []interface{}{&goovn.OvnCommand{}, goovn.ErrorExist},
OnCallMethodName: "PortGroupAddPort", OnCallMethodArgType: []string{"string", "string"}, RetArgList: []interface{}{&goovn.OvnCommand{}, nil},
},
{
OnCallMethodName: "Execute", OnCallMethodArgType: []string{"*goovn.OvnCommand"}, RetArgList: []interface{}{nil},
},
},
},
{
desc: "port group not found",
name: pgName,
portInfo: &lpInfo{name: lspName, uuid: lspUUID},
errMatch: fmt.Errorf("error adding port %s to port group %s (%v)", lspName, pgName, goovn.ErrorNotFound),
errMatch: fmt.Errorf("error preparing adding port %s to port group %s (%v)", lspName, pgName, goovn.ErrorNotFound),
onRetArgMockGoOvnNBClient: []ovntest.TestifyMockHelper{
{
OnCallMethodName: "PortGroupAddPort", OnCallMethodArgType: []string{"string", "string"}, RetArgList: []interface{}{&goovn.OvnCommand{}, goovn.ErrorNotFound},
Expand All @@ -250,7 +253,7 @@ func TestAddToPortGroup(t *testing.T) {
desc: "other PortGroupAddPort error",
name: pgName,
portInfo: &lpInfo{name: lspName, uuid: lspUUID},
errMatch: fmt.Errorf("error adding port %s to port group %s (%v)", lspName, pgName, otherError),
errMatch: fmt.Errorf("error preparing adding port %s to port group %s (%v)", lspName, pgName, otherError),
onRetArgMockGoOvnNBClient: []ovntest.TestifyMockHelper{
{
OnCallMethodName: "PortGroupAddPort", OnCallMethodArgType: []string{"string", "string"}, RetArgList: []interface{}{&goovn.OvnCommand{}, otherError},
Expand All @@ -261,7 +264,7 @@ func TestAddToPortGroup(t *testing.T) {
desc: "execute error",
name: pgName,
portInfo: &lpInfo{name: lspName, uuid: lspUUID},
errMatch: fmt.Errorf("execute error adding port %s to port group %s (%v)", lspName, pgName, execError),
errMatch: fmt.Errorf("error committing adding ports (%s) to port group %s (%v)", lspName, pgName, execError),
onRetArgMockGoOvnNBClient: []ovntest.TestifyMockHelper{
{
OnCallMethodName: "PortGroupAddPort", OnCallMethodArgType: []string{"string", "string"}, RetArgList: []interface{}{&goovn.OvnCommand{}, nil},
Expand Down Expand Up @@ -328,7 +331,7 @@ func TestDeleteFromPortGroup(t *testing.T) {
desc: "other PortGroupAddPort error",
name: pgName,
portInfo: &lpInfo{name: lspName, uuid: lspUUID},
errMatch: fmt.Errorf("error removing port %s from port group %s (%v)", lspName, pgName, otherError),
errMatch: fmt.Errorf("error preparing removing port %s from port group %s (%v)", lspName, pgName, otherError),
onRetArgMockGoOvnNBClient: []ovntest.TestifyMockHelper{
{
OnCallMethodName: "PortGroupRemovePort", OnCallMethodArgType: []string{"string", "string"}, RetArgList: []interface{}{&goovn.OvnCommand{}, otherError},
Expand All @@ -339,7 +342,7 @@ func TestDeleteFromPortGroup(t *testing.T) {
desc: "execute error",
name: pgName,
portInfo: &lpInfo{name: lspName, uuid: lspUUID},
errMatch: fmt.Errorf("execute error removing port %s from port group %s (%v)", lspName, pgName, execError),
errMatch: fmt.Errorf("error committing removing ports (%s) from port group %s (%v)", lspName, pgName, execError),
onRetArgMockGoOvnNBClient: []ovntest.TestifyMockHelper{
{
OnCallMethodName: "PortGroupRemovePort", OnCallMethodArgType: []string{"string", "string"}, RetArgList: []interface{}{&goovn.OvnCommand{}, nil},
Expand Down
33 changes: 30 additions & 3 deletions go-controller/pkg/testing/mock_portgroup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@ package testing
import (
"fmt"

"github.com/mitchellh/copystructure"

goovn "github.com/ebay/go-ovn"
"github.com/mitchellh/copystructure"
"k8s.io/klog/v2"
)

Expand Down Expand Up @@ -44,7 +43,29 @@ func (mock *MockOVNClient) PortGroupAdd(group string, ports []string, external_i

// Sets "ports" and/or "external_ids" on the port group named "group". It is an error if group does not exist.
func (mock *MockOVNClient) PortGroupUpdate(group string, ports []string, external_ids map[string]string) (*goovn.OvnCommand, error) {
return nil, fmt.Errorf("method %s is not implemented yet", functionName())
var pg *goovn.PortGroup
if pg, _ = mock.PortGroupGet(group); pg == nil {
return nil, goovn.ErrorNotFound
}

// We don't yet support updating external-ids
if external_ids != nil {
return nil, fmt.Errorf("method %s is not implemented yet", functionName())
}

return &goovn.OvnCommand{
Exe: &MockExecution{
handler: mock,
op: OpUpdate,
table: PortGroupType,
objName: group,
objUpdate: UpdateCache{
FieldType: PgLSPs,
FieldValue: ports,
UpdateOp: OpUpdate,
},
},
}, nil
}

// Add port to port group.
Expand Down Expand Up @@ -187,6 +208,12 @@ func (mock *MockOVNClient) updatePortGroupCache(pgName string, update UpdateCach
pg.Ports = append(pg.Ports, fmt.Sprintf("%v", update.FieldValue))
case OpDelete:
pg.Ports = removeElement(pg.Ports, fmt.Sprintf("%v", update.FieldValue))
case OpUpdate:
ports, ok := update.FieldValue.([]string)
if !ok {
panic("invalid port group update...")
}
pg.Ports = ports
default:
return fmt.Errorf("unrecognized update op: %s", update.UpdateOp)
}
Expand Down

0 comments on commit ced8a1f

Please sign in to comment.