| Message ID | 1499262318-17357-5-git-send-email-majopela@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Russell Bryant |
| Headers | show |
Only a couple really small suggestions on this one. On Wed, Jul 5, 2017 at 9:45 AM, Miguel Angel Ajo <majopela@redhat.com> wrote: > is_chassis_active now is only true for redirect-chassis ports > in the case of the specific lport being active on the > local chassis. > > This will naturally make the ARP responder / redirection openflow > rules automatically inserted/removed when a router goes active/backup. > > Signed-off-by: Miguel Angel Ajo <majopela@redhat.com> > --- > ovn/controller/binding.c | 27 ++++---------- > ovn/controller/binding.h | 3 +- > ovn/controller/gchassis.c | 47 ++++++++++++++++++++++- > ovn/controller/gchassis.h | 10 ++++- > ovn/controller/lflow.c | 51 +++++++++++++++++++------ > ovn/controller/lflow.h | 6 ++- > ovn/controller/lport.c | 2 +- > ovn/controller/lport.h | 1 + > ovn/controller/ovn-controller.c | 10 +++-- > tests/ovn.at | 82 ++++++++++++++++++++++++++++++++++++----- > 10 files changed, 189 insertions(+), 50 deletions(-) > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index f8e501d..a835145 100644 > --- a/ovn/controller/binding.c > +++ b/ovn/controller/binding.c > @@ -433,20 +433,10 @@ consider_local_datapath(struct controller_ctx *ctx, > chassis_index); > if (gateway_chassis && > gateway_chassis_contains(gateway_chassis, chassis_rec)) { > - struct gateway_chassis *gwc; > - LIST_FOR_EACH (gwc, node, gateway_chassis) { > - if (!gwc->db->chassis) { > - continue; > - } > - if (!strcmp(gwc->db->chassis->name, chassis_rec->name)) { > - /* sb_rec_port_binding->chassis should reflect master */ > - our_chassis = true; > - break; > - } > - if (sset_contains(active_tunnels, gwc->db->chassis->name)) { > - break; > - } > - } > + > + our_chassis = gateway_chassis_is_active( > + gateway_chassis, chassis_rec, active_tunnels); > + > add_local_datapath(ldatapaths, lports, binding_rec->datapath, > false, local_datapaths, our_chassis); > } > @@ -498,7 +488,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > const struct ldatapath_index *ldatapaths, > const struct lport_index *lports, > const struct chassis_index *chassis_index, > - struct hmap *local_datapaths, struct sset *local_lports) > + struct hmap *local_datapaths, struct sset *local_lports, > + struct sset *active_tunnels) > { > if (!chassis_rec) { > return; > @@ -507,13 +498,12 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > const struct sbrec_port_binding *binding_rec; > struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface); > struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces); > - struct sset active_tunnels = SSET_INITIALIZER(&active_tunnels); > struct hmap qos_map; > > hmap_init(&qos_map); > if (br_int) { > get_local_iface_ids(br_int, &lport_to_iface, local_lports, > - &egress_ifaces, &active_tunnels); > + &egress_ifaces, active_tunnels); > } > > /* Run through each binding record to see if it is resident on this > @@ -524,7 +514,7 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > chassis_rec, binding_rec, > sset_is_empty(&egress_ifaces) ? NULL : > &qos_map, local_datapaths, &lport_to_iface, > - local_lports, &active_tunnels); > + local_lports, active_tunnels); > > } > if (!sset_is_empty(&egress_ifaces) > @@ -537,7 +527,6 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > > shash_destroy(&lport_to_iface); > sset_destroy(&egress_ifaces); > - sset_destroy(&active_tunnels); > hmap_destroy(&qos_map); > } > > diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h > index f3a2f37..7d0fec6 100644 > --- a/ovn/controller/binding.h > +++ b/ovn/controller/binding.h > @@ -33,7 +33,8 @@ void binding_register_ovs_idl(struct ovsdb_idl *); > void binding_run(struct controller_ctx *, const struct ovsrec_bridge *br_int, > const struct sbrec_chassis *, const struct ldatapath_index *, > const struct lport_index *, const struct chassis_index *, > - struct hmap *local_datapaths, struct sset *all_lports); > + struct hmap *local_datapaths, struct sset *all_lports, > + struct sset *active_tunnels); > void bfd_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > const struct sbrec_chassis *chassis_rec, > struct hmap *local_datapaths, const struct chassis_index *); > diff --git a/ovn/controller/gchassis.c b/ovn/controller/gchassis.c > index 27d8f66..af99635 100644 > --- a/ovn/controller/gchassis.c > +++ b/ovn/controller/gchassis.c > @@ -17,6 +17,7 @@ > > #include "gchassis.h" > #include "lport.h" > +#include "lib/sset.h" > #include "openvswitch/vlog.h" > #include "ovn/lib/chassis-index.h" > #include "ovn/lib/ovn-sb-idl.h" > @@ -110,7 +111,7 @@ gateway_chassis_get_ordered(const struct sbrec_port_binding *binding, > } > > bool > -gateway_chassis_contains(struct ovs_list *gateway_chassis, > +gateway_chassis_contains(const struct ovs_list *gateway_chassis, > const struct sbrec_chassis *chassis) { > struct gateway_chassis *chassis_item; > if (gateway_chassis) { > @@ -174,3 +175,47 @@ gateway_chassis_in_pb_contains(const struct sbrec_port_binding *binding, > > return false; > } > + > +bool > +gateway_chassis_is_active(const struct ovs_list *gateway_chassis, > + const struct sbrec_chassis *local_chassis, > + const struct sset *active_tunnels) > +{ > + struct gateway_chassis *gwc; > + > + if (!gateway_chassis) { Maybe add "|| ovs_list_is_empty(gateway_chassis)" here? > + return false; > + } > + /* if there's only one chassis, and local chassis is on the list > + * it's not HA and it's the equivalent of being active */ > + if (ovs_list_is_short(gateway_chassis) && and if you check for an empty list above, you can use ovs_list_is_singleton() here, since I think that's what you really want. > + gateway_chassis_contains(gateway_chassis, local_chassis)) { > + return true; > + } > + > + /* if there are no other tunnels active, we assume that the > + * connection providing tunneling is down, hence we're down */ > + if (sset_is_empty(active_tunnels)) { > + return false; > + } > + > + /* gateway_chassis is an ordered list, by priority, of chassis > + * hosting the redirect of the port */ > + LIST_FOR_EACH (gwc, node, gateway_chassis) { > + if (!gwc->db->chassis) { > + continue; > + } > + /* if we found the chassis on the list, and we didn't exit before > + * on the active_tunnels check for other higher priority chassis > + * being active, then this chassis is master. */ > + if (!strcmp(gwc->db->chassis->name, local_chassis->name)) { > + return true; > + } > + /* if we find this specific chassis on the list to have an active > + * tunnel, then 'local_chassis' is not master */ > + if (sset_contains(active_tunnels, gwc->db->chassis->name)) { > + return false; > + } > + } > + return false; > +} > diff --git a/ovn/controller/gchassis.h b/ovn/controller/gchassis.h > index 46d5e42..5735aec 100644 > --- a/ovn/controller/gchassis.h > +++ b/ovn/controller/gchassis.h > @@ -26,6 +26,7 @@ struct ovsdb_idl; > struct sbrec_chassis; > struct sbrec_gateway_chassis; > struct sbrec_port_binding; > +struct sset; > > > /* Gateway_Chassis management > @@ -48,7 +49,7 @@ struct ovs_list *gateway_chassis_get_ordered( > > /* Checks if an specific chassis is contained in the gateway_chassis > * list */ > -bool gateway_chassis_contains(struct ovs_list *gateway_chassis, > +bool gateway_chassis_contains(const struct ovs_list *gateway_chassis, > const struct sbrec_chassis *chassis); > > /* Destroy a gateway_chassis list from memory */ > @@ -60,4 +61,11 @@ bool gateway_chassis_in_pb_contains( > const struct sbrec_port_binding *binding, > const struct sbrec_chassis *chassis); > > +/* Returns if the local chassis is active based on tunnel status > + * and a gateway_chassis list. This will always be true for a single > + * gateway_chassis */ Some alternative wording that I would find a bit more clear: "Returns true if the local chassis is the active gateway among a set of gateway_chassis. Return false if the local chassis is currently a backup in a set of multiple gateway_chassis." > +bool gateway_chassis_is_active( > + const struct ovs_list *gateway_chassis, > + const struct sbrec_chassis *local_chassis, > + const struct sset *active_tunnels); > #endif /* ovn/controller/gchassis.h */ > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > index 8cc5e7e..f868e1f 100644 > --- a/ovn/controller/lflow.c > +++ b/ovn/controller/lflow.c > @@ -54,10 +54,13 @@ struct lookup_port_aux { > struct condition_aux { > const struct lport_index *lports; > const struct sbrec_chassis *chassis; > + const struct sset *active_tunnels; > + const struct chassis_index *chassis_index; > }; > > static void consider_logical_flow(const struct lport_index *lports, > const struct mcgroup_index *mcgroups, > + const struct chassis_index *chassis_index, > const struct sbrec_logical_flow *lflow, > const struct hmap *local_datapaths, > struct group_table *group_table, > @@ -66,7 +69,8 @@ static void consider_logical_flow(const struct lport_index *lports, > struct hmap *dhcpv6_opts, > uint32_t *conj_id_ofs, > const struct shash *addr_sets, > - struct hmap *flow_table); > + struct hmap *flow_table, > + struct sset *active_tunnels); > > static bool > lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) > @@ -97,10 +101,24 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name) > > const struct sbrec_port_binding *pb > = lport_lookup_by_name(c_aux->lports, port_name); > - if (pb && pb->chassis && pb->chassis == c_aux->chassis) { > - return true; > + if (!pb) { > + return false; > + } > + if (strcmp(pb->type, "chassisredirect")) { > + /* for non-chassisredirect ports */ > + return pb->chassis && pb->chassis == c_aux->chassis; > } else { > - return gateway_chassis_in_pb_contains(pb, c_aux->chassis); > + struct ovs_list *gateway_chassis; > + gateway_chassis = gateway_chassis_get_ordered(pb, > + c_aux->chassis_index); > + if (gateway_chassis) { > + bool active = gateway_chassis_is_active(gateway_chassis, > + c_aux->chassis, > + c_aux->active_tunnels); > + gateway_chassis_destroy(gateway_chassis); > + return active; > + } > + return false; > } > } > > @@ -124,11 +142,13 @@ is_gateway_router(const struct sbrec_datapath_binding *ldp, > static void > add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, > const struct mcgroup_index *mcgroups, > + const struct chassis_index *chassis_index, > const struct hmap *local_datapaths, > struct group_table *group_table, > const struct sbrec_chassis *chassis, > const struct shash *addr_sets, > - struct hmap *flow_table) > + struct hmap *flow_table, > + struct sset *active_tunnels) > { > uint32_t conj_id_ofs = 1; > const struct sbrec_logical_flow *lflow; > @@ -149,10 +169,11 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, > } > > SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) { > - consider_logical_flow(lports, mcgroups, lflow, local_datapaths, > + consider_logical_flow(lports, mcgroups, chassis_index, > + lflow, local_datapaths, > group_table, chassis, > &dhcp_opts, &dhcpv6_opts, &conj_id_ofs, > - addr_sets, flow_table); > + addr_sets, flow_table, active_tunnels); > } > > dhcp_opts_destroy(&dhcp_opts); > @@ -162,6 +183,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, > static void > consider_logical_flow(const struct lport_index *lports, > const struct mcgroup_index *mcgroups, > + const struct chassis_index *chassis_index, > const struct sbrec_logical_flow *lflow, > const struct hmap *local_datapaths, > struct group_table *group_table, > @@ -170,7 +192,8 @@ consider_logical_flow(const struct lport_index *lports, > struct hmap *dhcpv6_opts, > uint32_t *conj_id_ofs, > const struct shash *addr_sets, > - struct hmap *flow_table) > + struct hmap *flow_table, > + struct sset *active_tunnels) > { > /* Determine translation of logical table IDs to physical table IDs. */ > bool ingress = !strcmp(lflow->pipeline, "ingress"); > @@ -267,7 +290,8 @@ consider_logical_flow(const struct lport_index *lports, > return; > } > > - struct condition_aux cond_aux = { lports, chassis }; > + struct condition_aux cond_aux = { lports, chassis, active_tunnels, > + chassis_index}; > expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux); > expr = expr_normalize(expr); > uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, > @@ -393,13 +417,16 @@ lflow_run(struct controller_ctx *ctx, > const struct sbrec_chassis *chassis, > const struct lport_index *lports, > const struct mcgroup_index *mcgroups, > + const struct chassis_index *chassis_index, > const struct hmap *local_datapaths, > struct group_table *group_table, > const struct shash *addr_sets, > - struct hmap *flow_table) > + struct hmap *flow_table, > + struct sset *active_tunnels) > { > - add_logical_flows(ctx, lports, mcgroups, local_datapaths, group_table, > - chassis, addr_sets, flow_table); > + add_logical_flows(ctx, lports, mcgroups, chassis_index, local_datapaths, > + group_table, chassis, addr_sets, flow_table, > + active_tunnels); > add_neighbor_flows(ctx, lports, flow_table); > } > > diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h > index a23cde0..484502f 100644 > --- a/ovn/controller/lflow.h > +++ b/ovn/controller/lflow.h > @@ -35,6 +35,7 @@ > > #include <stdint.h> > > +struct chassis_index; > struct controller_ctx; > struct group_table; > struct hmap; > @@ -42,6 +43,7 @@ struct lport_index; > struct mcgroup_index; > struct sbrec_chassis; > struct simap; > +struct sset; > struct uuid; > > /* OpenFlow table numbers. > @@ -66,10 +68,12 @@ void lflow_run(struct controller_ctx *, > const struct sbrec_chassis *chassis, > const struct lport_index *, > const struct mcgroup_index *, > + const struct chassis_index *, > const struct hmap *local_datapaths, > struct group_table *group_table, > const struct shash *addr_sets, > - struct hmap *flow_table); > + struct hmap *flow_table, > + struct sset *active_tunnels); > void lflow_destroy(void); > > #endif /* ovn/lflow.h */ > diff --git a/ovn/controller/lport.c b/ovn/controller/lport.c > index 906fda2..28f5d77 100644 > --- a/ovn/controller/lport.c > +++ b/ovn/controller/lport.c > @@ -15,11 +15,11 @@ > > #include <config.h> > > +#include "lib/sset.h" > #include "lport.h" > #include "hash.h" > #include "openvswitch/vlog.h" > #include "ovn/lib/ovn-sb-idl.h" > - > VLOG_DEFINE_THIS_MODULE(lport); > > static struct ldatapath *ldatapath_lookup_by_key__( > diff --git a/ovn/controller/lport.h b/ovn/controller/lport.h > index 8937ecb..9a5a5da 100644 > --- a/ovn/controller/lport.h > +++ b/ovn/controller/lport.h > @@ -26,6 +26,7 @@ struct sbrec_chassis; > struct sbrec_datapath_binding; > struct sbrec_port_binding; > > + > /* Database indexes. > * ================= > * > diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c > index c136715..1418a15 100644 > --- a/ovn/controller/ovn-controller.c > +++ b/ovn/controller/ovn-controller.c > @@ -623,6 +623,7 @@ main(int argc, char *argv[]) > * l2gateway ports for which options:l2gateway-chassis designates the > * local hypervisor, and localnet ports. */ > struct sset local_lports = SSET_INITIALIZER(&local_lports); > + struct sset active_tunnels = SSET_INITIALIZER(&active_tunnels); > > const struct ovsrec_bridge *br_int = get_br_int(&ctx); > const char *chassis_id = get_chassis_id(ctx.ovs_idl); > @@ -642,9 +643,9 @@ main(int argc, char *argv[]) > chassis = chassis_run(&ctx, chassis_id, br_int); > encaps_run(&ctx, br_int, chassis_id); > binding_run(&ctx, br_int, chassis, &ldatapaths, &lports, > - &chassis_index, &local_datapaths, &local_lports); > + &chassis_index, &local_datapaths, &local_lports, > + &active_tunnels); > } > - > if (br_int && chassis) { > struct shash addr_sets = SHASH_INITIALIZER(&addr_sets); > addr_sets_init(&ctx, &addr_sets); > @@ -663,8 +664,8 @@ main(int argc, char *argv[]) > > struct hmap flow_table = HMAP_INITIALIZER(&flow_table); > lflow_run(&ctx, chassis, &lports, &mcgroups, > - &local_datapaths, &group_table, > - &addr_sets, &flow_table); > + &chassis_index, &local_datapaths, &group_table, > + &addr_sets, &flow_table, &active_tunnels); > > bfd_run(&ctx, br_int, chassis, &local_datapaths, > &chassis_index); > @@ -720,6 +721,7 @@ main(int argc, char *argv[]) > ldatapath_index_destroy(&ldatapaths); > > sset_destroy(&local_lports); > + sset_destroy(&active_tunnels); > > struct local_datapath *cur_node, *next_node; > HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, &local_datapaths) { > diff --git a/tests/ovn.at b/tests/ovn.at > index ae17b01..398afee 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -7836,17 +7836,25 @@ for chassis in gw1 gw2 hv1 hv2; do > echo "------ $chassis dump ----------" > ovs-ofctl show br-int > ovs-ofctl dump-flows br-int > - echo "" > - echo "BFD (from $chassis):" > - # dump BFD config and status to the other chassis > - for chassis2 in gw1 gw2 hv1 hv2; do > - if [[ "$chassis" != "$chassis2" ]]; then > - echo " -> $chassis2:" > - echo " $(ovs-vsctl --bare --columns bfd,bfd_status find Interface name=ovn-$chassis2-0)" > - fi > - done > echo "--------------------------" > done > +function bfd_dump() { > + for chassis in gw1 gw2 hv1 hv2; do > + as $chassis > + echo "------ $chassis dump (BFD)----" > + echo "BFD (from $chassis):" > + # dump BFD config and status to the other chassis > + for chassis2 in gw1 gw2 hv1 hv2; do > + if [[ "$chassis" != "$chassis2" ]]; then > + echo " -> $chassis2:" > + echo " $(ovs-vsctl --bare --columns bfd,bfd_status find Interface name=ovn-$chassis2-0)" > + fi > + done > + echo "--------------------------" > + done > +} > + > +bfd_dump > > hv1_gw1_ofport=$(as hv1 ovs-vsctl --bare --columns ofport find Interface name=ovn-gw1-0) > hv1_gw2_ofport=$(as hv1 ovs-vsctl --bare --columns ofport find Interface name=ovn-gw2-0) > @@ -7864,13 +7872,36 @@ as hv1 ovs-ofctl dump-flows br-int table=32 > echo "--- hv2 ---" > as hv2 ovs-ofctl dump-flows br-int table=32 > > +gw1_chassis=$(ovn-sbctl --bare --columns=_uuid find Chassis name=gw1) > +gw2_chassis=$(ovn-sbctl --bare --columns=_uuid find Chassis name=gw2) > + > AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=32 | grep active_backup | grep slaves:$hv1_gw1_ofport,$hv1_gw2_ofport | wc -l], [0], [1 > ]) > > AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=32 | grep active_backup | grep slaves:$hv2_gw1_ofport,$hv2_gw2_ofport | wc -l], [0], [1 > ]) > > -# set higher priority to gw2 instead of gw1, and check for changes > +# make sure that flows for handling the outside router port reside on gw1 > +AT_CHECK([as gw1 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[1 > +]]) > +AT_CHECK([as gw2 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[0 > +]]) > + > +# make sure ARP responder flows for outside router port reside on gw1 too > +AT_CHECK([as gw1 ovs-ofctl dump-flows br-int table=9 | grep arp_tpa=192.168.0.101 | wc -l], [0], [[1 > +]]) > +AT_CHECK([as gw2 ovs-ofctl dump-flows br-int table=9 | grep arp_tpa=192.168.0.101 | wc -l], [0], [[0 > +]]) > + > + > + > +# check that the chassis redirect port has been claimed by the gw1 chassis > +AT_CHECK([ovn-sbctl --columns chassis --bare find Port_Binding logical_port=cr-outside | grep $gw1_chassis | wc -l], > + [0],[[1 > +]]) > + > + > +# at this point, we invert the priority of the gw chassis between gw1 and gw2 > > ovn-nbctl --id=@gc0 create Gateway_Chassis \ > name=outside_gw1 chassis_name=gw1 priority=10 -- \ > @@ -7878,15 +7909,21 @@ ovn-nbctl --id=@gc0 create Gateway_Chassis \ > name=outside_gw2 chassis_name=gw2 priority=20 -- \ > set Logical_Router_Port outside 'gateway_chassis=[@gc0,@gc1]' > > + > # XXX: Let the change propagate down to the ovn-controllers > sleep 2 > > +# we make sure that the hypervisors noticed, and inverted the slave ports > AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=32 | grep active_backup | grep slaves:$hv1_gw2_ofport,$hv1_gw1_ofport | wc -l], [0], [1 > ]) > > AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=32 | grep active_backup | grep slaves:$hv2_gw2_ofport,$hv2_gw1_ofport | wc -l], [0], [1 > ]) > > +# check that the chassis redirect port has been reclaimed by the gw2 chassis > +AT_CHECK([ovn-sbctl --columns chassis --bare find Port_Binding logical_port=cr-outside | grep $gw2_chassis | wc -l], > + [0],[[1 > +]]) > > # check BFD enablement on tunnel ports from gw1 ######### > as gw1 > @@ -7934,6 +7971,31 @@ AT_CHECK([ovs-vsctl --bare --columns bfd find Interface name=ovn-hv1-0],[0], > [[enable=false > ]]) > > +# make sure that flows for handling the outside router port reside on gw2 now > +AT_CHECK([as gw2 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[1 > +]]) > +AT_CHECK([as gw1 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[0 > +]]) > + > +# disconnect GW2 from the network, GW1 should take over > +as gw2 > +port=${sandbox}_br-phys > +as main ovs-vsctl del-port n1 $port > +sleep 4 > + > +bfd_dump > + > +# make sure that flows for handling the outside router port reside on gw2 now > +AT_CHECK([as gw1 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[1 > +]]) > +AT_CHECK([as gw2 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[0 > +]]) > + > +# check that the chassis redirect port has been reclaimed by the gw1 chassis > +AT_CHECK([ovn-sbctl --columns chassis --bare find Port_Binding logical_port=cr-outside | grep $gw1_chassis | wc -l], > + [0],[[1 > +]]) > + > OVN_CLEANUP([gw1],[gw2],[hv1],[hv2]) > > AT_CLEANUP > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index f8e501d..a835145 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -433,20 +433,10 @@ consider_local_datapath(struct controller_ctx *ctx, chassis_index); if (gateway_chassis && gateway_chassis_contains(gateway_chassis, chassis_rec)) { - struct gateway_chassis *gwc; - LIST_FOR_EACH (gwc, node, gateway_chassis) { - if (!gwc->db->chassis) { - continue; - } - if (!strcmp(gwc->db->chassis->name, chassis_rec->name)) { - /* sb_rec_port_binding->chassis should reflect master */ - our_chassis = true; - break; - } - if (sset_contains(active_tunnels, gwc->db->chassis->name)) { - break; - } - } + + our_chassis = gateway_chassis_is_active( + gateway_chassis, chassis_rec, active_tunnels); + add_local_datapath(ldatapaths, lports, binding_rec->datapath, false, local_datapaths, our_chassis); } @@ -498,7 +488,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, const struct ldatapath_index *ldatapaths, const struct lport_index *lports, const struct chassis_index *chassis_index, - struct hmap *local_datapaths, struct sset *local_lports) + struct hmap *local_datapaths, struct sset *local_lports, + struct sset *active_tunnels) { if (!chassis_rec) { return; @@ -507,13 +498,12 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, const struct sbrec_port_binding *binding_rec; struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface); struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces); - struct sset active_tunnels = SSET_INITIALIZER(&active_tunnels); struct hmap qos_map; hmap_init(&qos_map); if (br_int) { get_local_iface_ids(br_int, &lport_to_iface, local_lports, - &egress_ifaces, &active_tunnels); + &egress_ifaces, active_tunnels); } /* Run through each binding record to see if it is resident on this @@ -524,7 +514,7 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, chassis_rec, binding_rec, sset_is_empty(&egress_ifaces) ? NULL : &qos_map, local_datapaths, &lport_to_iface, - local_lports, &active_tunnels); + local_lports, active_tunnels); } if (!sset_is_empty(&egress_ifaces) @@ -537,7 +527,6 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, shash_destroy(&lport_to_iface); sset_destroy(&egress_ifaces); - sset_destroy(&active_tunnels); hmap_destroy(&qos_map); } diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h index f3a2f37..7d0fec6 100644 --- a/ovn/controller/binding.h +++ b/ovn/controller/binding.h @@ -33,7 +33,8 @@ void binding_register_ovs_idl(struct ovsdb_idl *); void binding_run(struct controller_ctx *, const struct ovsrec_bridge *br_int, const struct sbrec_chassis *, const struct ldatapath_index *, const struct lport_index *, const struct chassis_index *, - struct hmap *local_datapaths, struct sset *all_lports); + struct hmap *local_datapaths, struct sset *all_lports, + struct sset *active_tunnels); void bfd_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, const struct sbrec_chassis *chassis_rec, struct hmap *local_datapaths, const struct chassis_index *); diff --git a/ovn/controller/gchassis.c b/ovn/controller/gchassis.c index 27d8f66..af99635 100644 --- a/ovn/controller/gchassis.c +++ b/ovn/controller/gchassis.c @@ -17,6 +17,7 @@ #include "gchassis.h" #include "lport.h" +#include "lib/sset.h" #include "openvswitch/vlog.h" #include "ovn/lib/chassis-index.h" #include "ovn/lib/ovn-sb-idl.h" @@ -110,7 +111,7 @@ gateway_chassis_get_ordered(const struct sbrec_port_binding *binding, } bool -gateway_chassis_contains(struct ovs_list *gateway_chassis, +gateway_chassis_contains(const struct ovs_list *gateway_chassis, const struct sbrec_chassis *chassis) { struct gateway_chassis *chassis_item; if (gateway_chassis) { @@ -174,3 +175,47 @@ gateway_chassis_in_pb_contains(const struct sbrec_port_binding *binding, return false; } + +bool +gateway_chassis_is_active(const struct ovs_list *gateway_chassis, + const struct sbrec_chassis *local_chassis, + const struct sset *active_tunnels) +{ + struct gateway_chassis *gwc; + + if (!gateway_chassis) { + return false; + } + /* if there's only one chassis, and local chassis is on the list + * it's not HA and it's the equivalent of being active */ + if (ovs_list_is_short(gateway_chassis) && + gateway_chassis_contains(gateway_chassis, local_chassis)) { + return true; + } + + /* if there are no other tunnels active, we assume that the + * connection providing tunneling is down, hence we're down */ + if (sset_is_empty(active_tunnels)) { + return false; + } + + /* gateway_chassis is an ordered list, by priority, of chassis + * hosting the redirect of the port */ + LIST_FOR_EACH (gwc, node, gateway_chassis) { + if (!gwc->db->chassis) { + continue; + } + /* if we found the chassis on the list, and we didn't exit before + * on the active_tunnels check for other higher priority chassis + * being active, then this chassis is master. */ + if (!strcmp(gwc->db->chassis->name, local_chassis->name)) { + return true; + } + /* if we find this specific chassis on the list to have an active + * tunnel, then 'local_chassis' is not master */ + if (sset_contains(active_tunnels, gwc->db->chassis->name)) { + return false; + } + } + return false; +} diff --git a/ovn/controller/gchassis.h b/ovn/controller/gchassis.h index 46d5e42..5735aec 100644 --- a/ovn/controller/gchassis.h +++ b/ovn/controller/gchassis.h @@ -26,6 +26,7 @@ struct ovsdb_idl; struct sbrec_chassis; struct sbrec_gateway_chassis; struct sbrec_port_binding; +struct sset; /* Gateway_Chassis management @@ -48,7 +49,7 @@ struct ovs_list *gateway_chassis_get_ordered( /* Checks if an specific chassis is contained in the gateway_chassis * list */ -bool gateway_chassis_contains(struct ovs_list *gateway_chassis, +bool gateway_chassis_contains(const struct ovs_list *gateway_chassis, const struct sbrec_chassis *chassis); /* Destroy a gateway_chassis list from memory */ @@ -60,4 +61,11 @@ bool gateway_chassis_in_pb_contains( const struct sbrec_port_binding *binding, const struct sbrec_chassis *chassis); +/* Returns if the local chassis is active based on tunnel status + * and a gateway_chassis list. This will always be true for a single + * gateway_chassis */ +bool gateway_chassis_is_active( + const struct ovs_list *gateway_chassis, + const struct sbrec_chassis *local_chassis, + const struct sset *active_tunnels); #endif /* ovn/controller/gchassis.h */ diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index 8cc5e7e..f868e1f 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -54,10 +54,13 @@ struct lookup_port_aux { struct condition_aux { const struct lport_index *lports; const struct sbrec_chassis *chassis; + const struct sset *active_tunnels; + const struct chassis_index *chassis_index; }; static void consider_logical_flow(const struct lport_index *lports, const struct mcgroup_index *mcgroups, + const struct chassis_index *chassis_index, const struct sbrec_logical_flow *lflow, const struct hmap *local_datapaths, struct group_table *group_table, @@ -66,7 +69,8 @@ static void consider_logical_flow(const struct lport_index *lports, struct hmap *dhcpv6_opts, uint32_t *conj_id_ofs, const struct shash *addr_sets, - struct hmap *flow_table); + struct hmap *flow_table, + struct sset *active_tunnels); static bool lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) @@ -97,10 +101,24 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name) const struct sbrec_port_binding *pb = lport_lookup_by_name(c_aux->lports, port_name); - if (pb && pb->chassis && pb->chassis == c_aux->chassis) { - return true; + if (!pb) { + return false; + } + if (strcmp(pb->type, "chassisredirect")) { + /* for non-chassisredirect ports */ + return pb->chassis && pb->chassis == c_aux->chassis; } else { - return gateway_chassis_in_pb_contains(pb, c_aux->chassis); + struct ovs_list *gateway_chassis; + gateway_chassis = gateway_chassis_get_ordered(pb, + c_aux->chassis_index); + if (gateway_chassis) { + bool active = gateway_chassis_is_active(gateway_chassis, + c_aux->chassis, + c_aux->active_tunnels); + gateway_chassis_destroy(gateway_chassis); + return active; + } + return false; } } @@ -124,11 +142,13 @@ is_gateway_router(const struct sbrec_datapath_binding *ldp, static void add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, const struct mcgroup_index *mcgroups, + const struct chassis_index *chassis_index, const struct hmap *local_datapaths, struct group_table *group_table, const struct sbrec_chassis *chassis, const struct shash *addr_sets, - struct hmap *flow_table) + struct hmap *flow_table, + struct sset *active_tunnels) { uint32_t conj_id_ofs = 1; const struct sbrec_logical_flow *lflow; @@ -149,10 +169,11 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, } SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) { - consider_logical_flow(lports, mcgroups, lflow, local_datapaths, + consider_logical_flow(lports, mcgroups, chassis_index, + lflow, local_datapaths, group_table, chassis, &dhcp_opts, &dhcpv6_opts, &conj_id_ofs, - addr_sets, flow_table); + addr_sets, flow_table, active_tunnels); } dhcp_opts_destroy(&dhcp_opts); @@ -162,6 +183,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, static void consider_logical_flow(const struct lport_index *lports, const struct mcgroup_index *mcgroups, + const struct chassis_index *chassis_index, const struct sbrec_logical_flow *lflow, const struct hmap *local_datapaths, struct group_table *group_table, @@ -170,7 +192,8 @@ consider_logical_flow(const struct lport_index *lports, struct hmap *dhcpv6_opts, uint32_t *conj_id_ofs, const struct shash *addr_sets, - struct hmap *flow_table) + struct hmap *flow_table, + struct sset *active_tunnels) { /* Determine translation of logical table IDs to physical table IDs. */ bool ingress = !strcmp(lflow->pipeline, "ingress"); @@ -267,7 +290,8 @@ consider_logical_flow(const struct lport_index *lports, return; } - struct condition_aux cond_aux = { lports, chassis }; + struct condition_aux cond_aux = { lports, chassis, active_tunnels, + chassis_index}; expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux); expr = expr_normalize(expr); uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, @@ -393,13 +417,16 @@ lflow_run(struct controller_ctx *ctx, const struct sbrec_chassis *chassis, const struct lport_index *lports, const struct mcgroup_index *mcgroups, + const struct chassis_index *chassis_index, const struct hmap *local_datapaths, struct group_table *group_table, const struct shash *addr_sets, - struct hmap *flow_table) + struct hmap *flow_table, + struct sset *active_tunnels) { - add_logical_flows(ctx, lports, mcgroups, local_datapaths, group_table, - chassis, addr_sets, flow_table); + add_logical_flows(ctx, lports, mcgroups, chassis_index, local_datapaths, + group_table, chassis, addr_sets, flow_table, + active_tunnels); add_neighbor_flows(ctx, lports, flow_table); } diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h index a23cde0..484502f 100644 --- a/ovn/controller/lflow.h +++ b/ovn/controller/lflow.h @@ -35,6 +35,7 @@ #include <stdint.h> +struct chassis_index; struct controller_ctx; struct group_table; struct hmap; @@ -42,6 +43,7 @@ struct lport_index; struct mcgroup_index; struct sbrec_chassis; struct simap; +struct sset; struct uuid; /* OpenFlow table numbers. @@ -66,10 +68,12 @@ void lflow_run(struct controller_ctx *, const struct sbrec_chassis *chassis, const struct lport_index *, const struct mcgroup_index *, + const struct chassis_index *, const struct hmap *local_datapaths, struct group_table *group_table, const struct shash *addr_sets, - struct hmap *flow_table); + struct hmap *flow_table, + struct sset *active_tunnels); void lflow_destroy(void); #endif /* ovn/lflow.h */ diff --git a/ovn/controller/lport.c b/ovn/controller/lport.c index 906fda2..28f5d77 100644 --- a/ovn/controller/lport.c +++ b/ovn/controller/lport.c @@ -15,11 +15,11 @@ #include <config.h> +#include "lib/sset.h" #include "lport.h" #include "hash.h" #include "openvswitch/vlog.h" #include "ovn/lib/ovn-sb-idl.h" - VLOG_DEFINE_THIS_MODULE(lport); static struct ldatapath *ldatapath_lookup_by_key__( diff --git a/ovn/controller/lport.h b/ovn/controller/lport.h index 8937ecb..9a5a5da 100644 --- a/ovn/controller/lport.h +++ b/ovn/controller/lport.h @@ -26,6 +26,7 @@ struct sbrec_chassis; struct sbrec_datapath_binding; struct sbrec_port_binding; + /* Database indexes. * ================= * diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index c136715..1418a15 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -623,6 +623,7 @@ main(int argc, char *argv[]) * l2gateway ports for which options:l2gateway-chassis designates the * local hypervisor, and localnet ports. */ struct sset local_lports = SSET_INITIALIZER(&local_lports); + struct sset active_tunnels = SSET_INITIALIZER(&active_tunnels); const struct ovsrec_bridge *br_int = get_br_int(&ctx); const char *chassis_id = get_chassis_id(ctx.ovs_idl); @@ -642,9 +643,9 @@ main(int argc, char *argv[]) chassis = chassis_run(&ctx, chassis_id, br_int); encaps_run(&ctx, br_int, chassis_id); binding_run(&ctx, br_int, chassis, &ldatapaths, &lports, - &chassis_index, &local_datapaths, &local_lports); + &chassis_index, &local_datapaths, &local_lports, + &active_tunnels); } - if (br_int && chassis) { struct shash addr_sets = SHASH_INITIALIZER(&addr_sets); addr_sets_init(&ctx, &addr_sets); @@ -663,8 +664,8 @@ main(int argc, char *argv[]) struct hmap flow_table = HMAP_INITIALIZER(&flow_table); lflow_run(&ctx, chassis, &lports, &mcgroups, - &local_datapaths, &group_table, - &addr_sets, &flow_table); + &chassis_index, &local_datapaths, &group_table, + &addr_sets, &flow_table, &active_tunnels); bfd_run(&ctx, br_int, chassis, &local_datapaths, &chassis_index); @@ -720,6 +721,7 @@ main(int argc, char *argv[]) ldatapath_index_destroy(&ldatapaths); sset_destroy(&local_lports); + sset_destroy(&active_tunnels); struct local_datapath *cur_node, *next_node; HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, &local_datapaths) { diff --git a/tests/ovn.at b/tests/ovn.at index ae17b01..398afee 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -7836,17 +7836,25 @@ for chassis in gw1 gw2 hv1 hv2; do echo "------ $chassis dump ----------" ovs-ofctl show br-int ovs-ofctl dump-flows br-int - echo "" - echo "BFD (from $chassis):" - # dump BFD config and status to the other chassis - for chassis2 in gw1 gw2 hv1 hv2; do - if [[ "$chassis" != "$chassis2" ]]; then - echo " -> $chassis2:" - echo " $(ovs-vsctl --bare --columns bfd,bfd_status find Interface name=ovn-$chassis2-0)" - fi - done echo "--------------------------" done +function bfd_dump() { + for chassis in gw1 gw2 hv1 hv2; do + as $chassis + echo "------ $chassis dump (BFD)----" + echo "BFD (from $chassis):" + # dump BFD config and status to the other chassis + for chassis2 in gw1 gw2 hv1 hv2; do + if [[ "$chassis" != "$chassis2" ]]; then + echo " -> $chassis2:" + echo " $(ovs-vsctl --bare --columns bfd,bfd_status find Interface name=ovn-$chassis2-0)" + fi + done + echo "--------------------------" + done +} + +bfd_dump hv1_gw1_ofport=$(as hv1 ovs-vsctl --bare --columns ofport find Interface name=ovn-gw1-0) hv1_gw2_ofport=$(as hv1 ovs-vsctl --bare --columns ofport find Interface name=ovn-gw2-0) @@ -7864,13 +7872,36 @@ as hv1 ovs-ofctl dump-flows br-int table=32 echo "--- hv2 ---" as hv2 ovs-ofctl dump-flows br-int table=32 +gw1_chassis=$(ovn-sbctl --bare --columns=_uuid find Chassis name=gw1) +gw2_chassis=$(ovn-sbctl --bare --columns=_uuid find Chassis name=gw2) + AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=32 | grep active_backup | grep slaves:$hv1_gw1_ofport,$hv1_gw2_ofport | wc -l], [0], [1 ]) AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=32 | grep active_backup | grep slaves:$hv2_gw1_ofport,$hv2_gw2_ofport | wc -l], [0], [1 ]) -# set higher priority to gw2 instead of gw1, and check for changes +# make sure that flows for handling the outside router port reside on gw1 +AT_CHECK([as gw1 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[1 +]]) +AT_CHECK([as gw2 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[0 +]]) + +# make sure ARP responder flows for outside router port reside on gw1 too +AT_CHECK([as gw1 ovs-ofctl dump-flows br-int table=9 | grep arp_tpa=192.168.0.101 | wc -l], [0], [[1 +]]) +AT_CHECK([as gw2 ovs-ofctl dump-flows br-int table=9 | grep arp_tpa=192.168.0.101 | wc -l], [0], [[0 +]]) + + + +# check that the chassis redirect port has been claimed by the gw1 chassis +AT_CHECK([ovn-sbctl --columns chassis --bare find Port_Binding logical_port=cr-outside | grep $gw1_chassis | wc -l], + [0],[[1 +]]) + + +# at this point, we invert the priority of the gw chassis between gw1 and gw2 ovn-nbctl --id=@gc0 create Gateway_Chassis \ name=outside_gw1 chassis_name=gw1 priority=10 -- \ @@ -7878,15 +7909,21 @@ ovn-nbctl --id=@gc0 create Gateway_Chassis \ name=outside_gw2 chassis_name=gw2 priority=20 -- \ set Logical_Router_Port outside 'gateway_chassis=[@gc0,@gc1]' + # XXX: Let the change propagate down to the ovn-controllers sleep 2 +# we make sure that the hypervisors noticed, and inverted the slave ports AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=32 | grep active_backup | grep slaves:$hv1_gw2_ofport,$hv1_gw1_ofport | wc -l], [0], [1 ]) AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=32 | grep active_backup | grep slaves:$hv2_gw2_ofport,$hv2_gw1_ofport | wc -l], [0], [1 ]) +# check that the chassis redirect port has been reclaimed by the gw2 chassis +AT_CHECK([ovn-sbctl --columns chassis --bare find Port_Binding logical_port=cr-outside | grep $gw2_chassis | wc -l], + [0],[[1 +]]) # check BFD enablement on tunnel ports from gw1 ######### as gw1 @@ -7934,6 +7971,31 @@ AT_CHECK([ovs-vsctl --bare --columns bfd find Interface name=ovn-hv1-0],[0], [[enable=false ]]) +# make sure that flows for handling the outside router port reside on gw2 now +AT_CHECK([as gw2 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[1 +]]) +AT_CHECK([as gw1 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[0 +]]) + +# disconnect GW2 from the network, GW1 should take over +as gw2 +port=${sandbox}_br-phys +as main ovs-vsctl del-port n1 $port +sleep 4 + +bfd_dump + +# make sure that flows for handling the outside router port reside on gw2 now +AT_CHECK([as gw1 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[1 +]]) +AT_CHECK([as gw2 ovs-ofctl dump-flows br-int table=23 | grep 00:00:02:01:02:04 | wc -l], [0], [[0 +]]) + +# check that the chassis redirect port has been reclaimed by the gw1 chassis +AT_CHECK([ovn-sbctl --columns chassis --bare find Port_Binding logical_port=cr-outside | grep $gw1_chassis | wc -l], + [0],[[1 +]]) + OVN_CLEANUP([gw1],[gw2],[hv1],[hv2]) AT_CLEANUP
is_chassis_active now is only true for redirect-chassis ports in the case of the specific lport being active on the local chassis. This will naturally make the ARP responder / redirection openflow rules automatically inserted/removed when a router goes active/backup. Signed-off-by: Miguel Angel Ajo <majopela@redhat.com> --- ovn/controller/binding.c | 27 ++++---------- ovn/controller/binding.h | 3 +- ovn/controller/gchassis.c | 47 ++++++++++++++++++++++- ovn/controller/gchassis.h | 10 ++++- ovn/controller/lflow.c | 51 +++++++++++++++++++------ ovn/controller/lflow.h | 6 ++- ovn/controller/lport.c | 2 +- ovn/controller/lport.h | 1 + ovn/controller/ovn-controller.c | 10 +++-- tests/ovn.at | 82 ++++++++++++++++++++++++++++++++++++----- 10 files changed, 189 insertions(+), 50 deletions(-)