[ovs-dev] [PATCH ovn] controller: Fix toggling ct zone ids.

[Lorenzo Bianconi]

> From: Numan Siddique <numans at ovn.org>
> 
> ovn-controller maintains a shash of pending ct zone entries
> to flush the ct zone ids and to store/remove the allocated zone id
> in/from the OVS bridge.external_ids.  While adding an entry to
> the shash, the function 'add_pending_ct_zone_entry()' doesn't
> check for existing entry for the same key in shash.  If suppose
> there are multiple entries for the samestring key, then it results in
> an infinite loop of adding and deleting the key entries in
> OVS bridge.external_ids.
> 
> The pending ct zone entries are deleted from the shash when they
> reach the state - CT_ZONE_DB_SENT and when
> ovsdb_idl_loop_commit_and_wait(ovsidl) returns 1.  In a highly loaded
> compute node this loop gets triggered when this function doesn't return
> 1 and there are duplicate ct zone entries.
> 
> These duplicate entries are mostly observed for logical ports of
> type 'virtual' when this virtual port keeps moving from one chassis
> to another.  But this scenario can get triggered for other logical ports
> too.
> 
> *********************
> 2021-02-12T17:25:56.974Z|04363|jsonrpc|DBG|unix:/run/openvswitch/db.sock: send request, method="transact", params=["Open_vSwitch",{"mutations":[["external_ids","delete",["set",["ct-zone-313f5395-5170-4c4c-8820-e6288a765d27","ct-zone-efa8530b-93d5-45b3-b16f-edfe1856233c"]]],["external_ids","insert",["map",[["ct-zone-da4a2f90-aef9-4fea-ad6d-aed364fa9988","63"],["ct-zone-7a97e014-a753-473b-a0f3-b2ec6e9f11d6","71"]]]]],"where":[["_uuid","==",["uuid","653e7315-47b3-4c39-a5f9-665aa3dddb9e"]]],"op":"mutate","table":"Bridge"},{"comment":"ovn-controller\novn-controller: modifying OVS tunnels 'e3af60d7-3942-4aa2-84ad-e02dcd3b183d'","op":"comment"}], id=4336
> 2021-02-12T17:25:56.979Z|04364|jsonrpc|DBG|unix:/run/openvswitch/db.sock: received notification, method="update3", params=[["monid","Open_vSwitch"],"00000000-0000-0000-0000-000000000000",{"Bridge":{"653e7315-47b3-4c39-a5f9-665aa3dddb9e":{"modify":{"external_ids":["map",[["ct-zone-313f5395-5170-4c4c-8820-e6288a765d27","71"],["ct-zone-7a97e014-a753-473b-a0f3-b2ec6e9f11d6","71"],["ct-zone-da4a2f90-aef9-4fea-ad6d-aed364fa9988","63"],["ct-zone-efa8530b-93d5-45b3-b16f-edfe1856233c","63"]]]}}}}]
> 2021-02-12T17:25:56.988Z|04365|jsonrpc|DBG|unix:/run/openvswitch/db.sock: received reply, result=[{"count":1},{}], id=4336
> 2021-02-12T17:25:57.006Z|04366|jsonrpc|DBG|unix:/run/openvswitch/db.sock: send request, method="transact", params=["Open_vSwitch",{"mutations":[["external_ids","delete",["set",["ct-zone-da4a2f90-aef9-4fea-ad6d-aed364fa9988","ct-zone-7a97e014-a753-473b-a0f3-b2ec6e9f11d6"]]],["external_ids","insert",["map",[["ct-zone-313f5395-5170-4c4c-8820-e6288a765d27","71"],["ct-zone-efa8530b-93d5-45b3-b16f-edfe1856233c","63"]]]]],"where":[["_uuid","==",["uuid","653e7315-47b3-4c39-a5f9-665aa3dddb9e"]]],"op":"mutate","table":"Bridge"},{"comment":"ovn-controller\novn-controller: modifying OVS tunnels 'e3af60d7-3942-4aa2-84ad-e02dcd3b183d'","op":"comment"}], id=4337
> 2021-02-12T17:25:57.011Z|04367|jsonrpc|DBG|unix:/run/openvswitch/db.sock: received notification, method="update3", params=[["monid","Open_vSwitch"],"00000000-0000-0000-0000-000000000000",{"Bridge":{"653e7315-47b3-4c39-a5f9-665aa3dddb9e":{"modify":{"external_ids":["map",[["ct-zone-313f5395-5170-4c4c-8820-e6288a765d27","71"],["ct-zone-7a97e014-a753-473b-a0f3-b2ec6e9f11d6","71"],["ct-zone-da4a2f90-aef9-4fea-ad6d-aed364fa9988","63"],["ct-zone-efa8530b-93d5-45b3-b16f-edfe1856233c","63"]]]}}}}]
> ...
> ...
> *************************
> 
> This patch fixes this issue by using shash_replace() when adding the
> entry to the shash.
> 
> Note: I was not able to reproduce the issue with a test setup and
> hence couldn't add test cases.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1903210
> Signed-off-by: Numan Siddique <numans at ovn.org>

Acked-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>

> ---
>  controller/ovn-controller.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 61b809593..4343650fc 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -608,7 +608,18 @@ add_pending_ct_zone_entry(struct shash *pending_ct_zones,
>      pending->state = state; /* Skip flushing zone. */
>      pending->zone = zone;
>      pending->add = add;
> -    shash_add(pending_ct_zones, name, pending);
> +
> +    /* Its important that we add only one entry for the key 'name'.
> +     * Replace 'pending' with 'existing' and free up 'existing'.
> +     * Otherwise, we may end up in a continuous loop of adding
> +     * and deleting the zone entry in the 'external_ids' of
> +     * integration bridge.
> +     */
> +    struct ct_zone_pending_entry *existing =
> +        shash_replace(pending_ct_zones, name, pending);
> +    if (existing) {
> +        free(existing);
> +    }
>  }
>  
>  static void
> -- 
> 2.29.2
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>