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

[Laine Stump]

On 09/11/2014 05:15 PM, Eric Blake wrote:
> 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?


I thought of that as I was sending the patch, but didn't want to ruin
the momentum by going back to check if I needed to add a Requires: (I
was also curious if anyone else would think of it :-).

As it turns out, the iproute package (containing the ip command) is
included in the "core" set of packages used even for a minimal Fedora
install. However, we do have a "Requires: iproute", for the
libvirt-daemon package, and libvirt-daemon-config-network has "Requires:
libvirt-daemon", so we are guaranteed that the ip command will be present.


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

Yeah, things like this are the reason I asked for suggestions :-)

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

What if I captured the ip output, then did:

if echo ${routes} | grep -q " 192\\.168\\.$sub\\.0/24 "; then
   ...


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

I was really just going for brevity rather than efficiency, since it's
only going to run once, and the list of routes will likely not be very
long (although I suppose if someone installed libvirt on a machine that
was in use as a core router (with full list of routes for the entire
internet) it could be a consideration :-)

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

I'm sending a new version that captures the output of ip route show,
then uses it in nested case statements.