Do not always crash syncd with event process error, provide SAI API f… by jipanyang · Pull Request #315 · sonic-net/sonic-sairedis

jipanyang · 2018-04-26T01:45:15Z

…eedback for create/set/remove operations

Signed-off-by: Jipan Yang jipan.yang@alibaba-inc.com

The change is to provide feedback path to orchagent for libsai redis API create/set/remove operations.

SAI_OBJECT_TYPE_ROUTE_ENTRY has been excluded from the feedback set, one of the reason is the impact on operation latency when there is large amount of route entries.

NotificationProducer/NotificationConsumer channel has been used for feedback, which has lower overhead than Producer/Consumer channel ( about 50%).

create example:

set example:

2018-04-25.23:08:40.892350|s|SAI_OBJECT_TYPE_HOSTIF:oid:0xd000000000977|SAI_HOSTIF_ATTR_VLAN_TAG=SAI_HOSTIF_VLAN_TAG_STRIP
2018-04-25.23:08:40.893075|S|SAI_STATUS_SUCCESS|SAI_OBJECT_TYPE_HOSTIF:oid:0xd000000000977

remove example:

2018-04-26.00:07:40.480202|r|SAI_OBJECT_TYPE_VLAN_MEMBER:oid:0x270000000000c5
2018-04-26.00:07:40.481170|R|SAI_STATUS_SUCCESS|SAI_OBJECT_TYPE_VLAN_MEMBER:oid:0x270000000000c5

No change to SAI_OBJECT_TYPE_ROUTE_ENTRY:

2018-04-26.00:15:25.165330|c|SAI_OBJECT_TYPE_ROUTE_ENTRY:{"dest":"10.2.42.100/32","switch_id":"oid:0x21000000000000","vr":"oid:0x300000000003a"}|SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID=oid:0x
2018-04-26.00:15:25.166478|c|SAI_OBJECT_TYPE_ROUTE_ENTRY:{"dest":"10.2.55.20/32","switch_id":"oid:0x21000000000000","vr":"oid:0x300000000003a"}|SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID=oid:0x4
2018-04-26.00:15:25.167817|c|SAI_OBJECT_TYPE_ROUTE_ENTRY:{"dest":"10.1.201.217/32","switch_id":"oid:0x21000000000000","vr":"oid:0x300000000003a"}|SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID=oid:0
2018-04-26.00:15:25.172021|c|SAI_OBJECT_TYPE_ROUTE_ENTRY:{"dest":"10.1.175.25/32","switch_id":"oid:0x21000000000000","vr":"oid:0x300000000003a"}|SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID=oid:0x
2018-04-26.00:15:25.173406|c|SAI_OBJECT_TYPE_ROUTE_ENTRY:{"dest":"10.2.20.169/32","switch_id":"oid:0x21000000000000","vr":"oid:0x300000000003a"}|SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID=oid:0x

…eedback for create/set/remove operations Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

lguohan · 2018-04-26T05:35:53Z

we need to understand the impact of fast reboot because the change mainly impact the orchagent init, so do you have time on how much longer this impact the fast reboot test?

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

pavel-shirshov

As comments.

P.S.: I feel this feature will slow orchagent down. Have you measured SONiC start time with and without this feature?

pavel-shirshov · 2018-04-26T22:11:47Z

lib/inc/sai_redis.h

+// there is something wrong and we should fail
+#define CSR_RESPONSE_TIMEOUT (5*60*1000)
+
+extern sai_status_t redis_get_response(std::string &key, std::string op, sai_object_type_t object_type);


Can you please use const and references for the string parameters?
const std::string &key,
const std::string &op

pavel-shirshov · 2018-04-26T22:13:01Z

lib/inc/sairedis.h

        _In_ sai_bulk_op_error_mode_t mode,
        _Out_ sai_status_t *object_statuses);
+
+#define sai_return_obj_op_status(object_type)  ({   \


I think it's better to make a function.
You could make the function inline.

sai_return_obj_op_status() is to be used by both syncd and orchagent. sairedis.h is the common file used by both of them.

If it is function (inline or not), since sairedis.h will be included in some cpp files which don't have call to sai_return_obj_op_status(), there will be "function defined but not used" compilation error with current build setting.

You shouldn't declare the inline as static which I assume is what you did that triggered "function defined but not used".

pavel-shirshov · 2018-04-26T22:13:31Z

lib/src/sai_redis_generic_create.cpp

+// wait for response
+sai_status_t redis_get_response(
+    _In_ std::string &key,
+    _In_ std::string op,


const and references as above

pavel-shirshov · 2018-04-26T22:15:52Z

syncd/syncd.cpp

+
+    std::string str_status = sai_serialize_status(status);
+
+    SWSS_LOG_INFO("sending response for %s on %s with status: %s", op.c_str(), key.c_str(), str_status.c_str());


probably DEBUG would be better here?

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang · 2018-04-26T23:26:59Z

Will collect SONiC start time with and without this feature and post them here.

jipanyang · 2018-04-27T02:58:12Z

The exact time taken might be affected by many factors. I did systemctl restart swss with the same configuration and check sairedis.rec for certain mile stones.

Without feedback
from INIT_VIEW to last SAI_OBJECT_TYPE_ROUTER_INTERFACE (10 in total) created.
02:01:32.042732 ---- 02:01:46.824758 Time taken: 14.782 seconds.

With feedback
from INIT_VIEW to last SAI_OBJECT_TYPE_ROUTER_INTERFACE created.
02:13:06.480494 ---- 02:13:21.561555 Time taken: 15.081 seconds

There were 167 |C| create feedback, 258 |S| set feedback and 123 |R| remove feedback.

without_feedback.txt
with_feedback.txt

pavel-shirshov

I built the sairedis with this PR and found that SONiC didn't work as expected. orchagent fails on the first API call ->create_switch(). The call doesn't see any data from syncd for timeout time.

pavel-shirshov · 2018-05-02T23:14:31Z

lib/src/sai_redis_generic_create.cpp

+    swss::Select s;
+
+    sai_status_t status;
+    std::string request;


How request is used here? I see it's used for output only. I don't see any assignment here.

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

…e wait time to 30 seconds Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang · 2018-05-08T17:42:17Z

Link to the issue: #239

lguohan · 2019-07-16T14:52:09Z

@jipanyang , can we close this PR when #476 is merged?

jipanyang · 2019-07-16T16:38:12Z

Closing in favor of #476

jipanyang added 2 commits April 25, 2018 18:12

Do not always crash syncd with event process error, provide SAI API f…

2303654

…eedback for create/set/remove operations Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

Remove deprecated parameter from select()

9da645a

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

lguohan requested a review from pavel-shirshov April 26, 2018 05:36

Fix vs test error

c3c9658

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

pavel-shirshov suggested changes Apr 26, 2018

View reviewed changes

jipanyang added 3 commits April 26, 2018 15:18

The repose timeout should be 5 seconds instead of 5 minutes

1b88ba8

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

Add SWSS_LOG_ENTER() for the new function

f09f9dd

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

Address review comments for const reference and log level

58f14e3

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

pavel-shirshov approved these changes Apr 27, 2018

View reviewed changes

pavel-shirshov suggested changes May 2, 2018

View reviewed changes

jipanyang added 2 commits May 2, 2018 16:44

Remove unused local variable

0764065

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

libSAI API call switch create often takes up to 10 seconds, adjust th…

2bda66b

…e wait time to 30 seconds Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang closed this Jul 16, 2019


		std::string str_status = sai_serialize_status(status);

		SWSS_LOG_INFO("sending response for %s on %s with status: %s", op.c_str(), key.c_str(), str_status.c_str());

Conversation

jipanyang commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lguohan commented Apr 26, 2018

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

jipanyang Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

nikos-github Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

jipanyang commented Apr 26, 2018

Uh oh!

jipanyang commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov May 2, 2018

Choose a reason for hiding this comment

Uh oh!

jipanyang commented May 8, 2018

Uh oh!

lguohan commented Jul 16, 2019

Uh oh!

jipanyang commented Jul 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jipanyang commented Apr 26, 2018 •

edited

Loading

jipanyang commented Apr 27, 2018 •

edited

Loading