| Message ID | 20210707032032.2298941-1-ihrachys@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] Don't suppress localport traffic directed to external port | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
Bleep bloop. Greetings Ihar Hrachyshka, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: Failed to merge in the changes. hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 Don't suppress localport traffic directed to external port When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 7/7/21 5:20 AM, Ihar Hrachyshka wrote: > Recently, we stopped leaking localport traffic through localnet ports > into fabric to avoid unnecessary flipping between chassis hosting the > same localport. > > Despite the type name, in some scenarios localports are supposed to talk > outside the hosting chassis. Specifically, in OpenStack [1] metadata > service for SR-IOV ports is implemented as a localport hosted on another > chassis that is exposed to the chassis owning the SR-IOV port through an > "external" port. In this case, "leaking" localport traffic into fabric > is desirable. > > This patch inserts a higher priority flow per external port on the > same datapath that avoids dropping localport traffic. > > Fixes: 96959e56d634 ("physical: do not forward traffic from localport to > a localnet one") > > [1] https://docs.openstack.org/neutron/latest/admin/ovn/sriov.html > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > --- Hi Ihar, Thanks for working on this! I just had a glance at the change so this is not a full review. > controller/physical.c | 48 ++++++++++++++++++++++++++++++++++++++ > tests/ovn.at | 54 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 102 insertions(+) > > diff --git a/controller/physical.c b/controller/physical.c > index 17ca5afbb..c2de30941 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -920,6 +920,7 @@ get_binding_peer(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > static void > consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > + const struct sbrec_port_binding_table *pb_table, > enum mf_field_id mff_ovn_geneve, > const struct simap *ct_zones, > const struct sset *active_tunnels, > @@ -1281,6 +1282,49 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 160, > binding->header_.uuid.parts[0], &match, > ofpacts_p, &binding->header_.uuid); > + > + /* Localport traffic directed to external is *not* local. */ > + const struct sbrec_port_binding *peer; > + SBREC_PORT_BINDING_TABLE_FOR_EACH (peer, pb_table) { > + if (strcmp(peer->type, "external")) { > + continue; > + } > + if (peer->datapath->tunnel_key != dp_key) { > + continue; > + } > + if (strcmp(peer->chassis->name, chassis->name)) { > + continue; > + } Won't this create a scalability issue? If I'm reading this correctly, every time consider_port_binding() is called for a localnet port we'll be walking all port_bindings in the SB DB table (there can be a lot of them in scaled scenarios) and skip most of them because they're not of type external or they're not owned by the local chassis or they're on a different datapath. One option would be to use an IDL index instead (although that's still log(n) complexity for every localnet port, I think). Another option would be to precompute the set of external ports for each datapath so we don't have to walk all ports every time. > + > + ofpbuf_clear(ofpacts_p); > + for (int i = 0; i < MFF_N_LOG_REGS; i++) { > + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); > + } > + put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p); > + > + for (int i = 0; i < peer->n_mac; i++) { > + char *err_str; > + struct eth_addr peer_mac; > + if ((err_str = str_to_mac(peer->mac[i], &peer_mac))) { > + VLOG_WARN("Parsing MAC failed for external port: %s, " > + "with error: %s", peer->logical_port, err_str); This probably needs rate limiting. Regards, Dumitru
On Wed, Jul 7, 2021 at 4:16 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 7/7/21 5:20 AM, Ihar Hrachyshka wrote: > > Recently, we stopped leaking localport traffic through localnet ports > > into fabric to avoid unnecessary flipping between chassis hosting the > > same localport. > > > > Despite the type name, in some scenarios localports are supposed to talk > > outside the hosting chassis. Specifically, in OpenStack [1] metadata > > service for SR-IOV ports is implemented as a localport hosted on another > > chassis that is exposed to the chassis owning the SR-IOV port through an > > "external" port. In this case, "leaking" localport traffic into fabric > > is desirable. > > > > This patch inserts a higher priority flow per external port on the > > same datapath that avoids dropping localport traffic. > > > > Fixes: 96959e56d634 ("physical: do not forward traffic from localport to > > a localnet one") > > > > [1] https://docs.openstack.org/neutron/latest/admin/ovn/sriov.html > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > > --- > > Hi Ihar, > > Thanks for working on this! > > I just had a glance at the change so this is not a full review. > > > controller/physical.c | 48 ++++++++++++++++++++++++++++++++++++++ > > tests/ovn.at | 54 +++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 102 insertions(+) > > > > diff --git a/controller/physical.c b/controller/physical.c > > index 17ca5afbb..c2de30941 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -920,6 +920,7 @@ get_binding_peer(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > > static void > > consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > + const struct sbrec_port_binding_table *pb_table, > > enum mf_field_id mff_ovn_geneve, > > const struct simap *ct_zones, > > const struct sset *active_tunnels, > > @@ -1281,6 +1282,49 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 160, > > binding->header_.uuid.parts[0], &match, > > ofpacts_p, &binding->header_.uuid); > > + > > + /* Localport traffic directed to external is *not* local. */ > > + const struct sbrec_port_binding *peer; > > + SBREC_PORT_BINDING_TABLE_FOR_EACH (peer, pb_table) { > > + if (strcmp(peer->type, "external")) { > > + continue; > > + } > > + if (peer->datapath->tunnel_key != dp_key) { > > + continue; > > + } > > + if (strcmp(peer->chassis->name, chassis->name)) { > > + continue; > > + } > > Won't this create a scalability issue? If I'm reading this correctly, > every time consider_port_binding() is called for a localnet port we'll > be walking all port_bindings in the SB DB table (there can be a lot of > them in scaled scenarios) and skip most of them because they're not of > type external or they're not owned by the local chassis or they're on a > different datapath. > > One option would be to use an IDL index instead (although that's still > log(n) complexity for every localnet port, I think). Another option > would be to precompute the set of external ports for each datapath so we > don't have to walk all ports every time. > Thanks for the comment. I went with precalculation like we do for local_datapath->localnet_port. I hope I haven't missed any free() / clear () / destroy() calls. See v3 of the patch I just sent. > > + > > + ofpbuf_clear(ofpacts_p); > > + for (int i = 0; i < MFF_N_LOG_REGS; i++) { > > + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); > > + } > > + put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p); > > + > > + for (int i = 0; i < peer->n_mac; i++) { > > + char *err_str; > > + struct eth_addr peer_mac; > > + if ((err_str = str_to_mac(peer->mac[i], &peer_mac))) { > > + VLOG_WARN("Parsing MAC failed for external port: %s, " > > + "with error: %s", peer->logical_port, err_str); > > This probably needs rate limiting. > Done. > Regards, > Dumitru > Thanks for the review, Dumitru. I hope v3 is somewhat better. Let me know. Cheers, Ihar
diff --git a/controller/physical.c b/controller/physical.c index 17ca5afbb..c2de30941 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -920,6 +920,7 @@ get_binding_peer(struct ovsdb_idl_index *sbrec_port_binding_by_name, static void consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, + const struct sbrec_port_binding_table *pb_table, enum mf_field_id mff_ovn_geneve, const struct simap *ct_zones, const struct sset *active_tunnels, @@ -1281,6 +1282,49 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 160, binding->header_.uuid.parts[0], &match, ofpacts_p, &binding->header_.uuid); + + /* Localport traffic directed to external is *not* local. */ + const struct sbrec_port_binding *peer; + SBREC_PORT_BINDING_TABLE_FOR_EACH (peer, pb_table) { + if (strcmp(peer->type, "external")) { + continue; + } + if (peer->datapath->tunnel_key != dp_key) { + continue; + } + if (strcmp(peer->chassis->name, chassis->name)) { + continue; + } + + ofpbuf_clear(ofpacts_p); + for (int i = 0; i < MFF_N_LOG_REGS; i++) { + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); + } + put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p); + + for (int i = 0; i < peer->n_mac; i++) { + char *err_str; + struct eth_addr peer_mac; + if ((err_str = str_to_mac(peer->mac[i], &peer_mac))) { + VLOG_WARN("Parsing MAC failed for external port: %s, " + "with error: %s", peer->logical_port, err_str); + free(err_str); + continue; + } + + match_init_catchall(&match); + match_set_metadata(&match, htonll(dp_key)); + match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, + port_key); + match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0, + MLF_LOCALPORT, MLF_LOCALPORT); + match_set_dl_dst(&match, peer_mac); + + ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 170, + binding->header_.uuid.parts[0], &match, + ofpacts_p, &binding->header_.uuid); + } + } } } else if (!tun && !is_ha_remote) { @@ -1304,6 +1348,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, /* Resubmit to table 33. */ put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p); + ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, binding->header_.uuid.parts[0], &match, ofpacts_p, &binding->header_.uuid); @@ -1504,6 +1549,7 @@ physical_handle_port_binding_changes(struct physical_ctx *p_ctx, ofctrl_remove_flows(flow_table, &binding->header_.uuid); } consider_port_binding(p_ctx->sbrec_port_binding_by_name, + p_ctx->port_binding_table, p_ctx->mff_ovn_geneve, p_ctx->ct_zones, p_ctx->active_tunnels, p_ctx->local_datapaths, @@ -1684,6 +1730,7 @@ physical_run(struct physical_ctx *p_ctx, const struct sbrec_port_binding *binding; SBREC_PORT_BINDING_TABLE_FOR_EACH (binding, p_ctx->port_binding_table) { consider_port_binding(p_ctx->sbrec_port_binding_by_name, + p_ctx->port_binding_table, p_ctx->mff_ovn_geneve, p_ctx->ct_zones, p_ctx->active_tunnels, p_ctx->local_datapaths, binding, p_ctx->chassis, @@ -1932,6 +1979,7 @@ physical_handle_ovs_iface_changes(struct physical_ctx *p_ctx, simap_put(&localvif_to_ofport, iface_id, ofport); consider_port_binding(p_ctx->sbrec_port_binding_by_name, + p_ctx->port_binding_table, p_ctx->mff_ovn_geneve, p_ctx->ct_zones, p_ctx->active_tunnels, p_ctx->local_datapaths, diff --git a/tests/ovn.at b/tests/ovn.at index 8fd200cca..917c91b1f 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -12139,6 +12139,60 @@ OVN_CLEANUP([hv1]) AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn -- localport doesn't suppress gARP directed to external port]) + +send_garp() { + local inport=$1 eth_src=$2 eth_dst=$3 spa=$4 tpa=$5 + local request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa} + ovs-appctl netdev-dummy/receive $inport $request +} + +ovn_start +net_add n1 + +check ovs-vsctl add-br br-phys +check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys +ovn_attach n1 br-phys 192.168.0.1 + +check ovn-nbctl ls-add ls + +# create topology to allow to talk from localport through localnet to external +check ovs-vsctl add-port br-int lp -- set Interface lp external-ids:iface-id=lp +check ovn-nbctl lsp-add ls lp +check ovn-nbctl lsp-set-addresses lp "00:00:00:00:00:01 10.0.0.1" +check ovn-nbctl lsp-set-type lp localport + +check ovn-nbctl lsp-add ls ln +check ovn-nbctl lsp-set-addresses ln unknown +check ovn-nbctl lsp-set-type ln localnet +check ovn-nbctl lsp-set-options ln network_name=phys + +check ovn-nbctl --wait=sb ha-chassis-group-add hagrp +check ovn-nbctl --wait=sb ha-chassis-group-add-chassis hagrp main 10 + +check ovn-nbctl lsp-add ls lext +check ovn-nbctl lsp-set-addresses lext "00:00:00:00:00:02 10.0.0.2" +check ovn-nbctl lsp-set-type lext external + +# own external port +hagrp_uuid=`ovn-nbctl --bare --columns _uuid find ha_chassis_group name=hagrp` +check ovn-nbctl set logical_switch_port lext ha_chassis_group=$hagrp_uuid + +check ovn-nbctl --wait=hv sync + +spa=$(ip_to_hex 10 0 0 1) +tpa=$(ip_to_hex 10 0 0 2) +send_garp lp 000000000001 000000000002 $spa $tpa + +dnl external traffic from localport should be sent to localnet +AT_CHECK([tcpdump -r main/br-phys_n1-tx.pcap arp[[24:4]]=0x0a000002 | wc -l],[0],[dnl +1 +],[ignore]) + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD([ AT_SETUP([ovn -- 1 LR with HA distributed router gateway port]) ovn_start
Recently, we stopped leaking localport traffic through localnet ports into fabric to avoid unnecessary flipping between chassis hosting the same localport. Despite the type name, in some scenarios localports are supposed to talk outside the hosting chassis. Specifically, in OpenStack [1] metadata service for SR-IOV ports is implemented as a localport hosted on another chassis that is exposed to the chassis owning the SR-IOV port through an "external" port. In this case, "leaking" localport traffic into fabric is desirable. This patch inserts a higher priority flow per external port on the same datapath that avoids dropping localport traffic. Fixes: 96959e56d634 ("physical: do not forward traffic from localport to a localnet one") [1] https://docs.openstack.org/neutron/latest/admin/ovn/sriov.html Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> --- controller/physical.c | 48 ++++++++++++++++++++++++++++++++++++++ tests/ovn.at | 54 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+)