openshift / openshift-sdn

Apache License 2.0
69 stars 63 forks source link

Rewrite setup.sh in go [DO NOT MERGE] #166

Closed danwinship closed 8 years ago

danwinship commented 9 years ago

This is based on #87, but very heavily rewritten...

This adds pkg/ovs, pkg/ipcmd, and pkg/brctl (which all just wrap os/exec, they don't implement anything natively), and then makes kube and multitenant use them to replace a large chunk of the setup scripts.

One problem with moving code from .sh to .go (which causes a lot of ugliness in #87) is that the error checking gets a lot more annoying to deal with. I made two different attempts to simplify that here:

    otx := ovs.NewTransaction()
    otx.AddBridge(BR, "fail-mode=secure", "protocols=OpenFlow13")
    otx.AddPort(BR, VXLAN, 1, "type=vxlan", `options:remote_ip="flow"`, `options:key="flow"`)
    otx.AddPort(BR, TUN, 2, "type=internal")
    otx.AddPort(BR, VOVSBR, 9)
    otx.AddFlow(BR, "table=0,priority=100,arp,nw_dst=%s,actions=output:2", localSubnetGateway)
    ...
    err = otx.EndTransaction()
    if err != nil {
        return err
    }
    err = ipcmd.Link(VLINUXBR).
            Delete().IgnoreError().
            Add("type", "veth", "peer", "name", VOVSBR).
            Set("up").
            Set("txqueuelen", "0").
            GetError()
    if err != nil {
            return err
    }

Both of these approaches are suggested by various golang authorities in various places. There's also what we did for the firewall rules:

    rules := []FirewallRule{
            {firewalld.IPv4, "nat", "POSTROUTING", 0, []string{"-s", clusterNetworkCIDR, "!", "-d", clusterNetworkCIDR, "-j", "MASQUERADE"}},
            {firewalld.IPv4, "filter", "INPUT", 0, []string{"-p", "udp", "-m", "multiport", "--dports", "4789", "-m", "comment", "--comment", "001 vxlan incoming", "-j", "ACCEPT"}},
            ...
    }

    for _, rule := range rules {
            err := fw.EnsureRule(rule.ipv, rule.table, rule.chain, rule.priority, rule.args)
            if err != nil {
                    return err
            }
    }

which is simple and easily-understood, but seems inelegant to me...

What do people think about the various approaches? (I assume we want to be consistent rather than having each package implement this in a different way...)

rajatchopra commented 9 years ago

The one with transaction is best I feel. In my experience with c++ too that approach helped immensely with readability as well as debuggability.

dcbw commented 9 years ago

The transaction thing seems better to me.

But if we're going to all the work to touch the /sbin/ip stuff, we might as well just convert over to netlink. I did a lot of that in the flannel OVS bits already. Everyone seems to use "github.com/coreos/flannel/Godeps/_workspace/src/github.com/vishvananda/netlink" but there's also the libcontainer netlink stuff that Kubernetes already includes (Godeps/_workspace/src/github.com/docker/libcontainer/netlink/netlink.go). For example:

https://github.com/dcbw/flannel/blob/ovs/backend/ovs/device.go#L231 https://github.com/dcbw/flannel/blob/ovs/backend/ovs/device.go#L392

dcbw commented 9 years ago

Also, if you use netlink you don't need brctl...

dcbw commented 9 years ago

And even if you didn't want to use netlink, why bother with brctl, why not just use /sbin/ip to do the same thing?

danwinship commented 9 years ago

The one with transaction is best I feel.

OK, the current version of the branch uses the transaction approach in both pkg/ovs and pkg/ipcmd

But if we're going to all the work to touch the /sbin/ip stuff, we might as well just convert over to netlink.

Well, this moves us towards that, by abstracting away the details of calling the commands, so we could later port the pkg/ code to use netlink/libovs without needing to change any of the callers.

And even if you didn't want to use netlink, why bother with brctl, why not just use /sbin/ip to do the same thing?

Right, the latest version drops brctl and uses ip instead.

The branch now completely gets rid of the setup.sh scripts, although I did cheat a little bit by moving the docker setup stuff out into a separate script, which remains as a shell script because it's so shell-scripty... (sdodson was also talking about some use case on irc this morning where being able to do the docker setup separately from the rest of the sdn setup may be useful.) (And it seems like in the long term, it might be nice to get patches into docker so that we don't need to reconfigure and restart the daemon to get it to do what we want...)

danwinship commented 8 years ago

This PR got detached from its corresponding branch when I destroyed and recreated my openshift-sdn repo so I'm going to close it and open a new one.