In northd, Logical_Switch_Port.tag is set from tag_request.  A
nonzero request is copied directly into tag.  A request of 0 on a
child port (one with parent_name set) is replaced by a tag that is
unique within the parent's scope.

Currently, tag is not cleared when tag_request is removed, or when
tag_request=0 on a port without parent_name.  The stale derived tag
propagates to SB Port_Binding.tag, and ovn-controller keeps tagging
packets after the CMS removed the request.

This patch clears the derived tag in both cases.  It also stops
counting tags as in-use by a child port once their request is gone,
so the parent's pool is freed correctly.

Added tests for three transitions: clearing tag_request on a child
port, clearing tag_request on a non-child port, and setting
tag_request=0 on a non-child port.

Signed-off-by: Mykola Yurchenko <[email protected]>
---
 northd/northd.c | 14 ++++++++++++--
 tests/ovn.at    | 20 ++++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 7cf766c..6b97712 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -1344,8 +1344,10 @@ tag_alloc_add_existing_tags(struct hmap *tag_alloc_table,
                             const struct nbrec_logical_switch_port *nbsp)
 {
     /* Add the tags of already existing nested containers.  If there is no
-     * 'nbsp->parent_name' or no 'nbsp->tag' set, there is nothing to do. */
-    if (!nbsp->parent_name || !nbsp->parent_name[0] || !nbsp->tag) {
+     * 'nbsp->parent_name', no 'nbsp->tag', or no 'nbsp->tag_request' that
+     * still owns the tag, there is nothing to do. */
+    if (!nbsp->parent_name || !nbsp->parent_name[0] || !nbsp->tag ||
+        !nbsp->tag_request) {
         return;
     }

@@ -1359,6 +1361,11 @@ tag_alloc_create_new_tag(struct hmap *tag_alloc_table,
                          const struct nbrec_logical_switch_port *nbsp)
 {
     if (!nbsp->tag_request) {
+        /* 'tag' is derived from 'tag_request'.  If the request is gone,
+         * clear any previously derived tag. */
+        if (nbsp->tag) {
+            nbrec_logical_switch_port_set_tag(nbsp, NULL, 0);
+        }
         return;
     }

@@ -1387,6 +1394,9 @@ tag_alloc_create_new_tag(struct hmap *tag_alloc_table,
     } else if (*nbsp->tag_request != 0) {
         /* For everything else, copy the contents of 'tag_request' to 'tag'. */
         nbrec_logical_switch_port_set_tag(nbsp, nbsp->tag_request, 1);
+    } else if (nbsp->tag) {
+        /* tag_request == 0 with no parent: clear any previously set tag. */
+        nbrec_logical_switch_port_set_tag(nbsp, NULL, 0);
     }
 }


diff --git a/tests/ovn.at b/tests/ovn.at
index b0c1b3f..6f2660f 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -10408,6 +10408,15 @@ check_row_count Port_Binding 1 logical_port=c7 
tag=$c5_tag
 check_row_count Port_Binding 1 logical_port=c5 tag=$c5_tag
 check_row_count Port_Binding 2 parent_port=parent2 tag=$c5_tag

+AS_BOX([clear tag_request for child port])
+AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c8 parent2 42])
+AT_CHECK([ovn-nbctl lsp-get-tag c8], [0], [42
+])
+check_row_count Port_Binding 1 logical_port=c8 tag=42
+AT_CHECK([ovn-nbctl --wait=sb clear logical_switch_port c8 tag_request])
+AT_CHECK([ovn-nbctl lsp-get-tag c8], [0], [])
+check_column "" Port_Binding tag logical_port=c8
+
 AS_BOX([tag_request without parent_name])
 AT_CHECK([ovn-nbctl ls-add ls2])
 dnl When there is no parent_name provided (for say, 'localnet'), 'tag_request'
@@ -10426,6 +10435,17 @@ dnl change the tag_request.
 AT_CHECK([ovn-nbctl --wait=sb  set logical_switch_port local1 tag_request=50])
 AT_CHECK([ovn-nbctl lsp-get-tag local1], [0], [50
 ])
+dnl clear the tag_request.
+AT_CHECK([ovn-nbctl --wait=sb clear logical_switch_port local1 tag_request])
+AT_CHECK([ovn-nbctl lsp-get-tag local1], [0], [])
+check_column "" Port_Binding tag logical_port=local1
+dnl setting tag_request back to 0 should also clear a previously set tag.
+AT_CHECK([ovn-nbctl --wait=sb set logical_switch_port local1 tag_request=50])
+AT_CHECK([ovn-nbctl lsp-get-tag local1], [0], [50
+])
+AT_CHECK([ovn-nbctl --wait=sb set logical_switch_port local1 tag_request=0])
+AT_CHECK([ovn-nbctl lsp-get-tag local1], [0], [])
+check_column "" Port_Binding tag logical_port=local1

 OVN_CLEANUP_NORTHD
 AT_CLEANUP
--
2.43.5

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to