| Message ID | f33a595c87d734b3869cf2397047c227654c0030.1652984552.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] northd: add the capability to inherit logical routers lbs on logical switches | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Thu, May 19, 2022 at 2:23 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > Add the capability to automatically deploy a load-balancer on each > logical-switch connected to a logical router where the load-balancer > has been installed by the CMS. This patch allow to overcome the > distributed gw router scenario limitation where a load-balancer must be > installed on each datapath to properly reach the load-balancer. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2043543 > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Thanks for the patch. The documentation is missing for the new option in ovn-nb.xml. Can you also please enhance the test case to also do the following - when the option is cleared or set to false, the lb related lflows are deleted from the logical switch pipeline - And when the option is set to true, the load_balancer table in sb db is created with proper datapaths set. And also when the option is cleared. Also please add the NEWS entry for this. Otherwise, the patch LGTM. Thanks Numan > --- > northd/northd.c | 56 ++++++++++++++++++++++++++++++++++++++++++ > tests/ovn-northd.at | 59 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 115 insertions(+) > > diff --git a/northd/northd.c b/northd/northd.c > index 60983308d..4e06d6172 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -63,6 +63,8 @@ static bool lflow_hash_lock_initialized = false; > > static bool check_lsp_is_up; > > +static bool install_ls_lb_from_router; > + > /* MAC allocated for service monitor usage. Just one mac is allocated > * for this purpose and ovn-controller's on each chassis will make use > * of this mac when sending out the packets to monitor the services > @@ -4085,6 +4087,55 @@ build_lrouter_lbs_reachable_ips(struct hmap *datapaths, struct hmap *lbs) > } > } > > +static void > +build_lswitch_lbs_from_lrouter(struct hmap *datapaths, struct hmap *lbs) > +{ > + if (!install_ls_lb_from_router) { > + return; > + } > + > + struct ovn_datapath *od; > + HMAP_FOR_EACH (od, key_node, datapaths) { > + if (!od->nbs) { > + continue; > + } > + > + struct ovn_port *op; > + LIST_FOR_EACH (op, dp_node, &od->port_list) { > + if (!lsp_is_router(op->nbsp)) { > + continue; > + } > + if (!op->peer) { > + continue; > + } > + > + struct ovn_datapath *peer_od = op->peer->od; > + for (size_t i = 0; i < peer_od->nbr->n_load_balancer; i++) { > + bool installed = false; > + const struct uuid *lb_uuid = > + &peer_od->nbr->load_balancer[i]->header_.uuid; > + struct ovn_northd_lb *lb = ovn_northd_lb_find(lbs, lb_uuid); > + if (!lb) { > + continue; > + } > + > + for (size_t j = 0; j < lb->n_nb_ls; j++) { > + if (lb->nb_ls[j] == od) { > + installed = true; > + break; > + } > + } > + if (!installed) { > + ovn_northd_lb_add_ls(lb, od); > + } > + if (lb->nlb) { > + od->has_lb_vip |= lb_has_vip(lb->nlb); > + } > + } > + } > + } > +} > + > /* This must be called after all ports have been processed, i.e., after > * build_ports() because the reachability check requires the router ports > * networks to have been parsed. > @@ -4097,6 +4148,7 @@ build_lb_port_related_data(struct hmap *datapaths, struct hmap *ports, > build_lrouter_lbs_check(datapaths); > build_lrouter_lbs_reachable_ips(datapaths, lbs); > build_lb_svcs(input_data, ovnsb_txn, ports, lbs); > + build_lswitch_lbs_from_lrouter(datapaths, lbs); > } > > /* Syncs relevant load balancers (applied to logical switches) to the > @@ -15611,6 +15663,10 @@ ovnnb_db_run(struct northd_input *input_data, > "ignore_lsp_down", true); > default_acl_drop = smap_get_bool(&nb->options, "default_acl_drop", false); > > + install_ls_lb_from_router = smap_get_bool(&nb->options, > + "install_ls_lb_from_router", > + false); > + > build_datapaths(input_data, ovnsb_txn, &data->datapaths, &data->lr_list); > build_lbs(input_data, &data->datapaths, &data->lbs); > build_ports(input_data, ovnsb_txn, sbrec_chassis_by_name, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index ea311c7c0..9297c0c46 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -7289,3 +7289,62 @@ AT_CHECK([diff flows1 flows3]) > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([check install_ls_lb_from_router option]) > +ovn_start > + > +ovn-nbctl lr-add R1 > +ovn-nbctl set logical_router R1 options:chassis=hv1 > +ovn-nbctl lrp-add R1 R1-S0 02:ac:10:01:00:01 10.0.0.1/24 > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:01:01 20.0.0.1/24 > +ovn-nbctl lrp-add R1 R1-PUB 02:ac:20:01:01:01 172.16.0.1/24 > + > +ovn-nbctl ls-add S0 > +ovn-nbctl lsp-add S0 S0-R1 > +ovn-nbctl lsp-set-type S0-R1 router > +ovn-nbctl lsp-set-addresses S0-R1 02:ac:10:01:00:01 > +ovn-nbctl lsp-set-options S0-R1 router-port=R1-S0 > + > +ovn-nbctl ls-add S1 > +ovn-nbctl lsp-add S1 S1-R1 > +ovn-nbctl lsp-set-type S1-R1 router > +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:01:01 > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 > + > +# Add load balancers on the logical router R1 > +ovn-nbctl lb-add lb0 172.16.0.10:80 10.0.0.2:80 > +ovn-nbctl lr-lb-add R1 lb0 > + > +ovn-sbctl dump-flows S0 > S0flows > +ovn-sbctl dump-flows S1 > S1flows > + > +AT_CAPTURE_FILE([S0flows]) > +AT_CAPTURE_FILE([S1flows]) > + > +AT_CHECK([grep "ls_in_lb" S0flows | sort], [0], [dnl > + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) > +]) > +AT_CHECK([grep "ls_in_lb" S1flows | sort], [0], [dnl > + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) > +]) > + > +ovn-nbctl --wait=sb set NB_Global . options:install_ls_lb_from_router=true > + > +ovn-sbctl dump-flows S0 > S0flows > +ovn-sbctl dump-flows S1 > S1flows > + > +AT_CAPTURE_FILE([S0flows]) > +AT_CAPTURE_FILE([S1flows]) > + > +AT_CHECK([grep "ls_in_lb" S0flows | sort], [0], [dnl > + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) > + table=12(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > +]) > +AT_CHECK([grep "ls_in_lb" S1flows | sort], [0], [dnl > + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) > + table=12(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > +]) > + > +AT_CLEANUP > +]) > -- > 2.35.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On Thu, May 19, 2022 at 2:23 PM Lorenzo Bianconi > <lorenzo.bianconi@redhat.com> wrote: > > > > Add the capability to automatically deploy a load-balancer on each > > logical-switch connected to a logical router where the load-balancer > > has been installed by the CMS. This patch allow to overcome the > > distributed gw router scenario limitation where a load-balancer must be > > installed on each datapath to properly reach the load-balancer. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2043543 > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > Thanks for the patch. > > The documentation is missing for the new option in ovn-nb.xml. > > Can you also please enhance the test case to also do the following > - when the option is cleared or set to false, the lb related lflows > are deleted from the logical switch pipeline > - And when the option is set to true, the load_balancer table in > sb db is created with proper datapaths set. And also when the option > is cleared. > > Also please add the NEWS entry for this. > > Otherwise, the patch LGTM. Hi Numan, thx for the review. I will fix the comments in v2. Regards, Lorenzo > > Thanks > Numan > > > --- > > northd/northd.c | 56 ++++++++++++++++++++++++++++++++++++++++++ > > tests/ovn-northd.at | 59 +++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 115 insertions(+) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 60983308d..4e06d6172 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -63,6 +63,8 @@ static bool lflow_hash_lock_initialized = false; > > > > static bool check_lsp_is_up; > > > > +static bool install_ls_lb_from_router; > > + > > /* MAC allocated for service monitor usage. Just one mac is allocated > > * for this purpose and ovn-controller's on each chassis will make use > > * of this mac when sending out the packets to monitor the services > > @@ -4085,6 +4087,55 @@ build_lrouter_lbs_reachable_ips(struct hmap *datapaths, struct hmap *lbs) > > } > > } > > > > +static void > > +build_lswitch_lbs_from_lrouter(struct hmap *datapaths, struct hmap *lbs) > > +{ > > + if (!install_ls_lb_from_router) { > > + return; > > + } > > + > > + struct ovn_datapath *od; > > + HMAP_FOR_EACH (od, key_node, datapaths) { > > + if (!od->nbs) { > > + continue; > > + } > > + > > + struct ovn_port *op; > > + LIST_FOR_EACH (op, dp_node, &od->port_list) { > > + if (!lsp_is_router(op->nbsp)) { > > + continue; > > + } > > + if (!op->peer) { > > + continue; > > + } > > + > > + struct ovn_datapath *peer_od = op->peer->od; > > + for (size_t i = 0; i < peer_od->nbr->n_load_balancer; i++) { > > + bool installed = false; > > + const struct uuid *lb_uuid = > > + &peer_od->nbr->load_balancer[i]->header_.uuid; > > + struct ovn_northd_lb *lb = ovn_northd_lb_find(lbs, lb_uuid); > > + if (!lb) { > > + continue; > > + } > > + > > + for (size_t j = 0; j < lb->n_nb_ls; j++) { > > + if (lb->nb_ls[j] == od) { > > + installed = true; > > + break; > > + } > > + } > > + if (!installed) { > > + ovn_northd_lb_add_ls(lb, od); > > + } > > + if (lb->nlb) { > > + od->has_lb_vip |= lb_has_vip(lb->nlb); > > + } > > + } > > + } > > + } > > +} > > + > > /* This must be called after all ports have been processed, i.e., after > > * build_ports() because the reachability check requires the router ports > > * networks to have been parsed. > > @@ -4097,6 +4148,7 @@ build_lb_port_related_data(struct hmap *datapaths, struct hmap *ports, > > build_lrouter_lbs_check(datapaths); > > build_lrouter_lbs_reachable_ips(datapaths, lbs); > > build_lb_svcs(input_data, ovnsb_txn, ports, lbs); > > + build_lswitch_lbs_from_lrouter(datapaths, lbs); > > } > > > > /* Syncs relevant load balancers (applied to logical switches) to the > > @@ -15611,6 +15663,10 @@ ovnnb_db_run(struct northd_input *input_data, > > "ignore_lsp_down", true); > > default_acl_drop = smap_get_bool(&nb->options, "default_acl_drop", false); > > > > + install_ls_lb_from_router = smap_get_bool(&nb->options, > > + "install_ls_lb_from_router", > > + false); > > + > > build_datapaths(input_data, ovnsb_txn, &data->datapaths, &data->lr_list); > > build_lbs(input_data, &data->datapaths, &data->lbs); > > build_ports(input_data, ovnsb_txn, sbrec_chassis_by_name, > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index ea311c7c0..9297c0c46 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -7289,3 +7289,62 @@ AT_CHECK([diff flows1 flows3]) > > > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([check install_ls_lb_from_router option]) > > +ovn_start > > + > > +ovn-nbctl lr-add R1 > > +ovn-nbctl set logical_router R1 options:chassis=hv1 > > +ovn-nbctl lrp-add R1 R1-S0 02:ac:10:01:00:01 10.0.0.1/24 > > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:01:01 20.0.0.1/24 > > +ovn-nbctl lrp-add R1 R1-PUB 02:ac:20:01:01:01 172.16.0.1/24 > > + > > +ovn-nbctl ls-add S0 > > +ovn-nbctl lsp-add S0 S0-R1 > > +ovn-nbctl lsp-set-type S0-R1 router > > +ovn-nbctl lsp-set-addresses S0-R1 02:ac:10:01:00:01 > > +ovn-nbctl lsp-set-options S0-R1 router-port=R1-S0 > > + > > +ovn-nbctl ls-add S1 > > +ovn-nbctl lsp-add S1 S1-R1 > > +ovn-nbctl lsp-set-type S1-R1 router > > +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:01:01 > > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 > > + > > +# Add load balancers on the logical router R1 > > +ovn-nbctl lb-add lb0 172.16.0.10:80 10.0.0.2:80 > > +ovn-nbctl lr-lb-add R1 lb0 > > + > > +ovn-sbctl dump-flows S0 > S0flows > > +ovn-sbctl dump-flows S1 > S1flows > > + > > +AT_CAPTURE_FILE([S0flows]) > > +AT_CAPTURE_FILE([S1flows]) > > + > > +AT_CHECK([grep "ls_in_lb" S0flows | sort], [0], [dnl > > + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) > > +]) > > +AT_CHECK([grep "ls_in_lb" S1flows | sort], [0], [dnl > > + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) > > +]) > > + > > +ovn-nbctl --wait=sb set NB_Global . options:install_ls_lb_from_router=true > > + > > +ovn-sbctl dump-flows S0 > S0flows > > +ovn-sbctl dump-flows S1 > S1flows > > + > > +AT_CAPTURE_FILE([S0flows]) > > +AT_CAPTURE_FILE([S1flows]) > > + > > +AT_CHECK([grep "ls_in_lb" S0flows | sort], [0], [dnl > > + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) > > + table=12(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > > +]) > > +AT_CHECK([grep "ls_in_lb" S1flows | sort], [0], [dnl > > + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) > > + table=12(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > > +]) > > + > > +AT_CLEANUP > > +]) > > -- > > 2.35.3 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
diff --git a/northd/northd.c b/northd/northd.c index 60983308d..4e06d6172 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -63,6 +63,8 @@ static bool lflow_hash_lock_initialized = false; static bool check_lsp_is_up; +static bool install_ls_lb_from_router; + /* MAC allocated for service monitor usage. Just one mac is allocated * for this purpose and ovn-controller's on each chassis will make use * of this mac when sending out the packets to monitor the services @@ -4085,6 +4087,55 @@ build_lrouter_lbs_reachable_ips(struct hmap *datapaths, struct hmap *lbs) } } +static void +build_lswitch_lbs_from_lrouter(struct hmap *datapaths, struct hmap *lbs) +{ + if (!install_ls_lb_from_router) { + return; + } + + struct ovn_datapath *od; + HMAP_FOR_EACH (od, key_node, datapaths) { + if (!od->nbs) { + continue; + } + + struct ovn_port *op; + LIST_FOR_EACH (op, dp_node, &od->port_list) { + if (!lsp_is_router(op->nbsp)) { + continue; + } + if (!op->peer) { + continue; + } + + struct ovn_datapath *peer_od = op->peer->od; + for (size_t i = 0; i < peer_od->nbr->n_load_balancer; i++) { + bool installed = false; + const struct uuid *lb_uuid = + &peer_od->nbr->load_balancer[i]->header_.uuid; + struct ovn_northd_lb *lb = ovn_northd_lb_find(lbs, lb_uuid); + if (!lb) { + continue; + } + + for (size_t j = 0; j < lb->n_nb_ls; j++) { + if (lb->nb_ls[j] == od) { + installed = true; + break; + } + } + if (!installed) { + ovn_northd_lb_add_ls(lb, od); + } + if (lb->nlb) { + od->has_lb_vip |= lb_has_vip(lb->nlb); + } + } + } + } +} + /* This must be called after all ports have been processed, i.e., after * build_ports() because the reachability check requires the router ports * networks to have been parsed. @@ -4097,6 +4148,7 @@ build_lb_port_related_data(struct hmap *datapaths, struct hmap *ports, build_lrouter_lbs_check(datapaths); build_lrouter_lbs_reachable_ips(datapaths, lbs); build_lb_svcs(input_data, ovnsb_txn, ports, lbs); + build_lswitch_lbs_from_lrouter(datapaths, lbs); } /* Syncs relevant load balancers (applied to logical switches) to the @@ -15611,6 +15663,10 @@ ovnnb_db_run(struct northd_input *input_data, "ignore_lsp_down", true); default_acl_drop = smap_get_bool(&nb->options, "default_acl_drop", false); + install_ls_lb_from_router = smap_get_bool(&nb->options, + "install_ls_lb_from_router", + false); + build_datapaths(input_data, ovnsb_txn, &data->datapaths, &data->lr_list); build_lbs(input_data, &data->datapaths, &data->lbs); build_ports(input_data, ovnsb_txn, sbrec_chassis_by_name, diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index ea311c7c0..9297c0c46 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -7289,3 +7289,62 @@ AT_CHECK([diff flows1 flows3]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([check install_ls_lb_from_router option]) +ovn_start + +ovn-nbctl lr-add R1 +ovn-nbctl set logical_router R1 options:chassis=hv1 +ovn-nbctl lrp-add R1 R1-S0 02:ac:10:01:00:01 10.0.0.1/24 +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:01:01 20.0.0.1/24 +ovn-nbctl lrp-add R1 R1-PUB 02:ac:20:01:01:01 172.16.0.1/24 + +ovn-nbctl ls-add S0 +ovn-nbctl lsp-add S0 S0-R1 +ovn-nbctl lsp-set-type S0-R1 router +ovn-nbctl lsp-set-addresses S0-R1 02:ac:10:01:00:01 +ovn-nbctl lsp-set-options S0-R1 router-port=R1-S0 + +ovn-nbctl ls-add S1 +ovn-nbctl lsp-add S1 S1-R1 +ovn-nbctl lsp-set-type S1-R1 router +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:01:01 +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 + +# Add load balancers on the logical router R1 +ovn-nbctl lb-add lb0 172.16.0.10:80 10.0.0.2:80 +ovn-nbctl lr-lb-add R1 lb0 + +ovn-sbctl dump-flows S0 > S0flows +ovn-sbctl dump-flows S1 > S1flows + +AT_CAPTURE_FILE([S0flows]) +AT_CAPTURE_FILE([S1flows]) + +AT_CHECK([grep "ls_in_lb" S0flows | sort], [0], [dnl + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) +]) +AT_CHECK([grep "ls_in_lb" S1flows | sort], [0], [dnl + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) +]) + +ovn-nbctl --wait=sb set NB_Global . options:install_ls_lb_from_router=true + +ovn-sbctl dump-flows S0 > S0flows +ovn-sbctl dump-flows S1 > S1flows + +AT_CAPTURE_FILE([S0flows]) +AT_CAPTURE_FILE([S1flows]) + +AT_CHECK([grep "ls_in_lb" S0flows | sort], [0], [dnl + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) + table=12(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) +]) +AT_CHECK([grep "ls_in_lb" S1flows | sort], [0], [dnl + table=12(ls_in_lb ), priority=0 , match=(1), action=(next;) + table=12(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) +]) + +AT_CLEANUP +])
Add the capability to automatically deploy a load-balancer on each logical-switch connected to a logical router where the load-balancer has been installed by the CMS. This patch allow to overcome the distributed gw router scenario limitation where a load-balancer must be installed on each datapath to properly reach the load-balancer. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2043543 Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/northd.c | 56 ++++++++++++++++++++++++++++++++++++++++++ tests/ovn-northd.at | 59 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+)