make ovs socket file path as configurable property #142

pperiyasamy · 2020-11-18T13:53:54Z

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_file path can be specified and this is applied for all network attachment definition objects.

Signed-off-by: Periyasamy Palanisamy periyasamy.palanisamy@est.tech

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>

pperiyasamy · 2020-11-18T13:54:54Z

/release-note-none

phoracek

I like what you did there. I just have a couple of comments with concerns for readability of the new function. Overall it looks good. Thanks.

phoracek · 2020-11-25T16:03:50Z

docs/cni-plugin.md

+
+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


"ASCII-betically"

:D

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

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

Whereabouts rocks!

phoracek · 2020-11-25T16:06:29Z

pkg/plugin/plugin.go

+	if os.IsNotExist(err) {
+		return false
+	}
+	return true


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.

Yes, Correct! done.

phoracek · 2020-11-25T16:08:20Z

pkg/plugin/plugin.go

@@ -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"}


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

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

phoracek · 2020-11-25T16:08:31Z

pkg/plugin/plugin.go

@@ -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


I think this is redundant

phoracek · 2020-11-25T16:08:41Z

pkg/plugin/plugin.go

+			break
+		}
+	}
+	// merge both netconf and flatnetConf configurations


phoracek · 2020-11-25T16:11:24Z

pkg/plugin/plugin.go

@@ -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


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.

Good Point! done it now.

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>

pperiyasamy · 2020-11-26T09:41:48Z

/cc @JanScheurich

kubevirt-bot · 2020-11-26T09:41:50Z

@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:

/cc @JanScheurich

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.

phoracek

Just a nit. Other than that, I like the PR. I will give chance until the end of the week for Jan to comment.

phoracek · 2020-11-26T14:12:58Z

pkg/plugin/plugin.go

@@ -112,6 +117,61 @@ func loadNetConf(bytes []byte) (*netConf, error) {
 	return netconf, nil
 }

+func loadFlatNetConf(configPath string) (*netConf, error) {
+	confDirs := getOvsConfDir()


These are not directories but files.

yes, that makes sense. changed it.

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>

phoracek · 2020-11-27T15:50:59Z

/lgtm
/approve

kubevirt-bot · 2020-11-27T15:51:08Z

[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:

~~OWNERS~~ [phoracek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

make ovs socket file path as configurable property

0f7c0d2

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 18, 2020

kubevirt-bot requested review from phoracek and rhrazdil November 18, 2020 13:54

kubevirt-bot added the size/M label Nov 18, 2020

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 18, 2020

phoracek requested changes Nov 25, 2020

View reviewed changes

address review comments

ce489a2

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>

kubevirt-bot added size/L and removed size/M labels Nov 26, 2020

phoracek requested changes Nov 26, 2020

View reviewed changes

address review comments #2

72a197c

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>

kubevirt-bot assigned phoracek Nov 27, 2020

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2020

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2020

kubevirt-bot merged commit 8bf4250 into k8snetworkplumbingwg:master Nov 27, 2020

pperiyasamy mentioned this pull request Dec 17, 2020

OVS-CNI doesn't free IPs after container deleted #143

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make ovs socket file path as configurable property #142

make ovs socket file path as configurable property #142

pperiyasamy commented Nov 18, 2020

pperiyasamy commented Nov 18, 2020

phoracek left a comment

phoracek Nov 25, 2020

pperiyasamy Nov 26, 2020

phoracek Nov 26, 2020

pperiyasamy Nov 26, 2020

phoracek Nov 27, 2020

phoracek Nov 25, 2020

pperiyasamy Nov 26, 2020

phoracek Nov 25, 2020

pperiyasamy Nov 26, 2020

phoracek Nov 25, 2020

pperiyasamy Nov 26, 2020

phoracek Nov 25, 2020

pperiyasamy Nov 26, 2020

phoracek Nov 25, 2020

pperiyasamy Nov 26, 2020

pperiyasamy commented Nov 26, 2020

kubevirt-bot commented Nov 26, 2020

phoracek left a comment

phoracek Nov 26, 2020

pperiyasamy Nov 26, 2020

phoracek commented Nov 27, 2020

kubevirt-bot commented Nov 27, 2020

make ovs socket file path as configurable property #142

make ovs socket file path as configurable property #142

Conversation

pperiyasamy commented Nov 18, 2020

pperiyasamy commented Nov 18, 2020

phoracek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pperiyasamy commented Nov 26, 2020

kubevirt-bot commented Nov 26, 2020

phoracek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoracek commented Nov 27, 2020

kubevirt-bot commented Nov 27, 2020