[ovs-discuss] ipam allows address conflicts?

[Mark Michelson]

Hi Ben,

Thanks for reaching out. Right now it's not obvious why big endian 
architecture would cause duplicate IPs to be assigned. My initial 
thought was that the wrong byte ordering was being used when adding IP 
addresses to IPAM, but it appears that host byte ordering is being used 
consistently when performing the calculations.

James, is that the only IPAM-related test failure you are seeing? If so, 
that can help narrow where I need to look to fix this. Thanks.

On 3/5/19 9:04 PM, Ben Pfaff wrote:
> [adding some folks who've worked on IPAM lately]
> 
> On Tue, Mar 05, 2019 at 10:26:58AM +0000, James Page wrote:
>> Test 2768 is consistently failing on s390x in Ubuntu development.
>>
>> Note that this is the only big-endian architecture we build for so I would
>> suspect something endian-y as the root cause.
> 
> Yes, it's related to big-endian, in particular related to the OVS hash
> function, which produces different results depending on endianness.
> I can reproduce it on amd64 by changing the hash function:
> 
> ----------------------------------------------------------------------
> diff --git a/lib/hash.h b/lib/hash.h
> index eb3776500abe..c7fc6430c71d 100644
> --- a/lib/hash.h
> +++ b/lib/hash.h
> @@ -1,5 +1,5 @@
>   /*
> - * Copyright (c) 2008, 2009, 2010, 2012, 2013, 2014, 2016 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2012, 2013, 2014, 2016, 2019 Nicira, Inc.
>    *
>    * Licensed under the Apache License, Version 2.0 (the "License");
>    * you may not use this file except in compliance with the License.
> @@ -87,6 +87,7 @@ static inline uint32_t mhash_finish(uint32_t hash)
>       hash ^= hash >> 13;
>       hash *= 0xc2b2ae35;
>       hash ^= hash >> 16;
> +    hash = ~hash;
>       return hash;
>   }
>   
> @@ -184,7 +185,7 @@ static inline uint32_t hash_finish(uint64_t hash, uint64_t final)
>       /* The finishing multiplier 0x805204f3 has been experimentally
>        * derived to pass the testsuite hash tests. */
>       hash = _mm_crc32_u64(hash, final) * 0x805204f3;
> -    return hash ^ (uint32_t)hash >> 16; /* Increase entropy in LSBs. */
> +    return ~(hash ^ (uint32_t)hash >> 16); /* Increase entropy in LSBs. */
>   }
>   
>   /* Returns the hash of the 'n' 32-bit words at 'p_', starting from 'basis'.
> ----------------------------------------------------------------------
> 
> The actual failure might be due to a more serious bug though.  When I
> apply the following patch:
> 
> ----------------------------------------------------------------------
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 2af225a678e1..a53204f544e9 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -12273,6 +12273,8 @@ for i in 2 3 4; do
>       ovn-nbctl --wait=sb lsp-set-addresses swp$i "02:00:00:00:00:0$i dynamic"
>       cidr=$(ovn-nbctl get logical_switch_port swp$i dynamic_addresses |cut -f2 -d' '|cut -f1 -d\")
>       ovn-nbctl lrp-add ro$i rop$i 02:00:00:00:00:0$i $cidr/24 -- set logical_switch_port swp$i type=router options:router-port=rop$i addresses=router;
> +    printf "\nadded swp$i\n"
> +    ovn-nbctl list logical_switch_port
>       AT_CHECK_UNQUOTED([ovn-nbctl get logical_router_port rop$i networks], [0], [[["192.168.1.$i/24"]]
>   ])
>   done
> ----------------------------------------------------------------------
> 
> the test then reports that ports swp2 and swp3 are both being assigned
> dynamic address 192.168.1.2 (instead of 192.168.1.2 and 192.168.1.3,
> respectively), as you can see from the "dynamic_addresses" lines
> following "added swp3":
> 
> ----------------------------------------------------------------------
> 2772. ovn.at:12264: testing ovn -- ipam router ports ...
> creating ovn-sb database
> creating ovn-nb database
> starting ovn-northd
> starting backup ovn-northd
> 
> added swp2
> _uuid               : d78add30-9d8e-4d49-bea6-8f4bc947ab3b
> addresses           : [router]
> dhcpv4_options      : []
> dhcpv6_options      : []
> dynamic_addresses   : "02:00:00:00:00:02 192.168.1.2"
> enabled             : []
> external_ids        : {}
> name                : "swp2"
> options             : {router-port="rop2"}
> parent_name         : []
> port_security       : []
> tag                 : []
> tag_request         : []
> type                : router
> up                  : true
> ../../tests/ovn.at:12278: ovn-nbctl get logical_router_port rop$i networks
> 
> added swp3
> _uuid               : d78add30-9d8e-4d49-bea6-8f4bc947ab3b
> addresses           : [router]
> dhcpv4_options      : []
> dhcpv6_options      : []
> dynamic_addresses   : "02:00:00:00:00:02 192.168.1.2"
> enabled             : []
> external_ids        : {}
> name                : "swp2"
> options             : {router-port="rop2"}
> parent_name         : []
> port_security       : []
> tag                 : []
> tag_request         : []
> type                : router
> up                  : true
> 
> _uuid               : 0542f623-43a8-407d-adbe-0927999c78b1
> addresses           : [router]
> dhcpv4_options      : []
> dhcpv6_options      : []
> dynamic_addresses   : "02:00:00:00:00:03 192.168.1.2"
> enabled             : []
> external_ids        : {}
> name                : "swp3"
> options             : {router-port="rop3"}
> parent_name         : []
> port_security       : []
> tag                 : []
> tag_request         : []
> type                : router
> up                  : true
> ../../tests/ovn.at:12278: ovn-nbctl get logical_router_port rop$i networks
> --- -   2019-03-05 17:58:01.066306125 -0800
> +++ /home/blp/nicira/ovs/_build/tests/testsuite.dir/at-groups/2772/stdout       2019-03-05 17:58:01.060040470 -0800
> @@ -1,2 +1,2 @@
> -["192.168.1.3/24"]
> +["192.168.1.2/24"]
>   
> ----------------------------------------------------------------------
> 
> Mark and Lorenzo, this seems like something you should look into?
> 
> Thanks,
> 
> Ben.
>