| Message ID | 1491016286-86218-3-git-send-email-nic@opencloud.tech |
|---|---|
| State | Changes Requested |
| Headers | show |
> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao <nic@opencloud.tech> wrote: > > When bridge stp enabled, we enable the stp ports despite > ports are down. When initializing, this patch checks > link-state of ports and enable or disable them according > to their link-state. This patch also allow user to enable > and disable a port when bridge stp is running. > This describes what the patch does but gives little help for understanding why this change is needed. STP would notice that the link is down as it is not able to exchange BPDUs over that link. Also, a link that is in STP_DISABLED state forwards all traffic, so that when the link comes up, but before stp_run() manages to enable STP there would be a loop in the network. To prevent this it seems to me that we should leave STP enabled also when the link goes down, so that STP would have the chance to initially block to port when it comes back up. Jarno > Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> > --- > ofproto/ofproto-dpif.c | 41 +++++++++++++++++++++++++++- > tests/stp.at | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 113 insertions(+), 1 deletion(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 4beacda..f015131 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -2488,6 +2488,37 @@ update_stp_port_state(struct ofport_dpif *ofport) > } > } > > +static void > +stp_check_and_update_link_state(struct ofproto_dpif *ofproto) > +{ > + struct ofport *ofport_; > + struct ofport_dpif *ofport; > + bool up; > + > + HMAP_FOR_EACH (ofport_, hmap_node, &ofproto->up.ports) { > + ofport = ofport_dpif_cast(ofport_); > + up = netdev_get_carrier(ofport_->netdev); > + > + if (ofport->stp_port && > + up != (stp_port_get_state(ofport->stp_port) != STP_DISABLED)) { > + > + VLOG_DBG("bridge: %s, port: %s is %s, %s it", ofproto->up.name, > + netdev_get_name(ofport->up.netdev), > + up ? "up" : "down", > + up ? "enabling" : "disabling"); > + > + if (up) { > + stp_port_enable(ofport->stp_port); > + stp_port_set_aux(ofport->stp_port, ofport); > + } else { > + stp_port_disable(ofport->stp_port); > + } > + > + update_stp_port_state(ofport); > + } > + } > +} > + > /* Configures STP on 'ofport_' using the settings defined in 's'. The > * caller is responsible for assigning STP port numbers and ensuring > * there are no duplicates. */ > @@ -2518,7 +2549,12 @@ set_stp_port(struct ofport *ofport_, > /* Set name before enabling the port so that debugging messages can print > * the name. */ > stp_port_set_name(sp, netdev_get_name(ofport->up.netdev)); > - stp_port_enable(sp); > + > + if (netdev_get_carrier(ofport_->netdev)) { > + stp_port_enable(sp); > + } else { > + stp_port_disable(sp); > + } > > stp_port_set_aux(sp, ofport); > stp_port_set_priority(sp, s->priority); > @@ -2580,6 +2616,9 @@ stp_run(struct ofproto_dpif *ofproto) > stp_tick(ofproto->stp, MIN(INT_MAX, elapsed)); > ofproto->stp_last_tick = now; > } > + > + stp_check_and_update_link_state(ofproto); > + > while (stp_get_changed_port(ofproto->stp, &sp)) { > struct ofport_dpif *ofport = stp_port_get_aux(sp); > > diff --git a/tests/stp.at b/tests/stp.at > index 98632a8..de8f971 100644 > --- a/tests/stp.at > +++ b/tests/stp.at > @@ -420,6 +420,8 @@ AT_CHECK([ovs-vsctl add-port br1 p8 -- \ > set port p8 other_config:stp-enable=false -- \ > ]) > > +ovs-appctl netdev-dummy/set-admin-state up > + > ovs-appctl time/stop > > AT_CHECK([ovs-ofctl add-flow br0 "in_port=7 icmp actions=1"]) > @@ -519,6 +521,7 @@ AT_CHECK([ > set interface p6 type=dummy options:pstream=punix:$OVS_RUNDIR/p6.sock ofport_request=6 > ], [0]) > > +ovs-appctl netdev-dummy/set-admin-state up > > ovs-appctl time/stop > > @@ -633,6 +636,8 @@ AT_CHECK([ > set interface p2 type=dummy ofport_request=2 > ], [0]) > > +ovs-appctl netdev-dummy/set-admin-state up > + > ovs-appctl time/stop > > # give time for STP to move initially > @@ -653,6 +658,8 @@ AT_CHECK([ > set interface p3 type=dummy ofport_request=3 > ], [0]) > > +ovs-appctl netdev-dummy/set-admin-state p3 up > + > # The new stp port should be a listening state and other > # stp ports keep forwarding. > AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl > @@ -676,5 +683,71 @@ AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl > p3 designated listening 19 128.3 > ]) > > +AT_CLEANUP > + > +AT_SETUP([STP - check link-state when stp is running]) > +OVS_VSWITCHD_START([]) > + > +AT_CHECK([ > + ovs-vsctl -- \ > + set port br0 other_config:stp-enable=false -- \ > + set bridge br0 datapath-type=dummy stp_enable=true \ > + other-config:hwaddr=aa:66:aa:66:00:00 > +], [0]) > + > +AT_CHECK([ > + ovs-vsctl add-port br0 p1 -- \ > + set interface p1 type=dummy ofport_request=1 > + ovs-vsctl add-port br0 p2 -- \ > + set interface p2 type=dummy ofport_request=2 > +], [0]) > + > +ovs-appctl netdev-dummy/set-admin-state up > + > +ovs-appctl time/stop > + > +# give time for STP to move initially > +for i in $(seq 0 30); do > + ovs-appctl time/warp 1000 > +done > + > +AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl > + p1 designated forwarding 19 128.1 > +]) > +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl > + p2 designated forwarding 19 128.2 > +]) > + > +# add a stp port > +AT_CHECK([ > + ovs-vsctl add-port br0 p3 -- \ > + set interface p3 type=dummy ofport_request=3 > +], [0]) > + > +ovs-appctl netdev-dummy/set-admin-state p3 down > + > +# We should not show the p3 because its link-state is down > +AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl > + p1 designated forwarding 19 128.1 > +]) > +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl > + p2 designated forwarding 19 128.2 > +]) > +AT_CHECK([ovs-appctl stp/show br0 | grep p3], [1], [dnl > +]) > + > +ovs-appctl netdev-dummy/set-admin-state p3 up > + > +AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl > + p1 designated forwarding 19 128.1 > +]) > +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl > + p2 designated forwarding 19 128.2 > +]) > +AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl > + p3 designated listening 19 128.3 > +]) > + > + > OVS_VSWITCHD_STOP > AT_CLEANUP > -- > 1.8.3.1 > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> On Apr 21, 2017, at 8:52 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > >> >> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao <nic@opencloud.tech <mailto:nic@opencloud.tech>> wrote: >> >> When bridge stp enabled, we enable the stp ports despite >> ports are down. When initializing, this patch checks >> link-state of ports and enable or disable them according >> to their link-state. This patch also allow user to enable >> and disable a port when bridge stp is running. >> > > This describes what the patch does but gives little help for understanding why this change is needed. STP would notice that the link is down as it is not able to exchange BPDUs over that link. Also, a link that is in STP_DISABLED state forwards all traffic, so that when the link comes up, but before stp_run() manages to enable STP there would be a loop in the network. To prevent this it seems to me that we should leave STP enabled also when the link goes down, so that STP would have the chance to initially block to port when it comes back up. > > Jarno Yes, STP would notice that the link is down as it is not able to exchange BPDUs over that link. If a link is down(e.g ifconfig eth0 down), it will not forward any traffic whenever stp port is STP_DISABLE or STP_ENABLE. And is there a loop in the network ? And if one interface is down, the command ‘ovs-appctl stp/show’ still show it(its role is designated and state is forwarding). The forwarding state of interface which is down, may confuse the users and developers. I tested stp on cisco switch. If you shutdown the stp interface, you cannot get the interface info and the interface linked to it.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 4beacda..f015131 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2488,6 +2488,37 @@ update_stp_port_state(struct ofport_dpif *ofport) } } +static void +stp_check_and_update_link_state(struct ofproto_dpif *ofproto) +{ + struct ofport *ofport_; + struct ofport_dpif *ofport; + bool up; + + HMAP_FOR_EACH (ofport_, hmap_node, &ofproto->up.ports) { + ofport = ofport_dpif_cast(ofport_); + up = netdev_get_carrier(ofport_->netdev); + + if (ofport->stp_port && + up != (stp_port_get_state(ofport->stp_port) != STP_DISABLED)) { + + VLOG_DBG("bridge: %s, port: %s is %s, %s it", ofproto->up.name, + netdev_get_name(ofport->up.netdev), + up ? "up" : "down", + up ? "enabling" : "disabling"); + + if (up) { + stp_port_enable(ofport->stp_port); + stp_port_set_aux(ofport->stp_port, ofport); + } else { + stp_port_disable(ofport->stp_port); + } + + update_stp_port_state(ofport); + } + } +} + /* Configures STP on 'ofport_' using the settings defined in 's'. The * caller is responsible for assigning STP port numbers and ensuring * there are no duplicates. */ @@ -2518,7 +2549,12 @@ set_stp_port(struct ofport *ofport_, /* Set name before enabling the port so that debugging messages can print * the name. */ stp_port_set_name(sp, netdev_get_name(ofport->up.netdev)); - stp_port_enable(sp); + + if (netdev_get_carrier(ofport_->netdev)) { + stp_port_enable(sp); + } else { + stp_port_disable(sp); + } stp_port_set_aux(sp, ofport); stp_port_set_priority(sp, s->priority); @@ -2580,6 +2616,9 @@ stp_run(struct ofproto_dpif *ofproto) stp_tick(ofproto->stp, MIN(INT_MAX, elapsed)); ofproto->stp_last_tick = now; } + + stp_check_and_update_link_state(ofproto); + while (stp_get_changed_port(ofproto->stp, &sp)) { struct ofport_dpif *ofport = stp_port_get_aux(sp); diff --git a/tests/stp.at b/tests/stp.at index 98632a8..de8f971 100644 --- a/tests/stp.at +++ b/tests/stp.at @@ -420,6 +420,8 @@ AT_CHECK([ovs-vsctl add-port br1 p8 -- \ set port p8 other_config:stp-enable=false -- \ ]) +ovs-appctl netdev-dummy/set-admin-state up + ovs-appctl time/stop AT_CHECK([ovs-ofctl add-flow br0 "in_port=7 icmp actions=1"]) @@ -519,6 +521,7 @@ AT_CHECK([ set interface p6 type=dummy options:pstream=punix:$OVS_RUNDIR/p6.sock ofport_request=6 ], [0]) +ovs-appctl netdev-dummy/set-admin-state up ovs-appctl time/stop @@ -633,6 +636,8 @@ AT_CHECK([ set interface p2 type=dummy ofport_request=2 ], [0]) +ovs-appctl netdev-dummy/set-admin-state up + ovs-appctl time/stop # give time for STP to move initially @@ -653,6 +658,8 @@ AT_CHECK([ set interface p3 type=dummy ofport_request=3 ], [0]) +ovs-appctl netdev-dummy/set-admin-state p3 up + # The new stp port should be a listening state and other # stp ports keep forwarding. AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl @@ -676,5 +683,71 @@ AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl p3 designated listening 19 128.3 ]) +AT_CLEANUP + +AT_SETUP([STP - check link-state when stp is running]) +OVS_VSWITCHD_START([]) + +AT_CHECK([ + ovs-vsctl -- \ + set port br0 other_config:stp-enable=false -- \ + set bridge br0 datapath-type=dummy stp_enable=true \ + other-config:hwaddr=aa:66:aa:66:00:00 +], [0]) + +AT_CHECK([ + ovs-vsctl add-port br0 p1 -- \ + set interface p1 type=dummy ofport_request=1 + ovs-vsctl add-port br0 p2 -- \ + set interface p2 type=dummy ofport_request=2 +], [0]) + +ovs-appctl netdev-dummy/set-admin-state up + +ovs-appctl time/stop + +# give time for STP to move initially +for i in $(seq 0 30); do + ovs-appctl time/warp 1000 +done + +AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl + p1 designated forwarding 19 128.1 +]) +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl + p2 designated forwarding 19 128.2 +]) + +# add a stp port +AT_CHECK([ + ovs-vsctl add-port br0 p3 -- \ + set interface p3 type=dummy ofport_request=3 +], [0]) + +ovs-appctl netdev-dummy/set-admin-state p3 down + +# We should not show the p3 because its link-state is down +AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl + p1 designated forwarding 19 128.1 +]) +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl + p2 designated forwarding 19 128.2 +]) +AT_CHECK([ovs-appctl stp/show br0 | grep p3], [1], [dnl +]) + +ovs-appctl netdev-dummy/set-admin-state p3 up + +AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl + p1 designated forwarding 19 128.1 +]) +AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl + p2 designated forwarding 19 128.2 +]) +AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl + p3 designated listening 19 128.3 +]) + + OVS_VSWITCHD_STOP AT_CLEANUP
When bridge stp enabled, we enable the stp ports despite ports are down. When initializing, this patch checks link-state of ports and enable or disable them according to their link-state. This patch also allow user to enable and disable a port when bridge stp is running. Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> --- ofproto/ofproto-dpif.c | 41 +++++++++++++++++++++++++++- tests/stp.at | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-)