On Tue, Oct 14, 2014 at 03:52:39AM -0700, Gurucharan Shetty wrote:
> ovs-docker is a helper script to add network interfaces to
> docker containers and to attach them as ports to OVS bridge.
>
> This script can be further enhanced as we understand different
> use cases.
>
> Signed-off-by: Gurucharan Shetty <gshe...@nicira.com>
Thanks. I hope that users will find this useful.
It looks good to me. I have only minor comments, although now that I
look down at them I see that there are a lot of them.
I see that this script in several places can print error messages. It
should probably print these to stderr (by adding >&2). Maybe it
should prefix them by "$0:", also (or calculate "basename $0" as a
variable early on and use that).
As I started looking this over, I found myself wondering how to use
it. I see that the instructions are in the second patch. I would
consider squashing the patches together.
Some of the ovs-vsctl calls might benefit from ovs-vsctl's -f and -d
options, although I didn't really look carefully.
I am a little concerned about a hard-coded timeout of 5 seconds. It
is possible for a database operation to take longer, if the system is
busy or the database is very large:
> +ovs_vsctl () {
> + ovs-vsctl --timeout=5 "$@"
Should we specify a particular user, group, and permissions for this
directory? (I do not know what the correct ones are.) Also, I think
that some newer systems generally use /run instead of /var/run. I
don't know whether they have adapted "ip" to use /run:
> +create_netns_link () {
> + mkdir -p /var/run/netns
> + if [ ! -e /var/run/netns/"$PID" ]; then
> + ln -s /proc/"$PID"/ns/net /var/run/netns/"$PID"
> + NETNS_LINK="true"
> + fi
> +}
> +
> +delete_netns_link () {
> + if [ "$NETNS_LINK" = "true" ]; then
> + rm -f /var/run/netns/"$PID"
> + fi
> +}
It looks like delete_netns_link and create_netns_link are called in
pairs, with delete_netns_link cleaning up at the end of execution. Is
it a good idea to use the shell "trap" command so that
delete_netns_link gets called even if the script gets killed?
I don't think the () are necessary here:
> + if (ovs_vsctl --may-exist add-br "$BRIDGE"); then :; else
> + echo "Failed to create bridge $BRIDGE"
> + exit 1
> + fi
These are pretty specific substrings. They are fixed, for docker?
> + PORTNAME="${CONTAINER:0:8}${INTERFACE:0:5}"
I don't think the () are necessary here either:
> + # Add one end of veth to OVS bridge.
> + if (ovs_vsctl --may-exist add-port "$BRIDGE" "${PORTNAME}_l" \
> + -- set interface "${PORTNAME}_l" \
> + external_ids:container_id="$CONTAINER" \
> + external_ids:container_iface="$INTERFACE"); then :; else
> + echo "Failed to add "${PORTNAME}_l" port to bridge $BRIDGE"
> + ip link delete "${PORTNAME}_l"
> + delete_netns_link
> + exit 1
> + fi
Usually there's another '.' after the g: "e.g.:":
> + connection to Open vSwitch BRIDGE. e.g:
Here too:
> + Removes all Open vSwitch interfaces from CONTAINER. e.g:
> + ${UTIL} del-ports br-int c474a0e2830e
> +Options:
> + -h, --help display this help message.
> +EOF
> +}
I am not sure why there is a "while" loop at the end of the script. I
think that only one iteration is possible.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
