[Generic-P4Orch] Add ordered queue support to Orch class. - #4308
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| avail_counter = getCrmCounterValue(dvs, "EXT_TABLE_STATS:"+self._p4rt_viplb_obj.TBL_NAME, 'crm_stats_extension_table_available') | ||
| used_counter = getCrmCounterValue( | ||
| dvs, "EXT_TABLE_STATS:"+self._p4rt_viplb_obj.TBL_NAME, 'crm_stats_extension_table_used') | ||
| avail_counter = getCrmCounterValue( |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning test
| avail_counter = getCrmCounterValue(dvs, "EXT_TABLE_STATS:"+self._p4rt_viplb_obj.TBL_NAME, 'crm_stats_extension_table_available') | ||
| used_counter = getCrmCounterValue( | ||
| dvs, "EXT_TABLE_STATS:"+self._p4rt_viplb_obj.TBL_NAME, 'crm_stats_extension_table_used') | ||
| avail_counter = getCrmCounterValue( |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning test
| avail_counter = getCrmCounterValue(dvs, "EXT_TABLE_STATS:"+self._p4rt_viplb_obj.TBL_NAME, 'crm_stats_extension_table_available') | ||
| used_counter = getCrmCounterValue( | ||
| dvs, "EXT_TABLE_STATS:"+self._p4rt_viplb_obj.TBL_NAME, 'crm_stats_extension_table_used') | ||
| avail_counter = getCrmCounterValue( |
Check notice
Code scanning / CodeQL
Unused local variable Note test
divyagayathri-hcl
changed the title
|
@divyagayathri-hcl , if there are common/generic changes and p4Orch changes, then could you tag as [Generic-P4Orch]. @mint570 , for viz and merge to be done after signoff. |
Community can review just the common part and signoff |
There was a problem hiding this comment.
Pull request overview
This PR adds ordered queue support to the Orch class, specifically to support the P4Orch use case. The key change is transitioning P4Orch from a legacy Redis notification-based communication path (using NotificationConsumer/NotificationProducer) to a ZMQ-based path with an ordered std::deque (m_toSyncQueue) that preserves P4RT request ordering. This is important because P4 table entries have dependencies (e.g., router interface → neighbor → nexthop → route) that must be processed in order.
Changes:
- Added
m_toSyncQueue(astd::deque) andm_orderedQueueflag toConsumerBase, withaddToSyncrouting to the queue when ordered mode is enabled. - Refactored
P4Orchto inherit fromZmqOrchinstead ofOrch, replacing the legacyhandleP4rtNotificationnotification path with direct ZMQ-based ordered queue processing indoTask(ConsumerBase&). - Updated
ResponsePublisherto support ZMQ response delivery (caching responses and sending them inflush()), and updated test infrastructure accordingly.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
orchagent/orch.h / orchagent/orch.cpp |
Added m_toSyncQueue, m_orderedQueue, setOrderedQueue(), setOrderedQueueForAllConsumers() to ConsumerBase/Orch |
orchagent/p4orch/p4orch.h / p4orch.cpp |
P4Orch now inherits ZmqOrch, removes legacy notification path, adds new ordered queue processing in doTask |
orchagent/zmqorch.h / zmqorch.cpp |
Added dbPersistence parameter; drain now checks both m_toSync and m_toSyncQueue |
orchagent/response_publisher.h / .cpp |
Added ZMQ server support, ZMQ response caching, flush(warmboot) parameter |
orchagent/directory.h |
Replaced deprecated std::iterator with explicit iterator traits |
orchagent/orchdaemon.cpp |
Creates a dedicated p4OrchZmqServer for P4Orch |
tests/p4rt/util.py |
Migrates DBInterface from ProducerStateTable to ZmqProducerStateTable; adds verify_response method |
tests/p4rt/acl.py |
Moves APP_DB_TBL_NAME from per-class to module-level (incorrectly — see bug) |
tests/p4rt/test_*.py |
Update tests to use the new per-object verify_response() ZMQ-based API |
tests/mock_tests/*.cpp |
Update mock tests to new P4Orch constructor and ZmqConsumer-based test patterns |
lib/orch_zmq_config.cpp |
Passes oneToOneSync=false to general ZmqServer creation |
tests/conftest.py |
Adds ZMQ socket mount for p4orch ZMQ endpoint |
| if (manager == nullptr) { | ||
| status = ReturnCode(StatusCode::SWSS_RC_INVALID_PARAM) | ||
| << "Failed to find P4Orch Manager for key " << kfvKey(kco); | ||
| SWSS_LOG_ERROR("%s", status.message().c_str()); | ||
| m_publisher.publish(APP_P4RT_TABLE_NAME, kfvKey(kco), | ||
| kfvFieldsValues(kco), status, /*replace=*/true); | ||
| continue; |
There was a problem hiding this comment.
In doTask(ConsumerBase&), the status variable is set to an error at line 155 when manager == nullptr is detected. Because continue is used at line 160 rather than resetting status at the start of each iteration, the error persists. This means any subsequent entry with a valid manager will hit the !status.ok() check at line 162 and be silently published as SWSS_RC_NOT_EXECUTED. An invalid/unrecognized key in the middle of a batch will incorrectly suppress all subsequent valid entries with a misleading error code. If fail-fast is the intended behavior, the invalid-key case should not use status as the carrier for failure — it should be separate from the drain-failure status. To fix this, status should only be set by manager drain failures, not by manager-not-found events (which have their own independent error path). Consider resetting status to ReturnCode() (OK) after the not-found error is published, or separating the "invalid key" error path from the "batch failure" status variable.
There was a problem hiding this comment.
@prsunny Please confirm if we need these code level changes since it was already built and tested locally.
| if (table_name.rfind(p4orch::kTablePrefixEXT, 0) != std::string::npos) { | ||
| return m_p4TableToManagerMap[APP_P4RT_EXT_TABLES_MANAGER]; |
There was a problem hiding this comment.
The findManager function at line 227 uses table_name.rfind(p4orch::kTablePrefixEXT, 0) != std::string::npos to check if the table name starts with "EXT_". However, std::string::rfind(substr, pos) searches backward from pos, so passing 0 as the position only finds the substring if it starts at position 0 — this is equivalent to a prefix check. The condition is correct but it incorrectly returns the EXT manager even when table_name is empty: "".rfind("EXT_", 0) returns npos, which is correct. The actual concern is a subtle correctness issue: rfind semantically means "reverse find" which can be misleading. Consider using table_name.find(p4orch::kTablePrefixEXT) == 0 or table_name.rfind(p4orch::kTablePrefixEXT, 0) == 0 instead to make the prefix-check intent explicit. Note the current code uses != std::string::npos which happens to be equivalent but harder to understand.
There was a problem hiding this comment.
@prsunny Please confirm if we need these code level changes since it was already built and tested locally.
3cfa3f3 to
6995a51
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
6995a51 to
e903943
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
divyagayathri-hcl
changed the title
e903943 to
41dbe71
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
b473fb3 to
5c86460
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
5c86460 to
4e37819
Compare
|
/azp run |
2b82026 to
6a5c2c9
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
6a5c2c9 to
a5bc2c1
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@prabhataravind Can you help to close on this? |
|
@prabhataravind Can you please help with the review? |
|
@prabhataravind, Can you please help with the review? |
|
@prabhataravind This has been pending review for 2 weeks now, can you take a look? |
|
@prabhataravind and @prsunny, Could you please help with the review? |
|
looks like there are conflicts and coverage issue. |
a5bc2c1 to
f31907e
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: mint570 <runmingwu@google.com> Signed-off-by: divyagayathri-hcl <divyagayathri.s@hcl.com>
f31907e to
cc95dcc
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
cc95dcc to
f00b8dd
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…eToManagerMap in P4Orch::bake(). Signed-off-by: divyagayathri-hcl <divyagayathri.s@hcl.com>
f00b8dd to
66a63ae
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@prsunny, Added new test cases to increase the Coverage and conflicts are also resolved now. |
What I did
Added ordered queue support to the Orch class by introducing a std::deque to the ConsumerBase and refactoring P4Orch to use this mechanism for preserving task sequence.
Why I did it
This ensures that dependent P4 objects (like Router Interfaces and Neighbors) are processed in the correct order, which was not guaranteed by the default merging logic.
How I verified it
Verified the implementation using new and updated test cases in p4orch_test.cpp and generic P4Orch regression tests.
Details if related
The change includes logic to temporarily bypass the ordered queue during warm boot (bake()) to allow for proper state reconciliation.