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
