From e8f71cb2181d95da38f96f8e24eadcd21d3f907d Mon Sep 17 00:00:00 2001 From: Hongliang Liu Date: Mon, 16 Jan 2023 17:11:34 +0800 Subject: [PATCH 1/2] Add support for REMOVE_BUCKET operation in group This PR also upgrades golangci-lint version to v1.50.0 and fix some fmt issues. Signed-off-by: Hongliang Liu --- .gitignore | 1 + .golangci.yml | 6 ++---- Makefile | 2 +- VERSION | 2 +- ofctrl/dial.go | 1 + ofctrl/fgraph.go | 3 ++- ofctrl/fgraphFlood.go | 3 ++- ofctrl/fgraphFlow.go | 11 ++++++----- ofctrl/fgraphGroup.go | 18 ++++++++++++++---- ofctrl/fgraphOutput.go | 3 ++- ofctrl/fgraphSwitch.go | 3 ++- ofctrl/fgraphTable.go | 3 ++- ofctrl/ofSwitch.go | 3 ++- ofctrl/ofctrl_test.go | 3 ++- ofctrl/ofpacket_test.go | 3 ++- ofctrl/ovsdbDriver.go | 3 ++- 16 files changed, 44 insertions(+), 24 deletions(-) diff --git a/.gitignore b/.gitignore index 4e3eeb07..69c2d305 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,4 @@ .idea/ .vscode/ +vendor diff --git a/.golangci.yml b/.golangci.yml index a7c69123..a6fda629 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,6 +1,7 @@ run: tests: true timeout: 5m + deadline: 5m skip-files: - ".*\\.pb\\.go" skip-dirs-use-default: true @@ -12,13 +13,10 @@ linters-settings: linters: disable-all: true enable: # see https://golangci-lint.run/usage/linters/ - - deadcode + - unused - staticcheck - govet - gofmt - goimports - gosec - misspell - -run: - deadline: 5m diff --git a/Makefile b/Makefile index 8a3b04dd..178c3cb4 100755 --- a/Makefile +++ b/Makefile @@ -20,7 +20,7 @@ test: docker-test-integration # code linting .golangci-bin: @echo "===> Installing Golangci-lint <===" - @curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $@ v1.41.1 + @curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $@ v1.50.0 .PHONY: golangci golangci: .golangci-bin diff --git a/VERSION b/VERSION index 41a02aff..ca46cd28 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -v0.6.7 +v0.6.8 diff --git a/ofctrl/dial.go b/ofctrl/dial.go index 479025b8..7f983788 100644 --- a/ofctrl/dial.go +++ b/ofctrl/dial.go @@ -1,3 +1,4 @@ +//go:build linux || darwin // +build linux darwin package ofctrl diff --git a/ofctrl/fgraph.go b/ofctrl/fgraph.go index c2aff398..290fdfe9 100644 --- a/ofctrl/fgraph.go +++ b/ofctrl/fgraph.go @@ -1,4 +1,5 @@ -/*** +/* +** Copyright 2014 Cisco Systems Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/ofctrl/fgraphFlood.go b/ofctrl/fgraphFlood.go index 4249a76e..6e9fe737 100644 --- a/ofctrl/fgraphFlood.go +++ b/ofctrl/fgraphFlood.go @@ -1,4 +1,5 @@ -/*** +/* +** Copyright 2014 Cisco Systems Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/ofctrl/fgraphFlow.go b/ofctrl/fgraphFlow.go index eaf86a5f..5a83796e 100644 --- a/ofctrl/fgraphFlow.go +++ b/ofctrl/fgraphFlow.go @@ -1,4 +1,5 @@ -/*** +/* +** Copyright 2014 Cisco Systems Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); @@ -1906,10 +1907,10 @@ func (self *Flow) ConnTrack(commit bool, force bool, tableID *uint8, zoneID *uin // Special Actions to to the flow to set conjunctions // Note: -// 1) nclause should be in [2, 64]. -// 2) clause value should be less than or equals to ncluase, and its value should be started from 1. -// actual clause in libopenflow messages is started from 0, here would decrement 1 to keep the display -// value is consistent with expected configuration +// 1. nclause should be in [2, 64]. +// 2. clause value should be less than or equals to ncluase, and its value should be started from 1. +// actual clause in libopenflow messages is started from 0, here would decrement 1 to keep the display +// value is consistent with expected configuration func (self *Flow) AddConjunction(conjID uint32, clause uint8, nClause uint8) error { conjunction, err := NewNXConjunctionAction(conjID, clause, nClause) if err != nil { diff --git a/ofctrl/fgraphGroup.go b/ofctrl/fgraphGroup.go index 4714e1b3..665dadab 100644 --- a/ofctrl/fgraphGroup.go +++ b/ofctrl/fgraphGroup.go @@ -109,13 +109,23 @@ func (self *Group) getGroupModMessage(command int) *openflow15.GroupMod { groupMod.Type = openflow15.GT_FF } - for _, bkt := range self.Buckets { - // Add the bucket to group - groupMod.AddBucket(*bkt) + // In a request of type OFPGC_REMOVE_BUCKET, bucket list should be empty. + if command != openflow15.OFPGC_REMOVE_BUCKET { + for _, bkt := range self.Buckets { + // Add the bucket to group + groupMod.AddBucket(*bkt) + } } - if command == openflow15.OFPGC_INSERT_BUCKET { + // In a request of type OFPGC_INSERT_BUCKET, the command_bucket_id field is used to specify the position in the + // current installed bucket list to insert new buckets. Here place the buckets to the end of the current installed + // bucket list. groupMod.CommandBucketId = openflow15.OFPG_BUCKET_LAST + } else if command == openflow15.OFPGC_REMOVE_BUCKET { + // In a request of type OFPGC_REMOVE_BUCKET, the CommandBucketId field is used to specify the identifier of the + // bucket to remove from current installed bucket list. + groupMod.CommandBucketId = self.Buckets[0].BucketId + groupMod.Buckets = nil } groupMod.Command = uint16(command) return groupMod diff --git a/ofctrl/fgraphOutput.go b/ofctrl/fgraphOutput.go index be8d67c5..fe144df0 100644 --- a/ofctrl/fgraphOutput.go +++ b/ofctrl/fgraphOutput.go @@ -1,4 +1,5 @@ -/*** +/* +** Copyright 2014 Cisco Systems Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/ofctrl/fgraphSwitch.go b/ofctrl/fgraphSwitch.go index 5626db4f..5284d451 100644 --- a/ofctrl/fgraphSwitch.go +++ b/ofctrl/fgraphSwitch.go @@ -1,4 +1,5 @@ -/*** +/* +** Copyright 2014 Cisco Systems Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/ofctrl/fgraphTable.go b/ofctrl/fgraphTable.go index 3fef4072..a553d873 100755 --- a/ofctrl/fgraphTable.go +++ b/ofctrl/fgraphTable.go @@ -1,4 +1,5 @@ -/*** +/* +** Copyright 2014 Cisco Systems Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/ofctrl/ofSwitch.go b/ofctrl/ofSwitch.go index 51394020..c2dcb6dd 100755 --- a/ofctrl/ofSwitch.go +++ b/ofctrl/ofSwitch.go @@ -1,4 +1,5 @@ -/*** +/* +** Copyright 2014 Cisco Systems Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/ofctrl/ofctrl_test.go b/ofctrl/ofctrl_test.go index a5b9fa89..a9ced42d 100644 --- a/ofctrl/ofctrl_test.go +++ b/ofctrl/ofctrl_test.go @@ -1,4 +1,5 @@ -/*** +/* +** Copyright 2014 Cisco Systems Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/ofctrl/ofpacket_test.go b/ofctrl/ofpacket_test.go index 19e21e37..a01b0898 100644 --- a/ofctrl/ofpacket_test.go +++ b/ofctrl/ofpacket_test.go @@ -277,7 +277,8 @@ func generateTCPPacketOut(srcMAC, dstMAC net.HardwareAddr, srcIP net.IP, dstIP n } // keeping this in case it is useful later -//nolint:deadcode +// +//nolint:unused func generatePacketOut(srcMAC net.HardwareAddr, dstMAC net.HardwareAddr, srcIP net.IP, dstIP net.IP, outputPort *uint32, actions []OFAction) *PacketOut { var outPort uint32 if outputPort == nil { diff --git a/ofctrl/ovsdbDriver.go b/ofctrl/ovsdbDriver.go index 592200a5..e4a3d96f 100644 --- a/ofctrl/ovsdbDriver.go +++ b/ofctrl/ovsdbDriver.go @@ -479,7 +479,8 @@ func (self *OvsDriver) RemoveController(target string) error { // Check the local cache and see if the portname is taken already // HACK alert: This is used to pick next port number instead of managing -// port number space actively across agent restarts +// +// port number space actively across agent restarts func (self *OvsDriver) IsPortNamePresent(intfName string) bool { // lock the cache for read self.lock.RLock() From b005d07db30f5e7a661de604f2c7f3e2d15283bc Mon Sep 17 00:00:00 2001 From: Hongliang Liu Date: Thu, 2 Mar 2023 14:25:18 +0800 Subject: [PATCH 2/2] Update methods NewGroup, DeleteGroup and GetGroup of OFSwitch Add parameter `NewGroup` to decide whether to use cache in OFSwitch when create a group. Signed-off-by: Hongliang Liu --- VERSION | 2 +- ofctrl/fgraphSwitch.go | 37 ++++++++++++++++++++++++------------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/VERSION b/VERSION index ca46cd28..f992d273 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -v0.6.8 +v0.6.9 diff --git a/ofctrl/fgraphSwitch.go b/ofctrl/fgraphSwitch.go index 5284d451..a7566402 100644 --- a/ofctrl/fgraphSwitch.go +++ b/ofctrl/fgraphSwitch.go @@ -19,6 +19,7 @@ package ofctrl import ( "errors" + "fmt" "antrea.io/libOpenflow/openflow15" ) @@ -97,31 +98,41 @@ func (self *OFSwitch) DefaultTable() *Table { return self.tableDb[0] } -// Create a new group. return an error if it already exists -func (self *OFSwitch) NewGroup(groupId uint32, groupType GroupType) (*Group, error) { - // check if the group already exists - if self.groupDb[groupId] != nil { - return nil, errors.New("group already exists") +// NewGroup creates a new group; when using cache, returns an error if the group ID has already been cached, otherwise +// returns the group object; when not using cache, returns the new group object. +func (self *OFSwitch) NewGroup(groupId uint32, groupType GroupType, useCache bool) (*Group, error) { + // Check if the group already exists. + if useCache { + if self.groupDb[groupId] != nil { + return nil, errors.New("group already exists") + } } - // Create a new group + // Create a new group. group := newGroup(groupId, groupType, self) - // Save it in the DB - self.groupDb[groupId] = group + if useCache { + // Save it in cache. + self.groupDb[groupId] = group + } return group, nil } -// Delete a group. -// Return an error if there are flows refer pointing at it +// DeleteGroup deletes a group in cache. func (self *OFSwitch) DeleteGroup(groupId uint32) error { + if _, exists := self.groupDb[groupId]; !exists { + return fmt.Errorf("group %d does not exist in cache", groupId) + } delete(self.groupDb, groupId) return nil } -// GetGroup Returns a group -func (self *OFSwitch) GetGroup(groupId uint32) *Group { - return self.groupDb[groupId] +// GetGroup returns a group if it is cached. +func (self *OFSwitch) GetGroup(groupId uint32) (*Group, error) { + if _, exists := self.groupDb[groupId]; !exists { + return nil, fmt.Errorf("group %d does not exist in cache", groupId) + } + return self.groupDb[groupId], nil } // Create a new meter. return an error if it already exists