| Message ID | 20231102120021.89725-3-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | Expose CT limit via DB | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 11/2/23 13:00, Ales Musil wrote: > Add optional argument to dpctl ct-del-limits called > "default", which allows to remove the default limit > making it effectively system default. > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v6: Rebase on top of current master. > Address comments from Ilya: > - Adjust the log message so it doesn't report anything for default zone. > v5: Rebase on top of current master. > Address comments from Ilya: > - Correct the NEWS entry. > - Fix style related problems. > --- > NEWS | 3 +++ > lib/conntrack.c | 13 +++++++------ > lib/dpctl.c | 21 +++++++++++++++------ > tests/system-traffic.at | 26 ++++++++++++++++++++++++++ > 4 files changed, 51 insertions(+), 12 deletions(-) > > diff --git a/NEWS b/NEWS > index 6b45492f1..7bc27b687 100644 > --- a/NEWS > +++ b/NEWS > @@ -6,6 +6,9 @@ Post-v3.2.0 > from older version is supported but it may trigger more leader elections > during the process, and error logs complaining unrecognized fields may > be observed on old nodes. > + - ovs-appctl: > + * Added support removal of default CT zone limit, e.g. Added support for ... > + "ovs-appctl dpctl/ct-del-limits default". > > > v3.2.0 - 17 Aug 2023 > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 31f00a127..b533dd3df 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -404,13 +404,14 @@ zone_limit_delete(struct conntrack *ct, int32_t zone) > struct zone_limit *zl = zone_limit_lookup_protected(ct, zone); > if (zl) { > zone_limit_clean(ct, zl); > - ovs_mutex_unlock(&ct->ct_lock); > - VLOG_INFO("Deleted zone limit for zone %d", zone); > - } else { > - ovs_mutex_unlock(&ct->ct_lock); > - VLOG_INFO("Attempted delete of non-existent zone limit: zone %d", > - zone); > } > + > + if (zone != DEFAULT_ZONE) { > + VLOG_INFO(zl ? "Deleted zone limit for zone %d" : "Attempted delete" > + " of non-existent zone limit: zone %d", zone); Nit: Might be better to not split the string. E.g.: VLOG_INFO(zl ? "Deleted zone limit for zone %d" : "Attempted delete of non-existent zone limit: zone %d", zone); > + } > + > + ovs_mutex_unlock(&ct->ct_lock); > return 0; > } > > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 76f21a530..a8c654747 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -2291,14 +2291,23 @@ dpctl_ct_del_limits(int argc, const char *argv[], > int i = dp_arg_exists(argc, argv) ? 2 : 1; > struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits); > > - error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif); > + error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif); > if (error) { > return error; > } > > - error = parse_ct_limit_zones(argv[i], &zone_limits, &ds); > - if (error) { > - goto error; > + /* Parse default limit. */ > + if (!strcmp(argv[i], "default")) { > + ct_dpif_push_zone_limit(&zone_limits, OVS_ZONE_LIMIT_DEFAULT_ZONE, > + 0, 0); > + i++; > + } > + > + if (argc > i) { > + error = parse_ct_limit_zones(argv[i], &zone_limits, &ds); > + if (error) { > + goto error; > + } > } > > error = ct_dpif_del_limits(dpif, &zone_limits); > @@ -3031,8 +3040,8 @@ static const struct dpctl_command all_commands[] = { > { "ct-get-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_get_tcp_seq_chk, DP_RO }, > { "ct-set-limits", "[dp] [default=L] [zone=N,limit=L]...", 1, INT_MAX, > dpctl_ct_set_limits, DP_RO }, > - { "ct-del-limits", "[dp] zone=N1[,N2]...", 1, 2, dpctl_ct_del_limits, > - DP_RO }, > + { "ct-del-limits", "[dp] [default] [zone=N1[,N2]...]", 1, 3, > + dpctl_ct_del_limits, DP_RO }, > { "ct-get-limits", "[dp] [zone=N1[,N2]...]", 0, 2, dpctl_ct_get_limits, > DP_RO }, > { "ct-get-sweep-interval", "[dp]", 0, 1, dpctl_ct_get_sweep, DP_RO }, > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 7ea450202..b6c8d7faf 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -5195,6 +5195,32 @@ udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10. > udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3 > ]) > > +dnl Test ct-del-limits for default zone. > + > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=4,limit=4]) > +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl > +default limit=15 > +zone=4,limit=4,count=0 > +]) > + > +AT_CHECK([ovs-appctl dpctl/ct-del-limits default]) > +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl > +default limit=0 > +zone=4,limit=4,count=0 > +]) > + > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15]) > +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl > +default limit=15 > +zone=4,limit=4,count=0 > +]) > + > +AT_CHECK([ovs-appctl dpctl/ct-del-limits default zone=4]) > +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl > +default limit=0 > +zone=4,limit=0,count=0 > +]) > + > OVS_TRAFFIC_VSWITCHD_STOP(["dnl > /could not create datapath/d > /(Cannot allocate memory) on packet/d"])
On Tue, Nov 28, 2023 at 2:47 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 11/2/23 13:00, Ales Musil wrote: > > Add optional argument to dpctl ct-del-limits called > > "default", which allows to remove the default limit > > making it effectively system default. > > > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > v6: Rebase on top of current master. > > Address comments from Ilya: > > - Adjust the log message so it doesn't report anything for default > zone. > > v5: Rebase on top of current master. > > Address comments from Ilya: > > - Correct the NEWS entry. > > - Fix style related problems. > > --- > > NEWS | 3 +++ > > lib/conntrack.c | 13 +++++++------ > > lib/dpctl.c | 21 +++++++++++++++------ > > tests/system-traffic.at | 26 ++++++++++++++++++++++++++ > > 4 files changed, 51 insertions(+), 12 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 6b45492f1..7bc27b687 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -6,6 +6,9 @@ Post-v3.2.0 > > from older version is supported but it may trigger more leader > elections > > during the process, and error logs complaining unrecognized > fields may > > be observed on old nodes. > > + - ovs-appctl: > > + * Added support removal of default CT zone limit, e.g. > > Added support for ... > > > + "ovs-appctl dpctl/ct-del-limits default". > > > > > > v3.2.0 - 17 Aug 2023 > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 31f00a127..b533dd3df 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -404,13 +404,14 @@ zone_limit_delete(struct conntrack *ct, int32_t > zone) > > struct zone_limit *zl = zone_limit_lookup_protected(ct, zone); > > if (zl) { > > zone_limit_clean(ct, zl); > > - ovs_mutex_unlock(&ct->ct_lock); > > - VLOG_INFO("Deleted zone limit for zone %d", zone); > > - } else { > > - ovs_mutex_unlock(&ct->ct_lock); > > - VLOG_INFO("Attempted delete of non-existent zone limit: zone > %d", > > - zone); > > } > > + > > + if (zone != DEFAULT_ZONE) { > > + VLOG_INFO(zl ? "Deleted zone limit for zone %d" : "Attempted > delete" > > + " of non-existent zone limit: zone %d", zone); > > Nit: Might be better to not split the string. E.g.: > > VLOG_INFO(zl ? "Deleted zone limit for zone %d" > : "Attempted delete of non-existent zone limit: zone > %d", > zone); > > > + } > > + > > + ovs_mutex_unlock(&ct->ct_lock); > > return 0; > > } > > > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index 76f21a530..a8c654747 100644 > > --- a/lib/dpctl.c > > +++ b/lib/dpctl.c > > @@ -2291,14 +2291,23 @@ dpctl_ct_del_limits(int argc, const char *argv[], > > int i = dp_arg_exists(argc, argv) ? 2 : 1; > > struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits); > > > > - error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif); > > + error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif); > > if (error) { > > return error; > > } > > > > - error = parse_ct_limit_zones(argv[i], &zone_limits, &ds); > > - if (error) { > > - goto error; > > + /* Parse default limit. */ > > + if (!strcmp(argv[i], "default")) { > > + ct_dpif_push_zone_limit(&zone_limits, > OVS_ZONE_LIMIT_DEFAULT_ZONE, > > + 0, 0); > > + i++; > > + } > > + > > + if (argc > i) { > > + error = parse_ct_limit_zones(argv[i], &zone_limits, &ds); > > + if (error) { > > + goto error; > > + } > > } > > > > error = ct_dpif_del_limits(dpif, &zone_limits); > > @@ -3031,8 +3040,8 @@ static const struct dpctl_command all_commands[] = > { > > { "ct-get-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_get_tcp_seq_chk, > DP_RO }, > > { "ct-set-limits", "[dp] [default=L] [zone=N,limit=L]...", 1, > INT_MAX, > > dpctl_ct_set_limits, DP_RO }, > > - { "ct-del-limits", "[dp] zone=N1[,N2]...", 1, 2, > dpctl_ct_del_limits, > > - DP_RO }, > > + { "ct-del-limits", "[dp] [default] [zone=N1[,N2]...]", 1, 3, > > + dpctl_ct_del_limits, DP_RO }, > > { "ct-get-limits", "[dp] [zone=N1[,N2]...]", 0, 2, > dpctl_ct_get_limits, > > DP_RO }, > > { "ct-get-sweep-interval", "[dp]", 0, 1, dpctl_ct_get_sweep, DP_RO > }, > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > > index 7ea450202..b6c8d7faf 100644 > > --- a/tests/system-traffic.at > > +++ b/tests/system-traffic.at > > @@ -5195,6 +5195,32 @@ > udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10. > > > udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3 > > ]) > > > > +dnl Test ct-del-limits for default zone. > > + > > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=4,limit=4]) > > +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl > > +default limit=15 > > +zone=4,limit=4,count=0 > > +]) > > + > > +AT_CHECK([ovs-appctl dpctl/ct-del-limits default]) > > +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl > > +default limit=0 > > +zone=4,limit=4,count=0 > > +]) > > + > > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15]) > > +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl > > +default limit=15 > > +zone=4,limit=4,count=0 > > +]) > > + > > +AT_CHECK([ovs-appctl dpctl/ct-del-limits default zone=4]) > > +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl > > +default limit=0 > > +zone=4,limit=0,count=0 > > +]) > > + > > OVS_TRAFFIC_VSWITCHD_STOP(["dnl > > /could not create datapath/d > > /(Cannot allocate memory) on packet/d"]) > > Thank you for the review, everything should be addressed in v7. Thanks, Ales
diff --git a/NEWS b/NEWS index 6b45492f1..7bc27b687 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,9 @@ Post-v3.2.0 from older version is supported but it may trigger more leader elections during the process, and error logs complaining unrecognized fields may be observed on old nodes. + - ovs-appctl: + * Added support removal of default CT zone limit, e.g. + "ovs-appctl dpctl/ct-del-limits default". v3.2.0 - 17 Aug 2023 diff --git a/lib/conntrack.c b/lib/conntrack.c index 31f00a127..b533dd3df 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -404,13 +404,14 @@ zone_limit_delete(struct conntrack *ct, int32_t zone) struct zone_limit *zl = zone_limit_lookup_protected(ct, zone); if (zl) { zone_limit_clean(ct, zl); - ovs_mutex_unlock(&ct->ct_lock); - VLOG_INFO("Deleted zone limit for zone %d", zone); - } else { - ovs_mutex_unlock(&ct->ct_lock); - VLOG_INFO("Attempted delete of non-existent zone limit: zone %d", - zone); } + + if (zone != DEFAULT_ZONE) { + VLOG_INFO(zl ? "Deleted zone limit for zone %d" : "Attempted delete" + " of non-existent zone limit: zone %d", zone); + } + + ovs_mutex_unlock(&ct->ct_lock); return 0; } diff --git a/lib/dpctl.c b/lib/dpctl.c index 76f21a530..a8c654747 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -2291,14 +2291,23 @@ dpctl_ct_del_limits(int argc, const char *argv[], int i = dp_arg_exists(argc, argv) ? 2 : 1; struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits); - error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif); + error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif); if (error) { return error; } - error = parse_ct_limit_zones(argv[i], &zone_limits, &ds); - if (error) { - goto error; + /* Parse default limit. */ + if (!strcmp(argv[i], "default")) { + ct_dpif_push_zone_limit(&zone_limits, OVS_ZONE_LIMIT_DEFAULT_ZONE, + 0, 0); + i++; + } + + if (argc > i) { + error = parse_ct_limit_zones(argv[i], &zone_limits, &ds); + if (error) { + goto error; + } } error = ct_dpif_del_limits(dpif, &zone_limits); @@ -3031,8 +3040,8 @@ static const struct dpctl_command all_commands[] = { { "ct-get-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_get_tcp_seq_chk, DP_RO }, { "ct-set-limits", "[dp] [default=L] [zone=N,limit=L]...", 1, INT_MAX, dpctl_ct_set_limits, DP_RO }, - { "ct-del-limits", "[dp] zone=N1[,N2]...", 1, 2, dpctl_ct_del_limits, - DP_RO }, + { "ct-del-limits", "[dp] [default] [zone=N1[,N2]...]", 1, 3, + dpctl_ct_del_limits, DP_RO }, { "ct-get-limits", "[dp] [zone=N1[,N2]...]", 0, 2, dpctl_ct_get_limits, DP_RO }, { "ct-get-sweep-interval", "[dp]", 0, 1, dpctl_ct_get_sweep, DP_RO }, diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 7ea450202..b6c8d7faf 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -5195,6 +5195,32 @@ udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10. udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3 ]) +dnl Test ct-del-limits for default zone. + +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=4,limit=4]) +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl +default limit=15 +zone=4,limit=4,count=0 +]) + +AT_CHECK([ovs-appctl dpctl/ct-del-limits default]) +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl +default limit=0 +zone=4,limit=4,count=0 +]) + +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15]) +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl +default limit=15 +zone=4,limit=4,count=0 +]) + +AT_CHECK([ovs-appctl dpctl/ct-del-limits default zone=4]) +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl +default limit=0 +zone=4,limit=0,count=0 +]) + OVS_TRAFFIC_VSWITCHD_STOP(["dnl /could not create datapath/d /(Cannot allocate memory) on packet/d"])
Add optional argument to dpctl ct-del-limits called "default", which allows to remove the default limit making it effectively system default. Signed-off-by: Ales Musil <amusil@redhat.com> --- v6: Rebase on top of current master. Address comments from Ilya: - Adjust the log message so it doesn't report anything for default zone. v5: Rebase on top of current master. Address comments from Ilya: - Correct the NEWS entry. - Fix style related problems. --- NEWS | 3 +++ lib/conntrack.c | 13 +++++++------ lib/dpctl.c | 21 +++++++++++++++------ tests/system-traffic.at | 26 ++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 12 deletions(-)