hv_set_ifconfig.sh double check before setting ip

PATCH Superseded Eduardo Otubo 5 participants 8 messages Aug 28, 2017 – Aug 31, 2017 Hyper-V/Azure core and drivers

Share & tools

Eduardo Otubo

author

2017-08-28 10:01 UTC #0 lore raw

PATCH v2 1/1 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 <[email protected]>

---
 tools/hv/hv_set_ifconfig.sh | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

tools/hv/hv_set_ifconfig.sh +36 -8

index 735aafd64a3f..3a04b91f61e3 100755
@@ -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

Signature · 1 line

2.13.5

Olaf Hering

2017-08-28 13:48 UTC #1 lore raw

Quoted 1 line · Eduardo Otubo

+ sleep 30s;

Was this runtime tested?
Once this sleep(1) is done, HV_UTIL_TIMEOUT kicks in and the daemon dies.

Olaf

Stephen Hemminger

2017-08-28 15:16 UTC #2 lore raw

In reply to Eduardo Otubo

Quoted 10 lines · On Mon, 28 Aug 2017 12:01:21 +0200 Eduardo Otubo <[email protected]> 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 <[email protected]>

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.

Haiyang Zhang

maintainer

2017-08-28 15:56 UTC #3 lore raw

Quoted 24 lines

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Monday, August 28, 2017 11:16 AM
To: Eduardo Otubo <[email protected]>
Cc: [email protected]; [email protected]; Haiyang Zhang <[email protected]>; Stephen Hemminger <[email protected]>; David Miller <[email protected]>
Subject: Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
Quoted 4 lines · On Mon, 28 Aug 2017 12:01:21 +0200 Eduardo Otubo <[email protected]> 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
Quoted 1 line
the interface ip. Sometimes the interface has already been configured
by
Quoted 2 lines
network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file exists error"; in order to avoid this error this patch makes sure
double

Quoted 9 lines

Quoted 3 lines
checks the interface before trying anything.
Signed-off-by Eduardo Otubo <[email protected]>
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.

Eduardo Otubo

author

2017-08-28 16:05 UTC #4 lore raw

Quoted 26 lines · Haiyang Zhang

Quoted 26 lines
-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Monday, August 28, 2017 11:16 AM
To: Eduardo Otubo <[email protected]>
Cc: [email protected]; [email protected]; Haiyang Zhang <[email protected]>; Stephen Hemminger <[email protected]>; David Miller <[email protected]>
Subject: Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
Quoted 4 lines · On Mon, 28 Aug 2017 12:01:21 +0200 Eduardo Otubo <[email protected]> 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
Quoted 1 line
the interface ip. Sometimes the interface has already been configured
by
Quoted 2 lines
network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file exists error"; in order to avoid this error this patch makes sure
double
Quoted 2 lines
checks the interface before trying anything.

Quoted 7 lines

Quoted 7 lines
Quoted 1 line
Signed-off-by Eduardo Otubo <[email protected]>
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.

Signature · 2 lines

Eduardo Otubo
Senior Software Engineer @ RedHat

KY Srinivasan

2017-08-28 16:48 UTC #5 lore raw

In reply to Haiyang Zhang

Quoted 40 lines

-----Original Message-----
From: Haiyang Zhang
Sent: Monday, August 28, 2017 8:57 AM
To: Stephen Hemminger <[email protected]>; Eduardo Otubo <[email protected]>; KY Srinivasan <[email protected]>
Cc: [email protected]; [email protected]; Stephen Hemminger <[email protected]>; David Miller <[email protected]>
Subject: RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
Quoted 26 lines
-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Monday, August 28, 2017 11:16 AM
To: Eduardo Otubo <[email protected]>
Cc: [email protected]; [email protected]; Haiyang Zhang <[email protected]>; Stephen Hemminger <[email protected]>; David Miller <[email protected]>
Subject: Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
Quoted 4 lines · On Mon, 28 Aug 2017 12:01:21 +0200 Eduardo Otubo <[email protected]> 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
Quoted 1 line
the interface ip. Sometimes the interface has already been configured
by
Quoted 2 lines
network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file exists error"; in order to avoid this error this patch makes sure
double
Quoted 3 lines
checks the interface before trying anything.
Signed-off-by Eduardo Otubo <[email protected]>

Quoted 8 lines

Quoted 5 lines
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.

K. Y

Quoted 1 line

Eduardo Otubo

author

2017-08-31 08:16 UTC #6 lore raw

Quoted 46 lines · KY Srinivasan

Quoted 46 lines
-----Original Message-----
From: Haiyang Zhang
Sent: Monday, August 28, 2017 8:57 AM
To: Stephen Hemminger <[email protected]>; Eduardo Otubo <[email protected]>; KY Srinivasan <[email protected]>
Cc: [email protected]; [email protected]; Stephen Hemminger <[email protected]>; David Miller <[email protected]>
Subject: RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
Quoted 31 lines
-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Monday, August 28, 2017 11:16 AM
To: Eduardo Otubo <[email protected]>
Cc: [email protected]; [email protected]; Haiyang Zhang <[email protected]>; Stephen Hemminger <[email protected]>; David Miller <[email protected]>
Subject: Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
Quoted 4 lines · On Mon, 28 Aug 2017 12:01:21 +0200 Eduardo Otubo <[email protected]> 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
Quoted 1 line
the interface ip. Sometimes the interface has already been configured
by
Quoted 2 lines
network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file exists error"; in order to avoid this error this patch makes sure
double
Quoted 3 lines
checks the interface before trying anything.
Signed-off-by Eduardo Otubo <[email protected]>
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.

Quoted 7 lines

Quoted 2 lines
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?

KY Srinivasan

2017-08-31 20:21 UTC #7 lore raw

Quoted 73 lines

-----Original Message-----
From: Eduardo Otubo [mailto:[email protected]]
Sent: Thursday, August 31, 2017 1:17 AM
To: KY Srinivasan <[email protected]>
Cc: Haiyang Zhang <[email protected]>; Stephen Hemminger <[email protected]>; [email protected]; [email protected]; Stephen Hemminger <[email protected]>; David Miller <[email protected]>
Subject: Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
Quoted 6 lines · KY Srinivasan
Quoted 6 lines
-----Original Message-----
From: Haiyang Zhang
Sent: Monday, August 28, 2017 8:57 AM
To: Stephen Hemminger <[email protected]>; Eduardo
Otubo
Quoted 8 lines
Quoted 8 lines
<[email protected]>; KY Srinivasan <[email protected]>
Cc: [email protected]; [email protected]; Stephen Hemminger <[email protected]>; David Miller <[email protected]>
Subject: RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Monday, August 28, 2017 11:16 AM
To: Eduardo Otubo <[email protected]>
Cc: [email protected]; [email protected];
Haiyang
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Zhang <[email protected]>; Stephen Hemminger <[email protected]>; David Miller <[email protected]>
Subject: Re: [PATCHv2] hv_set_ifconfig.sh double check before setting
ip
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 4 lines · On Mon, 28 Aug 2017 12:01:21 +0200 Eduardo Otubo <[email protected]> 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
Quoted 1 line
the interface ip. Sometimes the interface has already been
configured
Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
by
Quoted 2 lines
network daemon, in this case hv_set_ifconfig causes "RTNETLINK: file exists error"; in order to avoid this error this patch makes sure
double
Quoted 3 lines
checks the interface before trying anything.
Signed-off-by Eduardo Otubo <[email protected]>
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.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 1 line
KVP supports IP injection to enable VM migration. For this scenario, I think
we recommend that NM be
Quoted 2 lines
disabled.

Quoted 7 lines

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?

Given that this is just an example script to be customized on a per Distro basis, do you still see value in making this change upstream. In any case, as Olaf noted the delay will cause problems. One option would be to simply fail if network scripts are running. As you say, if you are making sure that the KVP daemon is only run after the network script have executed, this restriction (failing if the scripts are active) should not be an issue.

Regards,

K. Y