make ovs socket file path as configurable property #142
Conversation
Signed-off-by: Periyasamy Palanisamy <[email protected]>
kubevirt-bot
added
dco-signoff: yes
|
/release-note-none |
kubevirt-bot
added
release-note-none
|
|
||
| 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 |
There was a problem hiding this comment.
Oh, that was not a request for change, I just liked the way you name it :)
There was a problem hiding this comment.
ok ;) actually this doc and implementation approach inspired from whereabouts cni!
pkg/plugin/plugin.go
Outdated
| if os.IsNotExist(err) { | ||
| return false | ||
| } | ||
| return true |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, Correct! done.
pkg/plugin/plugin.go
Outdated
| @@ -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 | |||
| 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.
Could we make this a constant on the top of the file? For better visibility.
There was a problem hiding this comment.
this is a slice of string type, can't make it as a constant. just moved it into a method.
pkg/plugin/plugin.go
Outdated
| @@ -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 | |||
pkg/plugin/plugin.go
Outdated
| break | ||
| } | ||
| } | ||
| // merge both netconf and flatnetConf configurations |
pkg/plugin/plugin.go
Outdated
| @@ -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.
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.
There was a problem hiding this comment.
Good Point! done it now.
Signed-off-by: Periyasamy Palanisamy <[email protected]>
|
/cc @JanScheurich |
|
@pperiyasamy: GitHub didn't allow me to request PR reviews from the following users: JanScheurich. Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
pkg/plugin/plugin.go
Outdated
| @@ -112,6 +117,61 @@ func loadNetConf(bytes []byte) (*netConf, error) { | |||
| return netconf, nil | |||
| } | |||
|
|
|||
| func loadFlatNetConf(configPath string) (*netConf, error) { | |||
| confDirs := getOvsConfDir() | |||
There was a problem hiding this comment.
These are not directories but files.
There was a problem hiding this comment.
yes, that makes sense. changed it.
Signed-off-by: Periyasamy Palanisamy <[email protected]>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: phoracek, pperiyasamy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
The ovs-cni currently hardcodes the ovs unix socket file path to
/var/run/openvswitch/db.sock.But this would change across deployments. Hence introducing a global flat file configuration so that
socket_filepath can be specified and this is applied for all network attachment definition objects.Signed-off-by: Periyasamy Palanisamy [email protected]