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. 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 > <mailto:vlodint...@croc.ru>> wrote: > > > > Thanks Numan. > > > > regards, > > Vladislav Odintsov > > > > > On 16 Aug 2022, at 04:56, Numan Siddique <num...@ovn.org > > > <mailto:num...@ovn.org>> wrote: > > > > > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov <odiv...@gmail.com > > > <mailto: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 > > >> <mailto: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 > > >> <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 <http://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 <http://ovn-northd.at/> > > >> b/tests/ovn-northd.at <http://ovn-northd.at/> > > >> index 5b5eeb0ee..3ffa39419 100644 > > >> --- a/tests/ovn-northd.at <http://ovn-northd.at/> > > >> +++ b/tests/ovn-northd.at <http://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 > > >> <http://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 <mailto:d...@openvswitch.org> > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > _______________________________________________ > > > dev mailing list > > > d...@openvswitch.org <mailto:d...@openvswitch.org> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > _______________________________________________ > > dev mailing list > > d...@openvswitch.org <mailto:d...@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
