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

Tue Jul 1 03:15:41 UTC 2008

On Mon, Jun 30, 2008 at 5:51 PM, Perry N. Myers <pmyers at redhat.com> wrote:
> Chris Lalancette wrote:
>>
>> 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.
>
> Will fix.
>
>> <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?
>
> It's part of the -a flag to the script.  -a is to 'rebuild all' and in
> reality if you're going to rebuild the appliance, you probably should always
> rebuild the node/wui RPMs, etc.
>
>> <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.
>
> We talked about this offline, and you were misreading the code.  $bridge
> here refers to eth1, eth2 (i.e. whatever you passed in on -e flag)
>
> However you did point out two things here, first is that the check for
> ifcfg-eth1 may not be sufficient to detect whether or not the designated
> interface is running.  Secondly there may be portability issues with using
> ifconfig.  I'll see if I can refactor these checks to use something like ip.
>  Suggestions from our friends working in other distros would be appreciated
> here.

What makes this more portable than the same thing using ifconfig?
"ip addr show $bridge > /dev/null 2>&1 ; bridge_dev_present=$?"

Are there distros that use something other than net-tools for /sbin/ifconfig?
Since there is no real distro agnostic way to do this, you'll end up
special casing
major distro versions like redhat and debian. The majority of
distributions are based
upon those two anyways. Even if it is slackware based, suse has enough
redhat-isms
you could probably make the checks for suse the same as redhat.

+    test -f $NET_SCRIPTS/ifcfg-$bridge && die "$bridge defined in
$NET_SCRIPTS, aborting!"

... somewhere near the top...
if [ -e /etc/redhat-release ]; then
    DISTRO=redhat
elif [ -e /etc/debian_version ]; then
    DISTRO=debian
fi

ifconfig "$bridge" ...  && croak because the interface is already up
and active...

case "$DISTRO" in
    redhat)
         test -f "$NET_SCRIPTS/ifcfg-$bridge"; bridge_int_undefined=$?
    ;;
    debian)
        grep -qs "$bridge" /etc/network/interfaces; bridge_int_undefined=$?
    ;;
    *) you should never be here
esac

if [ $bridge_int_undefined = 0 ]; then
    die "$bridge defined in $NET_SCRIPTS, aborting!"
fi

Maybe there is a way to seperate all "distro specific checks" to a common
shell functions file? Then you could have the common shell functions file return
variables and keep the individual scripts a bit cleaner? This might be
overkill, but
there isn't a generic way to configure persistent network settings on
Linux. If you
absolutely need that feature, you might think about abstracting things
out a bit more.

Also, why not be a bit more magical? Instead of dying if $bridge is
defined, why not pick
the next available interface? Especially for a developer appliance?

>>> +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
>
> There are two things that need to be done.  First is to start the bridge
> *now*, second is to put the stuff in rc.local to have it start on reboot.
>  We can't just put it in rc.local and then execute rc.local because we don't
> know what else is in rc.local and it might be bad to reexecute that file.
>  So rather than duplicating the brctl and ifconfig lines in the script, I
> write it once to a temp file, put the contents of the temp file in rc.local
> and then execute the temp file now so that the bridge comes up immediately.
>  Make more sense now?
>
> Again, if ifconfig is a bad command to use for portability suggestions for
> more portable commands would be appreciated.

Your approach seems sane to me.

-- 
Jeff Schroeder

Don't drink and derive, alcohol and analysis don't mix.
http://www.digitalprognosis.com