-
Notifications
You must be signed in to change notification settings - Fork 77
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,8 +24,10 @@ import ( | |
"encoding/json" | ||
"errors" | ||
"fmt" | ||
"io/ioutil" | ||
"log" | ||
"net" | ||
"os" | ||
"runtime" | ||
"sort" | ||
"time" | ||
|
@@ -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" | ||
|
||
|
@@ -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 { | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is redundant There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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 | ||
} | ||
|
@@ -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 | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ASCII-betically"
:D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whereabouts rocks!