On Thu, Aug 18, 2022 at 12:50 PM Vladislav Odintsov <odiv...@gmail.com> wrote:
> > > Regards, > Vladislav Odintsov > > On 18 Aug 2022, at 20:07, Han Zhou <hz...@ovn.org> wrote: > > > > On Thu, Aug 18, 2022 at 8:54 AM Vladislav Odintsov <odiv...@gmail.com> > wrote: > >> Hi Han, >> >> As I could understand, admission control flows were introduced for >> "gateway/outside" LSs, with GW LRPs. >> The scenario, which was covered with this change shouldn’t overlap with a >> "gateway/outside" LS scenario: >> User creates an LS, adds normal VIF ports (VMs) to LS and additionally he >> may want to attach vtep lport to this LS to extend L2 domain outside of the >> OVN with VTEP. I can’t imaging user wants to have a L3 GW LRP here - this >> could be the only one problem place. >> > > Thanks for explaining, but I am still confused. Here is your original > description of the problem from the earlier thread: > --- > The initial problem is that user may want to create simple topology: > > <vm1> - <ls1 192.168.0.0/24> - <LR> - <ls2 192.168.1.0/24> - <vm2> > > And add VTEP LSP to ls2 (setting GW chassis to LRP in ls2). > If user wants to add GW services to this setup, VMs from ls2 will not be > able to access > external network due to lr_in_admission rules. > --- > So, the LRP to LS2 (Let's call it LRP-LS2) is the one with GW chassis set, > which means it is L3 GW LRP (DGP), right? LS2 has a DGP, a VTEP, and a VIF > (VM2), and you want to allow traffic to LRP-LS2 not only on the GW chassis, > but also on any HVs, right? Maybe you are saying, you don't connect a > localnet port to the LS2, and LS2 is an overlay network, instead of > underlay? > > > Yes this is exactly what I mean. > > But if that's the case why would you set the gateway-chassis to it at all? > > > This is the only way how to enable routing with HW VTEP gateway (which is > a L2 service). > You can check this out [1] to see more details about GW chassis for the > VTEP L3 scenario. > I see. Sorry that I didn't know the usage of gateway chassis in this way, but for the patch [1], I wonder if the VTEP chassis (where the ovn-controller-vtep is running) should be responsible for replying ARP requests for the LRP IP from the LS's ARP responder pipeline, instead of requiring a DGP to perform this? I didn't expect DGP to be required for VTEP to work. But anyway, this answers my question to the current patch. Thanks! Han > > 1: > https://github.com/ovn-org/ovn/commit/4e90bcf55c2ef1ab9940836455e4bfe36e57f307 > > > Thanks, > han > > >> And this is_chassis_redirect() check is not added to flow only if VTEP >> lport is in associated LS. All other cases left untouched. >> >> Let me know if you have any questions. >> >> Regards, >> Vladislav Odintsov >> >> On 18 Aug 2022, at 18:45, Han Zhou <hz...@ovn.org> wrote: >> >> >> On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav <vlodint...@croc.ru> >> wrote: >> > >> > Thanks Numan. >> > >> > regards, >> > Vladislav Odintsov >> > >> > > On 16 Aug 2022, at 04:56, Numan Siddique <num...@ovn.org> wrote: >> > > >> > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov < >> odiv...@gmail.com> wrote: >> > >> >> > >> If LRP's logical switch has attached LSP of vtep type, the >> > >> is_chassis_resident() part is not added to lflow to allow traffic >> > >> originated from logical switch to reach LR services (LBs, NAT). >> > >> >> >> Sorry that I just noticed this and have a question here. I think we had >> is_chassis_resident() for the admission control flows for a reason. I don't >> remember all details, but probably one of the reasons is to prevent >> underlay BUM packets to be handled by multiple chassises? >> If we disable that check, would it create problems? Is VTEP attached LS >> immune to those problems? >> >> Thanks, >> Han >> >> > >> > >> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com> >> > > >> > > Thanks for v2 and for the test case. >> > > Applied the patch to main. >> > > >> > > Numan >> > > >> > > >> > >> --- >> > >> This is a continuation from [1] as a v2 edition after Numan's review. >> > >> - reworked to use od->has_vtep_lports and removed lrp's confusing >> option >> > >> 'is_distributed' >> > >> - added related tests >> > >> - updated ovn-northd flows docs >> > >> >> > >> 1: >> https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/396796.html >> > >> --- >> > >> northd/northd.c | 33 +++++++++++++++++--- >> > >> northd/ovn-northd.8.xml | 4 +++ >> > >> tests/ovn-northd.at | 69 >> +++++++++++++++++++++++++++++++++++++++++ >> > >> 3 files changed, 102 insertions(+), 4 deletions(-) >> > >> >> > >> diff --git a/northd/northd.c b/northd/northd.c >> > >> index facd41a59..b1e9ffc87 100644 >> > >> --- a/northd/northd.c >> > >> +++ b/northd/northd.c >> > >> @@ -637,6 +637,7 @@ struct ovn_datapath { >> > >> bool has_lb_vip; >> > >> bool has_unknown; >> > >> bool has_acls; >> > >> + bool has_vtep_lports; >> > >> >> > >> /* IPAM data. */ >> > >> struct ipam_info ipam_info; >> > >> @@ -1847,6 +1848,12 @@ lsp_is_localnet(const struct >> nbrec_logical_switch_port *nbsp) >> > >> return !strcmp(nbsp->type, "localnet"); >> > >> } >> > >> >> > >> +static bool >> > >> +lsp_is_vtep(const struct nbrec_logical_switch_port *nbsp) >> > >> +{ >> > >> + return !strcmp(nbsp->type, "vtep"); >> > >> +} >> > >> + >> > >> static bool >> > >> localnet_can_learn_mac(const struct nbrec_logical_switch_port *nbsp) >> > >> { >> > >> @@ -2655,6 +2662,10 @@ join_logical_ports(struct northd_input >> *input_data, >> > >> od->localnet_ports[od->n_localnet_ports++] = op; >> > >> } >> > >> >> > >> + if (lsp_is_vtep(nbsp)) { >> > >> + od->has_vtep_lports = true; >> > >> + } >> > >> + >> > >> op->lsp_addrs >> > >> = xmalloc(sizeof *op->lsp_addrs * >> nbsp->n_addresses); >> > >> for (size_t j = 0; j < nbsp->n_addresses; j++) { >> > >> @@ -5518,7 +5529,7 @@ build_lswitch_port_sec_op(struct ovn_port *op, >> struct hmap *lflows, >> > >> ds_put_format(actions, "set_queue(%s); ", queue_id); >> > >> } >> > >> >> > >> - if (!strcmp(op->nbsp->type, "vtep")) { >> > >> + if (lsp_is_vtep(op->nbsp)) { >> > >> ds_put_format(actions, REGBIT_FROM_RAMP" = 1; "); >> > >> ds_put_format(actions, "next(pipeline=ingress, table=%d);", >> > >> ovn_stage_get_table(S_SWITCH_IN_HAIRPIN)); >> > >> @@ -10894,6 +10905,22 @@ build_gateway_mtu_flow(struct hmap *lflows, >> struct ovn_port *op, >> > >> va_end(extra_actions_args); >> > >> } >> > >> >> > >> +static bool >> > >> +consider_l3dwg_port_is_centralized(struct ovn_port *op) >> > >> +{ >> > >> + if (op->peer && op->peer->od->has_vtep_lports) { >> > >> + return false; >> > >> + } >> > >> + >> > >> + if (is_l3dgw_port(op)) { >> > >> + /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s >> > >> + * should only be received on the gateway chassis. */ >> > >> + return true; >> > >> + } >> > >> + >> > >> + return false; >> > >> +} >> > >> + >> > >> /* Logical router ingress Table 0: L2 Admission Control >> > >> * This table drops packets that the router shouldn’t see at all >> based >> > >> * on their Ethernet headers. >> > >> @@ -10930,9 +10957,7 @@ build_adm_ctrl_flows_for_lrouter_port( >> > >> ds_clear(match); >> > >> ds_put_format(match, "eth.dst == %s && inport == %s", >> > >> op->lrp_networks.ea_s, op->json_key); >> > >> - if (is_l3dgw_port(op)) { >> > >> - /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s >> > >> - * should only be received on the gateway chassis. */ >> > >> + if (consider_l3dwg_port_is_centralized(op)) { >> > >> ds_put_format(match, " && is_chassis_resident(%s)", >> > >> op->cr_port->json_key); >> > >> } >> > >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml >> > >> index ff21c0737..9b6459d9e 100644 >> > >> --- a/northd/ovn-northd.8.xml >> > >> +++ b/northd/ovn-northd.8.xml >> > >> @@ -2114,6 +2114,10 @@ output; >> > >> gateway chassis), the above flow matching >> > >> <code>eth.dst == <var>E</var></code> is only programmed on >> > >> the gateway port instance on the gateway chassis. >> > >> + If LRP's logical switch has attached LSP of >> <code>vtep</code> type, >> > >> + the <code>is_chassis_resident()</code> part is not added >> to lflow to >> > >> + allow traffic originated from logical switch to reach LR >> services >> > >> + (LBs, NAT). >> > >> </p> >> > >> >> > >> <p> >> > >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> > >> index 5b5eeb0ee..3ffa39419 100644 >> > >> --- a/tests/ovn-northd.at >> > >> +++ b/tests/ovn-northd.at >> > >> @@ -5747,6 +5747,75 @@ AT_CHECK([grep lr_in_gw_redirect lrflows | >> grep cr-DR | sed 's/table=../table=?? >> > >> AT_CLEANUP >> > >> ]) >> > >> >> > >> +OVN_FOR_EACH_NORTHD([ >> > >> +AT_SETUP([ovn-northd -- lr admission with vtep lports]) >> > >> +AT_KEYWORDS([multiple-l3dgw-ports]) >> > >> +ovn_start NORTHD_TYPE >> > >> +check ovn-sbctl chassis-add ch1 geneve 127.0.0.2 >> > >> + >> > >> +check ovn-nbctl lr-add lr1 >> > >> +check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:01 10.0.0.1/24 >> > >> +check ovn-nbctl ls-add ls1 >> > >> +check ovn-nbctl lsp-add ls1 lsp1 -- \ >> > >> + lsp-set-addresses lsp1 router -- \ >> > >> + lsp-set-type lsp1 router -- \ >> > >> + lsp-set-options lsp1 router-port=lrp1 >> > >> + >> > >> +# ensure initial flows are installed without is_chassis_resident >> match part >> > >> +ovn-nbctl --wait=sb sync >> > >> +ovn-sbctl dump-flows lr1 > lrflows >> > >> +AT_CAPTURE_FILE([lrflows]) >> > >> + >> > >> +# Check the flows in lr_in_admission stage >> > >> +AT_CHECK([grep lr_in_admission lrflows | grep lrp1 | sed >> 's/table=../table=??/' | sort], [0], [dnl >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.dst == >> 00:00:00:00:00:01 && inport == "lrp1"), action=(xreg0[[0..47]] = >> 00:00:00:00:00:01; next;) >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.mcast >> && inport == "lrp1"), action=(xreg0[[0..47]] = 00:00:00:00:00:01; next;) >> > >> +]) >> > >> + >> > >> +# make lrp a cr-port and check its flows >> > >> +check ovn-nbctl lrp-set-gateway-chassis lrp1 ch1 >> > >> + >> > >> +ovn-nbctl --wait=sb sync >> > >> +ovn-sbctl dump-flows lr1 > lrflows >> > >> +AT_CAPTURE_FILE([lrflows]) >> > >> + >> > >> +# Check the flows in lr_in_admission stage >> > >> +AT_CHECK([grep lr_in_admission lrflows | grep lrp1 | sed >> 's/table=../table=??/' | sort], [0], [dnl >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.dst == >> 00:00:00:00:00:01 && inport == "lrp1" && is_chassis_resident("cr-lrp1")), >> action=(xreg0[[0..47]] = 00:00:00:00:00:01; next;) >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.mcast >> && inport == "lrp1"), action=(xreg0[[0..47]] = 00:00:00:00:00:01; next;) >> > >> +]) >> > >> + >> > >> +# attach vtep logical port to logical switch and check flows. >> > >> +# there should not be is_chassis_resident part. >> > >> +check ovn-nbctl lsp-add ls1 lsp-vtep -- lsp-set-type lsp-vtep vtep >> > >> + >> > >> +ovn-nbctl --wait=sb sync >> > >> +ovn-sbctl dump-flows lr1 > lrflows >> > >> +AT_CAPTURE_FILE([lrflows]) >> > >> + >> > >> +# Check the flows in lr_in_admission stage >> > >> +AT_CHECK([grep lr_in_admission lrflows | grep lrp1 | sed >> 's/table=../table=??/' | sort], [0], [dnl >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.dst == >> 00:00:00:00:00:01 && inport == "lrp1"), action=(xreg0[[0..47]] = >> 00:00:00:00:00:01; next;) >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.mcast >> && inport == "lrp1"), action=(xreg0[[0..47]] = 00:00:00:00:00:01; next;) >> > >> +]) >> > >> + >> > >> +# delete vtep lport and check lrp has is_chassis_resident match >> part again. >> > >> +check ovn-nbctl lsp-del lsp-vtep >> > >> + >> > >> +ovn-nbctl --wait=sb sync >> > >> +ovn-sbctl dump-flows lr1 > lrflows >> > >> +AT_CAPTURE_FILE([lrflows]) >> > >> + >> > >> +# Check the flows in lr_in_admission stage >> > >> +AT_CHECK([grep lr_in_admission lrflows | grep lrp1 | sed >> 's/table=../table=??/' | sort], [0], [dnl >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.dst == >> 00:00:00:00:00:01 && inport == "lrp1" && is_chassis_resident("cr-lrp1")), >> action=(xreg0[[0..47]] = 00:00:00:00:00:01; next;) >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.mcast >> && inport == "lrp1"), action=(xreg0[[0..47]] = 00:00:00:00:00:01; next;) >> > >> +]) >> > >> + >> > >> +AT_CLEANUP >> > >> +]) >> > >> + >> > >> + >> > >> OVN_FOR_EACH_NORTHD([ >> > >> AT_SETUP([check options:requested-chassis fills requested_chassis >> col]) >> > >> ovn_start NORTHD_TYPE >> > >> -- >> > >> 2.36.1 >> > >> >> > >> _______________________________________________ >> > >> dev mailing list >> > >> d...@openvswitch.org >> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > _______________________________________________ >> > > dev mailing list >> > > d...@openvswitch.org >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ >> > dev mailing list >> > d...@openvswitch.org >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev