northd: Skip transient IDL records.

When multiple OVSDB updates have been received since the last northd run
it's possible that the IDL tracks changes for database entities that
were added _and also_ removed from the last time the northd processing
engine run.  In some cases, those may appear as being simultaneously
"new" and "deleted".

Currently, the only tables for which this can be a problem are
NB.Load_Balancer and NB.Load_Balancer_Group.

Skip these "transient records" to avoid adding soon to be deleted rows
to the tracked_lb_data->crupdated_lbs records.  These are used to build
'northd' I-P engine state in northd_handle_lb_data_changes().

We also avoid crashing if "unexpected" deletes are reported by the IDL.
This is likely due to a bug in the IDL [0] but it's easy to avoid on the
northd side.

This commit also adds a test case which _might_ detect the issue when
run under valgrind.  The test case can't always detect the problem
because a prerequisite for a Load Balancer to be "transient" is that the
IDL processes the update that removes it from the NB Load_Balancer table
and from the Load_Balancer_Group row that was referring to it in the
following order: Load_Balancer_Group table update first and then
Load_Balancer deletion.  The order is controlled by the way
'struct shash' hashes records (table names in this case) and that's
arch and/or compiler dependent.

[0] https://issues.redhat.com/browse/FDP-193

Fixes: a24eed5 ("northd: Add initial I-P for load balancer and load balancer groups")
Reported-at: https://issues.redhat.com/browse/FDP-181
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Numan Siddique <numans@ovn.org>

northd/en-lb-data.c

@@ -144,6 +144,12 @@ lb_data_load_balancer_handler(struct engine_node *node, void *data)
 
     const struct nbrec_load_balancer *tracked_lb;
     NBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (tracked_lb, nb_lb_table) {
+        /* "New" + "Deleted" is a no-op. */
+        if (nbrec_load_balancer_is_new(tracked_lb)
+            && nbrec_load_balancer_is_deleted(tracked_lb)) {
+            continue;
+        }
+
         struct ovn_northd_lb *lb;
         if (nbrec_load_balancer_is_new(tracked_lb)) {
             /* New load balancer. */
@@ -153,19 +159,22 @@ lb_data_load_balancer_handler(struct engine_node *node, void *data)
             add_crupdated_lb_to_tracked_data(lb, trk_lb_data,
                                              lb->health_checks);
             trk_lb_data->has_routable_lb |= lb->routable;
-        } else if (nbrec_load_balancer_is_deleted(tracked_lb)) {
-            lb = ovn_northd_lb_find(&lb_data->lbs,
-                                    &tracked_lb->header_.uuid);
-            ovs_assert(lb);
+            continue;
+        }
+
+        /* Protect against "spurious" deletes reported by the IDL. */
+        lb = ovn_northd_lb_find(&lb_data->lbs, &tracked_lb->header_.uuid);
+        if (!lb) {
+            continue;
+        }
+
+        if (nbrec_load_balancer_is_deleted(tracked_lb)) {
             hmap_remove(&lb_data->lbs, &lb->hmap_node);
             add_deleted_lb_to_tracked_data(lb, trk_lb_data,
                                            lb->health_checks);
             trk_lb_data->has_routable_lb |= lb->routable;
         } else {
             /* Load balancer updated. */
-            lb = ovn_northd_lb_find(&lb_data->lbs,
-                                    &tracked_lb->header_.uuid);
-            ovs_assert(lb);
             bool health_checks = lb->health_checks;
             struct sset old_ips_v4 = SSET_INITIALIZER(&old_ips_v4);
             struct sset old_ips_v6 = SSET_INITIALIZER(&old_ips_v6);
@@ -217,6 +226,12 @@ lb_data_load_balancer_group_handler(struct engine_node *node, void *data)
     const struct nbrec_load_balancer_group *tracked_lb_group;
     NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH_TRACKED (tracked_lb_group,
                                                       nb_lbg_table) {
+        /* "New" + "Deleted" is a no-op. */
+        if (nbrec_load_balancer_group_is_new(tracked_lb_group)
+            && nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
+            continue;
+        }
+
         if (nbrec_load_balancer_group_is_new(tracked_lb_group)) {
             struct ovn_lb_group *lb_group =
                 create_lb_group(tracked_lb_group, &lb_data->lbs,
@@ -228,21 +243,22 @@ lb_data_load_balancer_group_handler(struct engine_node *node, void *data)
             }
 
             trk_lb_data->has_routable_lb |= lb_group->has_routable_lb;
-        } else if (nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
-            struct ovn_lb_group *lb_group;
-            lb_group = ovn_lb_group_find(&lb_data->lbgrps,
-                                         &tracked_lb_group->header_.uuid);
-            ovs_assert(lb_group);
+            continue;
+        }
+
+        /* Protect against "spurious" deletes reported by the IDL. */
+        struct ovn_lb_group *lb_group;
+        lb_group = ovn_lb_group_find(&lb_data->lbgrps,
+                                     &tracked_lb_group->header_.uuid);
+        if (!lb_group) {
+            continue;
+        }
+
+        if (nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
             hmap_remove(&lb_data->lbgrps, &lb_group->hmap_node);
             add_deleted_lbgrp_to_tracked_data(lb_group, trk_lb_data);
             trk_lb_data->has_routable_lb |= lb_group->has_routable_lb;
         } else {
-
-            struct ovn_lb_group *lb_group;
-            lb_group = ovn_lb_group_find(&lb_data->lbgrps,
-                                         &tracked_lb_group->header_.uuid);
-            ovs_assert(lb_group);
-
             /* Determine the lbs which are added or deleted for this
              * lb group and add them to tracked data.
              * Eg.  If an lb group lbg1 before the update had [lb1, lb2, lb3]

tests/ovn-macros.at

@@ -891,6 +891,16 @@ start_scapy_server() {
     && on_exit "test -e \"$pidfile\" && ovs-appctl -t $ctlfile exit"
 }
 
+sleep_northd() {
+  echo Northd going to sleep
+  AT_CHECK([kill -STOP $(cat northd/ovn-northd.pid)])
+}
+
+wake_up_northd() {
+  echo Northd waking up
+  AT_CHECK([kill -CONT $(cat northd/ovn-northd.pid)])
+}
+
 sleep_sb() {
   echo SB going to sleep
   AT_CHECK([kill -STOP $(cat ovn-sb/ovsdb-server.pid)])

tests/ovn-northd.at

@@ -10913,3 +10913,56 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
 
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Load balancer incremental processing - batched updates])
+ovn_start
+
+# Check the scenario when a LB is created and quickly deleted (northd
+# processes this in a single iteration).
+
+check ovn-nbctl ls-add sw0
+lbg_uuid=$(ovn-nbctl --wait=sb create load_balancer_group name=lbg)
+check ovn-nbctl --wait=sb add logical_switch sw0 load_balancer_group $lbg_uuid
+
+# Pause ovn-northd.
+sleep_northd
+
+check ovn-nbctl lb-add lb-temp 50.0.0.10:80 50.0.0.20:8080
+lb_temp_uuid=$(fetch_column nb:Load_Balancer _uuid name=lb-temp)
+check ovn-nbctl add load_balancer_group $lbg_uuid load_balancer $lb_temp_uuid
+check ovn-nbctl lb-del lb-temp
+
+# Let ovn-northd process all the updates that happened since it went to sleep.
+wake_up_northd
+
+# Add a new load balancer to make sure northd still functions properly.
+check ovn-nbctl lb-add lb1 50.0.0.10:80 50.0.0.30:8080
+lb1_uuid=$(fetch_column nb:Load_Balancer _uuid name=lb1)
+check ovn-nbctl add load_balancer_group $lbg_uuid load_balancer $lb1_uuid
+
+check ovn-nbctl --wait=sb sync
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+# Re-check the same scenario but now also batch the additional LB creation.
+sleep_northd
+check ovn-nbctl lb-add lb-temp 50.0.0.10:80 50.0.0.20:8080
+lb_temp_uuid=$(fetch_column nb:Load_Balancer _uuid name=lb-temp)
+check ovn-nbctl add load_balancer_group $lbg_uuid load_balancer $lb_temp_uuid
+check ovn-nbctl lb-del lb-temp
+
+# Add a new load balancer to make sure northd still functions properly.
+check ovn-nbctl lb-add lb2 50.0.0.10:80 50.0.0.30:8080
+lb2_uuid=$(fetch_column nb:Load_Balancer _uuid name=lb2)
+check ovn-nbctl add load_balancer_group $lbg_uuid load_balancer $lb2_uuid
+
+wake_up_northd
+check ovn-nbctl --wait=sb sync
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE status], [0], [dnl
+Status: active
+])
+
+AT_CLEANUP
+])