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

make ovs socket file path as configurable property #142

Merged
merged 3 commits into from
Nov 27, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
29 changes: 29 additions & 0 deletions docs/cni-plugin.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,35 @@ Another example with a trunk port and jumbo frames:
* `mtu` (integer, optional): MTU.
* `trunk` (optional): List of VLAN ID's and/or ranges of accepted VLAN
ID's.
* `configuration_path` (optional): configuration file containing ovsdb
socket file path, etc.

### Flatfile Configuation

There is one option for flat file configuration:

* `configuration_path`: A file path to a OVS CNI configuration file.

OVS CNI will look for the configuration in these locations, in this order:

* The location specified by the `configuration_path` option.
* `/etc/kubernetes/cni/net.d/ovs.d/ovs.conf`
* `/etc/cni/net.d/ovs.d/ovs.conf`

You may specify the `configuration_path` to point to another location should it be desired.

Any options added to the `ovs.conf` are overridden by configuration options that are in the
CNI configuration (e.g. in a custom resource `NetworkAttachmentDefinition` used by Multus CNI
or in the first file ASCII-betically in the CNI configuration directory -- which is
Copy link
Member

Choose a reason for hiding this comment

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

"ASCII-betically"

:D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that was not a request for change, I just liked the way you name it :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok ;) actually this doc and implementation approach inspired from whereabouts cni!

Copy link
Member

Choose a reason for hiding this comment

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

Whereabouts rocks!

`/etc/cni/net.d/` by default).

The sample content of ovs.conf (in JSON format) is as follows:

```json
{
"socket_file": "/usr/local/var/run/openvswitch/db.sock"
}
```

## Manual Testing

Expand Down
10 changes: 7 additions & 3 deletions pkg/ovsdb/ovsdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,16 @@ func NewOvsDriver(ovsSocket string) (*OvsDriver, error) {
}

// Create a new OVS driver for a bridge with Unix socket
func NewOvsBridgeDriver(bridgeName string) (*OvsBridgeDriver, error) {
func NewOvsBridgeDriver(bridgeName, socketFile string) (*OvsBridgeDriver, error) {
ovsDriver := new(OvsBridgeDriver)

ovsDB, err := libovsdb.ConnectWithUnixSocket("/var/run/openvswitch/db.sock")
if socketFile == "" {
socketFile = "/var/run/openvswitch/db.sock"
}

ovsDB, err := libovsdb.ConnectWithUnixSocket(socketFile)
if err != nil {
return nil, fmt.Errorf("failed to connect to ovsdb error: %v", err)
return nil, fmt.Errorf("failed to connect to ovsdb socket %s: error: %v", socketFile, err)
}

// Setup state
Expand Down
60 changes: 53 additions & 7 deletions pkg/plugin/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ import (
"encoding/json"
"errors"
"fmt"
"io/ioutil"
"log"
"net"
"os"
"runtime"
"sort"
"time"
Expand All @@ -36,6 +38,7 @@ import (
"github.com/containernetworking/plugins/pkg/ip"
"github.com/containernetworking/plugins/pkg/ipam"
"github.com/containernetworking/plugins/pkg/ns"
"github.com/imdario/mergo"
"github.com/j-keck/arping"
"github.com/vishvananda/netlink"

Expand All @@ -51,11 +54,13 @@ const (

type netConf struct {
types.NetConf
BrName string `json:"bridge,omitempty"`
VlanTag *uint `json:"vlan"`
MTU int `json:"mtu"`
Trunk []*trunk `json:"trunk,omitempty"`
DeviceID string `json:"deviceID"` // PCI address of a VF in valid sysfs format
BrName string `json:"bridge,omitempty"`
VlanTag *uint `json:"vlan"`
MTU int `json:"mtu"`
Trunk []*trunk `json:"trunk,omitempty"`
DeviceID string `json:"deviceID"` // PCI address of a VF in valid sysfs format
ConfigurationPath string `json:"configuration_path"`
SocketFile string `json:"socket_file"`
}

type trunk struct {
Expand Down Expand Up @@ -109,9 +114,50 @@ func loadNetConf(bytes []byte) (*netConf, error) {
return nil, fmt.Errorf("failed to load netconf: %v", err)
}

// default config file dir paths
Copy link
Member

Choose a reason for hiding this comment

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

I think this is redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

Copy link
Member

Choose a reason for hiding this comment

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

Can we move this whole new block loading the file to a separate function? And then have one top level function which would merge loadNetConf and loadFlatNetConf? Would be easier to follow IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good Point! done it now.

confdirs := []string{"/etc/kubernetes/cni/net.d/ovs.d/ovs.conf", "/etc/cni/net.d/ovs.d/ovs.conf"}
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this a constant on the top of the file? For better visibility.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a slice of string type, can't make it as a constant. just moved it into a method.

// if netconf contains config path already, use that as first
if netconf.ConfigurationPath != "" {
confdirs = append([]string{netconf.ConfigurationPath}, confdirs...)
}
// loop through the path and parse the JSON config
flatnetConf := &netConf{}
for _, confpath := range confdirs {
if pathExists(confpath) {
jsonFile, err := os.Open(confpath)
if err != nil {
return nil, fmt.Errorf("open ovs config file %s error: %v", confpath, err)
}
defer jsonFile.Close()
jsonBytes, err := ioutil.ReadAll(jsonFile)
if err != nil {
return nil, fmt.Errorf("load ovs config file %s: error: %v", confpath, err)
}
if err := json.Unmarshal(jsonBytes, flatnetConf); err != nil {
return nil, fmt.Errorf("parse ovs config file %s: error: %v", confpath, err)
}
break
}
}
// merge both netconf and flatnetConf configurations
Copy link
Member

Choose a reason for hiding this comment

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

Redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

if err := mergo.Merge(netconf, flatnetConf); err != nil {
return nil, fmt.Errorf("merge with ovs config file: error: %v", err)
}

return netconf, nil
}

func pathExists(path string) bool {
_, err := os.Stat(path)
if err == nil {
return true
}
if os.IsNotExist(err) {
return false
}
return true
Copy link
Member

Choose a reason for hiding this comment

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

This happens if the path maybe exists, however, we are not able to stat it. Does it make sense to continue with the execution of CNI in this case? I would vote to either return the error or just panic right away.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, Correct! done.

}

func generateRandomMac() net.HardwareAddr {
prefix := []byte{0x02, 0x00, 0x00} // local unicast prefix
suffix := make([]byte, 3)
Expand Down Expand Up @@ -306,7 +352,7 @@ func CmdAdd(args *skel.CmdArgs) error {
return err
}

ovsDriver, err := ovsdb.NewOvsBridgeDriver(bridgeName)
ovsDriver, err := ovsdb.NewOvsBridgeDriver(bridgeName, netconf.SocketFile)
if err != nil {
return err
}
Expand Down Expand Up @@ -480,7 +526,7 @@ func CmdDel(args *skel.CmdArgs) error {
return err
}

ovsDriver, err := ovsdb.NewOvsBridgeDriver(bridgeName)
ovsDriver, err := ovsdb.NewOvsBridgeDriver(bridgeName, netconf.SocketFile)
if err != nil {
return err
}
Expand Down