[ovs-dev] [PATCH v2] ovn-controller: Fix chassis ovn-sbdb record init

[Ben Pfaff]

On Wed, May 22, 2019 at 01:26:03PM +0200, Dumitru Ceara wrote:
> The chassis_run code didn't take into account the scenario when the
> system-id was changed in the Open_vSwitch table. Due to this the code
> was trying to insert a new Chassis record in the OVN_Southbound DB with
> the same Encaps as the previous Chassis record. The transaction used
> to insert the new records was aborting due to the ["type", "ip"]
> index constraint violation as we were creating new Encap entries with
> the same "type" and "ip" as the old ones.
> 
> In order to fix this issue the flow is now:
> 1. the first time ovn-controller initializes the Chassis entry (shortly
> after start up) we first check if there is a stale Chassis record in the
> OVN_Southbound DB by checking if any of the old Encap entries associated
> to the Chassis record match the new tunnel configuration. If found it
> means that ovn-controller didn't shutdown gracefully last time it was
> run so it didn't cleanup the Chassis table. Potentially in the meantime
> the OVS system-id was also changed. We then update the stale entry with
> the new configuration and store the last configured chassis-id in memory
> to avoid walking the Chassis table every time.
> 2. for subsequent chassis_run calls we use the last configured
> chassis-id stored at the previous step to lookup the old Chassis record.
> 3. when ovn-controller shuts down gracefully we lookup the Chassis
> record based on the chassis-id stored in memory at steps 1 and 2 above.
> This is to avoid failing to cleanup the Chassis record in OVN_Southbound
> DB if the OVS system-id changes between the last call to chassis_run and
> chassis_cleanup.
> 
> With this commit we also:
> - refactor chassis.c to abstract the string processing and use
>   library data structures (e.g., sset)
> - rename the get_chassis_id function in ovn-controller.c to
>   get_ovs_chassis_id to avoid confusion with the newly added
>   chassis_get_id function from chassis.c which returns the last
>   successfully configured chassis-id.
> - add a test case in ovn-controller.at to check that OVS system-id
>   changes are properly propagated to OVN_Southbound DB

Thanks for working on this.

This is a large patch that incorporates both a bug fix and refactoring.
I would prefer to see the refactoring broken out into a separate patch.
Preferably, it would go after the bug fix so that the bug fix could be
backported by itself (if necessary), but if the refactoring is essential
to the bug fix then the refactoring could go first.

Would you mind breaking the patch apart?

Thanks,

Ben.