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

[Perry N. Myers]

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.

> <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

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.

Perry

> and be done with it?
> 
> Chris Lalancette


-- 
|=-        Red Hat, Engineering, Emerging Technologies, Boston        -=|
|=-                     Email: pmyers at redhat.com                      -=|
|=-         Office: +1 412 474 3552   Mobile: +1 703 362 9622         -=|
|=- GnuPG: E65E4F3D 88F9 F1C9 C2F3 1303 01FE 817C C5D2 8B91 E65E 4F3D -=|