Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness
No, we don't add features to release branches. On Wed, Apr 26, 2017 at 04:10:36PM +, László Sürü wrote: > Thanks for the bfd update! > > Just one more question, > Is the backporting of liveness propagation to OVS 2.7 branch planned also? > > Thanks and regards > Laszlo > > -Original Message- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Friday, April 21, 2017 7:39 PM > To: László Sürü > Cc: ovs-dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as > link aliveness > > Thanks for the revision. > > I still saw the failures, so I spent some time looking at why. It turned out > to be related to the two different database transactions used to configure > BFD. When I combined the two transactions into one, e.g. changed > > AT_CHECK([ovs-vsctl add-br br1 -- \ > set bridge br1 datapath-type=dummy \ > other-config:hwaddr=aa:55:aa:56:00:00 -- \ > add-port br1 p1 -- set Interface p1 type=patch \ > options:peer=p0 ofport_request=2 -- \ > add-port br0 p0 -- set Interface p0 type=patch \ > options:peer=p1 ofport_request=1]) > > AT_CHECK([ovs-vsctl \ > set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 > -- \ > set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100]) > > into > > AT_CHECK([ovs-vsctl add-br br1 -- \ > set bridge br1 datapath-type=dummy \ > other-config:hwaddr=aa:55:aa:56:00:00 -- \ > add-port br1 p1 -- set Interface p1 type=patch \ > options:peer=p0 ofport_request=2 -- \ > add-port br0 p0 -- set Interface p0 type=patch \ > options:peer=p1 ofport_request=1 -- \ > set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 > -- \ > set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100]) > > I no longer saw the failures. > > I made that change in each of the BFD tests. I also got rid of the use of > the GNU grep --no-group-separator option, which BSD doesn't appear to support. > > With those changes, I applied this to master. Thanks again! > > On Thu, Apr 20, 2017 at 03:41:57PM +, László Sürü wrote: > > Thanks for the review and comments. > > All unit tests were passing for me before with 'make check', I haven't seen > > failing unit tests. > > It might have been a temporary misbehavior. > > > > I've applied the proposed changes and successfully rerun the tests > > also with 'testsuite' this time based on your attached log (see attachment). > > Also, I've adapted the tests to the new OF 1.6 test as well. > > > > Please find the modified patched below in this mail. > > > > Thanks and regards > > Laszlo > > > > -Original Message- > > From: Ben Pfaff [mailto:b...@ovn.org] > > Sent: Saturday, April 15, 2017 6:44 AM > > To: László Sürü > > Cc: ovs-dev@openvswitch.org > > Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable > > flag as link aliveness > > > > Thanks a lot for the revised patch! It will be good to get this done > > properly. > > > > I'm appending some changes that I suggest folding in to better match the > > usual OVS coding style. > > > > However, when I run the testsuite, I get the failures below. Do you see > > these too? Can you take a look at them? I'm also attaching the full > > testsuite.log in case it helps. > > > > bfd > > > > 22: bfd - liveness propagation - OF1.3 FAILED (bfd.at:847) > > 23: bfd - liveness propagation - OF1.4 FAILED (bfd.at:920) > > 24: bfd - liveness propagation - OF1.5 FAILED (bfd.at:993) > > > > ofproto > > > > 890: ofproto - mod-port (OpenFlow 1.6) FAILED > > (ofproto.at:1308) > > > > --8<--cut here-->8-- > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index > > 218e8eb..c82640d 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -1894,6 +1894,16 @@ port_modified(struct ofport *port_) > > bfd_set_netdev(port->bfd, netdev); > > } > > > > +/* Set liveness, unless the link is administratively or > > + * operationally down or link monitoring false */ > > +if (!(port->up.pp.config & OFPUTIL_PC_PORT_DOW
Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness
Thanks for the bfd update!
Just one more question,
Is the backporting of liveness propagation to OVS 2.7 branch planned also?
Thanks and regards
Laszlo
-Original Message-
From: Ben Pfaff [mailto:b...@ovn.org]
Sent: Friday, April 21, 2017 7:39 PM
To: László Sürü
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link
aliveness
Thanks for the revision.
I still saw the failures, so I spent some time looking at why. It turned out
to be related to the two different database transactions used to configure BFD.
When I combined the two transactions into one, e.g. changed
AT_CHECK([ovs-vsctl add-br br1 -- \
set bridge br1 datapath-type=dummy \
other-config:hwaddr=aa:55:aa:56:00:00 -- \
add-port br1 p1 -- set Interface p1 type=patch \
options:peer=p0 ofport_request=2 -- \
add-port br0 p0 -- set Interface p0 type=patch \
options:peer=p1 ofport_request=1])
AT_CHECK([ovs-vsctl \
set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 --
\
set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])
into
AT_CHECK([ovs-vsctl add-br br1 -- \
set bridge br1 datapath-type=dummy \
other-config:hwaddr=aa:55:aa:56:00:00 -- \
add-port br1 p1 -- set Interface p1 type=patch \
options:peer=p0 ofport_request=2 -- \
add-port br0 p0 -- set Interface p0 type=patch \
options:peer=p1 ofport_request=1 -- \
set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 --
\
set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])
I no longer saw the failures.
I made that change in each of the BFD tests. I also got rid of the use of the
GNU grep --no-group-separator option, which BSD doesn't appear to support.
With those changes, I applied this to master. Thanks again!
On Thu, Apr 20, 2017 at 03:41:57PM +, László Sürü wrote:
> Thanks for the review and comments.
> All unit tests were passing for me before with 'make check', I haven't seen
> failing unit tests.
> It might have been a temporary misbehavior.
>
> I've applied the proposed changes and successfully rerun the tests
> also with 'testsuite' this time based on your attached log (see attachment).
> Also, I've adapted the tests to the new OF 1.6 test as well.
>
> Please find the modified patched below in this mail.
>
> Thanks and regards
> Laszlo
>
> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Saturday, April 15, 2017 6:44 AM
> To: László Sürü
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable
> flag as link aliveness
>
> Thanks a lot for the revised patch! It will be good to get this done
> properly.
>
> I'm appending some changes that I suggest folding in to better match the
> usual OVS coding style.
>
> However, when I run the testsuite, I get the failures below. Do you see
> these too? Can you take a look at them? I'm also attaching the full
> testsuite.log in case it helps.
>
> bfd
>
> 22: bfd - liveness propagation - OF1.3 FAILED (bfd.at:847)
> 23: bfd - liveness propagation - OF1.4 FAILED (bfd.at:920)
> 24: bfd - liveness propagation - OF1.5 FAILED (bfd.at:993)
>
> ofproto
>
> 890: ofproto - mod-port (OpenFlow 1.6) FAILED (ofproto.at:1308)
>
> --8<--cut here-->8--
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index
> 218e8eb..c82640d 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1894,6 +1894,16 @@ port_modified(struct ofport *port_)
> bfd_set_netdev(port->bfd, netdev);
> }
>
> +/* Set liveness, unless the link is administratively or
> + * operationally down or link monitoring false */
> +if (!(port->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
> +!(port->up.pp.state & OFPUTIL_PS_LINK_DOWN) &&
> +port->may_enable) {
> +port->up.pp.state |= OFPUTIL_PS_LIVE;
> +} else {
> +port->up.pp.state &= ~OFPUTIL_PS_LIVE;
> +}
> +
> ofproto_dpif_monitor_port_update(port, port->bfd, port->cfm,
> port->lldp,
> &port->up.pp.hw_addr);
>
> @@ -3457,6 +3467,19 @@ port_run(struct ofport_dpif *ofport)
> if (ofport->rstp_port) {
> rstp_port_set_mac_operational(ofport->rstp_port, enable);
> }
> +
> +/* Propagate liveness, unless the lin
Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness
Thanks for the revision.
I still saw the failures, so I spent some time looking at why. It
turned out to be related to the two different database transactions used
to configure BFD. When I combined the two transactions into one,
e.g. changed
AT_CHECK([ovs-vsctl add-br br1 -- \
set bridge br1 datapath-type=dummy \
other-config:hwaddr=aa:55:aa:56:00:00 -- \
add-port br1 p1 -- set Interface p1 type=patch \
options:peer=p0 ofport_request=2 -- \
add-port br0 p0 -- set Interface p0 type=patch \
options:peer=p1 ofport_request=1])
AT_CHECK([ovs-vsctl \
set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 --
\
set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])
into
AT_CHECK([ovs-vsctl add-br br1 -- \
set bridge br1 datapath-type=dummy \
other-config:hwaddr=aa:55:aa:56:00:00 -- \
add-port br1 p1 -- set Interface p1 type=patch \
options:peer=p0 ofport_request=2 -- \
add-port br0 p0 -- set Interface p0 type=patch \
options:peer=p1 ofport_request=1 -- \
set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 --
\
set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])
I no longer saw the failures.
I made that change in each of the BFD tests. I also got rid of the use
of the GNU grep --no-group-separator option, which BSD doesn't appear to
support.
With those changes, I applied this to master. Thanks again!
On Thu, Apr 20, 2017 at 03:41:57PM +, László Sürü wrote:
> Thanks for the review and comments.
> All unit tests were passing for me before with 'make check', I haven't seen
> failing unit tests.
> It might have been a temporary misbehavior.
>
> I've applied the proposed changes and successfully rerun the tests
> also with 'testsuite' this time based on your attached log (see attachment).
> Also, I've adapted the tests to the new OF 1.6 test as well.
>
> Please find the modified patched below in this mail.
>
> Thanks and regards
> Laszlo
>
> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Saturday, April 15, 2017 6:44 AM
> To: László Sürü
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as
> link aliveness
>
> Thanks a lot for the revised patch! It will be good to get this done
> properly.
>
> I'm appending some changes that I suggest folding in to better match the
> usual OVS coding style.
>
> However, when I run the testsuite, I get the failures below. Do you see
> these too? Can you take a look at them? I'm also attaching the full
> testsuite.log in case it helps.
>
> bfd
>
> 22: bfd - liveness propagation - OF1.3 FAILED (bfd.at:847)
> 23: bfd - liveness propagation - OF1.4 FAILED (bfd.at:920)
> 24: bfd - liveness propagation - OF1.5 FAILED (bfd.at:993)
>
> ofproto
>
> 890: ofproto - mod-port (OpenFlow 1.6) FAILED (ofproto.at:1308)
>
> --8<--cut here-->8--
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 218e8eb..c82640d 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1894,6 +1894,16 @@ port_modified(struct ofport *port_)
> bfd_set_netdev(port->bfd, netdev);
> }
>
> +/* Set liveness, unless the link is administratively or
> + * operationally down or link monitoring false */
> +if (!(port->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
> +!(port->up.pp.state & OFPUTIL_PS_LINK_DOWN) &&
> +port->may_enable) {
> +port->up.pp.state |= OFPUTIL_PS_LIVE;
> +} else {
> +port->up.pp.state &= ~OFPUTIL_PS_LIVE;
> +}
> +
> ofproto_dpif_monitor_port_update(port, port->bfd, port->cfm,
> port->lldp, &port->up.pp.hw_addr);
>
> @@ -3457,6 +3467,19 @@ port_run(struct ofport_dpif *ofport)
> if (ofport->rstp_port) {
> rstp_port_set_mac_operational(ofport->rstp_port, enable);
> }
> +
> +/* Propagate liveness, unless the link is administratively or
> + * operationally down. */
> +if (!(ofport->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
> +!(ofport->up.pp.state & OFPUTIL_PS_LINK_DOWN)) {
> +enum ofputil_port_state of_state = ofport->up.pp.state;
> +if (enable) {
> +of_state |= OFPUTIL_PS_LIVE;
> +}
Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness
Thanks for the review and comments.
All unit tests were passing for me before with 'make check', I haven't seen
failing unit tests.
It might have been a temporary misbehavior.
I've applied the proposed changes and successfully rerun the tests
also with 'testsuite' this time based on your attached log (see attachment).
Also, I've adapted the tests to the new OF 1.6 test as well.
Please find the modified patched below in this mail.
Thanks and regards
Laszlo
-Original Message-
From: Ben Pfaff [mailto:b...@ovn.org]
Sent: Saturday, April 15, 2017 6:44 AM
To: László Sürü
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link
aliveness
Thanks a lot for the revised patch! It will be good to get this done properly.
I'm appending some changes that I suggest folding in to better match the usual
OVS coding style.
However, when I run the testsuite, I get the failures below. Do you see these
too? Can you take a look at them? I'm also attaching the full testsuite.log
in case it helps.
bfd
22: bfd - liveness propagation - OF1.3 FAILED (bfd.at:847)
23: bfd - liveness propagation - OF1.4 FAILED (bfd.at:920)
24: bfd - liveness propagation - OF1.5 FAILED (bfd.at:993)
ofproto
890: ofproto - mod-port (OpenFlow 1.6) FAILED (ofproto.at:1308)
--8<--cut here-->8--
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 218e8eb..c82640d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1894,6 +1894,16 @@ port_modified(struct ofport *port_)
bfd_set_netdev(port->bfd, netdev);
}
+/* Set liveness, unless the link is administratively or
+ * operationally down or link monitoring false */
+if (!(port->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
+!(port->up.pp.state & OFPUTIL_PS_LINK_DOWN) &&
+port->may_enable) {
+port->up.pp.state |= OFPUTIL_PS_LIVE;
+} else {
+port->up.pp.state &= ~OFPUTIL_PS_LIVE;
+}
+
ofproto_dpif_monitor_port_update(port, port->bfd, port->cfm,
port->lldp, &port->up.pp.hw_addr);
@@ -3457,6 +3467,19 @@ port_run(struct ofport_dpif *ofport)
if (ofport->rstp_port) {
rstp_port_set_mac_operational(ofport->rstp_port, enable);
}
+
+/* Propagate liveness, unless the link is administratively or
+ * operationally down. */
+if (!(ofport->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
+!(ofport->up.pp.state & OFPUTIL_PS_LINK_DOWN)) {
+enum ofputil_port_state of_state = ofport->up.pp.state;
+if (enable) {
+of_state |= OFPUTIL_PS_LIVE;
+} else {
+of_state &= ~OFPUTIL_PS_LIVE;
+}
+ofproto_port_set_state(&ofport->up, of_state);
+}
}
ofport->may_enable = enable;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 7440d5b..d41e35a 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2473,9 +2473,6 @@ ofport_modified(struct ofport *port, struct
ofputil_phy_port *pp)
port->pp.peer = pp->peer;
port->pp.curr_speed = pp->curr_speed;
port->pp.max_speed = pp->max_speed;
-
-connmgr_send_port_status(port->ofproto->connmgr, NULL,
- &port->pp, OFPPR_MODIFY);
}
/* Update OpenFlow 'state' in 'port' and notify controller. */
@@ -2633,7 +2630,8 @@ update_port(struct ofproto *ofproto, const char *name)
struct netdev *old_netdev = port->netdev;
/* 'name' hasn't changed location. Any properties changed? */
-if (!ofport_equal(&port->pp, &pp)) {
+bool port_changed = !ofport_equal(&port->pp, &pp);
+if (port_changed) {
ofport_modified(port, &pp);
}
@@ -2649,6 +2647,12 @@ update_port(struct ofproto *ofproto, const char *name)
port->ofproto->ofproto_class->port_modified(port);
}
+/* Send status update, if any port property changed */
+if (port_changed) {
+connmgr_send_port_status(port->ofproto->connmgr, NULL,
+ &port->pp, OFPPR_MODIFY);
+}
+
netdev_close(old_netdev);
} else {
/* If 'port' is nonnull then its name differs from 'name' and thus
diff --git a/tests/bfd.at b/tests/bfd.at
index dee8124..81c7db2 100644
--- a/tests/bfd.at
+++ b/tests/bfd.at
@@ -830,3 +830,222 @@ BFD_CHECK([p1], [false], [false], [none], [down],
[Control Detection Time Expire
OVS_VSWITCHD_
Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness
Thanks for the comments! I've applied the recommended style.
Indeed, ofport_modified() in the first patch may reported wrong status - even
if only for a short time.
To avoid this, liveness handling at port state change has been moved out to
port_modified() instead and
port status update (connmgr_send_port_status()) is called only after that by
update_port().
Please find the modified patch below - working fine for me.
Thanks and regards
Laszlo
-Original Message-
From: Ben Pfaff [mailto:b...@ovn.org]
Sent: Friday, March 17, 2017 7:21 PM
To: László Sürü
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link
aliveness
On Fri, Mar 03, 2017 at 01:00:05PM +, László Sürü wrote:
> hereby I'm sending the implementation of link liveness propagation
> functionality including the additional functionalities requested last year
> (last activity at Wed Mar 16 23:13:24 UTC 2016).
>
> The idea is to use OFPPS_LIVE bit to propagate link aliveness state towards
> the controller also when sending port status.
> The ofport->may_enable flag could be used for this purpose, thus any change
> in LIVE bit is propagated towards conrtoller in OFPT_PORT_STATUS message.
> OFPPS_LIVE bit is set only when links is not down not administratively,
> neither operationally as recommended in OF papers.
> I added 9 new unit tests to verify link state changes when monitored with
> cfm, bfd or lacp for OF 1.3, OF 1.4 and OF 1.5.
> I updated related unit tests according to the changes of ofproto-dpif.
Thanks a lot for the revision!
I am a little concerned about ofport_modified(). It seems like this does not
honor the status of protocols like BFD or CFM. I think this means that, if a
port changes for any reason, liveness will initially ignore the status of these
protocols, until the next time their status changes. That is probably not a
good situation. Do you have any comments on that?
I'm passing along an incremental I recommend folding into the next revision of
the patch, to make it better fit the usual OVS coding style.
(I do hope that the next revision will take less than a year!)
Thanks,
Ben.
--8<--cut here-->8--
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 523adad..95b283a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1894,6 +1894,16 @@ port_modified(struct ofport *port_)
bfd_set_netdev(port->bfd, netdev);
}
+/* Set liveness, unless the link is administratively or
+ * operationally down or link monitoring false */
+if (!(port->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
+!(port->up.pp.state & OFPUTIL_PS_LINK_DOWN) &&
+(port->may_enable)) {
+port->up.pp.state |= OFPUTIL_PS_LIVE;
+} else {
+port->up.pp.state &= ~OFPUTIL_PS_LIVE;
+}
+
ofproto_dpif_monitor_port_update(port, port->bfd, port->cfm,
port->lldp, &port->up.pp.hw_addr);
@@ -3457,6 +3467,19 @@ port_run(struct ofport_dpif *ofport)
if (ofport->rstp_port) {
rstp_port_set_mac_operational(ofport->rstp_port, enable);
}
+
+/* Propagate liveness, unless the link is administratively or
+ * operationally down. */
+if (!(ofport->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
+!(ofport->up.pp.state & OFPUTIL_PS_LINK_DOWN)) {
+enum ofputil_port_state of_state = ofport->up.pp.state;
+if (enable) {
+of_state |= OFPUTIL_PS_LIVE;
+} else {
+of_state &= ~OFPUTIL_PS_LIVE;
+}
+ofproto_port_set_state(&ofport->up, of_state);
+}
}
ofport->may_enable = enable;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 84ea95b..6db9ec6 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2470,9 +2470,6 @@ ofport_modified(struct ofport *port, struct
ofputil_phy_port *pp)
port->pp.peer = pp->peer;
port->pp.curr_speed = pp->curr_speed;
port->pp.max_speed = pp->max_speed;
-
-connmgr_send_port_status(port->ofproto->connmgr, NULL,
- &port->pp, OFPPR_MODIFY);
}
/* Update OpenFlow 'state' in 'port' and notify controller. */
@@ -2616,6 +2613,7 @@ update_port(struct ofproto *ofproto, const char *name)
struct netdev *netdev;
struct ofport *port;
int error = 0;
+bool port_changed;
COVERAGE_INC(ofproto_update_port);
@@ -2630,7 +2628,7 @@ update_port(struct ofproto *ofproto, const char *name)
struct netdev *old_netdev = port->netdev;
/* 'name' hasn't changed location. Any properties changed? */
-if
Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness
On Fri, Mar 03, 2017 at 01:00:05PM +, László Sürü wrote:
> hereby I'm sending the implementation of link liveness propagation
> functionality
> including the additional functionalities requested last year (last activity
> at Wed Mar 16 23:13:24 UTC 2016).
>
> The idea is to use OFPPS_LIVE bit to propagate link aliveness state towards
> the controller also when sending port status.
> The ofport->may_enable flag could be used for this purpose, thus any change
> in LIVE bit is propagated towards conrtoller in OFPT_PORT_STATUS message.
> OFPPS_LIVE bit is set only when links is not down not administratively,
> neither operationally as recommended in OF papers.
> I added 9 new unit tests to verify link state changes when monitored with
> cfm, bfd or lacp for OF 1.3, OF 1.4 and OF 1.5.
> I updated related unit tests according to the changes of ofproto-dpif.
Thanks a lot for the revision!
I am a little concerned about ofport_modified(). It seems like this
does not honor the status of protocols like BFD or CFM. I think this
means that, if a port changes for any reason, liveness will initially
ignore the status of these protocols, until the next time their status
changes. That is probably not a good situation. Do you have any
comments on that?
I'm passing along an incremental I recommend folding into the next
revision of the patch, to make it better fit the usual OVS coding style.
(I do hope that the next revision will take less than a year!)
Thanks,
Ben.
--8<--cut here-->8--
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index b3f3a56921d3..450123a043a2 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3453,7 +3453,6 @@ port_run(struct ofport_dpif *ofport)
if (ofport->may_enable != enable) {
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
-enum ofputil_port_state of_state;
ofproto->backer->need_revalidate = REV_PORT_TOGGLED;
@@ -3461,13 +3460,16 @@ port_run(struct ofport_dpif *ofport)
rstp_port_set_mac_operational(ofport->rstp_port, enable);
}
-/* liveness is propagated only,
- * when link is nor administratively neither operatively down */
+/* Propagate liveness, unless the link is administratively or
+ * operationally down. */
if (!(ofport->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
!(ofport->up.pp.state & OFPUTIL_PS_LINK_DOWN)) {
-of_state = ofport->up.pp.state;
-(enable) ? (of_state |= OFPUTIL_PS_LIVE)
- : (of_state &= ~OFPUTIL_PS_LIVE);
+enum ofputil_port_state of_state = ofport->up.pp.state;
+if (enable) {
+of_state |= OFPUTIL_PS_LIVE;
+} else {
+of_state &= ~OFPUTIL_PS_LIVE;
+}
ofproto_port_set_state(&ofport->up, of_state);
}
}
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 65cb558355bc..3bb88dcb270a 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2464,7 +2464,7 @@ ofport_modified(struct ofport *port, struct
ofputil_phy_port *pp)
| (pp->config & OFPUTIL_PC_PORT_DOWN));
port->pp.state = ((port->pp.state & ~OFPUTIL_PS_LINK_DOWN)
| (pp->state & OFPUTIL_PS_LINK_DOWN));
-/* set LIVE bit aligned with PORT_DOWN and LINK_DOWN bits */
+/* Initialize LIVE bit aligned with PORT_DOWN and LINK_DOWN bits. */
if ((port->pp.config & OFPUTIL_PC_PORT_DOWN) ||
(port->pp.state & OFPUTIL_PS_LINK_DOWN)) {
port->pp.state &= ~OFPUTIL_PS_LIVE;
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness
Hi,
hereby I'm sending the implementation of link liveness propagation
functionality
including the additional functionalities requested last year (last activity at
Wed Mar 16 23:13:24 UTC 2016).
The idea is to use OFPPS_LIVE bit to propagate link aliveness state towards the
controller also when sending port status.
The ofport->may_enable flag could be used for this purpose, thus any change in
LIVE bit is propagated towards conrtoller in OFPT_PORT_STATUS message.
OFPPS_LIVE bit is set only when links is not down not administratively, neither
operationally as recommended in OF papers.
I added 9 new unit tests to verify link state changes when monitored with cfm,
bfd or lacp for OF 1.3, OF 1.4 and OF 1.5.
I updated related unit tests according to the changes of ofproto-dpif.
Signed-off-by: László Sürü
Co-authored-by: Zoltán Balogh
Signed-off-by: Zoltán Balogh
Co-authored-by: Jan Scheurich
Signed-off-by: Jan Scheurich
---
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 89c7b7f..0b8b5d6 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3325,12 +3325,23 @@ port_run(struct ofport_dpif *ofport)
if (ofport->may_enable != enable) {
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
+enum ofputil_port_state of_state;
ofproto->backer->need_revalidate = REV_PORT_TOGGLED;
if (ofport->rstp_port) {
rstp_port_set_mac_operational(ofport->rstp_port, enable);
}
+
+/* liveness is propagated only,
+ * when link is nor administratively neither operatively down */
+if (!(ofport->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
+!(ofport->up.pp.state & OFPUTIL_PS_LINK_DOWN)) {
+of_state = ofport->up.pp.state;
+(enable) ? (of_state |= OFPUTIL_PS_LIVE)
+ : (of_state &= ~OFPUTIL_PS_LIVE);
+ofproto_port_set_state(&ofport->up, of_state);
+}
}
ofport->may_enable = enable;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index ef4b1d9..06dd9a7 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2462,6 +2462,13 @@ ofport_modified(struct ofport *port, struct
ofputil_phy_port *pp)
| (pp->config & OFPUTIL_PC_PORT_DOWN));
port->pp.state = ((port->pp.state & ~OFPUTIL_PS_LINK_DOWN)
| (pp->state & OFPUTIL_PS_LINK_DOWN));
+/* set LIVE bit aligned with PORT_DOWN and LINK_DOWN bits */
+if ((port->pp.config & OFPUTIL_PC_PORT_DOWN) ||
+(port->pp.state & OFPUTIL_PS_LINK_DOWN)) {
+port->pp.state &= ~OFPUTIL_PS_LIVE;
+} else {
+port->pp.state |= OFPUTIL_PS_LIVE;
+}
port->pp.curr = pp->curr;
port->pp.advertised = pp->advertised;
port->pp.supported = pp->supported;
diff --git a/tests/bfd.at b/tests/bfd.at
index 46a507a..74ce0e6 100644
--- a/tests/bfd.at
+++ b/tests/bfd.at
@@ -794,3 +794,222 @@ BFD_VSCTL_LIST_IFACE([p1], ["s/^.*flap_count=\(.*\),
forwarding.*$/\1/p"], ["1"]
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+# test bfd: liveness propagation - OF1.3.
+AT_SETUP([bfd - liveness propagation - OF1.3])
+OVS_VSWITCHD_START
+AT_CHECK([ovs-ofctl -O OpenFlow13 -P standard monitor br0 --detach --no-chdir
--pidfile])
+check_liveness () {
+printf '\n\n--- check_liveness %d ---\n\n\n' $1
+shift
+
+ echo >>expout "OFPT_PORT_STATUS (OF1.3): MOD: 1(p0): addr:
+ config: 0
+ state: $1
+ speed: 0 Mbps now, 0 Mbps max"
+
+ AT_CHECK(
+ [[sed '
+s/ (xid=0x[0-9a-fA-F]*)//
+s/ *duration.*//
+s/addr:[0-9a-fA-F:]*/addr:/' < monitor.log|grep --no-group-separator -A3 "MOD:
1(p0)"]],
+ [0], [expout])
+}
+: > expout
+ovs-appctl -t ovs-ofctl ofctl/barrier
+ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
+# Set miss_send_len to 128, enabling port_status messages to our service
connection.
+ovs-appctl -t ovs-ofctl ofctl/send 0409000c012345670080
+#Create 2 bridges connected by patch ports and enable bfd
+AT_CHECK([ovs-vsctl add-br br1 -- \
+ set bridge br1 datapath-type=dummy \
+ other-config:hwaddr=aa:55:aa:56:00:00 -- \
+ add-port br1 p1 -- set Interface p1 type=patch \
+ options:peer=p0 ofport_request=2 -- \
+ add-port br0 p0 -- set Interface p0 type=patch \
+ options:peer=p1 ofport_request=1])
+
+AT_CHECK([ovs-vsctl \
+ set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 -- \
+ set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])
+
+ovs-appctl time/stop
+# Disable the stats update to prevent the race between ovsdb updating
+# stats and ovs-vsctl cmd closing the jsonrpc session.
+AT_CHECK([ovs-vsctl set Open_vSwitch .
other_config:stats-update-interval=5000])
+
+# wait for a while to stablize bfd.
+ovs-appctl time/warp 10100 100
+BFD_CHECK([p0], [true], [false], [none], [up], [No Diagnostic], [none], [up],
[No Diagnostic])
+BFD_CHECK([p1], [true], [false], [none],
