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

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.