| Message ID | 20231018075634.75983-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 |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 10/18/23 09:56, 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> > --- > v5: Rebase on top of current master. > Address comments from Ilya: > - Correct the NEWS entry. > - Fix style related problems. > --- > NEWS | 3 +++ > lib/dpctl.c | 21 +++++++++++++++------ > tests/system-traffic.at | 26 ++++++++++++++++++++++++++ > 3 files changed, 44 insertions(+), 6 deletions(-) Hi, Ales. Thanks for the patch! It may need some extra changes in zone_limit_delete() though. While removing a defualt zone it will now log zone '-1', which is not particularly user-friendly. Also, the second time we'll try to remove the default limit the command will fail unable to find the entry for the default zone. It probably shouldn't fail, because default zone limit is always there, even if unlimited. We do always report a default value even if it wasn't configured. Best regards, Ilya Maximets.
On Wed, Oct 25, 2023 at 1:39 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 10/18/23 09:56, 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> > > --- > > v5: Rebase on top of current master. > > Address comments from Ilya: > > - Correct the NEWS entry. > > - Fix style related problems. > > --- > > NEWS | 3 +++ > > lib/dpctl.c | 21 +++++++++++++++------ > > tests/system-traffic.at | 26 ++++++++++++++++++++++++++ > > 3 files changed, 44 insertions(+), 6 deletions(-) > > Hi, Ales. Thanks for the patch! > It may need some extra changes in zone_limit_delete() though. > While removing a defualt zone it will now log zone '-1', which > is not particularly user-friendly. Also, the second time we'll > try to remove the default limit the command will fail unable to > find the entry for the default zone. It probably shouldn't fail, > because default zone limit is always there, even if unlimited. > We do always report a default value even if it wasn't configured. > > Best regards, Ilya Maximets. > Thank you for the review. I made the change so it behaves the same way as before, meaning it doesn't report anything for the default zone. Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA amusil@redhat.com
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/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 418cd32fe..25e8c3f75 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> --- v5: Rebase on top of current master. Address comments from Ilya: - Correct the NEWS entry. - Fix style related problems. --- NEWS | 3 +++ lib/dpctl.c | 21 +++++++++++++++------ tests/system-traffic.at | 26 ++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 6 deletions(-)