Re: [ovs-dev] [PATCH] connmgr: Fix ofconn configuration on vswitchd startup.
On Sat, 23 Sept 2023 at 08:32, Eelco Chaudron wrote: > > Hi Brad, > > See one style comment below, the rest of the changes seem fine. > However, would it be possible to add a unit test for this fix? > Hi Eelco, Thanks for the review and feedback. I have sent a V2 patch which addresses the code formatting issue and adds a unit test. Thanks, Brad ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] connmgr: Fix ofconn configuration on vswitchd startup.
On 15 Sep 2023, at 6:36, Brad Cowie wrote:
> ofconn connection parameters, such as probe_interval and max_backoff,
> are always set to their default values when vswitchd starts up even if
> the user has configured these to be something different in ovsdb:
>
> $ ovs-vsctl set controller UUID inactivity_probe=9000
>
> $ journalctl -u ovs-vswitchd.service | grep "inactivity"
> ovs|10895|rconn|DBG|dp1<->tcp:127.0.0.1:6653: idle 9 seconds,
> sending inactivity probe
>
> $ systemctl restart openvswitch-switch.service
>
> $ journalctl -u ovs-vswitchd.service | grep "inactivity"
> ovs|00848|rconn|DBG|dp1<->tcp:127.0.0.1:6653: idle 5 seconds,
> sending inactivity probe
>
> This bug was introduced by commit a0baa7df (connmgr: Make treatment of
> active and passive connections more uniform.).
>
> This happens because ofservice_reconfigure() loops over each
> ofconn in ofservice->conns and calls ofconn_reconfigure() on it
> to set the configuration parameters, however when ofservice_reconfigure()
> is called from ofservice_create(), ofservice->conns hasn't been populated
> yet so ofconn_reconfigure() is never called.
>
> This commit moves the ofservice_reconfigure() call to ofconn_create()
> where ofservice->conns is populated.
>
> This commit also removes the hardcoded default values for
> inactivity_probe (5s) and max_backoff (8s) on initial creation
> of the ofservice, as these config values are available from the
> ofproto_controller struct c.
>
> Signed-off-by: Brad Cowie
Hi Brad,
See one style comment below, the rest of the changes seem fine.
However, would it be possible to add a unit test for this fix?
Cheers,
Eelco
> ---
> ofproto/connmgr.c | 12 +---
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> index b092e9e04..eafed8fe7 100644
> --- a/ofproto/connmgr.c
> +++ b/ofproto/connmgr.c
> @@ -1209,7 +1209,7 @@ ofconn_create(struct ofservice *ofservice, struct rconn
> *rconn,
> hmap_init(&ofconn->bundles);
> ofconn->next_bundle_expiry_check = time_msec() + BUNDLE_EXPIRY_INTERVAL;
>
> -ofconn_set_rate_limit(ofconn, settings->rate_limit,
> settings->burst_limit);
> +ofservice_reconfigure(ofservice, settings);
>
> ovs_mutex_unlock(&ofproto_mutex);
> }
> @@ -1915,10 +1915,7 @@ connmgr_count_hidden_rules(const struct connmgr *mgr)
> }
>
> /* Creates a new ofservice for 'target' in 'mgr'. Returns 0 if successful,
> - * otherwise a positive errno value.
> - *
> - * ofservice_reconfigure() must be called to fully configure the new
> - * ofservice. */
> + * otherwise a positive errno value. */
> static void
> ofservice_create(struct connmgr *mgr, const char *target,
> const struct ofproto_controller *c)
> @@ -1928,7 +1925,9 @@ ofservice_create(struct connmgr *mgr, const char
> *target,
> struct rconn *rconn = NULL;
> if (!vconn_verify_name(target)) {
> char *name = ofconn_make_name(mgr, target);
> -rconn = rconn_create(5, 8, c->dscp, c->allowed_versions);
> +rconn = rconn_create(
> +c->probe_interval, c->max_backoff,
> +c->dscp, c->allowed_versions);
In OVS if possible the split happens on an argument boundary. For example this
would become:
rconn = rconn_create(c->probe_interval, c->max_backoff,
c->dscp, c->allowed_versions);
> rconn_connect(rconn, target, name);
> free(name);
> } else if (!pvconn_verify_name(target)) {
> @@ -1951,7 +1950,6 @@ ofservice_create(struct connmgr *mgr, const char
> *target,
> ofservice->rconn = rconn;
> ofservice->pvconn = pvconn;
> ofservice->s = *c;
> -ofservice_reconfigure(ofservice, c);
>
> VLOG_INFO("%s: added %s controller \"%s\"",
>mgr->name, ofconn_type_to_string(ofservice->type), target);
> --
> 2.34.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] connmgr: Fix ofconn configuration on vswitchd startup.
On Fri, Sep 15, 2023 at 04:36:21AM +, Brad Cowie wrote: > ofconn connection parameters, such as probe_interval and max_backoff, > are always set to their default values when vswitchd starts up even if > the user has configured these to be something different in ovsdb: > > $ ovs-vsctl set controller UUID inactivity_probe=9000 > > $ journalctl -u ovs-vswitchd.service | grep "inactivity" > ovs|10895|rconn|DBG|dp1<->tcp:127.0.0.1:6653: idle 9 seconds, > sending inactivity probe > > $ systemctl restart openvswitch-switch.service > > $ journalctl -u ovs-vswitchd.service | grep "inactivity" > ovs|00848|rconn|DBG|dp1<->tcp:127.0.0.1:6653: idle 5 seconds, > sending inactivity probe > > This bug was introduced by commit a0baa7df (connmgr: Make treatment of > active and passive connections more uniform.). > > This happens because ofservice_reconfigure() loops over each > ofconn in ofservice->conns and calls ofconn_reconfigure() on it > to set the configuration parameters, however when ofservice_reconfigure() > is called from ofservice_create(), ofservice->conns hasn't been populated > yet so ofconn_reconfigure() is never called. > > This commit moves the ofservice_reconfigure() call to ofconn_create() > where ofservice->conns is populated. > > This commit also removes the hardcoded default values for > inactivity_probe (5s) and max_backoff (8s) on initial creation > of the ofservice, as these config values are available from the > ofproto_controller struct c. > > Signed-off-by: Brad Cowie Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] connmgr: Fix ofconn configuration on vswitchd startup.
ofconn connection parameters, such as probe_interval and max_backoff,
are always set to their default values when vswitchd starts up even if
the user has configured these to be something different in ovsdb:
$ ovs-vsctl set controller UUID inactivity_probe=9000
$ journalctl -u ovs-vswitchd.service | grep "inactivity"
ovs|10895|rconn|DBG|dp1<->tcp:127.0.0.1:6653: idle 9 seconds,
sending inactivity probe
$ systemctl restart openvswitch-switch.service
$ journalctl -u ovs-vswitchd.service | grep "inactivity"
ovs|00848|rconn|DBG|dp1<->tcp:127.0.0.1:6653: idle 5 seconds,
sending inactivity probe
This bug was introduced by commit a0baa7df (connmgr: Make treatment of
active and passive connections more uniform.).
This happens because ofservice_reconfigure() loops over each
ofconn in ofservice->conns and calls ofconn_reconfigure() on it
to set the configuration parameters, however when ofservice_reconfigure()
is called from ofservice_create(), ofservice->conns hasn't been populated
yet so ofconn_reconfigure() is never called.
This commit moves the ofservice_reconfigure() call to ofconn_create()
where ofservice->conns is populated.
This commit also removes the hardcoded default values for
inactivity_probe (5s) and max_backoff (8s) on initial creation
of the ofservice, as these config values are available from the
ofproto_controller struct c.
Signed-off-by: Brad Cowie
---
ofproto/connmgr.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index b092e9e04..eafed8fe7 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1209,7 +1209,7 @@ ofconn_create(struct ofservice *ofservice, struct rconn
*rconn,
hmap_init(&ofconn->bundles);
ofconn->next_bundle_expiry_check = time_msec() + BUNDLE_EXPIRY_INTERVAL;
-ofconn_set_rate_limit(ofconn, settings->rate_limit, settings->burst_limit);
+ofservice_reconfigure(ofservice, settings);
ovs_mutex_unlock(&ofproto_mutex);
}
@@ -1915,10 +1915,7 @@ connmgr_count_hidden_rules(const struct connmgr *mgr)
}
/* Creates a new ofservice for 'target' in 'mgr'. Returns 0 if successful,
- * otherwise a positive errno value.
- *
- * ofservice_reconfigure() must be called to fully configure the new
- * ofservice. */
+ * otherwise a positive errno value. */
static void
ofservice_create(struct connmgr *mgr, const char *target,
const struct ofproto_controller *c)
@@ -1928,7 +1925,9 @@ ofservice_create(struct connmgr *mgr, const char *target,
struct rconn *rconn = NULL;
if (!vconn_verify_name(target)) {
char *name = ofconn_make_name(mgr, target);
-rconn = rconn_create(5, 8, c->dscp, c->allowed_versions);
+rconn = rconn_create(
+c->probe_interval, c->max_backoff,
+c->dscp, c->allowed_versions);
rconn_connect(rconn, target, name);
free(name);
} else if (!pvconn_verify_name(target)) {
@@ -1951,7 +1950,6 @@ ofservice_create(struct connmgr *mgr, const char *target,
ofservice->rconn = rconn;
ofservice->pvconn = pvconn;
ofservice->s = *c;
-ofservice_reconfigure(ofservice, c);
VLOG_INFO("%s: added %s controller \"%s\"",
mgr->name, ofconn_type_to_string(ofservice->type), target);
--
2.34.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
