Re: [PATCHv2] tools: hv: hv_set_ifconfig.sh double check before setting ip
On Fri, Dec 08, 2017 at 12:33:38PM +0100, Olaf Hering wrote: > On Fri, Dec 08, Eduardo Otubo wrote: > > > tools/hv/hv_set_ifconfig.sh | 45 > > +++-- > > 1 file changed, 43 insertions(+), 2 deletions(-) > > > +# let's wait for 3 minutes > > Was this codepath runtime tested? > > Last time this came up, the conclusion was that Windows terminates the > "KVP connection" after 60 seconds. After that, the VM must be rebooted > because the kernel does not deal with the long-running script. It is > also not clear what the kernel is supposed to do: either silently report > success and hope the scripts reports success one day, or report error > independent from what the script actually returns. New KVP requests > would need to be queued either way. > Now that you mentioned, I guess my tests never touched this part. But I guess we're going to fix this downstream-only setting a rule on systemd to start hypervkvpd only after NetworkManager started. Please drop the reviews on this. Regards, ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCHv2] tools: hv: hv_set_ifconfig.sh double check before setting ip
This patch fixes the behavior of the hv_set_ifconfig script when setting
the interface ip. Sometimes the interface has already been configured by
network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file
exists error"; in order to avoid this error this patch makes sure double
checks the interface before trying anything.
Signed-off-by: Eduardo Otubo <ot...@redhat.com>
---
v2: wrap the interface configuration inside a safe wAY to avoid
interaction with network script.
tools/hv/hv_set_ifconfig.sh | 45 +++--
1 file changed, 43 insertions(+), 2 deletions(-)
diff --git a/tools/hv/hv_set_ifconfig.sh b/tools/hv/hv_set_ifconfig.sh
index 7ed9f85ef908..b6429703cc98 100755
--- a/tools/hv/hv_set_ifconfig.sh
+++ b/tools/hv/hv_set_ifconfig.sh
@@ -61,5 +61,46 @@ cp $1 /etc/sysconfig/network-scripts/
interface=$(echo $1 | awk -F - '{ print $2 }')
-/sbin/ifdown $interface 2>/dev/null
-/sbin/ifup $interface 2>/dev/null
+current_ip=$(ip addr show $interface|sed -En 's/.*inet
(addr:)?(([0-9*\.){3}[0-9]*).*/\2/p');
+config_file_ip=$(grep IPADDR
/etc/sysconfig/network-scripts/ifcfg-$interface|cut -d"=" -f2);
+
+current_ipv6=$(ip addr show $interface|sed -E 's/^*.inet6\ (.*)\ scope\
global/\1/');
+config_file_ipv6=$(grep IPV6ADDR
/etc/sysconfig/network-scripts/ifcfg-eth-$interface|cut -d"=" -f2);
+config_file_ipv6_netmask=$(grep IPV6NETMASK
/etc/sysconfig/network-scripts/ifcfg-eth-$interface|cut -d"=" -f2);
+config_file_ipv6=${config_file_ipv6}/${config_file_ipv6_netmask};
+
+configure_interface(){
+# only set the IP if the network service has not done yet
+if [[ ${current_ip} != *${config_file_ip}* || ${current_ipv6} !=
${config_file_ipv6} ]]; then
+/sbin/ifdown $interface 2>/dev/null
+/sbin/ifup $interface 2>/dev/null
+fi
+}
+
+error_exit(){
+message=$1
+logger "KVP daemon: $message"
+exit 1;
+}
+
+if [ -e /var/run/subsys/network ]; then
+# network script is already up
+# it is safe to configure the interface
+configure_interface;
+else
+i=0;
+while [ ! -e /var/run/subsys/network ]; do
+# network script might be still starting.
+# let's wait for 3 minutes
+sleep 1m;
+((i++));
+
+# if network service doens't come up in 3 minutes
+# perhaps there's no network service at all
+[[ $i == 3 ]] && break;
+done
+
+# at this point it doesn't matter if network service is up or down
+# we're safe either way
+configure_interface;
+fi
--
2.13.6
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
On Mon, Aug 28, 2017 at 04:48:32PM +, KY Srinivasan wrote: > > > > -Original Message- > > From: Haiyang Zhang > > Sent: Monday, August 28, 2017 8:57 AM > > To: Stephen Hemminger <step...@networkplumber.org>; Eduardo Otubo > > <ot...@redhat.com>; KY Srinivasan <k...@microsoft.com> > > Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; Stephen > > Hemminger <sthem...@microsoft.com>; David Miller > > <da...@davemloft.net> > > Subject: RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip > > > > > > > > > -Original Message- > > > From: Stephen Hemminger [mailto:step...@networkplumber.org] > > > Sent: Monday, August 28, 2017 11:16 AM > > > To: Eduardo Otubo <ot...@redhat.com> > > > Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; Haiyang > > > Zhang <haiya...@microsoft.com>; Stephen Hemminger > > > <sthem...@microsoft.com>; David Miller <da...@davemloft.net> > > > Subject: Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip > > > > > > On Mon, 28 Aug 2017 12:01:21 +0200 > > > Eduardo Otubo <ot...@redhat.com> wrote: > > > > > > > v2: The script is now a little bit safer so it doesn't conflicts with > > > > network daemon trying to set configurations at the same time. > > > > > > > > This patch fixes the behavior of the hv_set_ifconfig script when > > > setting > > > > the interface ip. Sometimes the interface has already been configured > > > by > > > > network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file > > > > exists error"; in order to avoid this error this patch makes sure > > > double > > > > checks the interface before trying anything. > > > > > > > > Signed-off-by: Eduardo Otubo <ot...@redhat.com> > > > > > > Adding new dependency on systemd is not going to make this script > > > even less useful. I wonder why the script still exists at all? Most of > > > the > > > Linux distro's can already setup HV networking without it. > > > > > > > This script is used by a host to inject IP into guests. KY knows more > > details about it. > > I wrote this script initially to provide an example script for Distros to > base their solution on. > KVP supports IP injection to enable VM migration. For this scenario, I think > we recommend that NM be > disabled. > So, what you're saying is that this should be fixed downstream, instead? This solution seems pretty safe for me and long term we can think about something else that could get rid of this script. So NM or whatever is in use can actually do the configuration. Any chance to have this patch ACK'd as a form of a short term solution? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
On Mon, Aug 28, 2017 at 03:56:44PM +, Haiyang Zhang wrote: > > > > -Original Message- > > From: Stephen Hemminger [mailto:step...@networkplumber.org] > > Sent: Monday, August 28, 2017 11:16 AM > > To: Eduardo Otubo <ot...@redhat.com> > > Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; Haiyang > > Zhang <haiya...@microsoft.com>; Stephen Hemminger > > <sthem...@microsoft.com>; David Miller <da...@davemloft.net> > > Subject: Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip > > > > On Mon, 28 Aug 2017 12:01:21 +0200 > > Eduardo Otubo <ot...@redhat.com> wrote: > > > > > v2: The script is now a little bit safer so it doesn't conflicts with > > > network daemon trying to set configurations at the same time. > > > > > > This patch fixes the behavior of the hv_set_ifconfig script when > > setting > > > the interface ip. Sometimes the interface has already been configured > > by > > > network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file > > > exists error"; in order to avoid this error this patch makes sure > > double > > > checks the interface before trying anything. > > > > > > Signed-off-by: Eduardo Otubo <ot...@redhat.com> > > > > Adding new dependency on systemd is not going to make this script > > even less useful. I wonder why the script still exists at all? Most of > > the > > Linux distro's can already setup HV networking without it. > > Also, this script is meant to run only on RHEL guests, as written on the header of this very same file. -- Eduardo Otubo Senior Software Engineer @ RedHat ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCHv2] hv_set_ifconfig.sh double check before setting ip
v2: The script is now a little bit safer so it doesn't conflicts with
network daemon trying to set configurations at the same time.
This patch fixes the behavior of the hv_set_ifconfig script when setting
the interface ip. Sometimes the interface has already been configured by
network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file
exists error"; in order to avoid this error this patch makes sure double
checks the interface before trying anything.
Signed-off-by: Eduardo Otubo <ot...@redhat.com>
---
tools/hv/hv_set_ifconfig.sh | 44
1 file changed, 36 insertions(+), 8 deletions(-)
diff --git a/tools/hv/hv_set_ifconfig.sh b/tools/hv/hv_set_ifconfig.sh
index 735aafd64a3f..3a04b91f61e3 100755
--- a/tools/hv/hv_set_ifconfig.sh
+++ b/tools/hv/hv_set_ifconfig.sh
@@ -46,19 +46,47 @@
# is expected to return the configuration that is set via the SET
# call.
#
+interface=$(echo $1 | awk -F - '{ print $2 }')
+current_ip=$(ip addr show $interface|grep "inet ");
+config_file_ip=$(grep IPADDR $1|cut -d"=" -f2);
+current_ipv6=$(ip addr show $interface|grep "inet6 ");
+config_file_ipv6=$(grep IPV6ADDR $1|cut -d"=" -f2);
+config_file_ipv6_netmask=$(grep IPV6NETMASK $1|cut -d"=" -f2);
+config_file_ipv6=${config_file_ipv6}/${config_file_ipv6_netmask};
-echo "IPV6INIT=yes" >> $1
-echo "NM_CONTROLLED=no" >> $1
-echo "PEERDNS=yes" >> $1
-echo "ONBOOT=yes" >> $1
+network_service_state=$(/bin/systemctl is-active network);
+while [[ ${network_service_state} == "activating" \
+ || ${network_service_state} == "deactivating" ]]; do
+# Network script is still working. let's wait a bit.
+# The default timeout for systemd is 90s.
+sleep 30s;
+((i++));
+network_service_state=$(/bin/systemctl is-active network);
-cp $1 /etc/sysconfig/network-scripts/
+# If network service doens't come up or down in 90s we log the
+# error and give up.
+if [[ $i == 3 ]]; then
+logger "Couldn't set IP address for fail-over interface"\
+" because network daemon might be busy. Try to"\
+" if-down $interface && if-up $interface"\
+" manually later.";
+exit 1;
+fi
+done
+# Only set the IP if it's not configured yet.
+if [[ $(test "${current_ip#*$config_file_ip}") == "$config_file_ip" \
+|| $(test "${current_ipv6#*$config_file_ipv6}") == "$current_ipv6" ]]; then
+echo "IPV6INIT=yes" >> $1
+echo "NM_CONTROLLED=no" >> $1
+echo "PEERDNS=yes" >> $1
+echo "ONBOOT=yes" >> $1
-interface=$(echo $1 | awk -F - '{ print $2 }')
+cp $1 /etc/sysconfig/network-scripts/
-/sbin/ifdown $interface 2>/dev/null
-/sbin/ifup $interface 2>/dev/null
+/sbin/ifdown $interface 2>/dev/null
+/sbin/ifup $interface 2>/dev/null
+fi
--
2.13.5
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] hv_set_ifconfig.sh double check before setting ip
On 08/09/2017 11:02 AM, Eduardo Otubo wrote: On 08/09/2017 06:11 AM, David Miller wrote: From: Eduardo Otubo <ot...@redhat.com> Date: Tue, 8 Aug 2017 15:53:45 +0200 This patch fixes the behavior of the hv_set_ifconfig script when setting the interface ip. Sometimes the interface has already been configured by network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file exists error"; in order to avoid this error this patch makes sure double checks the interface before trying anything. Signed-off-by: Eduardo Otubo <ot...@redhat.com> And if the daemon sets the address after you test it but before you try to set it in the script, what happens? This is why I hate changes like this. They don't remove the problem, they make it smaller. And smaller in a bad way. Smaller makes the problem even more harder to diagnose when it happens. There is implicitly no synchonization between network configuration daemons and things people run by hand like this script. So, caveat emptor. I'm not applying this, sorry. But also, looking from a different point of view, the current upstream solution does not avoid the problems you mentioned. My fix at least avoids double configuration and RTNETLINK errors. So perhaps you could consider this as "a better version walking towards an ideal fix"? This is just part of the resolution, actually. For RHEL I also configure hyperv-daemons' systemd config file to be run only after network service is up. So perhaps my solution should be distro-agnostic and only involve this script as part of it? In this case I'll elaborate a little more then. Thanks for the comment. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] hv_set_ifconfig.sh double check before setting ip
This patch fixes the behavior of the hv_set_ifconfig script when setting
the interface ip. Sometimes the interface has already been configured by
network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file
exists error"; in order to avoid this error this patch makes sure double
checks the interface before trying anything.
Signed-off-by: Eduardo Otubo <ot...@redhat.com>
---
tools/hv/hv_set_ifconfig.sh | 29 ++---
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/tools/hv/hv_set_ifconfig.sh b/tools/hv/hv_set_ifconfig.sh
index 735aafd64a3f..ba5e4aa4efe2 100755
--- a/tools/hv/hv_set_ifconfig.sh
+++ b/tools/hv/hv_set_ifconfig.sh
@@ -46,19 +46,26 @@
# is expected to return the configuration that is set via the SET
# call.
#
+interface=$(echo $1 | awk -F - '{ print $2 }')
+current_ip=$(ip addr show $interface|grep "inet ");
+config_file_ip=$(grep IPADDR $1|cut -d"=" -f2);
+current_ipv6=$(ip addr show $interface|grep "inet6 ");
+config_file_ipv6=$(grep IPV6ADDR $1|cut -d"=" -f2);
+config_file_ipv6_netmask=$(grep IPV6NETMASK $1|cut -d"=" -f2);
+config_file_ipv6=${config_file_ipv6}/${config_file_ipv6_netmask};
-echo "IPV6INIT=yes" >> $1
-echo "NM_CONTROLLED=no" >> $1
-echo "PEERDNS=yes" >> $1
-echo "ONBOOT=yes" >> $1
-
-
-cp $1 /etc/sysconfig/network-scripts/
-
+# only set the IP if it's not configured yet
+if [[ $(test "${current_ip#*$config_file_ip}") == "$config_file_ip" \
+|| $(test "${current_ipv6#*$config_file_ipv6}") == "$current_ipv6" ]]; then
+echo "IPV6INIT=yes" >> $1
+echo "NM_CONTROLLED=no" >> $1
+echo "PEERDNS=yes" >> $1
+echo "ONBOOT=yes" >> $1
-interface=$(echo $1 | awk -F - '{ print $2 }')
+cp $1 /etc/sysconfig/network-scripts/
-/sbin/ifdown $interface 2>/dev/null
-/sbin/ifup $interface 2>/dev/null
+/sbin/ifdown $interface 2>/dev/null
+/sbin/ifup $interface 2>/dev/null
+fi
--
2.13.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
