[libvirt] [PATCH RFC] network: try to eliminate default network conflict during package install

[Eric Blake]

On 09/10/2014 11:54 AM, Laine Stump wrote:
> Sometimes libvirt is installed on a host that is already using the
> network 192.168.122.0/24. If the libvirt-daemon-config-network package
> is installed, this creates a conflict, since that package has been
> hard-coded to create a virtual network that also uses
> 192.168.122.0/24. In the past libvirt has attempted to warn of /
> remediate this situation by checking for conflicting routes when the
> network is started, but it turns out that isn't always useful (for
> example in the case that the *other* interface/network creating the
> conflict hasn't yet been started at the time libvirtd start its owm

s/owm/own/

> networks).
> 
> This patch attempts to catch the problem earlier - at install
> time. During the %post install for libvirt-daemon-config-network, we
> look through the output of "ip route show" for a route that exactly
> matches 192.1 68.122.0/24, and if found we search for a similar route
> that *doesn't* match (e.g. 192.168.123.0/24). When we find an
> available route, we just replace all occurences of "122" in the

s/occurences/occurrences/

> default.xml that is being created with ${new_sub}. This could
> obviously be made more complicated - automatically determine the
> existing network address and mask from examining the template
> default.xml, etc, but this scripting is simpler and gets the job done
> as long as we continue to use 192.168.122.0/24 in the template. (If
> anyone with mad bash skillz wants to suggest something to do that, by
> all means please do).

Is it worth adding comments into the template that the string "122" is
magic and must not be altered without also considering distro packaging?

> 
> This is intended to at least "further reduce" the problems detailed in:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=811967
> 
> I acknowledge that it doesn't help for cases of pre-built cloud images
> (or live images that are created on real hardware and then run in a
> virtual machine), but it should at least eliminate the troubles
> encountered by individuals doing one-off installs (and could be used
> to stifle complaints for live images, as long as libvirtd was running
> on the system where the live image compose took place (or the compose
> was itself done in a virtual machine that had a 192.168.122.0/24
> interface address).

No good suggestions on how to help those situations.

> ---
> 
> The question here is: "Will this help some people's situation without
> causing new problems for anyone else?" I wouldn't mind pushing this
> patch, but also wouldn't mind if it was just the catalyst for
> discussion that leads to a better solution. We do need *some kind* of
> solution though, as more and more people are installing OSes that
> include the libvirt package in virtual machines, and are running into
> this problem with increasing frequency.
> 
>  libvirt.spec.in | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index a6a58cf..539d9ef 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1728,8 +1728,32 @@ fi
>      %if %{with_network}
>  %post daemon-config-network
>  if test $1 -eq 1 && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml ; then
> +    # see if the network used by default network creates a conflict,
> +    # and try to resolve it
> +    orig_sub=122
> +    sub=${orig_sub}
> +    net=192.168.${sub}.0/24
> +    routes=$(ip route show | cut -d' ' -f1)

How do we know that 'ip' is installed and available?  Do we need any
Requires:, and/or making the script robust to 'ip' failing?

> +    for route in $routes; do
> +      if [ "${net}" = "${route}" ]; then
> +        # there was a match, so we need to look for an unused subnet

Rather than using cut and a shell for loop, why not just use grep? [1]

if ip route show | grep -q "^192\\.168\\.$sub\\.0/24 "; then

> +        for new_sub in $(seq 123 254); do

seq is a GNU coreutils extension that can't be used in shell code
designed to be portable everywhere; but this is a spec file for use by
rpms where we know it will be installed.  So I'm fine with using it.
(If this were a configure script, I would have suggested using:
new_sub=123
while [ $new_sub -lt 255 ]; do
  ...
  new_sub=$((new_sub + 1))
done

but that's overkill for this scenario.)

> +          new_net="192.168.${new_sub}.0/24"
> +          usable=yes
> +          for route in ${routes}; do

[1] Oh, I see.  You captured the ip output once, and are now scanning it
multiple times.  In _that_ case, piping ip to grep on each loop is not
as efficient.

But you could still do the lookup in shell instead of spawning child
processes, and without needing a shell for loop:

routes=$(ip route show)
nl='
'
case $nl$routes in
  *"${nl}192.168.$new_sub.0/24 "*) # code if found
  *) # code if not found
esac

> +            [ "${new_net}" = "${route}" ] && usable=no

Might be slightly faster if you skip the tail end of the for loop after
a collision, as in:

if [ "$new_net" = "$route" ]; then
  usable=no
  break
fi

that is, if you don't go with my case statement way to make the shell do
the iteration over the entire input in one pass.

> +          done
> +          if [ "${usable}" = "yes" ]; then
> +            sub=${new_sub}
> +            break;
> +          fi
> +        done
> +      fi
> +    done
> +
>      UUID=`/usr/bin/uuidgen`
> -    sed -e "s,</name>,</name>\n  <uuid>$UUID</uuid>," \
> +    sed -e "s/${orig_sub}/${sub}/g" \
> +        -e "s,</name>,</name>\n  <uuid>$UUID</uuid>," \
>           < %{_datadir}/libvirt/networks/default.xml \
>           > %{_sysconfdir}/libvirt/qemu/networks/default.xml
>      ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml
> 

Overall, looks like a good idea.  Your approach works, without giving me
too much grief, so up to you if you want to spin a v2 incorporating some
of my ideas, or leave it as-is.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140911/ee28d9ed/attachment-0001.sig>