Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sonic-cli] Fix sonic-db-cli output format not backward compatible with python version issue. #631

Merged
merged 10 commits into from
Jun 23, 2022

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Jun 14, 2022

Why I did it

Fix sonic-db-cli output format not backward compatible with python version issue. Which break some E2E test.

How I did it

Re-write redis reply format method, keep same format with redis-py.

How to verify it

Add c++ unit test to cover all code.
Pass all E2E test scenario.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

Re-write sonic-cli with c++ for sonic startup performance issue

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

common/replyformatter.cpp Outdated Show resolved Hide resolved
common/replyformatter.cpp Outdated Show resolved Hide resolved
@liuh-80
Copy link
Contributor Author

liuh-80 commented Jun 15, 2022

Some UT failed, but seems not related with code change because also failed in another PR which only change code coverafe rate:

test_MirrorSessionAddModifyAndDelete failed (1 runs remaining out of 2).
<class 'ValueError'>
filedescriptor out of range in select()
[<TracebackEntry /agent/_work/1/s/sonic-swss/tests/conftest.py:1794>, <TracebackEntry /agent/_work/1/s/sonic-swss/tests/conftest.py:1775>, <TracebackEntry /agent/_work/1/s/sonic-swss/tests/conftest.py:591>, <TracebackEntry /agent/_work/1/s/sonic-swss/tests/conftest.py:451>, <TracebackEntry /usr/local/lib/python3.8/dist-packages/docker/models/containers.py:198>, <TracebackEntry /usr/local/lib/python3.8/dist-packages/docker/utils/decorators.py:19>, <TracebackEntry /usr/local/lib/python3.8/dist-packages/docker/api/exec_api.py:169>, <TracebackEntry /usr/local/lib/python3.8/dist-packages/docker/api/client.py:422>, <TracebackEntry /usr/local/lib/python3.8/dist-packages/docker/utils/socket.py:137>, <TracebackEntry /usr/local/lib/python3.8/dist-packages/docker/api/client.py:416>, <TracebackEntry /usr/local/lib/python3.8/dist-packages/docker/utils/socket.py:94>, <TracebackEntry /usr/local/lib/python3.8/dist-packages/docker/utils/socket.py:66>, <TracebackEntry /usr/local/lib/python3.8/dist-packages/docker/utils/socket.py:51>, <TracebackEntry /usr/local/lib/python3.8/dist-packages/docker/utils/socket.py:31>]

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jun 15, 2022

Still need this test PR to verify the cli change not break any E2E test case: sonic-net/sonic-buildimage#11130

@liuh-80 liuh-80 requested a review from qiluo-msft June 15, 2022 09:02
@liuh-80
Copy link
Contributor Author

liuh-80 commented Jun 15, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80 liuh-80 marked this pull request as ready for review June 15, 2022 09:32
common/redisreply.cpp Outdated Show resolved Hide resolved
common/redisreply.cpp Outdated Show resolved Hide resolved
common/redisreply.cpp Outdated Show resolved Hide resolved
common/replyformatter.h Outdated Show resolved Hide resolved

result << ")";

return result.str();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result

would you like to use boost::algorithm::join?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found some discussion about boost join and stringstream, seems stringstream has better performance: https://stackoverflow.com/questions/23551653/boostjoin-and-boosttransformed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know. Actually I found some functions in common/stringutility.h. Could you try reuse or add common functions there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, add new method to stringutility.h and improve code to use it.

@@ -165,7 +168,8 @@ int executeCommands(
based on our usage in SONiC, None and list type output from python API needs to be modified
with these changes, it is enough for us to mimic redis-cli in SONiC so far since no application uses tty mode redis-cli output
*/
cout << reply.to_string() << endl;
auto commandName = getCommandName(commands);
cout << RedisReply::to_string(reply.getContext(), commandName) << endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_string

I see the code improved much. To make it even better, is it possible to move getCommandName() logic into RedisCommand? It's very easy to get command name in its ctor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not easy because RedisCommand can handle a format string as parameter, for example: "hgetall %s %d".
So, if we move this method to RedisCommand the logic will be much complex, because we need parse user input as before.

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jun 17, 2022

Following test randomly failed, not related with code change in this PR, seems hardware timer issue:

tests: tests.cpp:843: void test_watchdog_timer_clock_rollback(): Assertion `settimeofday(&currentTime, NULL) == 0' failed.
/bin/bash: line 5: 13004 Aborted (core dumped) ${dir}$tst

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jun 17, 2022

@kcudnik, could you please check if my fix for your review comments is OK?

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jun 20, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jun 21, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jun 21, 2022

Need wait for this PR merge first, which fix the merge validation failed issue:
#633

@qiluo-msft
Copy link
Contributor

Could you merge latest master?

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jun 23, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80 liuh-80 merged commit ecf5bbc into sonic-net:master Jun 23, 2022
liuh-80 added a commit to liuh-80/sonic-swss-common that referenced this pull request Jun 23, 2022
…th python version issue. (sonic-net#631)

#### Why I did it
Fix sonic-db-cli output format not backward compatible with python version issue. Which break some E2E test.

#### How I did it
Re-write redis reply format method, keep same format with redis-py.

#### How to verify it
Add c++ unit test to cover all code.
Pass all E2E test scenario.

#### Which release branch to backport (provide reason below if selected)

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111

#### Description for the changelog
Re-write sonic-cli with c++ for sonic startup performance issue

#### Link to config_db schema for YANG module changes
<!--
Provide a link to config_db schema for the table for which YANG model
is defined
Link should point to correct section on https://github.com/Azure/SONiC/wiki/Configuration.
-->

#### A picture of a cute animal (not mandatory but encouraged)
liuh-80 added a commit that referenced this pull request Jun 27, 2022
…th python version issue. (#631) (#637)

#### Why I did it
Fix sonic-db-cli output format not backward compatible with python version issue. Which break some E2E test.

#### How I did it
Re-write redis reply format method, keep same format with redis-py.

#### How to verify it
Add c++ unit test to cover all code.
Pass all E2E test scenario.

#### Which release branch to backport (provide reason below if selected)

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111

#### Description for the changelog
Re-write sonic-cli with c++ for sonic startup performance issue

#### Link to config_db schema for YANG module changes
<!--
Provide a link to config_db schema for the table for which YANG model
is defined
Link should point to correct section on https://github.com/Azure/SONiC/wiki/Configuration.
-->

#### A picture of a cute animal (not mandatory but encouraged)
itamar-talmon pushed a commit to itamar-talmon/sonic-swss-common that referenced this pull request Jul 19, 2022
…th python version issue. (sonic-net#631)

#### Why I did it
Fix sonic-db-cli output format not backward compatible with python version issue. Which break some E2E test.

#### How I did it
Re-write redis reply format method, keep same format with redis-py.

#### How to verify it
Add c++ unit test to cover all code.
Pass all E2E test scenario.

#### Which release branch to backport (provide reason below if selected)

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111

#### Description for the changelog
Re-write sonic-cli with c++ for sonic startup performance issue

#### Link to config_db schema for YANG module changes
<!--
Provide a link to config_db schema for the table for which YANG model
is defined
Link should point to correct section on https://github.com/Azure/SONiC/wiki/Configuration.
-->

#### A picture of a cute animal (not mandatory but encouraged)
yxieca pushed a commit that referenced this pull request Aug 8, 2022
…th python version issue. (#631)

#### Why I did it
Fix sonic-db-cli output format not backward compatible with python version issue. Which break some E2E test.

#### How I did it
Re-write redis reply format method, keep same format with redis-py.

#### How to verify it
Add c++ unit test to cover all code.
Pass all E2E test scenario.

#### Which release branch to backport (provide reason below if selected)

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111

#### Description for the changelog
Re-write sonic-cli with c++ for sonic startup performance issue

#### Link to config_db schema for YANG module changes
<!--
Provide a link to config_db schema for the table for which YANG model
is defined
Link should point to correct section on https://github.com/Azure/SONiC/wiki/Configuration.
-->

#### A picture of a cute animal (not mandatory but encouraged)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants