On Wed, Oct 25, 2023 at 2:52 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
> On 10/18/23 09:56, Ales Musil wrote:
> > Make sure that if any zone limit was set via DB
> > all zones are forced to be set there also. This
> > is done by tracking which datapath has zone limit
> > protection and it is reflected in the dpctl command.
> >
> > If the datapath is protected the dpctl command will
> > return permission error.
> >
> > Signed-off-by: Ales Musil <amu...@redhat.com>
> > ---
> > v5: Rebase on top of current master.
> > Address comments from Ilya:
> > - Add more user friendly error message to the dpctl.
> > - Fix style related problems.
> > v4: Rebase on top of current master.
> > Make the protection datapath wide.
> > ---
> > lib/ct-dpif.c | 27 +++++++++++++++++++++++++++
> > lib/ct-dpif.h | 2 ++
> > lib/dpctl.c | 12 ++++++++++++
> > ofproto/ofproto-dpif.c | 14 ++++++++++++++
> > ofproto/ofproto-provider.h | 5 +++++
> > ofproto/ofproto.c | 11 +++++++++++
> > ofproto/ofproto.h | 2 ++
> > tests/system-traffic.at | 36 ++++++++++++++++++++++++++++++++++++
> > vswitchd/bridge.c | 7 +++++++
> > 9 files changed, 116 insertions(+)
> >
> > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> > index 2ee045164..41d2dc4d7 100644
> > --- a/lib/ct-dpif.c
> > +++ b/lib/ct-dpif.c
> > @@ -23,6 +23,7 @@
> > #include "openvswitch/ofp-ct.h"
> > #include "openvswitch/ofp-parse.h"
> > #include "openvswitch/vlog.h"
> > +#include "sset.h"
> >
> > VLOG_DEFINE_THIS_MODULE(ct_dpif);
> >
> > @@ -32,6 +33,10 @@ struct flags {
> > const char *name;
> > };
> >
> > +/* Protection for CT zone limit per datapath. */
> > +static struct sset ct_limit_protection =
> > + SSET_INITIALIZER(&ct_limit_protection);
> > +
> > static void ct_dpif_format_counters(struct ds *,
> > const struct ct_dpif_counters *);
> > static void ct_dpif_format_timestamp(struct ds *,
> > @@ -1064,3 +1069,25 @@ ct_dpif_get_features(struct dpif *dpif, enum
> ct_features *features)
> > ? dpif->dpif_class->ct_get_features(dpif, features)
> > : EOPNOTSUPP);
> > }
> > +
> > +void
> > +ct_dpif_set_zone_limit_protection(struct dpif *dpif, bool protected)
> > +{
> > + if (sset_contains(&ct_limit_protection, dpif->full_name) ==
> protected) {
> > + return;
> > + }
> > +
> > + if (protected) {
> > + sset_add(&ct_limit_protection, dpif->full_name);
> > + } else {
> > + sset_find_and_delete(&ct_limit_protection, dpif->full_name);
> > + }
> > + VLOG_INFO("The CT zone limit protection is %s for \"%s\".",
> > + protected ? "enabled" : "disabled", dpif->full_name);
>
> This message is only useful for users who already use dpctl.
> And they will see the error while trying to use it. I don't
> think we need this message in the log file.
>
> > +}
> > +
> > +bool
> > +ct_dpif_is_zone_limit_protected(struct dpif *dpif)
> > +{
> > + return sset_contains(&ct_limit_protection, dpif->full_name);
> > +}
> > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> > index c8a7c155e..c3786d5ae 100644
> > --- a/lib/ct-dpif.h
> > +++ b/lib/ct-dpif.h
> > @@ -350,5 +350,7 @@ int ct_dpif_get_timeout_policy_name(struct dpif
> *dpif, uint32_t tp_id,
> > uint16_t dl_type, uint8_t nw_proto,
> > char **tp_name, bool *is_generic);
> > int ct_dpif_get_features(struct dpif *dpif, enum ct_features *features);
> > +void ct_dpif_set_zone_limit_protection(struct dpif *, bool protected);
> > +bool ct_dpif_is_zone_limit_protected(struct dpif *);
> >
> > #endif /* CT_DPIF_H */
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index a8c654747..8c87ff9e8 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > @@ -2234,6 +2234,12 @@ dpctl_ct_set_limits(int argc, const char *argv[],
> > ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0);
> > }
> >
> > + if (ct_dpif_is_zone_limit_protected(dpif)) {
> > + ds_put_cstr(&ds, "the zone limits are set via DB");
>
> I'd suggest something more user-friendly, e.g. "The zone limits are set
> via database, use 'ovs-vsctl set-zone-limit <...>' instead."
>
> > + error = EPERM;
> > + goto error;
> > + }
> > +
> > error = ct_dpif_set_limits(dpif, &zone_limits);
> > if (!error) {
> > ct_dpif_free_zone_limits(&zone_limits);
> > @@ -2310,6 +2316,12 @@ dpctl_ct_del_limits(int argc, const char *argv[],
> > }
> > }
> >
> > + if (ct_dpif_is_zone_limit_protected(dpif)) {
> > + ds_put_cstr(&ds, "the zone limits are set via DB");
>
> The same here, but with 'del-zone-limit'.
>
> > + error = EPERM;
> > + goto error;
> > + }
> > +
> > error = ct_dpif_del_limits(dpif, &zone_limits);
> > if (!error) {
> > goto out;
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 6a931a806..7c5360b67 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -5663,6 +5663,19 @@ ct_zone_limits_commit(struct dpif_backer *backer)
> > }
> > }
> >
> > +static void
> > +ct_zone_limit_protection_update(const char *datapath_type, bool
> protected)
> > +{
> > + struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> > + datapath_type);
> > + if (!backer) {
> > + return;
> > + }
> > +
> > + ct_dpif_set_zone_limit_protection(backer->dpif, protected);
> > +}
> > +
> > +
>
> Too many empty lines.
>
> > static void
> > get_datapath_cap(const char *datapath_type, struct smap *cap)
> > {
> > @@ -6953,4 +6966,5 @@ const struct ofproto_class ofproto_dpif_class = {
> > ct_set_zone_timeout_policy,
> > ct_del_zone_timeout_policy,
> > ct_zone_limit_update,
> > + ct_zone_limit_protection_update,
> > };
> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index 33fb99280..e1d72b6df 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -1925,6 +1925,11 @@ struct ofproto_class {
> > /* Updates the CT zone limit for specified zone. */
> > void (*ct_zone_limit_update)(const char *dp_type, int32_t zone,
> > int64_t *limit);
> > +
> > + /* Sets the CT zone limit protection to "protected" for the
> specified
> > + * datapath type. */
> > + void (*ct_zone_limit_protection_update)(const char *dp_type,
> > + bool protected);
> > };
> >
> > extern const struct ofproto_class ofproto_dpif_class;
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 649add089..122a06f30 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -1038,6 +1038,17 @@ ofproto_ct_zone_limit_update(const char
> *datapath_type, int32_t zone_id,
> > }
> > }
> >
> > +void
> > +ofproto_ct_zone_limit_protection_update(const char *datapath_type,
> > + bool protected)
> > +{
> > + datapath_type = ofproto_normalize_type(datapath_type);
> > + const struct ofproto_class *class =
> ofproto_class_find__(datapath_type);
> > +
> > + if (class && class->ct_zone_limit_protection_update) {
> > + class->ct_zone_limit_protection_update(datapath_type,
> protected);
> > + }
> > +}
> >
> > /* Spanning Tree Protocol (STP) configuration. */
> >
> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > index 7ce6a65e1..1c07df275 100644
> > --- a/ofproto/ofproto.h
> > +++ b/ofproto/ofproto.h
> > @@ -386,6 +386,8 @@ void ofproto_ct_del_zone_timeout_policy(const char
> *datapath_type,
> > uint16_t zone);
> > void ofproto_ct_zone_limit_update(const char *datapath_type, int32_t
> zone_id,
> > int64_t *limit);
> > +void ofproto_ct_zone_limit_protection_update(const char *datapath_type,
> > + bool protected);
> > void ofproto_get_datapath_cap(const char *datapath_type,
> > struct smap *dp_cap);
> >
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 445a9ffbd..df5c7f3d7 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -5275,6 +5275,42 @@ OVS_WAIT_UNTIL_EQUAL([ovs-appctl
> dpctl/ct-get-limits], [dnl
> > default limit=0
> > zone=0,limit=3,count=0])
> >
> > +dnl Try to overwrite the zone limit via dpctl command.
> > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=3,limit=5
> zone=0,limit=5], [2], [ignore], [ignore])
>
> Would be better to not ignore the error message.
> Same for tests below.
>
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=0
> > +zone=0,limit=3,count=0
> > +])
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0], [2], [ignore],
> [ignore])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=0
> > +zone=0,limit=3,count=0
> > +])
> > +
> > +AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE zone=0])
> > +AT_CHECK([ovs-vsctl set-zone-limit $DP_TYPE default limit=10])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=10
> > +])
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=1,limit=5],
> [2], [ignore], [ignore])
> > +
> > +dnl Delete all zones from DB, that should remove the protection.
> > +AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE default])
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=1,limit=5])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=15
> > +zone=1,limit=5,count=0
> > +])
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=1])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=15
> > +])
> > +
> > OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> > /could not create datapath/d
> > /(Cannot allocate memory) on packet/d"])
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 4545556b5..0fe348a77 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -739,6 +739,7 @@ datapath_destroy(struct datapath *dp)
> > NULL);
> > }
> >
> > + ofproto_ct_zone_limit_protection_update(dp->type, false);
> > hmap_remove(&all_datapaths, &dp->node);
> > hmap_destroy(&dp->ct_zones);
> > free(dp->type);
> > @@ -751,6 +752,7 @@ static void
> > ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath
> *dp_cfg)
> > {
> > struct ct_zone *ct_zone;
> > + bool protected = false;
> >
> > /* Add new 'ct_zone's or update existing 'ct_zone's based on the
> database
> > * state. */
> > @@ -784,6 +786,8 @@ ct_zones_reconfigure(struct datapath *dp, struct
> ovsrec_datapath *dp_cfg)
> > }
> >
> > ct_zone->last_used = idl_seqno;
> > +
> > + protected = protected || !!zone_cfg->limit;
> > }
> >
> > /* Purge 'ct_zone's no longer found in the database. */
> > @@ -804,6 +808,9 @@ ct_zones_reconfigure(struct datapath *dp, struct
> ovsrec_datapath *dp_cfg)
> > dp->ct_default_limit = default_limit;
> > }
> >
> > + protected = protected || !!dp_cfg->ct_zone_default_limit;
> > +
> > + ofproto_ct_zone_limit_protection_update(dp->type, protected);
> > }
> >
> > static void
>
>
Thank you for the review, all should be addressed in v6.
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
