Skip to content
New issue

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

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

Already on GitHub? # to your account

Improve Consumer interface to support ZMQ based Producer/Consumer table. #2562

Merged
merged 25 commits into from
May 12, 2023

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Dec 8, 2022

What I did
Improve Consumer interface to support ZMQ based Producer/Consumer table.

Why I did it
To improve route create performance, swsscommon lib will add ZMQ based Producer/Consumer table.
Because currently Consumer interface only support Redis based Producer/Consumer table, so improve this interface to support ZMQ based Producer/Consumer table.

ZMQ based Producer/Consumer table can find in this PR: sonic-net/sonic-swss-common#715

How I verified it
Pass all UT.

Details if related

@liuh-80
Copy link
Contributor Author

liuh-80 commented Dec 15, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jan 4, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jan 5, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80 liuh-80 requested a review from qiluo-msft January 5, 2023 05:26
@liuh-80 liuh-80 marked this pull request as ready for review January 5, 2023 05:26
@liuh-80
Copy link
Contributor Author

liuh-80 commented Jan 5, 2023

There are few UTs randomly failed, every time failed UTs are different.

@@ -45,12 +45,12 @@ IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c
{
auto subscriberStateTable = new swss::SubscriberStateTable(stateDb,
STATE_PORT_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, 100);
auto stateConsumer = new Consumer(subscriberStateTable, this, STATE_PORT_TABLE_NAME);
auto stateConsumer = new TableConsumer(subscriberStateTable, this, STATE_PORT_TABLE_NAME);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please clarify the following:

  1. Is this interface generic, that both zmq and redis are supported?
  2. If not, how do we ensure backward compatibility? For e.g, warmboot cases
  3. Can we reuse the existing infra and chose zmq for only certain cases (like dash)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Is this interface generic, that both zmq and redis are supported?
    Yes, this change is generic, TableConsumer is for redis, for ZMQ ther will be a ZmqConsumer add by another PR, here is POC pr show all change: https://github.com/sonic-net/sonic-swss/pull/2560/files

  2. If not, how do we ensure backward compatibility? For e.g, warmboot cases
    This PR only change interface, still using redis based consumer after this PR. will not break any backward compatibility.

  3. Can we reuse the existing infra and chose zmq for only certain cases (like dash)
    Yes, we still using existing infra in most case. the current plan is only use ZMQ for dash.
    This can be done by change code in Orch::addConsumer(), when add consumer for dash related tables, add a ZqmConsumer, for all other case still add TableConsumer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, if TableConsumer and Consumer are both for redis, then why change the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 'Consumer' now become a base class, and 'TableConsumer' is only for Redis based swss::SubscriberStateTable and swss::ConsumerStateTable, in another PR, I will add 'ZmqConsumer' class for swss::ZmqConsumerStateTable.

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jan 9, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80
Copy link
Contributor Author

liuh-80 commented Jan 13, 2023

UT failed because test case found 2 virtual route, still investigation why this happen, maybe a random issue:

2023-01-12T13:05:51.9237991Z _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
2023-01-12T13:05:51.9238550Z test_sub_port_intf.py:86: in connect_dbs
2023-01-12T13:05:51.9239103Z self.default_vrf_oid = self.get_default_vrf_oid()
2023-01-12T13:05:51.9239663Z _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
2023-01-12T13:05:51.9240001Z
2023-01-12T13:05:51.9240478Z self = <test_sub_port_intf.TestSubPortIntf object at 0x7fc88f4879a0>
2023-01-12T13:05:51.9240834Z
2023-01-12T13:05:51.9241267Z def get_default_vrf_oid(self):
2023-01-12T13:05:51.9241777Z oids = self.get_oids(ASIC_VIRTUAL_ROUTER_TABLE)
2023-01-12T13:05:51.9242388Z > assert len(oids) == 1, "Wrong # of default vrfs: %d, expected #: 1." % (len(oids))
2023-01-12T13:05:51.9243217Z E AssertionError: Wrong # of default vrfs: 2, expected #: 1.
2023-01-12T13:05:51.9243707Z E assert 2 == 1
2023-01-12T13:05:51.9244678Z E -2
2023-01-12T13:05:51.9245068Z E +1

@liuh-80
Copy link
Contributor Author

liuh-80 commented Mar 2, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80 liuh-80 force-pushed the dev/liuh/improve_comsumer_interface branch from 352696f to a77db41 Compare April 17, 2023 07:25
@liuh-80
Copy link
Contributor Author

liuh-80 commented Apr 18, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80
Copy link
Contributor Author

liuh-80 commented Apr 19, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80
Copy link
Contributor Author

liuh-80 commented Apr 25, 2023

Created Dash side PR for review: https://github.com/nvidia-sonic/sonic-swss/pull/1

@liuh-80
Copy link
Contributor Author

liuh-80 commented Apr 25, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80 liuh-80 force-pushed the dev/liuh/improve_comsumer_interface branch from 12b6892 to 47e5a1b Compare April 26, 2023 02:21
@liuh-80
Copy link
Contributor Author

liuh-80 commented Apr 29, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80 liuh-80 force-pushed the dev/liuh/improve_comsumer_interface branch from dca13df to c7e4cee Compare April 29, 2023 02:02
@liuh-80 liuh-80 merged commit a215441 into sonic-net:master May 12, 2023
liuh-80 added a commit to liuh-80/sonic-swss that referenced this pull request May 17, 2023
…le. (sonic-net#2562)

**What I did**
Improve Consumer interface to support ZMQ based Producer/Consumer table.

**Why I did it**
To improve route create performance, swsscommon lib will add ZMQ based Producer/Consumer table.
Because currently Consumer interface only support Redis based Producer/Consumer table, so improve this interface to support ZMQ based Producer/Consumer table.

ZMQ based Producer/Consumer table can find in this PR: sonic-net/sonic-swss-common#715

**How I verified it**
Pass all UT.

**Details if related**
liuh-80 added a commit that referenced this pull request Jun 8, 2023
…le. (#2562) (#2778)

This is a cherry-pick PR for #2562

**What I did**
Improve Consumer interface to support ZMQ based Producer/Consumer table.

**Why I did it**
To improve route create performance, swsscommon lib will add ZMQ based Producer/Consumer table. Because currently Consumer interface only support Redis based Producer/Consumer table, so improve this interface to support ZMQ based Producer/Consumer table.

ZMQ based Producer/Consumer table can find in this PR: sonic-net/sonic-swss-common#715

**How I verified it**
Pass all UT.

**Details if related**

<!--
Please make sure you have read and understood the contribution guildlines:
https://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md

1. Make sure your commit includes a signature generted with `git commit -s`
2. Make sure your commit title follows the correct format: [component]: description
3. Make sure your commit message contains enough details about the change and related tests
4. Make sure your pull request adds related reviewers, asignees, labels

Please also provide the following information in this pull request:
-->

**What I did**

**Why I did it**

**How I verified it**

**Details if related**
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants