Re: [ovs-dev] [PATCH v2] ovs-lib: Keep internal interface ip during upgrade.

[Daniele Di Proietto]

On 22/07/2016 12:54, "Ben Pfaff"  wrote:

>On Tue, Jun 21, 2016 at 07:27:30PM -0700, Daniele Di Proietto wrote:
>> Commit 9b5422a98f81("ovs-lib: Try to call exit before killing.")
>> introduced a problem where internal interfaces are destroyed and
>> recreated, losing their IP address.
>
>Acked-by: Ben Pfaff 

Thanks for the review, pushed to master
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] ovs-lib: Keep internal interface ip during upgrade.

[Daniele Di Proietto]

2016-07-22 16:02 GMT-07:00 Daniele Di Proietto :

>
>
> 2016-06-22 9:52 GMT-07:00 Darrell Ball :
>
>> On Tue, Jun 21, 2016 at 7:27 PM, Daniele Di Proietto <
>> diproiet...@vmware.com
>> > wrote:
>>
>> > Commit 9b5422a98f81("ovs-lib: Try to call exit before killing.")
>> > introduced a problem where internal interfaces are destroyed and
>> > recreated, losing their IP address.
>> >
>> > Commit 9aad5a5a96ba("ovs-vswitchd: Preserve datapath ports across
>> > graceful shutdown.") fixed the problem by changing ovs-vswitchd
>> > to preserve the ports on `ovs-appctl exit`.  Unfortunately, this fix is
>> > not enough during upgrade from <= 2.5.0, where an old ovs-vswitchd is
>> > running (without the fix) and a new ovs-lib script is performing the
>> > restart.
>> >
>> > The problem seem to affect both RHEL and ubuntu.
>> >
>> > This commit fixes the upgrade by looking at the running daemon
>> > version and avoid using `ovs-appctl exit` if it's < 2.5.90.
>> >
>> > Suggested-by: Gurucharan Shetty 
>> > Signed-off-by: Daniele Di Proietto 
>>
>>
>> 1) Is it normal in this code base to embed specific version numbers in a
>> generic library file ?
>>
>
> v1 of this patch had the check in ovs-ctl, but we thought that other
> daemons might be affected (every daemon used to be killed during restart),
> so we moved it to ovs-lib
>
>
>> 2) If coming from < 2.5.90 then the problem that Commit 9b5422a98f81 was
>> trying to fix
>> will exist ?
>>
>
> On < 2.5.90 the problem already existed on restart and with this patch it
> will show once more during upgrade.
>
>
>>
>>In general, do you need to document this somewhere at user level (
>> install.md or somewhere else) ?
>>
>
> IMHO this is a problem of the system scripts, and with this commit the
> system scripts are fixed, so that the user doesn't need to worry about this.
>
>
We discussed this offline and I misunderstood what you meant, sorry.

You suggested to document that the problem fixed by 9b5422a98f81 affects
also update.

We agreed that's not important given that in 2.5.90 we made some backwards
incompatible changes for DPDK.

Thanks,

Daniele
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] ovs-lib: Keep internal interface ip during upgrade.

[Daniele Di Proietto]

2016-06-22 9:52 GMT-07:00 Darrell Ball :

> On Tue, Jun 21, 2016 at 7:27 PM, Daniele Di Proietto <
> diproiet...@vmware.com
> > wrote:
>
> > Commit 9b5422a98f81("ovs-lib: Try to call exit before killing.")
> > introduced a problem where internal interfaces are destroyed and
> > recreated, losing their IP address.
> >
> > Commit 9aad5a5a96ba("ovs-vswitchd: Preserve datapath ports across
> > graceful shutdown.") fixed the problem by changing ovs-vswitchd
> > to preserve the ports on `ovs-appctl exit`.  Unfortunately, this fix is
> > not enough during upgrade from <= 2.5.0, where an old ovs-vswitchd is
> > running (without the fix) and a new ovs-lib script is performing the
> > restart.
> >
> > The problem seem to affect both RHEL and ubuntu.
> >
> > This commit fixes the upgrade by looking at the running daemon
> > version and avoid using `ovs-appctl exit` if it's < 2.5.90.
> >
> > Suggested-by: Gurucharan Shetty 
> > Signed-off-by: Daniele Di Proietto 
>
>
> 1) Is it normal in this code base to embed specific version numbers in a
> generic library file ?
>

v1 of this patch had the check in ovs-ctl, but we thought that other
daemons might be affected (every daemon used to be killed during restart),
so we moved it to ovs-lib


> 2) If coming from < 2.5.90 then the problem that Commit 9b5422a98f81 was
> trying to fix
> will exist ?
>

On < 2.5.90 the problem already existed on restart and with this patch it
will show once more during upgrade.


>
>In general, do you need to document this somewhere at user level (
> install.md or somewhere else) ?
>

IMHO this is a problem of the system scripts, and with this commit the
system scripts are fixed, so that the user doesn't need to worry about this.

Thanks,

Daniele
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] ovs-lib: Keep internal interface ip during upgrade.

[Ben Pfaff]

On Tue, Jun 21, 2016 at 07:27:30PM -0700, Daniele Di Proietto wrote:
> Commit 9b5422a98f81("ovs-lib: Try to call exit before killing.")
> introduced a problem where internal interfaces are destroyed and
> recreated, losing their IP address.

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] ovs-lib: Keep internal interface ip during upgrade.

[Darrell Ball]

On Tue, Jun 21, 2016 at 7:27 PM, Daniele Di Proietto  wrote:

> Commit 9b5422a98f81("ovs-lib: Try to call exit before killing.")
> introduced a problem where internal interfaces are destroyed and
> recreated, losing their IP address.
>
> Commit 9aad5a5a96ba("ovs-vswitchd: Preserve datapath ports across
> graceful shutdown.") fixed the problem by changing ovs-vswitchd
> to preserve the ports on `ovs-appctl exit`.  Unfortunately, this fix is
> not enough during upgrade from <= 2.5.0, where an old ovs-vswitchd is
> running (without the fix) and a new ovs-lib script is performing the
> restart.
>
> The problem seem to affect both RHEL and ubuntu.
>
> This commit fixes the upgrade by looking at the running daemon
> version and avoid using `ovs-appctl exit` if it's < 2.5.90.
>
> Suggested-by: Gurucharan Shetty 
> Signed-off-by: Daniele Di Proietto 
> ---
> v1->v2:
> `if` condition was broken, plus `-a` and `-o` parameters for `test` are not
> portable.  This version uses a more general approach with awk.  Also, awk
> is
> now used to get the version string from ovs-appctl.
> ---
>  utilities/ovs-lib.in | 37 +
>  1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
> index 773efb3..7fcd734 100644
> --- a/utilities/ovs-lib.in
> +++ b/utilities/ovs-lib.in
> @@ -132,6 +132,22 @@ pid_comm_check () {
>  [ "$1" = "`cat /proc/$2/comm`" ]
>  }
>
> +# version_geq version_a version_b
> +#
> +# Compare (dot separated) version numbers.  Returns true (exit code 0) if
> +# version_a is greater or equal than version_b, otherwise false (exit
> code 1).
> +version_geq() {
> +echo $1 $2 | awk '{
> +n1 = split($1, a, ".");
> +n2 = split($2, b, ".");
> +n = (n1 > n2) ? n1 : n2;
> +for (i = 1; i <= n; i++) {
> +if (a[i]+0 < b[i]+0) exit 1
> +if (a[i]+0 > b[i]+0) exit 0
> +}
> +}'
> +}
> +
>  start_daemon () {
>  priority=$1
>  wrapper=$2
> @@ -202,10 +218,23 @@ start_daemon () {
>  stop_daemon () {
>  if test -e "$rundir/$1.pid"; then
>  if pid=`cat "$rundir/$1.pid"`; then
> -for action in EXIT .1 .25 .65 1 \
> -  TERM .1 .25 .65 1 1 1 1 \
> -  KILL 1 1 1 2 10 15 30 \
> -  FAIL; do
> +
> +graceful="EXIT .1 .25 .65 1"
> +actions="TERM .1 .25 .65 1 1 1 1 \
> + KILL 1 1 1 2 10 15 30 \
> + FAIL"
> +version=`ovs-appctl -T 1 -t $rundir/$1.$pid.ctl version \
> + | awk 'NR==1{print $NF}'`
> +
> +# Use `ovs-appctl exit` only if the running daemon version
> +# is >= 2.5.90.  This script might be used during upgrade to
> +# stop older versions of daemons which do not behave correctly
> +# with `ovs-appctl exit` (e.g. ovs-vswitchd <= 2.5.0 deletes
> +# internal ports).
> +if version_geq "$version" "2.5.90"; then
>


1) Is it normal in this code base to embed specific version numbers in a
generic library file ?

2) If coming from < 2.5.90 then the problem that Commit 9b5422a98f81 was
trying to fix
will exist ?

   In general, do you need to document this somewhere at user level (
install.md or somewhere else) ?



> +actions="$graceful $actions"
> +fi
> +for action in $actions; do
>  if pid_exists "$pid" >/dev/null 2>&1; then :; else
>  return 0
>  fi
> --
> 2.8.1
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] ovs-lib: Keep internal interface ip during upgrade.

[Guru Shetty]

On 21 June 2016 at 19:27, Daniele Di Proietto 
wrote:

> Commit 9b5422a98f81("ovs-lib: Try to call exit before killing.")
> introduced a problem where internal interfaces are destroyed and
> recreated, losing their IP address.
>
> Commit 9aad5a5a96ba("ovs-vswitchd: Preserve datapath ports across
> graceful shutdown.") fixed the problem by changing ovs-vswitchd
> to preserve the ports on `ovs-appctl exit`.  Unfortunately, this fix is
> not enough during upgrade from <= 2.5.0, where an old ovs-vswitchd is
> running (without the fix) and a new ovs-lib script is performing the
> restart.
>
> The problem seem to affect both RHEL and ubuntu.
>
> This commit fixes the upgrade by looking at the running daemon
> version and avoid using `ovs-appctl exit` if it's < 2.5.90.
>
> Suggested-by: Gurucharan Shetty 
> Signed-off-by: Daniele Di Proietto 
>

Not an awk expert to know about portability issues, but based on some
online manuals, this looks good to me.
Acked-by: Gurucharan Shetty 



> ---
> v1->v2:
> `if` condition was broken, plus `-a` and `-o` parameters for `test` are not
> portable.  This version uses a more general approach with awk.  Also, awk
> is
> now used to get the version string from ovs-appctl.
> ---
>  utilities/ovs-lib.in | 37 +
>  1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
> index 773efb3..7fcd734 100644
> --- a/utilities/ovs-lib.in
> +++ b/utilities/ovs-lib.in
> @@ -132,6 +132,22 @@ pid_comm_check () {
>  [ "$1" = "`cat /proc/$2/comm`" ]
>  }
>
> +# version_geq version_a version_b
> +#
> +# Compare (dot separated) version numbers.  Returns true (exit code 0) if
> +# version_a is greater or equal than version_b, otherwise false (exit
> code 1).
> +version_geq() {
> +echo $1 $2 | awk '{
> +n1 = split($1, a, ".");
> +n2 = split($2, b, ".");
> +n = (n1 > n2) ? n1 : n2;
> +for (i = 1; i <= n; i++) {
> +if (a[i]+0 < b[i]+0) exit 1
> +if (a[i]+0 > b[i]+0) exit 0
> +}
> +}'
> +}
> +
>  start_daemon () {
>  priority=$1
>  wrapper=$2
> @@ -202,10 +218,23 @@ start_daemon () {
>  stop_daemon () {
>  if test -e "$rundir/$1.pid"; then
>  if pid=`cat "$rundir/$1.pid"`; then
> -for action in EXIT .1 .25 .65 1 \
> -  TERM .1 .25 .65 1 1 1 1 \
> -  KILL 1 1 1 2 10 15 30 \
> -  FAIL; do
> +
> +graceful="EXIT .1 .25 .65 1"
> +actions="TERM .1 .25 .65 1 1 1 1 \
> + KILL 1 1 1 2 10 15 30 \
> + FAIL"
> +version=`ovs-appctl -T 1 -t $rundir/$1.$pid.ctl version \
> + | awk 'NR==1{print $NF}'`
> +
> +# Use `ovs-appctl exit` only if the running daemon version
> +# is >= 2.5.90.  This script might be used during upgrade to
> +# stop older versions of daemons which do not behave correctly
> +# with `ovs-appctl exit` (e.g. ovs-vswitchd <= 2.5.0 deletes
> +# internal ports).
> +if version_geq "$version" "2.5.90"; then
> +actions="$graceful $actions"
> +fi
> +for action in $actions; do
>  if pid_exists "$pid" >/dev/null 2>&1; then :; else
>  return 0
>  fi
> --
> 2.8.1
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2] ovs-lib: Keep internal interface ip during upgrade.

[Daniele Di Proietto]

Commit 9b5422a98f81("ovs-lib: Try to call exit before killing.")
introduced a problem where internal interfaces are destroyed and
recreated, losing their IP address.

Commit 9aad5a5a96ba("ovs-vswitchd: Preserve datapath ports across
graceful shutdown.") fixed the problem by changing ovs-vswitchd
to preserve the ports on `ovs-appctl exit`.  Unfortunately, this fix is
not enough during upgrade from <= 2.5.0, where an old ovs-vswitchd is
running (without the fix) and a new ovs-lib script is performing the
restart.

The problem seem to affect both RHEL and ubuntu.

This commit fixes the upgrade by looking at the running daemon
version and avoid using `ovs-appctl exit` if it's < 2.5.90.

Suggested-by: Gurucharan Shetty 
Signed-off-by: Daniele Di Proietto 
---
v1->v2:
`if` condition was broken, plus `-a` and `-o` parameters for `test` are not
portable.  This version uses a more general approach with awk.  Also, awk is
now used to get the version string from ovs-appctl.
---
 utilities/ovs-lib.in | 37 +
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
index 773efb3..7fcd734 100644
--- a/utilities/ovs-lib.in
+++ b/utilities/ovs-lib.in
@@ -132,6 +132,22 @@ pid_comm_check () {
 [ "$1" = "`cat /proc/$2/comm`" ]
 }
 
+# version_geq version_a version_b
+#
+# Compare (dot separated) version numbers.  Returns true (exit code 0) if
+# version_a is greater or equal than version_b, otherwise false (exit code 1).
+version_geq() {
+echo $1 $2 | awk '{
+n1 = split($1, a, ".");
+n2 = split($2, b, ".");
+n = (n1 > n2) ? n1 : n2;
+for (i = 1; i <= n; i++) {
+if (a[i]+0 < b[i]+0) exit 1
+if (a[i]+0 > b[i]+0) exit 0
+}
+}'
+}
+
 start_daemon () {
 priority=$1
 wrapper=$2
@@ -202,10 +218,23 @@ start_daemon () {
 stop_daemon () {
 if test -e "$rundir/$1.pid"; then
 if pid=`cat "$rundir/$1.pid"`; then
-for action in EXIT .1 .25 .65 1 \
-  TERM .1 .25 .65 1 1 1 1 \
-  KILL 1 1 1 2 10 15 30 \
-  FAIL; do
+
+graceful="EXIT .1 .25 .65 1"
+actions="TERM .1 .25 .65 1 1 1 1 \
+ KILL 1 1 1 2 10 15 30 \
+ FAIL"
+version=`ovs-appctl -T 1 -t $rundir/$1.$pid.ctl version \
+ | awk 'NR==1{print $NF}'`
+
+# Use `ovs-appctl exit` only if the running daemon version
+# is >= 2.5.90.  This script might be used during upgrade to
+# stop older versions of daemons which do not behave correctly
+# with `ovs-appctl exit` (e.g. ovs-vswitchd <= 2.5.0 deletes
+# internal ports).
+if version_geq "$version" "2.5.90"; then
+actions="$graceful $actions"
+fi
+for action in $actions; do
 if pid_exists "$pid" >/dev/null 2>&1; then :; else
 return 0
 fi
-- 
2.8.1

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev