[ovs-dev] [PATCH v2] OVN: select a random mac_prefix if not provided

[Lorenzo Bianconi]

> On Tue, Feb 26, 2019 at 11:41 PM Lorenzo Bianconi <
> lorenzo.bianconi at redhat.com> wrote:
> 
> > Select a random IPAM mac_prefix if it has not been provided by the user.
> > With this patch the admin can avoid to configure mac_prefix in order to
> > avoid L2 address collisions if multiple OVN deployments share the same
> > broadcast domain.
> > Remove MAC_ADDR_PREFIX definitions/occurrences since now mac_prefix is
> > always provided to ovn-northd
> >
> > Tested-by: Miguel Duarte de Mora Barroso <mdbarroso at redhat.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> >
> 
> Looks good to me. Just a  comment below.
> 
> 
> 
> > ---
> > Changes since v1:
> > - add entry in NEWS
> > - do not modify the idl object in-place but update it running
> >   nbrec_nb_global_set_options
> > ---
> >  NEWS                    |  2 ++
> >  ovn/northd/ovn-northd.c | 35 ++++++++++++++++-------------------
> >  tests/ovn.at            |  3 +++
> >  3 files changed, 21 insertions(+), 19 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index c86c13a23..b3b347036 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -17,6 +17,8 @@ Post-v2.11.0
> >         conntrack fragmentation support.
> >       * New "ovs-appctl dpctl/ipf-get-status" command for userspace
> > datapath
> >         conntrack fragmentation support.
> > +   - OVN:
> > +     * Select IPAM mac_prefix in a random manner if not provided by the
> > user
> >

[...]

> 
> There is no test case which  tests that ovn-northd has set the
> options:mac_prefix in the NB DB table.
> My suggestion would be to either add another test case or enhance this test
> case itself
> to  wait for the options:mac_prefix is set in NB_Global by ovn-northd ,
> read the prefix value
> and validate that the mac address of logical port "p0" is set accordingly.
> And then you can set the mac_prefix to "0a:00:00..."
> and expect that "p0" 's mac address is updated accordingly.
> 
> I tested manually this patch and it works as expected. It would be nice to
> have a test case so that we don't have any regressions
> later.
> 
> Thanks
> Numan

Hi Numan,

thx for the review.
Maybe I did not get exactly what you mean here but I guess 'mac_prefix' is
already tested when we create sw6
(https://github.com/openvswitch/ovs/blob/master/tests/ovn.at#L5921) since we
switch mac_prefix from 0a:00:00 to 00:11:22. Am I missing something?

Regards,
Lorenzo

> 
> 
>  ovn-nbctl ls-add sw0
> >  ovn-nbctl lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
> >  ovn-nbctl --wait=sb add Logical-Switch sw0 other_config subnet=
> > 192.168.1.0/24
> > @@ -5981,6 +5982,7 @@ ovn_start
> >  ovn-nbctl lr-add R1
> >
> >  # Test for a ping using dynamically allocated addresses.
> > +ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="0a:00:00:00:00:00"
> >  ovn-nbctl ls-add foo -- add Logical_Switch foo other_config subnet=
> > 192.168.1.0/24
> >  ovn-nbctl ls-add alice -- add Logical_Switch alice other_config subnet=
> > 192.168.2.0/24
> 
> 
> 
> 
> >
> > @@ -12232,6 +12234,7 @@ AT_CLEANUP
> >  AT_SETUP([ovn -- ipam to non-ipam])
> >  ovn_start
> >
> > +ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="0a:00:00:00:00:00"
> >  ovn-nbctl ls-add sw0
> >  ovn-nbctl lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
> >  ovn-nbctl --wait=sb add Logical-Switch sw0 other_config subnet=
> > 192.168.1.0/24
> > --
> > 2.20.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >