On Tue, Mar 20, 2018 at 4:33 AM, Han Zhou <[email protected]> wrote: > > > On Mon, Mar 19, 2018 at 12:52 PM, <[email protected]> wrote: > > > > From: Numan Siddique <[email protected]> > > > > ovn-controller is not considering Port_Binding option "requested-chassis" > > when adding flows in table 0 and table 65. This patch adds this check. > > Hi Numan, > > The check looks good, but sorry that I didn't understand the problem. E.g. > there are 2 host A and B, and lsp1 is set as iface-id on both hosts. When > requested-chassis for lsp1 is A, then in SB DB the port-binding should be > on A. Then in physical.c when processing the port binding for lsp1, it > should always point to host A. So how could the VIF on B be reachable? > Could you help explain a little bit? > > Sorry. I should have updated the commit message with more details. So I sent out a v2 with the updated commit message - https://patchwork.ozlabs.org/patch/888106/ In this case what is happening is that ovn-controller on A is updating the lsp1's Port_Binding.chassis to A which is as expected and it also considers lsp1 is residing on it. But ovn-controller running on B is also adding the OF flows in table 0 and table 65 for lsp1's vif interface instead of considering lsp1 as a remote port. This is because the simap "localvif_to_ofport" will have an entry for lsp1 and the code here never gets hit - https://github.com/openvswitch/ovs/blob/master/ovn/controller/physical.c#L480 and instead this gets hit - https://github.com/openvswitch/ovs/blob/master/ovn/controller/physical.c#L507 .
You could run the updated test in this patch without the fix and can see the failures. Thanks Numan > Thanks, > Han > > > > Reported-by: Marcin Mirecki <[email protected]> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-March/ > 345266.html > > > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > ovn/controller/physical.c | 11 +++++++++++ > > tests/ovn.at | 32 +++++++++++++++++++++++++++----- > > 2 files changed, 38 insertions(+), 5 deletions(-) > > > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > > index 5a80e2cda..84113ebd2 100644 > > --- a/ovn/controller/physical.c > > +++ b/ovn/controller/physical.c > > @@ -466,6 +466,17 @@ consider_port_binding(struct controller_ctx *ctx, > > } else { > > ofport = u16_to_ofp(simap_get(&localvif_to_ofport, > > binding->logical_port)); > > + const char *requested_chassis = smap_get(&binding->options, > > + "requested-chassis"); > > + if (ofport && requested_chassis && > > + strcmp(requested_chassis, chassis->name) && > > + strcmp(requested_chassis, chassis->hostname)) { > > + /* Even though there is an ofport for this port_binding, it > is > > + * requested on a different chassis. So ignore this ofport. > > + */ > > + ofport = 0; > > + } > > + > > if ((!strcmp(binding->type, "localnet") > > || !strcmp(binding->type, "l2gateway")) > > && ofport && binding->tag) { > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 5f985f345..75b32ef1a 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -9112,13 +9112,15 @@ sim_add hv1 > > as hv1 > > ovs-vsctl add-br br-phys > > ovn_attach n1 br-phys 192.168.0.11 > > -ovs-vsctl -- add-port br-int hv1-vif0 > > +ovs-vsctl -- add-port br-int hv1-vif0 -- \ > > +set Interface hv1-vif0 ofport-request=1 > > > > sim_add hv2 > > as hv2 > > ovs-vsctl add-br br-phys > > ovn_attach n1 br-phys 192.168.0.12 > > -ovs-vsctl -- add-port br-int hv2-vif0 > > +ovs-vsctl -- add-port br-int hv2-vif0 -- \ > > +set Interface hv2-vif0 ofport-request=1 > > > > # Allow only chassis hv1 to bind logical port lsp0. > > ovn-nbctl lsp-set-options lsp0 requested-chassis=hv1 > > @@ -9138,7 +9140,11 @@ ovs-vsctl set interface hv2-vif0 > external-ids:iface-id=lsp0 > > OVS_WAIT_UNTIL([test 1 = $(grep -c "Not claiming lport lsp0" > hv2/ovn-controller.log)]) > > AT_CHECK([test x$(ovn-sbctl --bare --columns chassis find port_binding > logical_port=lsp0) = x], [0], []) > > > > -# (2) Chassis hv1 should bind lsp0 when physical to logical mapping > exists on hv1. > > +# (2) Chassis hv2 should not add flows in OFTABLE_PHY_TO_LOG and > OFTABLE_LOG_TO_PHY tables. > > +AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=0 | grep in_port=1], > [1], []) > > +AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=65 | grep output], > [1], []) > > + > > +# (3) Chassis hv1 should bind lsp0 when physical to logical mapping > exists on hv1. > > echo "verifying that hv1 binds lsp0 when hv1 physical/logical mapping > is added" > > as hv1 > > ovs-vsctl set interface hv1-vif0 external-ids:iface-id=lsp0 > > @@ -9146,7 +9152,12 @@ ovs-vsctl set interface hv1-vif0 > external-ids:iface-id=lsp0 > > OVS_WAIT_UNTIL([test 1 = $(grep -c "Claiming lport lsp0" > hv1/ovn-controller.log)]) > > AT_CHECK([test $(ovn-sbctl --bare --columns chassis find port_binding > logical_port=lsp0) = "$hv1_uuid"], [0], []) > > > > -# (3) Chassis hv1 should release lsp0 binding and chassis hv2 should > bind lsp0 when > > +# (4) Chassis hv1 should add flows in OFTABLE_PHY_TO_LOG and > OFTABLE_LOG_TO_PHY tables. > > +as hv1 ovs-ofctl dump-flows br-int > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep in_port=1], > [0], [ignore]) > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=65 | grep > actions=output:1], [0], [ignore]) > > + > > +# (5) Chassis hv1 should release lsp0 binding and chassis hv2 should > bind lsp0 when > > # the requested chassis for lsp0 is changed from hv1 to hv2. > > echo "verifying that lsp0 binding moves when requested-chassis is > changed" > > > > @@ -9154,6 +9165,13 @@ ovn-nbctl lsp-set-options lsp0 > requested-chassis=hv2 > > OVS_WAIT_UNTIL([test 1 = $(grep -c "Releasing lport lsp0 from this > chassis" hv1/ovn-controller.log)]) > > OVS_WAIT_UNTIL([test $(ovn-sbctl --bare --columns chassis find > port_binding logical_port=lsp0) = "$hv2_uuid"]) > > > > +# (6) Chassis hv2 should add flows and hv1 should not. > > +AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=0 | grep in_port=1], > [0], [ignore]) > > +AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=65 | grep > actions=output:1], [0], [ignore]) > > + > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep in_port=1], > [1], []) > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=65 | grep output], > [1], []) > > + > > OVN_CLEANUP([hv1],[hv2]) > > > > AT_CLEANUP > > @@ -9170,7 +9188,7 @@ sim_add hv1 > > as hv1 > > ovs-vsctl add-br br-phys > > ovn_attach n1 br-phys 192.168.0.11 > > -ovs-vsctl -- add-port br-int hv1-vif0 > > +ovs-vsctl -- add-port br-int hv1-vif0 -- set Interface hv1-vif0 > ofport-request=1 > > > > hv1_hostname=$(ovn-sbctl --bare --columns hostname find Chassis > name=hv1) > > echo "hv1_hostname=${hv1_hostname}" > > @@ -9181,11 +9199,15 @@ hv1_uuid=$(ovn-sbctl --bare --columns _uuid find > Chassis name=hv1) > > echo "hv1_uuid=${hv1_uuid}" > > OVS_WAIT_UNTIL([test 1 = $(grep -c "Claiming lport lsp0" > hv1/ovn-controller.log)]) > > AT_CHECK([test x$(ovn-sbctl --bare --columns chassis find port_binding > logical_port=lsp0) = x"$hv1_uuid"], [0], []) > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep in_port=1], > [0], [ignore]) > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=65 | grep > actions=output:1], [0], [ignore]) > > > > ovn-nbctl --wait=hv --timeout=3 lsp-set-options lsp0 > requested-chassis=non-existant-chassis > > OVS_WAIT_UNTIL([test 1 = $(grep -c "Releasing lport lsp0 from this > chassis" hv1/ovn-controller.log)]) > > ovn-nbctl --wait=hv --timeout=3 sync > > AT_CHECK([test x$(ovn-sbctl --bare --columns chassis find port_binding > logical_port=lsp0) = x], [0], []) > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep in_port=1], > [1], []) > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=65 | grep output], > [1], []) > > > > OVN_CLEANUP([hv1]) > > > > -- > > 2.14.3 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
