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.

Sign up for GitHub

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

Jump to bottom

Golang versioned ovn-kube-util #113

Merged
merged 5 commits into from
May 2, 2017
Merged

Golang versioned ovn-kube-util #113

merged 5 commits into from
May 2, 2017

Conversation

feiskyer
Copy link
Contributor

This PR adds golang versioned ovs-kube-util.

cc/ @shettyg

rajatchopra
rajatchopra reviewed Apr 28, 2017
View reviewed changes
Copy link

@rajatchopra rajatchopra left a comment

Choose a reason for hiding this comment

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

Done first pass review. Looks fine mostly. Little concerned about re-entrancy of this util. But can be addressed later.

go-controller/cmd/ovn-kube-util/app/nics-to-bridge.go
"strings"
"syscall"

"github.com/Sirupsen/logrus"
Copy link

Choose a reason for hiding this comment

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

At some point we should converge on glog or logrus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer logrus. WDYT?

Copy link

Choose a reason for hiding this comment

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

logrus is just fine. Just seeking consistency. Pulling out glog in another PR then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. will do

go-controller/Makefile Outdated
@@ -7,6 +7,7 @@ export OUT_DIR
# make all
all build:
hack/build-go.sh cmd/ovnkube/ovnkube.go
hack/build-go.sh cmd/ovn-kube-util/main.go
Copy link

Choose a reason for hiding this comment

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

Ok for now, but maybe we should merge all into single binary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, a unified binary with subcommands if preferred in the future.

go-controller/cmd/ovn-kube-util/app/nics-to-bridge.go Outdated

for _, nic := range args {
if err := nicToBridge(nic); err != nil {
return err
Copy link

Choose a reason for hiding this comment

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

Not quit, but possibly accumulate the errors in ErrorList and try them all out?
We could then report which all failed and one can know which succeeded. Useful since we do not roll back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK. Updated.

go-controller/cmd/ovn-kube-util/app/nics-to-bridge.go Outdated
}

bridge := getBridgeName(iface)
stdout, stderr, err := util.RunOVSVsctl("add-br", bridge,
Copy link

Choose a reason for hiding this comment

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

Is there an '--if-exists' flag to ovs-vsctl add-br? I cannot recall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you means --may-exist?

@shettyg
Copy link
Collaborator

shettyg commented Apr 28, 2017

This does not compile for me. I get the following error:

vendor/github.com/vishvananda/netlink/addr_linux.go:10:2: cannot find package "github.com/vishvananda/netlink/nl" in any of:

@feiskyer
Add go version of ovn-kube-util
f733cac 
Signed-off-by: Pengfei Ni <feiskyer@gmail.com>
@feiskyer
Copy link
Contributor Author

@shettyg Added back the missing vendors. PTAL.

@shettyg
Copy link
Collaborator

shettyg commented Apr 29, 2017

@feiskyer
Thanks. It compiles now. But, looks like the broadcast address is not being properly moved to the bridge.

$tunctl -t p0
$ifconfig p0 192.168.50.2
$ifconfig p0
p0        Link encap:Ethernet  HWaddr 50:9a:1a:7d:21:1c  
          inet addr:192.168.50.2  Bcast:192.168.50.255  Mask:255.255.255.0
          UP BROADCAST MULTICAST  MTU:1500  Metric:1
          RX packets:0 errors:0 dropped:0 overruns:0 frame:0
          TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:500 
          RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)

$ovn-kube-util nics-to-bridge p0

$ifconfig brp0
brp0      Link encap:Ethernet  HWaddr 50:9a:1a:7d:21:1c  
          inet addr:192.168.50.2  Bcast:0.0.0.0  Mask:255.255.255.0
          UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
          RX packets:0 errors:0 dropped:0 overruns:0 frame:0
          TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:1000 
          RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)

@feiskyer
Copy link
Contributor Author

@shettyg Sorry, didn't notice this problem. It is because vishvananda/netlink always set broadcast to nil when getting addrs for ifaces. Filed a PR vishvananda/netlink#226 to fix this.

@feiskyer
Fix lint issues
4ebcf91 
Signed-off-by: Pengfei Ni <feiskyer@gmail.com>
@feiskyer
Copy link
Contributor Author

feiskyer commented May 1, 2017 •
edited

Added a new commit with the fix. Should work properly now.

@shettyg
Copy link
Collaborator

shettyg commented May 1, 2017 •
edited

@feiskyer

LGTM

What is the typical strategy when it comes to vendor fixes in golang? Looks like here, you have updated our local vendor code. But, in the future if someone updates the vendor code, we will miss your fix, isn't it?

The other option is to have a local function that does the correct code and use it instead of the upstream vendor code. And then remove local code when the upstream has the fix. I am trying to understand what do other projects that use vendoring do?

@feiskyer
Copy link
Contributor Author

feiskyer commented May 1, 2017 •
edited

We usually fix the problem outside vendors, but I'm expecting the fix could be merged soon in the upstream code, e.g. before this one. Let's wait a while for the upstream fix. If it is too slow, then a local fix is prefered.

@rajatchopra
Copy link

Same strategy in the openshift project. Fix with a specially marked commit and PR (e.g. keyword UPSTREAM) and actually rebase from all upstream when starting a new development release.

@feiskyer
Add addrList for listing broadcasts
cad213e 
addrList is a workround for also getting broadcast addr.
It should be replaced by netlink.AddrList after
vishvananda/netlink#226 is merged.

Signed-off-by: Pengfei Ni <feiskyer@gmail.com>
@feiskyer
Copy link
Contributor Author

feiskyer commented May 2, 2017

@shettyg Added a local version addrList for a workaround.

@shettyg shettyg merged commit 5aef7f4 into ovn-org:master May 2, 2017
@feiskyer feiskyer deleted the nics-to-bridge branch May 3, 2017 01:49
@feiskyer feiskyer mentioned this pull request May 3, 2017
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajatchopra rajatchopra rajatchopra left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@feiskyer @shettyg @rajatchopra