Hi Simon,
thanks for the review.
I agree with all your comments and I’ll address them in v2.
Regards,
Vladislav Odintsov
> On 18 Jan 2023, at 16:08, Simon Horman <simon.hor...@corigine.com> wrote:
>
> On Thu, Dec 22, 2022 at 08:43:08PM +0300, Vladislav Odintsov wrote:
>> There were two issues prior to this patch:
>> 1. It was unable to have connectivity to networks over a router in
>> physical network connected through VTEP (ramp) gateway.
>> Consider next topology:
>>
>> ovn-nbctl lr-add lr1
>> ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:01 10.0.0.1/24
>> ovn-nbctl ls-add ls1
>> ovn-nbctl lrp-add ls1 lsp1 -- \
>> lsp-set-addresses lsp1 router -- \
>> lsp-set-type lsp1 router -- \
>> lsp-set-options lsp1 router-port=lrp1
>> ovn-nbctl lsp-add ls1 lsp-vtep -- \
>> lsp-set-type lsp-vtep vtep -- \
>> lsp-set-addresses lsp-vtep unknown -- \
>> lsp-set-options lsp-vtep vtep-physical-switch=<..>
>> vtep-logical-switch=<..>
>> ovn-nbctl lr-route-add lr1 192.168.0.0/24 10.0.0.100
>>
>> If one issues ping from lsp1 to some address from 192.168.0.0/24 (via
>> vtep lsp), to enable routing support with vtep it is required to set
>> redirect chassis or ha chassis group on lrp1. This topology didn't
>> provide connectivity. Now such traffic flow will work properly.
>>
>> 2. Traffic from lport in one subnet to vtep lport in another subnet of
>> same LR previously traversed via l3gw chassis, now in 'to vtep lport'
>> direction goes directly from hypervisor handling lport to VTEP (RAMP)
>> switch. In the opposite direction traffic still goes from VTEP (RAMP)
>> switch through l3gw chassis and then to hypervisor.
>
> I think it would be good to explain how the logic changes
> addresses these problems. And breifly describe which tests have been added.
>
>>
>> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com>
>> ---
>> northd/northd.c | 16 +++++++++++++++-
>> tests/ovn-northd.at | 26 +++++++++++++++++++++++++-
>> 2 files changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 4751feab4..07fb0ab9a 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -3362,7 +3362,12 @@ ovn_port_update_sbrec(struct northd_input *input_data,
>> }
>> smap_add(&new, "distributed-port", op->nbrp->name);
>>
>> - bool always_redirect = !op->od->has_distributed_nat;
>> + bool always_redirect = !(
>> + op->od->has_distributed_nat ||
>> + (op->l3dgw_port->peer &&
>> + op->l3dgw_port->peer->od->has_vtep_lports)
>> + );
>
> This seems slightly easier on my brain:
>
> bool always_redirect =
> !op->od->has_distributed_nat &&
> !(op->l3dgw_port->peer &&
> op->l3dgw_port->peer->od->has_vtep_lports)
>
>> +
>> if (redirect_type) {
>> smap_add(&new, "redirect-type", redirect_type);
>> /* XXX Why can't we enable always-redirect when redirect-type
>> @@ -12815,6 +12820,15 @@ build_gateway_redirect_flows_for_lrouter(
>> return;
>> }
>> for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
>> + if (od->l3dgw_ports[i]->peer &&
>> + od->l3dgw_ports[i]->peer->od->has_vtep_lports) {
>
> This condition appears twice in this patch.
> Perhaps a helper would make its meaning more obvious.
>
>> + /* Skip adding redirect rule for vtep-enabled l3dgw ports.
>> + Traffic from hypervisor to VTEP (ramp) switch should go in
>> + distributed manner. Only returning routed traffic must go
>> + through centralized gateway (or ha-chassis-group). */
>
> nit:
> /* Multi-line comments
> * look like this. */
>
>> + continue;
>> + }
>> +
>> const struct ovsdb_idl_row *stage_hint = NULL;
>> bool add_def_flow = true;
>>
>
> ...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
