[ovs-dev] [PATCH 1/2] ovs-docker: Integrate docker containers with Open vSwitch.

Ben Pfaff blp at nicira.com
Wed Oct 15 23:21:54 UTC 2014


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 <gshetty at 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.



More information about the dev mailing list