On 28 Sep 2023, at 3:27, 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 <[email protected]> > Acked-by: Simon Horman <[email protected]> Thanks for sending a v2, see some styling issues below. Cheers, Eelco > --- > ofproto/connmgr.c | 11 ++++------- > tests/ofproto.at | 25 +++++++++++++++++++++++++ > 2 files changed, 29 insertions(+), 7 deletions(-) > > diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c > index b092e9e04..d1bd5edee 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,8 @@ 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); Indentation is off here, it should align to the opening parenthesis of the function. + 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 +1949,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); > diff --git a/tests/ofproto.at b/tests/ofproto.at > index 2fa8486a8..a9541d709 100644 > --- a/tests/ofproto.at > +++ b/tests/ofproto.at > @@ -6720,3 +6720,28 @@ > recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0806),arp(tip=172.31.1 > > OVS_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([ofproto - configure inactivity probe interval]) Add a new line before a comment. > +# Set 6 second inactivity probe interval (default is 5 seconds) All comments should end with a dot. > +OVS_VSWITCHD_START([set-controller br0 unix:testcontroller \ > + -- set Controller br0 inactivity_probe=6000], [], [], > + [-vfile:rconn:dbg]) + \n > +# Start test openflow controller + . > +on_exit 'kill `cat ovs-testcontroller.pid`' > +AT_CHECK([ovs-testcontroller -vsyslog:off --detach --no-chdir --pidfile > punix:testcontroller], > + [0], [ignore]) I would swap the above two commands, so we will only try to kill the controller if started successfully. > +OVS_WAIT_UNTIL([test -e testcontroller]) + \n > +# After 6 seconds of inactivity there should be a log message + . > +OVS_WAIT_UNTIL([grep "sending inactivity probe" ovs-vswitchd.log]) > +AT_CHECK([grep "idle 6 seconds, sending inactivity probe" ovs-vswitchd.log], > [], [stdout]) Can we not move the OVS_WAIT_UNTIL to the second command, and skip the first? + \n > +# Restart ovs-vswitchd and empty the ovs-vswitchd log file + . > +OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) > +echo > ovs-vswitchd.log I think we should move the log, so it’s easy to debug if needed. Something like: mv ovs-vswitchd.log ovs-vswitchd_1.log > +AT_CHECK([ovs-vswitchd --enable-dummy --disable-system > --disable-system-route --detach \ > + --no-chdir --pidfile --log-file -vfile:rconn:dbg -vvconn > -vofproto_dpif -vunixctl], > + [0], [], [stderr]) + \n > +# After 6 seconds of inactivity there should be a log message + . > +OVS_WAIT_UNTIL([grep "sending inactivity probe" ovs-vswitchd.log]) > +AT_CHECK([grep "idle 6 seconds, sending inactivity probe" ovs-vswitchd.log], > [], [stdout]) + \n > +OVS_VSWITCHD_STOP(["/br0<->unix:testcontroller: connection failed/d"]) > +AT_CLEANUP > -- > 2.34.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
