[Ovirt-devel] [PATCH] Merge developer/bundled appliance into a single appliance: ovirt-appliance

[Chris Lalancette]

Perry Myers wrote:
> The separation of the ovirt appliance image into developer/bundled versions
> is not necessary, since the only differences between them are in how the
> host bridge is configured.  Developer assumes that you only have a single
> NIC on your host so you can't have a separate ovirt network managed by
> the appliance.  Bundled assumes you have the second NIC available.

Yes, very good point.  I totally agree; the only real difference is in the
underlying setup, so unifying them is good.

> 
> These patches remove the separation between bundled/developer.  The new
> appliance is called ovirt-appliance and can be installed with or without
> a bridge to a real network device.  In either case dummy managed nodes
> are allowed (they can be used in conjunction with the real managed nodes
> or just ignored in that case)

Yep, also a good point.

> Both build-all.sh and create-wui-appliance.sh have the developer/bundled
> flags removed, and replaced with -e flag to indicate the ethernet device
> to use as the bridge.  If this is omitted it is assumed that the ovirtbr
> will not bridge to a physical network.  The bridge (if to a physical net)
> is made persistent in rc.local (eventually libvirt should handle this)
> If the device to use on the bridge is used already, the script aborts
> (we don't want to mess up someones eth0 config by accident)

Yay!  Removal of a command-line switch :).  And yes, totally agreed about the
eth0 checking; we can't fix it up (and really probably shouldn't blindly
override the user's decision for eth0), so just detecting with a good error is
the best we can do here.

> diff --git a/build-all.sh b/build-all.sh
> index 6e27957..d231241 100755
> --- a/build-all.sh
> +++ b/build-all.sh
> @@ -29,16 +29,15 @@ DEP_RPMS="createrepo httpd kvm libvirt livecd-tools pungi-1.2.18.1"
>  usage() {
>      case $# in 1) warn "$1"; try_h; exit 1;; esac
>      cat <<EOF
> -Usage: $ME [-w] [-n] [-p init|update] [-s] [-d|-b] [-a] [-c] [-v git|release|none]
> +Usage: $ME [-w] [-n] [-p init|update] [-s] [-a] [-c] [-v git|release|none]

Minor nit; you forgot to put the [-e nic] as part of the initial Usage string.

<snip>

> @@ -57,19 +56,18 @@ update_wui=0 update_node=0
>  update_pungi=0 update_app=0
>  include_src=0
>  cleanup=0
> -app_type=
>  version_type=git
> +bridge=
>  err=0 help=0
> -while getopts wnp:sdbahcv: c; do
> +while getopts wnp:sahcv:e: c; do
>      case $c in
>          w) update_wui=1;;
>          n) update_node=1;;
>          p) update_pungi=$OPTARG;;
>          s) include_src=1;;
> -        d) update_app=1; app_type="-v";;
> -        b) update_app=1; app_type="-b";;

Not 100% certain of this, but don't you need to set the "update_app" flag
somewhere now that you've removed the options that set it?

<snip>

> +if [ -n "$bridge" ]; then
> +    ifconfig $bridge > /dev/null 2>&1 ; bridge_dev_present=$?
> +    test $bridge_dev_present != 0 && die "$bridge device not present, aborting!"
> +    test -f $NET_SCRIPTS/ifcfg-$bridge && die "$bridge defined in $NET_SCRIPTS, aborting!"
>  fi

Unless I'm missing something (and please point out if I am), this looks to be
the only check for already existing networking devices.  If this is the only
check, I don't think it is strong enough.  In particular, if you are doing this
over a remote link, and happen to pass eth0 as the device you want to bridge, I
think you will still make it by this check and then totally hose up your remote
connection.  I think we need to check both if the device is present, and if the
device is currently configured with an IP address.  In that case, we would
abort, since we can't make any guarantees about not hosing up the connection.

<snip>

> +if [ -n "$bridge" ]; then
> +    # FIXME: unfortunately, these two can't be done by libvirt at the
> +    # moment, so we do them by hand here and persist the config by
> +    # by adding to rc.local
> +    echo "Adding new bridge $bridge"
> +    TMPBRCTL=$(mktemp) || exit 1
> +    cat > $TMPBRCTL << EOF
> +brctl addif $BRIDGENAME $bridge # $BRIDGENAME
> +ifconfig $bridge up # $BRIDGENAME
> +EOF
> +    chmod a+x $TMPBRCTL
> +
> +    cat $TMPBRCTL >> /etc/rc.d/rc.local
> +
> +    $TMPBRCTL
> +    rm $TMPBRCTL

Is there any reason you need the TMPBRCTL file at all?  Can't you just:

cat >> /etc/rc.d/rc.local << EOF
brctl addif $BRIDGENAME $bridge # $BRIDGENAME
ifconfig $bridge up # $BRIDGENAME
EOF

and be done with it?

Chris Lalancette