All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv2] hv_set_ifconfig.sh double check before setting ip
@ 2017-08-28 10:01 Eduardo Otubo
  2017-08-28 13:47 ` Olaf Hering
  2017-08-28 15:16 ` Stephen Hemminger
  0 siblings, 2 replies; 8+ messages in thread
From: Eduardo Otubo @ 2017-08-28 10:01 UTC (permalink / raw)
  To: linux-kernel
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, devel,
	David Miller

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 <otubo@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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
  2017-08-28 10:01 [PATCHv2] hv_set_ifconfig.sh double check before setting ip Eduardo Otubo
@ 2017-08-28 13:47 ` Olaf Hering
  2017-08-28 15:16 ` Stephen Hemminger
  1 sibling, 0 replies; 8+ messages in thread
From: Olaf Hering @ 2017-08-28 13:47 UTC (permalink / raw)
  To: Eduardo Otubo
  Cc: linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
	Stephen Hemminger, devel, David Miller

[-- Attachment #1: Type: text/plain, Size: 162 bytes --]

On Mon, Aug 28, Eduardo Otubo wrote:

> +    sleep 30s;

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

Olaf

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
  2017-08-28 10:01 [PATCHv2] hv_set_ifconfig.sh double check before setting ip Eduardo Otubo
  2017-08-28 13:47 ` Olaf Hering
@ 2017-08-28 15:16 ` Stephen Hemminger
  2017-08-28 15:56   ` Haiyang Zhang
  1 sibling, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2017-08-28 15:16 UTC (permalink / raw)
  To: Eduardo Otubo
  Cc: linux-kernel, devel, Haiyang Zhang, Stephen Hemminger, David Miller

On Mon, 28 Aug 2017 12:01:21 +0200
Eduardo Otubo <otubo@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 <otubo@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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
  2017-08-28 15:16 ` Stephen Hemminger
@ 2017-08-28 15:56   ` Haiyang Zhang
  2017-08-28 16:05     ` Eduardo Otubo
  2017-08-28 16:48     ` KY Srinivasan
  0 siblings, 2 replies; 8+ messages in thread
From: Haiyang Zhang @ 2017-08-28 15:56 UTC (permalink / raw)
  To: Stephen Hemminger, Eduardo Otubo, KY Srinivasan
  Cc: linux-kernel, devel, Stephen Hemminger, David Miller



> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, August 28, 2017 11:16 AM
> To: Eduardo Otubo <otubo@redhat.com>
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; Haiyang
> Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; David Miller <davem@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 <otubo@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 <otubo@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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
  2017-08-28 15:56   ` Haiyang Zhang
@ 2017-08-28 16:05     ` Eduardo Otubo
  2017-08-28 16:48     ` KY Srinivasan
  1 sibling, 0 replies; 8+ messages in thread
From: Eduardo Otubo @ 2017-08-28 16:05 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: Stephen Hemminger, KY Srinivasan, linux-kernel, devel,
	Stephen Hemminger, David Miller

On Mon, Aug 28, 2017 at 03:56:44PM +0000, Haiyang Zhang wrote:
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Monday, August 28, 2017 11:16 AM
> > To: Eduardo Otubo <otubo@redhat.com>
> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; Haiyang
> > Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> > <sthemmin@microsoft.com>; David Miller <davem@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 <otubo@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 <otubo@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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
  2017-08-28 15:56   ` Haiyang Zhang
  2017-08-28 16:05     ` Eduardo Otubo
@ 2017-08-28 16:48     ` KY Srinivasan
  2017-08-31  8:16       ` Eduardo Otubo
  1 sibling, 1 reply; 8+ messages in thread
From: KY Srinivasan @ 2017-08-28 16:48 UTC (permalink / raw)
  To: Haiyang Zhang, Stephen Hemminger, Eduardo Otubo
  Cc: linux-kernel, devel, Stephen Hemminger, David Miller



> -----Original Message-----
> From: Haiyang Zhang
> Sent: Monday, August 28, 2017 8:57 AM
> To: Stephen Hemminger <stephen@networkplumber.org>; Eduardo Otubo
> <otubo@redhat.com>; KY Srinivasan <kys@microsoft.com>
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; Stephen
> Hemminger <sthemmin@microsoft.com>; David Miller
> <davem@davemloft.net>
> Subject: RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
> 
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Monday, August 28, 2017 11:16 AM
> > To: Eduardo Otubo <otubo@redhat.com>
> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; Haiyang
> > Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> > <sthemmin@microsoft.com>; David Miller <davem@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 <otubo@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 <otubo@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.

K. Y
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
  2017-08-28 16:48     ` KY Srinivasan
@ 2017-08-31  8:16       ` Eduardo Otubo
  2017-08-31 20:21         ` KY Srinivasan
  0 siblings, 1 reply; 8+ messages in thread
From: Eduardo Otubo @ 2017-08-31  8:16 UTC (permalink / raw)
  To: KY Srinivasan
  Cc: Haiyang Zhang, Stephen Hemminger, linux-kernel, devel,
	Stephen Hemminger, David Miller

On Mon, Aug 28, 2017 at 04:48:32PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Haiyang Zhang
> > Sent: Monday, August 28, 2017 8:57 AM
> > To: Stephen Hemminger <stephen@networkplumber.org>; Eduardo Otubo
> > <otubo@redhat.com>; KY Srinivasan <kys@microsoft.com>
> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; Stephen
> > Hemminger <sthemmin@microsoft.com>; David Miller
> > <davem@davemloft.net>
> > Subject: RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Monday, August 28, 2017 11:16 AM
> > > To: Eduardo Otubo <otubo@redhat.com>
> > > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; Haiyang
> > > Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> > > <sthemmin@microsoft.com>; David Miller <davem@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 <otubo@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 <otubo@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?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
  2017-08-31  8:16       ` Eduardo Otubo
@ 2017-08-31 20:21         ` KY Srinivasan
  0 siblings, 0 replies; 8+ messages in thread
From: KY Srinivasan @ 2017-08-31 20:21 UTC (permalink / raw)
  To: Eduardo Otubo
  Cc: Haiyang Zhang, Stephen Hemminger, linux-kernel, devel,
	Stephen Hemminger, David Miller



> -----Original Message-----
> From: Eduardo Otubo [mailto:otubo@redhat.com]
> Sent: Thursday, August 31, 2017 1:17 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> <stephen@networkplumber.org>; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; Stephen Hemminger
> <sthemmin@microsoft.com>; David Miller <davem@davemloft.net>
> Subject: Re: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
> 
> On Mon, Aug 28, 2017 at 04:48:32PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Haiyang Zhang
> > > Sent: Monday, August 28, 2017 8:57 AM
> > > To: Stephen Hemminger <stephen@networkplumber.org>; Eduardo
> Otubo
> > > <otubo@redhat.com>; KY Srinivasan <kys@microsoft.com>
> > > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; Stephen
> > > Hemminger <sthemmin@microsoft.com>; David Miller
> > > <davem@davemloft.net>
> > > Subject: RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Monday, August 28, 2017 11:16 AM
> > > > To: Eduardo Otubo <otubo@redhat.com>
> > > > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> Haiyang
> > > > Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> > > > <sthemmin@microsoft.com>; David Miller <davem@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 <otubo@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 <otubo@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?

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 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-08-31 20:21 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-28 10:01 [PATCHv2] hv_set_ifconfig.sh double check before setting ip Eduardo Otubo
2017-08-28 13:47 ` Olaf Hering
2017-08-28 15:16 ` Stephen Hemminger
2017-08-28 15:56   ` Haiyang Zhang
2017-08-28 16:05     ` Eduardo Otubo
2017-08-28 16:48     ` KY Srinivasan
2017-08-31  8:16       ` Eduardo Otubo
2017-08-31 20:21         ` KY Srinivasan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.