Thx a lot for fixing this, pushed to branch-2.3
On Fri, May 30, 2014 at 1:07 PM, Alex Wang <al...@nicira.com> wrote:
> Acked-by: Alex Wang <al...@nicira.com>
>
> I'm about to apply it with the following additional fold in.
>
> Basically, move the 'bool force' before the struct which holds
> returned bfd/cfm status.
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index bae5e6a..7f59b94 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1826,8 +1826,8 @@ out:
> }
>
> static int
> -get_cfm_status(const struct ofport *ofport_,
> - struct ofproto_cfm_status *status, bool force)
> +get_cfm_status(const struct ofport *ofport_, bool force,
> + struct ofproto_cfm_status *status)
> {
> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
> int ret = 0;
> @@ -1868,7 +1868,7 @@ set_bfd(struct ofport *ofport_, const struct smap
> *cfg)
> }
>
> static int
> -get_bfd_status(struct ofport *ofport_, struct smap *smap, bool force)
> +get_bfd_status(struct ofport *ofport_, bool force, struct smap *smap)
> {
> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
> int ret = 0;
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 84b726d..639b501 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1449,8 +1449,8 @@ struct ofproto_class {
> * The caller must provide and own '*status', and it must free the
> array
> * returned in 'status->rmps'. '*status' is indeterminate if the
> return
> * value is non-zero. */
> - int (*get_cfm_status)(const struct ofport *ofport,
> - struct ofproto_cfm_status *status, bool force);
> + int (*get_cfm_status)(const struct ofport *ofport, bool force,
> + struct ofproto_cfm_status *status);
>
> /* Configures BFD on 'ofport'.
> *
> @@ -1472,7 +1472,7 @@ struct ofproto_class {
> *
> * EOPNOTSUPP as a return value indicates that this ofproto_class
> does not
> * support BFD, as does a null pointer. */
> - int (*get_bfd_status)(struct ofport *ofport, struct smap *smap, bool
> force);
> + int (*get_bfd_status)(struct ofport *ofport, bool force, struct smap
> *smap);
>
> /* Configures spanning tree protocol (STP) on 'ofproto' using the
> * settings defined in 's'.
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index a501032..608ddad 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1026,11 +1026,11 @@ ofproto_port_set_bfd(struct ofproto *ofproto,
> ofp_port_t ofp_port,
> * The caller must provide and own '*status'. */
> int
> ofproto_port_get_bfd_status(struct ofproto *ofproto, ofp_port_t ofp_port,
> - struct smap *status, bool force)
> + bool force, struct smap *status)
> {
> struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
> return (ofport && ofproto->ofproto_class->get_bfd_status
> - ? ofproto->ofproto_class->get_bfd_status(ofport, status,
> force)
> + ? ofproto->ofproto_class->get_bfd_status(ofport, force,
> status)
> : EOPNOTSUPP);
> }
>
> @@ -3730,11 +3730,11 @@ ofproto_get_netflow_ids(const struct ofproto
> *ofproto,
> * '*status' is indeterminate if the return value is non-zero. */
> int
> ofproto_port_get_cfm_status(const struct ofproto *ofproto, ofp_port_t
> ofp_port,
> - struct ofproto_cfm_status *status, bool force)
> + bool force, struct ofproto_cfm_status *status)
> {
> struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
> return (ofport && ofproto->ofproto_class->get_cfm_status
> - ? ofproto->ofproto_class->get_cfm_status(ofport, status,
> force)
> + ? ofproto->ofproto_class->get_cfm_status(ofport, force,
> status)
> : EOPNOTSUPP);
> }
>
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index b18e11a..9a06849 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -265,7 +265,7 @@ void ofproto_port_set_cfm(struct ofproto *, ofp_port_t
> ofp_port,
> void ofproto_port_set_bfd(struct ofproto *, ofp_port_t ofp_port,
> const struct smap *cfg);
> int ofproto_port_get_bfd_status(struct ofproto *, ofp_port_t ofp_port,
> - struct smap *, bool force);
> + bool force, struct smap *);
> int ofproto_port_is_lacp_current(struct ofproto *, ofp_port_t ofp_port);
> int ofproto_port_set_stp(struct ofproto *, ofp_port_t ofp_port,
> const struct ofproto_port_stp_settings *);
> @@ -420,9 +420,8 @@ struct ofproto_cfm_status {
> };
>
> int ofproto_port_get_cfm_status(const struct ofproto *,
> - ofp_port_t ofp_port,
> - struct ofproto_cfm_status *,
> - bool force);
> + ofp_port_t ofp_port, bool force,
> + struct ofproto_cfm_status *);
> ^L
> /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
> *
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 57eb817..039c62e 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1913,8 +1913,8 @@ iface_refresh_ofproto_status(struct iface *iface)
>
> smap_init(&smap);
> error = ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
> - iface->ofp_port, &smap,
> - force_status_commit);
> + iface->ofp_port,
> force_status_commit,
> + &smap);
> if (error >= 0) {
> ovsrec_interface_set_bfd_status(iface->cfg, &smap);
> }
> @@ -1931,8 +1931,8 @@ iface_refresh_cfm_stats(struct iface *iface)
> int error;
>
> error = ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
> - iface->ofp_port, &status,
> - force_status_commit);
> + iface->ofp_port,
> force_status_commit,
> + &status);
> if (error < 0) {
> /* Do nothing if there is no status change since last update. */
> } else if (error > 0) {
>
>
>
>
> On Fri, May 30, 2014 at 11:38 AM, Ryan Wilson <wr...@nicira.com> wrote:
>
>> Bridge, port and interface status changes are not sent to the
>> database if the connectivity and netdev sequence numbers have not
>> changed. However, if the previous database transaction fails, then
>> status changes will not be updated in the database until the
>> connectivity and netdev sequence numbers change again. This could
>> leave the database in an incorrect state for a long period of time.
>>
>> This patch always sends status changes to the database if the last
>> transaction was not successful.
>>
>> Bug #1256577
>> Signed-off-by: Ryan Wilson <wr...@nicira.com>
>>
>> ---
>> v2: Addressed Alex's comments, edited commit message to be more
>> accurate
>> v3: Remove iface_refresh_netdev_status() from iface_create() upon
>> further discussion with Alex
>> v4: Changed force_status_update to force_status_commit since it now
>> is set to true when status == 'TXN_INCOMPLETE'. Status txn also
>> needs to be run every STATUS_CHECK_AGAIN_MSEC if last transaction
>> was not successful.
>> v5: Add static identifier to force_status_commit.Add force update
>> BFD and CFM status.
>> ---
>> ofproto/ofproto-dpif.c | 8 ++++----
>> ofproto/ofproto-provider.h | 25 ++++++++++++++++---------
>> ofproto/ofproto.c | 31 ++++++++++++++++++-------------
>> ofproto/ofproto.h | 5 +++--
>> vswitchd/bridge.c | 41
>> ++++++++++++++++++++++++++++++-----------
>> 5 files changed, 71 insertions(+), 39 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 06be234..4b73158 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -1821,13 +1821,13 @@ out:
>>
>> static int
>> get_cfm_status(const struct ofport *ofport_,
>> - struct ofproto_cfm_status *status)
>> + struct ofproto_cfm_status *status, bool force)
>> {
>> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
>> int ret = 0;
>>
>> if (ofport->cfm) {
>> - if (cfm_check_status_change(ofport->cfm)) {
>> + if (cfm_check_status_change(ofport->cfm) || force) {
>> status->faults = cfm_get_fault(ofport->cfm);
>> status->flap_count = cfm_get_flap_count(ofport->cfm);
>> status->remote_opstate = cfm_get_opup(ofport->cfm);
>> @@ -1862,13 +1862,13 @@ set_bfd(struct ofport *ofport_, const struct smap
>> *cfg)
>> }
>>
>> static int
>> -get_bfd_status(struct ofport *ofport_, struct smap *smap)
>> +get_bfd_status(struct ofport *ofport_, struct smap *smap, bool force)
>> {
>> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
>> int ret = 0;
>>
>> if (ofport->bfd) {
>> - if (bfd_check_status_change(ofport->bfd)) {
>> + if (bfd_check_status_change(ofport->bfd) || force) {
>> bfd_get_status(ofport->bfd, smap);
>> } else {
>> ret = NO_STATUS_CHANGE;
>> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
>> index ff539b9..212ed24 100644
>> --- a/ofproto/ofproto-provider.h
>> +++ b/ofproto/ofproto-provider.h
>> @@ -1442,10 +1442,13 @@ struct ofproto_class {
>> * support CFM, as does a null pointer. */
>> int (*set_cfm)(struct ofport *ofport, const struct cfm_settings *s);
>>
>> - /* Checks the status of CFM configured on 'ofport'. Returns 0 if the
>> - * port's CFM status was successfully stored into '*status'. Returns
>> - * negative number if there is no status change since last update.
>> - * Returns positive errno otherwise.
>> + /* Checks the status of CFM configured on 'ofport' and stores port's
>> CFM
>> + * status in '*status'. If 'force' is set to true, status will be
>> returned
>> + * even if there is no status change since last update.
>> + *
>> + * Returns 0 on success. Returns a negative number if there is no
>> status
>> + * change since last update and 'force' is set to false. Returns
>> positive
>> + * errno otherwise.
>> *
>> * EOPNOTSUPP as a return value indicates that this ofproto_class
>> does not
>> * support CFM, as does a null pointer.
>> @@ -1454,7 +1457,7 @@ struct ofproto_class {
>> * returned in 'status->rmps'. '*status' is indeterminate if the
>> return
>> * value is non-zero. */
>> int (*get_cfm_status)(const struct ofport *ofport,
>> - struct ofproto_cfm_status *status);
>> + struct ofproto_cfm_status *status, bool force);
>>
>> /* Configures BFD on 'ofport'.
>> *
>> @@ -1466,13 +1469,17 @@ struct ofproto_class {
>> * support BFD, as does a null pointer. */
>> int (*set_bfd)(struct ofport *ofport, const struct smap *cfg);
>>
>> - /* Populates 'smap' with the status of BFD on 'ofport'. Returns 0 on
>> - * success. Returns a negative number if there is no status change
>> since
>> - * last update. Returns a positive errno otherwise.
>> + /* Populates 'smap' with the status of BFD on 'ofport'. If 'force'
>> is set
>> + * to true, status will be returned even if there is no status
>> change since
>> + * last update.
>> + *
>> + * Returns 0 on success. Returns a negative number if there is no
>> status
>> + * change since last update and 'force' is set to false. Returns a
>> positive
>> + * errno otherwise.
>> *
>> * EOPNOTSUPP as a return value indicates that this ofproto_class
>> does not
>> * support BFD, as does a null pointer. */
>> - int (*get_bfd_status)(struct ofport *ofport, struct smap *smap);
>> + int (*get_bfd_status)(struct ofport *ofport, struct smap *smap, bool
>> force);
>>
>> /* Configures spanning tree protocol (STP) on 'ofproto' using the
>> * settings defined in 's'.
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 24a709b..2b872b7 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -1020,19 +1020,22 @@ ofproto_port_set_bfd(struct ofproto *ofproto,
>> ofp_port_t ofp_port,
>> }
>> }
>>
>> -/* Populates 'status' with the status of BFD on 'ofport'. Returns 0 on
>> - * success. Returns a negative number if there is no status change since
>> - * last update. Returns a positive errno otherwise. Has no effect if
>> - * 'ofp_port' is not an OpenFlow port in 'ofproto'.
>> +/* Populates 'status' with the status of BFD on 'ofport'. If 'force' is
>> set to
>> + * true, status will be returned even if there is no status change since
>> last
>> + * update.
>> + *
>> + * Returns 0 on success. Returns a negative number if there is no
>> status change
>> + * since last update and 'force' is set to false. Returns a positive
>> errno
>> + * otherwise. Has no effect if 'ofp_port' is not an OpenFlow port in
>> 'ofproto'.
>> *
>> * The caller must provide and own '*status'. */
>> int
>> ofproto_port_get_bfd_status(struct ofproto *ofproto, ofp_port_t ofp_port,
>> - struct smap *status)
>> + struct smap *status, bool force)
>> {
>> struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
>> return (ofport && ofproto->ofproto_class->get_bfd_status
>> - ? ofproto->ofproto_class->get_bfd_status(ofport, status)
>> + ? ofproto->ofproto_class->get_bfd_status(ofport, status,
>> force)
>> : EOPNOTSUPP);
>> }
>>
>> @@ -3738,21 +3741,23 @@ ofproto_get_netflow_ids(const struct ofproto
>> *ofproto,
>> ofproto->ofproto_class->get_netflow_ids(ofproto, engine_type,
>> engine_id);
>> }
>>
>> -/* Checks the status of CFM configured on 'ofp_port' within 'ofproto'.
>> - * Returns 0 if the port's CFM status was successfully stored into
>> - * '*status'. Returns positive errno if the port did not have CFM
>> - * configured. Returns negative number if there is no status change
>> - * since last update.
>> +/* Checks the status of CFM configured on 'ofp_port' within 'ofproto'
>> and stores
>> + * the port's CFM status in '*status'. If 'force' is set to true,
>> status will
>> + * be returned even if there is no status change since last update.
>> + *
>> + * Returns 0 on success. Returns a negative number if there is no status
>> + * change since last update and 'force' is set to false. Returns
>> positive errno
>> + * if the port did not have CFM configured.
>> *
>> * The caller must provide and own '*status', and must free
>> 'status->rmps'.
>> * '*status' is indeterminate if the return value is non-zero. */
>> int
>> ofproto_port_get_cfm_status(const struct ofproto *ofproto, ofp_port_t
>> ofp_port,
>> - struct ofproto_cfm_status *status)
>> + struct ofproto_cfm_status *status, bool
>> force)
>> {
>> struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
>> return (ofport && ofproto->ofproto_class->get_cfm_status
>> - ? ofproto->ofproto_class->get_cfm_status(ofport, status)
>> + ? ofproto->ofproto_class->get_cfm_status(ofport, status,
>> force)
>> : EOPNOTSUPP);
>> }
>>
>> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
>> index de078b7..aa7b95d 100644
>> --- a/ofproto/ofproto.h
>> +++ b/ofproto/ofproto.h
>> @@ -265,7 +265,7 @@ void ofproto_port_set_cfm(struct ofproto *,
>> ofp_port_t ofp_port,
>> void ofproto_port_set_bfd(struct ofproto *, ofp_port_t ofp_port,
>> const struct smap *cfg);
>> int ofproto_port_get_bfd_status(struct ofproto *, ofp_port_t ofp_port,
>> - struct smap *);
>> + struct smap *, bool force);
>> int ofproto_port_is_lacp_current(struct ofproto *, ofp_port_t ofp_port);
>> int ofproto_port_set_stp(struct ofproto *, ofp_port_t ofp_port,
>> const struct ofproto_port_stp_settings *);
>> @@ -424,7 +424,8 @@ struct ofproto_cfm_status {
>>
>> int ofproto_port_get_cfm_status(const struct ofproto *,
>> ofp_port_t ofp_port,
>> - struct ofproto_cfm_status *);
>> + struct ofproto_cfm_status *,
>> + bool force);
>>
>> /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
>> *
>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>> index 9764c1f..a20e950 100644
>> --- a/vswitchd/bridge.c
>> +++ b/vswitchd/bridge.c
>> @@ -172,9 +172,15 @@ static uint64_t connectivity_seqno = LLONG_MIN;
>> * we check the return status of each update transaction and do not
>> start new
>> * update if the previous transaction status is 'TXN_INCOMPLETE'.
>> *
>> - * 'statux_txn' is NULL if there is no ongoing status update.
>> + * 'status_txn' is NULL if there is no ongoing status update.
>> + *
>> + * If the previous database transaction was incomplete or failed (is not
>> + * 'TXN_SUCCESS' or 'TXN_UNCHANGED'), 'force_status_commit' is set to
>> true.
>> + * This means that 'status_txn' must be committed next iteration of
>> bridge_run()
>> + * even if the connectivity or netdev sequence numbers do not change.
>> */
>> static struct ovsdb_idl_txn *status_txn;
>> +static bool force_status_commit = true;
>>
>> /* When the status update transaction returns 'TXN_INCOMPLETE', should
>> register a
>> * timeout in 'STATUS_CHECK_AGAIN_MSEC' to check again. */
>> @@ -1547,7 +1553,6 @@ iface_create(struct bridge *br, const struct
>> ovsrec_interface *iface_cfg,
>>
>> /* Populate initial status in database. */
>> iface_refresh_stats(iface);
>> - iface_refresh_netdev_status(iface);
>>
>> /* Add bond fake iface if necessary. */
>> if (port_is_bond_fake_iface(port)) {
>> @@ -1820,7 +1825,8 @@ iface_refresh_netdev_status(struct iface *iface)
>> return;
>> }
>>
>> - if (iface->change_seq == netdev_get_change_seq(iface->netdev)) {
>> + if (iface->change_seq == netdev_get_change_seq(iface->netdev)
>> + && !force_status_commit) {
>> return;
>> }
>>
>> @@ -1913,7 +1919,8 @@ iface_refresh_ofproto_status(struct iface *iface)
>>
>> smap_init(&smap);
>> error = ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
>> - iface->ofp_port, &smap);
>> + iface->ofp_port, &smap,
>> + force_status_commit);
>> if (error >= 0) {
>> ovsrec_interface_set_bfd_status(iface->cfg, &smap);
>> }
>> @@ -1930,7 +1937,8 @@ iface_refresh_cfm_stats(struct iface *iface)
>> int error;
>>
>> error = ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
>> - iface->ofp_port, &status);
>> + iface->ofp_port, &status,
>> + force_status_commit);
>> if (error < 0) {
>> /* Do nothing if there is no status change since last update. */
>> } else if (error > 0) {
>> @@ -2420,7 +2428,7 @@ bridge_run(void)
>>
>> /* Check the need to update status. */
>> seq = seq_read(connectivity_seq_get());
>> - if (seq != connectivity_seqno) {
>> + if (seq != connectivity_seqno || force_status_commit) {
>> connectivity_seqno = seq;
>> status_txn = ovsdb_idl_txn_create(idl);
>> HMAP_FOR_EACH (br, node, &all_bridges) {
>> @@ -2444,6 +2452,17 @@ bridge_run(void)
>> enum ovsdb_idl_txn_status status;
>>
>> status = ovsdb_idl_txn_commit(status_txn);
>> +
>> + /* If the transaction is incomplete or fails, 'status_txn'
>> + * needs to be committed next iteration of bridge_run() even if
>> + * connectivity or netdev sequence numbers do not change. */
>> + if (status == TXN_SUCCESS || status == TXN_UNCHANGED)
>> + {
>> + force_status_commit = false;
>> + } else {
>> + force_status_commit = true;
>> + }
>> +
>> /* Do not destroy "status_txn" if the transaction is
>> * "TXN_INCOMPLETE". */
>> if (status != TXN_INCOMPLETE) {
>> @@ -2483,11 +2502,11 @@ bridge_wait(void)
>> poll_timer_wait_until(stats_timer);
>> }
>>
>> - /* If the status database transaction is 'TXN_INCOMPLETE' in this
>> run,
>> - * register a timeout in 'STATUS_CHECK_AGAIN_MSEC'. Else, wait on
>> the
>> - * global connectivity sequence number. Note, this also helps batch
>> - * multiple status changes into one transaction. */
>> - if (status_txn) {
>> + /* If the status database transaction is 'TXN_INCOMPLETE' or is
>> + * unsuccessful, register a timeout in 'STATUS_CHECK_AGAIN_MSEC'.
>> Else,
>> + * wait on the global connectivity sequence number. Note, this also
>> helps
>> + * batch multiple status changes into one transaction. */
>> + if (force_status_commit) {
>> poll_timer_wait_until(time_msec() + STATUS_CHECK_AGAIN_MSEC);
>> } else {
>> seq_wait(connectivity_seq_get(), connectivity_seqno);
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
