[ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

Darrell Ball dlu998 at gmail.com
Tue Aug 20 02:41:23 UTC 2019


On Mon, Aug 19, 2019 at 12:42 PM Darrell Ball <dlu998 at gmail.com> wrote:

>
>
> On Mon, Aug 19, 2019 at 10:52 AM Yi-Hung Wei <yihung.wei at gmail.com> wrote:
>
>> On Fri, Aug 16, 2019 at 5:07 PM Darrell Ball <dlu998 at gmail.com> wrote:
>> >
>> > Thanks for the patch
>> >
>> > Pls let me know if this incremental works for you.
>> > Main change is logging fix for timeout policy deletion.
>> >
>> > Darrell
>> >
>> > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> > index 1d4ee60..00d957b 100644
>> > --- a/lib/dpif-netlink.c
>> > +++ b/lib/dpif-netlink.c
>> > @@ -2822,11 +2822,10 @@ dpif_netlink_ct_dump_done(struct dpif *dpif
>> OVS_UNUSED,
>> >                            struct ct_dpif_dump_state *dump_)
>> >  {
>> >      struct dpif_netlink_ct_dump_state *dump;
>> > -    int err;
>> >
>> >      INIT_CONTAINER(dump, dump_, up);
>> >
>> > -    err = nl_ct_dump_done(dump->nl_ct_dump);
>> > +    int err = nl_ct_dump_done(dump->nl_ct_dump);
>> >      free(dump);
>> >      return err;
>> >  }
>> > @@ -3335,7 +3334,8 @@ dpif_netlink_ct_del_timeout_policy(struct dpif
>> *dpif OVS_UNUSED,
>> >              err = 0;
>> >          }
>> >          if (err) {
>> > -            VLOG_WARN_RL(&error_rl, "failed to delete timeout policy
>> %s (%s)",
>> > +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1,
>> 1);
>>
>> Thanks for the diff.  It looks good in general.
>>
>> I agree on the main concern of the proposed diff which is the original
>> rate limit in dpif-netlink (VLOG_RATE_LIMIT_INIT(9999, 5)) may log too
>> much duplicated information.  However, since we may delete more than
>> one one timeout policy in a minute, so lowering the rate limit to
>> VLOG_RATE_LIMIT_INIT(1, 1) may miss some useful information.   I would
>> use somewhere in the between (VLOG_RATE_LIMIT_INIT(5, 5)) in the next
>> version.
>>
>
> TBH, I am not sure we care lots about this information. I was even
> debating changing it debug level.
> We have 4 billion datapath timeout profile IDs, so it is unlikely we will
> run out.
> Eventually, they will get cleaned up by the retry thingy.
>
> Also, I am not sure what action we will take by seeing these logs anyhow.
>
> Spamming the log is more of a concern.
>

After more testing, I noticed there are a couple other aspects we might
have overlooked.

1/ We don't have a log for a successful deletion attempt.

2/ When we try to delete 1 of the 6 associated Netfilter timeout policies
and it fails, we don't try to delete
 the remaining ones and bail out of the deletion loop early.

Keeping the INFO level change (from WARN), addressing '1' and '2' above and
also folding in
your idea to keep the overall log generation rate a little higher, I ended
up with:

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 1d4ee60..3926cfd 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2822,11 +2822,10 @@ dpif_netlink_ct_dump_done(struct dpif *dpif
OVS_UNUSED,
                           struct ct_dpif_dump_state *dump_)
 {
     struct dpif_netlink_ct_dump_state *dump;
-    int err;

     INIT_CONTAINER(dump, dump_, up);

-    err = nl_ct_dump_done(dump->nl_ct_dump);
+    int err = nl_ct_dump_done(dump->nl_ct_dump);
     free(dump);
     return err;
 }
@@ -3334,14 +3333,12 @@ dpif_netlink_ct_del_timeout_policy(struct dpif
*dpif OVS_UNUSED,
         if (err == ENOENT) {
             err = 0;
         }
-        if (err) {
-            VLOG_WARN_RL(&error_rl, "failed to delete timeout policy %s
(%s)",
-                         ds_cstr(&nl_tp_name), ovs_strerror(err));
-            goto out;
-        }
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
+        VLOG_INFO_RL(&rl, err ? "failed to delete timeout policy %s (%s)"
+                              : "deleted timeout policy %s (%s)",
+                     ds_cstr(&nl_tp_name), ovs_strerror(err));
     }

-out:
     ds_destroy(&nl_tp_name);
     return err;
 }
@@ -3392,10 +3389,9 @@ dpif_netlink_ct_timeout_policy_dump_start(struct
dpif *dpif OVS_UNUSED,
                                           void **statep)
 {
     struct dpif_netlink_ct_timeout_policy_dump_state *dump_state;
-    int err;

     *statep = dump_state = xzalloc(sizeof *dump_state);
-    err = nl_ct_timeout_policy_dump_start(&dump_state->nl_dump_state);
+    int err = nl_ct_timeout_policy_dump_start(&dump_state->nl_dump_state);
     if (err) {
         free(dump_state);
         return err;

Running the system test (from Patch 9) and adding a deletion request for
the in-use timeout policy yields

> 2019-08-20T01:28:41.004Z|00203|dpif_netlink|INFO|deleted timeout policy
ovs_tp_0_tcp4 (Success)
> 2019-08-20T01:28:41.004Z|00204|dpif_netlink|INFO|failed to delete timeout
policy ovs_tp_0_udp4 (Device or resource busy)
> 2019-08-20T01:28:41.004Z|00205|dpif_netlink|INFO|failed to delete timeout
policy ovs_tp_0_icmp4 (Device or resource busy)
> 2019-08-20T01:28:41.004Z|00206|dpif_netlink|INFO|deleted timeout policy
ovs_tp_0_tcp6 (Success)
> 2019-08-20T01:28:41.004Z|00207|dpif_netlink|INFO|deleted timeout policy
ovs_tp_0_udp6 (Success)
> 2019-08-20T01:28:41.004Z|00208|dpif_netlink|INFO|deleted timeout policy
ovs_tp_0_icmpv6 (Success)




>
>
>>
>> Thanks,
>>
>> -Yi-Hung
>>
>


More information about the dev mailing list