Acked-by: Justin Pettit <jpet...@nicira.com>
On Tue, Jul 22, 2014 at 3:58 PM, Ben Pfaff <b...@nicira.com> wrote:
> Only OpenFlow 1.4 and later support role status messages, but this code
> tried to send them to all controllers, which caused an assertion failure.
>
> Also, add tests to check that role status messages work, and that they
> don't cause trouble with OF1.2.
>
> Reported-by: Anup Khadka <khadka...@gmail.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> AUTHORS | 1 +
> lib/ofp-util.c | 27 ++++++----
> ofproto/connmgr.c | 5 +-
> tests/ofproto.at | 147
> ++++++++++++++++++++++++++++++++++++++++++-----------
> 4 files changed, 139 insertions(+), 41 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index 3e97bbe..b505a07 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -161,6 +161,7 @@ Andreas Beckmann deb...@abeckmann.de
> Andrei Andone andrei.and...@softvision.ro
> Anshuman Manral anshuman.man...@outlook.com
> Anton Matsiuk anton.mats...@gmail.com
> +Anup Khadka khadka...@gmail.com
> Anuprem Chalvadi achalv...@vmware.com
> Ariel Tubaltsev atubalt...@vmware.com
> Atzm Watanabe a...@stratosphere.co.jp
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 912def9..a91031c 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -4977,22 +4977,31 @@ ofputil_encode_role_reply(const struct ofp_header
> *request,
> return buf;
> }
>
> +/* Encodes "role status" message 'status' for sending in the given
> + * 'protocol'. Returns the role status message, if 'protocol' supports
> them,
> + * otherwise a null pointer. */
> struct ofpbuf *
> ofputil_encode_role_status(const struct ofputil_role_status *status,
> enum ofputil_protocol protocol)
> {
> - struct ofpbuf *buf;
> enum ofp_version version;
> - struct ofp14_role_status *rstatus;
>
> version = ofputil_protocol_to_ofp_version(protocol);
> - buf = ofpraw_alloc_xid(OFPRAW_OFPT14_ROLE_STATUS, version, htonl(0),
> 0);
> - rstatus = ofpbuf_put_zeros(buf, sizeof *rstatus);
> - rstatus->role = htonl(status->role);
> - rstatus->reason = status->reason;
> - rstatus->generation_id = htonll(status->generation_id);
> -
> - return buf;
> + if (version >= OFP14_VERSION) {
> + struct ofp14_role_status *rstatus;
> + struct ofpbuf *buf;
> +
> + buf = ofpraw_alloc_xid(OFPRAW_OFPT14_ROLE_STATUS, version,
> htonl(0),
> + 0);
> + rstatus = ofpbuf_put_zeros(buf, sizeof *rstatus);
> + rstatus->role = htonl(status->role);
> + rstatus->reason = status->reason;
> + rstatus->generation_id = htonll(status->generation_id);
> +
> + return buf;
> + } else {
> + return NULL;
> + }
> }
>
> enum ofperr
> diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> index be99529..05fe0c4 100644
> --- a/ofproto/connmgr.c
> +++ b/ofproto/connmgr.c
> @@ -920,8 +920,9 @@ ofconn_send_role_status(struct ofconn *ofconn,
> uint32_t role, uint8_t reason)
> ofconn_get_master_election_id(ofconn, &status.generation_id);
>
> buf = ofputil_encode_role_status(&status,
> ofconn_get_protocol(ofconn));
> -
> - ofconn_send(ofconn, buf, NULL);
> + if (buf) {
> + ofconn_send(ofconn, buf, NULL);
> + }
> }
>
> /* Changes 'ofconn''s role to 'role'. If 'role' is OFPCR12_ROLE_MASTER
> then
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 729ef7b..3a55ce3 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1875,41 +1875,128 @@ dnl This test checks that the role
> request/response messaging works
> dnl and that generation_id is handled properly.
> AT_SETUP([ofproto - controller role (OpenFlow 1.2)])
> OVS_VSWITCHD_START
> -AT_CHECK([ovs-ofctl -O OpenFlow12 monitor br0 --detach --no-chdir
> --pidfile])
> -
> -ovs-appctl -t ovs-ofctl ofctl/barrier
> -ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
> -: > expout
> -: > experr
> +ON_EXIT([kill `cat c1.pid c2.pid`])
> +
> +# Start two ovs-ofctl controller processes.
> +AT_CAPTURE_FILE([monitor1.log])
> +AT_CAPTURE_FILE([expout1])
> +AT_CAPTURE_FILE([experr1])
> +AT_CAPTURE_FILE([monitor2.log])
> +AT_CAPTURE_FILE([expout2])
> +AT_CAPTURE_FILE([experr2])
> +for i in 1 2; do
> + AT_CHECK([ovs-ofctl -O OpenFlow12 monitor br0 --detach --no-chdir
> --pidfile=`pwd`/c$i.pid --unixctl=`pwd`/c$i])
> + ovs-appctl -t `pwd`/c$i ofctl/barrier
> + ovs-appctl -t `pwd`/c$i ofctl/set-output-file monitor$i.log
> + : > expout$i
> + : > experr$i
> +
> + # find out current role
> + ovs-appctl -t `pwd`/c$i ofctl/send
> 031800180000000200000000000000000000000000000000
> + echo >>experr$i "send: OFPT_ROLE_REQUEST (OF1.2): role=nochange"
> + echo >>expout$i "OFPT_ROLE_REPLY (OF1.2): role=equal"
> +done
>
> -# find out current role
> -ovs-appctl -t ovs-ofctl ofctl/send
> 031800180000000200000000000000000000000000000000
> -echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x2): role=nochange"
> -echo >>expout "OFPT_ROLE_REPLY (OF1.2) (xid=0x2): role=equal"
> +# controller 1: Become slave (generation_id is initially undefined, so
> +# 2^63+2 should not be stale)
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 031800180000000300000003000000008000000000000002
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.2): role=slave
> generation_id=9223372036854775810"
> +echo >>expout1 "OFPT_ROLE_REPLY (OF1.2): role=slave
> generation_id=9223372036854775810"
> +
> +# controller 2: Become master.
> +ovs-appctl -t `pwd`/c2 ofctl/send
> 031800180000000300000002000000008000000000000003
> +echo >>experr2 "send: OFPT_ROLE_REQUEST (OF1.2): role=master
> generation_id=9223372036854775811"
> +echo >>expout2 "OFPT_ROLE_REPLY (OF1.2): role=master
> generation_id=9223372036854775811"
> +
> +# controller 1: Try to become the master using a stale generation ID
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 031800180000000400000002000000000000000000000003
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.2): role=master
> generation_id=3"
> +echo >>expout1 "OFPT_ERROR (OF1.2): OFPRRFC_STALE"
> +echo >>expout1 "OFPT_ROLE_REQUEST (OF1.2): role=master generation_id=3"
> +
> +# controller 1: Become master using a valid generation ID
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 031800180000000500000002000000000000000000000001
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.2): role=master
> generation_id=1"
> +echo >>expout1 "OFPT_ROLE_REPLY (OF1.2): role=master generation_id=1"
> +
> +for i in 1 2; do
> + ovs-appctl -t `pwd`/c$i ofctl/barrier
> + echo >>expout$i "OFPT_BARRIER_REPLY (OF1.2):"
> +done
>
> -# Become slave (generation_id is initially undefined, so 2^63+2 should
> not be stale)
> -ovs-appctl -t ovs-ofctl ofctl/send
> 031800180000000300000003000000008000000000000002
> -echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x3): role=slave
> generation_id=9223372036854775810"
> -echo >>expout "OFPT_ROLE_REPLY (OF1.2) (xid=0x3): role=slave
> generation_id=9223372036854775810"
> +# Check output.
> +for i in 1 2; do
> + cp expout$i expout
> + AT_CHECK([grep -v '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
> + cp experr$i expout
> + AT_CHECK([grep '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
> +done
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
>
> -# Try to become the master using a stale generation ID
> -ovs-appctl -t ovs-ofctl ofctl/send
> 031800180000000400000002000000000000000000000002
> -echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x4): role=master
> generation_id=2"
> -echo >>expout "OFPT_ERROR (OF1.2) (xid=0x4): OFPRRFC_STALE"
> -echo >>expout "OFPT_ROLE_REQUEST (OF1.2) (xid=0x4): role=master
> generation_id=2"
> -
> -# Become master using a valid generation ID
> -ovs-appctl -t ovs-ofctl ofctl/send
> 031800180000000500000002000000000000000000000001
> -echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x5): role=master
> generation_id=1"
> -echo >>expout "OFPT_ROLE_REPLY (OF1.2) (xid=0x5): role=master
> generation_id=1"
> -ovs-appctl -t ovs-ofctl ofctl/barrier
> -echo >>expout "OFPT_BARRIER_REPLY (OF1.2) (xid=0x3):"
> +dnl This test checks that the role request/response messaging works,
> +dnl that generation_id is handled properly, and that role status update
> +dnl messages are sent when a controller's role gets changed from master
> +dnl to slave.
> +AT_SETUP([ofproto - controller role (OpenFlow 1.4)])
> +OVS_VSWITCHD_START
> +ON_EXIT([kill `cat c1.pid c2.pid`])
> +
> +# Start two ovs-ofctl controller processes.
> +AT_CAPTURE_FILE([monitor1.log])
> +AT_CAPTURE_FILE([expout1])
> +AT_CAPTURE_FILE([experr1])
> +AT_CAPTURE_FILE([monitor2.log])
> +AT_CAPTURE_FILE([expout2])
> +AT_CAPTURE_FILE([experr2])
> +for i in 1 2; do
> + AT_CHECK([ovs-ofctl -O OpenFlow14 monitor br0 --detach --no-chdir
> --pidfile=`pwd`/c$i.pid --unixctl=`pwd`/c$i])
> + ovs-appctl -t `pwd`/c$i ofctl/barrier
> + ovs-appctl -t `pwd`/c$i ofctl/set-output-file monitor$i.log
> + : > expout$i
> + : > experr$i
> +
> + # find out current role
> + ovs-appctl -t `pwd`/c$i ofctl/send
> 051800180000000200000000000000000000000000000000
> + echo >>experr$i "send: OFPT_ROLE_REQUEST (OF1.4): role=nochange"
> + echo >>expout$i "OFPT_ROLE_REPLY (OF1.4): role=equal"
> +done
>
> -AT_CHECK([grep -v '^send:' monitor.log], [0], [expout])
> -mv experr expout
> -AT_CHECK([grep '^send:' monitor.log], [0], [expout])
> +# controller 1: Become slave (generation_id is initially undefined, so
> +# 2^63+2 should not be stale)
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 051800180000000300000003000000008000000000000002
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.4): role=slave
> generation_id=9223372036854775810"
> +echo >>expout1 "OFPT_ROLE_REPLY (OF1.4): role=slave
> generation_id=9223372036854775810"
> +
> +# controller 2: Become master.
> +ovs-appctl -t `pwd`/c2 ofctl/send
> 051800180000000300000002000000008000000000000003
> +echo >>experr2 "send: OFPT_ROLE_REQUEST (OF1.4): role=master
> generation_id=9223372036854775811"
> +echo >>expout2 "OFPT_ROLE_REPLY (OF1.4): role=master
> generation_id=9223372036854775811"
> +
> +# controller 1: Try to become the master using a stale generation ID
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 051800180000000400000002000000000000000000000003
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.4): role=master
> generation_id=3"
> +echo >>expout1 "OFPT_ERROR (OF1.4): OFPRRFC_STALE"
> +echo >>expout1 "OFPT_ROLE_REQUEST (OF1.4): role=master generation_id=3"
> +
> +# controller 1: Become master using a valid generation ID
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 051800180000000500000002000000000000000000000001
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.4): role=master
> generation_id=1"
> +echo >>expout1 "OFPT_ROLE_REPLY (OF1.4): role=master generation_id=1"
> +echo >>expout2 "OFPT_ROLE_STATUS (OF1.4): role=slave generation_id=1
> reason=master_request"
> +
> +for i in 1 2; do
> + ovs-appctl -t `pwd`/c$i ofctl/barrier
> + echo >>expout$i "OFPT_BARRIER_REPLY (OF1.4):"
> +done
>
> -ovs-appctl -t ovs-ofctl exit
> +# Check output.
> +for i in 1 2; do
> + cp expout$i expout
> + AT_CHECK([grep -v '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
> + cp experr$i expout
> + AT_CHECK([grep '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
> +done
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
