mbox series

[ovs-dev,ovn,0/6,v4] Support logical switches with multiple localnet ports

Message ID 20200511140904.209409-1-ihrachys@redhat.com
Headers show
Series Support logical switches with multiple localnet ports | expand

Message

Ihar Hrachyshka May 11, 2020, 2:08 p.m. UTC
Hi all,

This series is to allow for multiple localnet ports to be present in a
logical switch. Even before the series, it was allowed to create
multiple ports of this kind but they were handled inconsistently.

This series uses logical switches with multiple localnet ports
(LSMLP) to implement what is called "routed provider networks" in
OpenStack. To elaborate, this allows to natively model a network that
has multiple segments with implied L3 routing between the segments
realized by the fabric. A user can operate such a network as a single
entity instead of deciding on which segment to choose for their
port bindings.

The assumption in this implementation is that while a logical switch can
have multiple localnet ports, each chassis has only one of corresponding
physical networks mapped. Meaning, if the same LS has localnet ports for
networks A, B, and C, then each chassis can either be mapped to A, B, or
C, but not several of the networks. (Note this doesn't mean that a
chassis can't be mapped to network D, as long as it doesn't have a
corresponding localnet port in this same logical switch.)

Known bugs:
- test case validating broadcast behavior fails on slow machines because
  of other service traffic getting in the way. Working on it.

Ihar Hrachyshka (6):
  Spin out flow generation into build_dhcpv4_options_flows
  Spin out flow generation into build_dhcpv6_options_flows
  Spin out flow generation into build_pre_acl_flows_for_nbsp
  Spin out flow generation into
    build_drop_arp_nd_flows_for_unbound_router_ports
  Support logical switches with multiple localnet ports
  Log missing bridge per localnet port just once

---
v2: rebase on top of series that refactors code dealing with localnet ports.
v2: tests: send packets both ways, more test scenarios covered.
v2: use x2nrealloc to allocate ->localnet_ports.
v2: use n_localnet_ports counter instead of localnet_ports pointer to detect
    switches with localnet ports.
v3: adjusted documentation to be more explicit about how multiple localnet
    ports scenario should be used in practice.
v3: more tests (broadcast, multiple co-hosted switches with multiple
    localnet ports)
v4: sent as a series, fixed test description to reflect we test
    broadcast only.
---

 controller/binding.c   |  16 ++
 controller/patch.c     |  34 ++-
 northd/ovn-northd.c    | 485 ++++++++++++++++++++-------------------
 ovn-architecture.7.xml |  50 +++--
 ovn-nb.xml             |  23 +-
 ovn-sb.xml             |  21 +-
 tests/ovn.at           | 500 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 870 insertions(+), 259 deletions(-)

Comments

Numan Siddique May 11, 2020, 4:03 p.m. UTC | #1
On Mon, May 11, 2020 at 7:39 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote:

> Hi all,
>
> This series is to allow for multiple localnet ports to be present in a
> logical switch. Even before the series, it was allowed to create
> multiple ports of this kind but they were handled inconsistently.
>
> This series uses logical switches with multiple localnet ports
> (LSMLP) to implement what is called "routed provider networks" in
> OpenStack. To elaborate, this allows to natively model a network that
> has multiple segments with implied L3 routing between the segments
> realized by the fabric. A user can operate such a network as a single
> entity instead of deciding on which segment to choose for their
> port bindings.
>
> The assumption in this implementation is that while a logical switch can
> have multiple localnet ports, each chassis has only one of corresponding
> physical networks mapped. Meaning, if the same LS has localnet ports for
> networks A, B, and C, then each chassis can either be mapped to A, B, or
> C, but not several of the networks. (Note this doesn't mean that a
> chassis can't be mapped to network D, as long as it doesn't have a
> corresponding localnet port in this same logical switch.)
>
> Known bugs:
> - test case validating broadcast behavior fails on slow machines because
>   of other service traffic getting in the way. Working on it.
>
> Ihar Hrachyshka (6):
>   Spin out flow generation into build_dhcpv4_options_flows
>   Spin out flow generation into build_dhcpv6_options_flows
>   Spin out flow generation into build_pre_acl_flows_for_nbsp
>   Spin out flow generation into
>     build_drop_arp_nd_flows_for_unbound_router_ports
>   Support logical switches with multiple localnet ports
>   Log missing bridge per localnet port just once
>
> ---
> v2: rebase on top of series that refactors code dealing with localnet
> ports.
> v2: tests: send packets both ways, more test scenarios covered.
> v2: use x2nrealloc to allocate ->localnet_ports.
> v2: use n_localnet_ports counter instead of localnet_ports pointer to
> detect
>     switches with localnet ports.
> v3: adjusted documentation to be more explicit about how multiple localnet
>     ports scenario should be used in practice.
> v3: more tests (broadcast, multiple co-hosted switches with multiple
>     localnet ports)
> v4: sent as a series, fixed test description to reflect we test
>     broadcast only.
> ---
>

For Patches 1 to 4 - Acked-by: Numan Siddique <numans@ovn.org>

With one suggestion

Can you move the out function arguments  (arguments which gets modified) to
at the end.

Eg.

Change the below from

static void
build_dhcpv4_options_flows(struct ovn_port *op, struct hmap *lflows,
                           struct lport_addresses *lsp_addrs,
                           const char *json_key, bool is_external)

TO

static void
build_dhcpv4_options_flows(struct ovn_port *op,
                           struct lport_addresses *lsp_addrs,
                           const char *json_key, bool is_external,
                            struct hmap *lflows)

Since 'lflows' is modified.

I know that ovn-northd.c has many functions which don't follow this
method, but we can do so for newer functions atleast :).

Thanks
Numan








>  controller/binding.c   |  16 ++
>  controller/patch.c     |  34 ++-
>  northd/ovn-northd.c    | 485 ++++++++++++++++++++-------------------
>  ovn-architecture.7.xml |  50 +++--
>  ovn-nb.xml             |  23 +-
>  ovn-sb.xml             |  21 +-
>  tests/ovn.at           | 500 +++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 870 insertions(+), 259 deletions(-)
>
> --
> 2.26.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Ihar Hrachyshka May 11, 2020, 5:01 p.m. UTC | #2
On 5/11/20 12:03 PM, Numan Siddique wrote:
> On Mon, May 11, 2020 at 7:39 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote:
>
>> Hi all,
>>
>> This series is to allow for multiple localnet ports to be present in a
>> logical switch. Even before the series, it was allowed to create
>> multiple ports of this kind but they were handled inconsistently.
>>
>> This series uses logical switches with multiple localnet ports
>> (LSMLP) to implement what is called "routed provider networks" in
>> OpenStack. To elaborate, this allows to natively model a network that
>> has multiple segments with implied L3 routing between the segments
>> realized by the fabric. A user can operate such a network as a single
>> entity instead of deciding on which segment to choose for their
>> port bindings.
>>
>> The assumption in this implementation is that while a logical switch can
>> have multiple localnet ports, each chassis has only one of corresponding
>> physical networks mapped. Meaning, if the same LS has localnet ports for
>> networks A, B, and C, then each chassis can either be mapped to A, B, or
>> C, but not several of the networks. (Note this doesn't mean that a
>> chassis can't be mapped to network D, as long as it doesn't have a
>> corresponding localnet port in this same logical switch.)
>>
>> Known bugs:
>> - test case validating broadcast behavior fails on slow machines because
>>    of other service traffic getting in the way. Working on it.
>>
>> Ihar Hrachyshka (6):
>>    Spin out flow generation into build_dhcpv4_options_flows
>>    Spin out flow generation into build_dhcpv6_options_flows
>>    Spin out flow generation into build_pre_acl_flows_for_nbsp
>>    Spin out flow generation into
>>      build_drop_arp_nd_flows_for_unbound_router_ports
>>    Support logical switches with multiple localnet ports
>>    Log missing bridge per localnet port just once
>>
>> ---
>> v2: rebase on top of series that refactors code dealing with localnet
>> ports.
>> v2: tests: send packets both ways, more test scenarios covered.
>> v2: use x2nrealloc to allocate ->localnet_ports.
>> v2: use n_localnet_ports counter instead of localnet_ports pointer to
>> detect
>>      switches with localnet ports.
>> v3: adjusted documentation to be more explicit about how multiple localnet
>>      ports scenario should be used in practice.
>> v3: more tests (broadcast, multiple co-hosted switches with multiple
>>      localnet ports)
>> v4: sent as a series, fixed test description to reflect we test
>>      broadcast only.
>> ---
>>
> For Patches 1 to 4 - Acked-by: Numan Siddique <numans@ovn.org>
>
> With one suggestion
>
> Can you move the out function arguments  (arguments which gets modified) to
> at the end.


Thanks! Fixed, pushed a new series version (with a fix for the test case 
failure mentioned in the cover letter).


>
> Eg.
>
> Change the below from
>
> static void
> build_dhcpv4_options_flows(struct ovn_port *op, struct hmap *lflows,
>                             struct lport_addresses *lsp_addrs,
>                             const char *json_key, bool is_external)
>
> TO
>
> static void
> build_dhcpv4_options_flows(struct ovn_port *op,
>                             struct lport_addresses *lsp_addrs,
>                             const char *json_key, bool is_external,
>                              struct hmap *lflows)
>
> Since 'lflows' is modified.
>
> I know that ovn-northd.c has many functions which don't follow this
> method, but we can do so for newer functions atleast :).
>
> Thanks
> Numan
>
>
>
>
>
>
>
>
>>   controller/binding.c   |  16 ++
>>   controller/patch.c     |  34 ++-
>>   northd/ovn-northd.c    | 485 ++++++++++++++++++++-------------------
>>   ovn-architecture.7.xml |  50 +++--
>>   ovn-nb.xml             |  23 +-
>>   ovn-sb.xml             |  21 +-
>>   tests/ovn.at           | 500 +++++++++++++++++++++++++++++++++++++++++
>>   7 files changed, 870 insertions(+), 259 deletions(-)
>>
>> --
>> 2.26.2
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>