P4Runtime : Status Code additions #545

donNewtonAlpha · 2021-10-22T15:01:09Z

This PR adds batch operation functions and additional Status Codes necessary for the updated API coming in the follow on PR.

bocon13 · 2021-10-22T16:34:05Z

cc: @mint570 @ravi861 @prsunny

mint570 · 2021-10-22T22:10:54Z

common/producerstatetable.h

+    // In set(), the op is ignored.
+    void set(const std::vector<KeyOpFieldsValuesTuple>& values);
+
+    void del(const std::vector<std::string>& keys);


Can we add unit test on these two new APIs?
We can add the test in redis_state_ut.cpp or redis_piped_state_ut.cpp.
(I can help in this if needed.)

@donNewtonAlpha
We have provided the unit test for these. Please integrate them into this pr when you have time. Thanks.

these APIs are removed

prsunny · 2021-10-23T00:42:54Z

Please give a link to HLD for this change.

bocon13 · 2021-10-25T18:41:22Z

Please give a link to HLD for this change.

Batch updates are introduced and required by the P4Orch:
https://github.com/pins/SONiC/blob/master/doc/pins/p4orch_hld.md#p4orch-managers

bocon13 · 2021-10-27T19:19:55Z

@qiluo-msft what do you think about this new API?

qiluo-msft · 2021-11-08T17:14:06Z

Please change the PR title to more descriptive. People in the swss-common context do not know PINS, Pr2, batch ops.

In reply to: 963379161

* Add batch set/delete() to ProducerStateTable * Add StatusCode enum and functions to convert between string and enum values. * Allow exists() to check for whitespace. This is only to allow whitespace when we check for existence. We can already create entries with whitespace. * Add SWSS return code SWSS_RC_UNIMPLEMENTED * Fix json error, refer to nlohmann/json#590 Submission containing materials of a third party: Copyright Google LLC; Licensed under Apache 2.0 Co-authored-by: Akarsh Gupta <akarshgupta@google.com> Co-authored-by: Jay Hu <jiarenhu@google.com> Co-authored-by: Manali Kumar <manalikumar@google.com> Co-authored-by: Robert J. Halstead <rhalstea@google.com> Co-authored-by: Runming Wu <runmingwu@google.com> Co-authored-by: Yilan Ji <yilanji@google.com> Signed-off-by: Don Newton don@opennetworking.org

signed-of-by:Don Newton<don@opennetworking.org>

common/producerstatetable.cpp

common/producerstatetable.h

bocon13 · 2021-11-09T18:22:48Z

common/json.hpp

@@ -5394,7 +5394,7 @@ class basic_json
                {
                    assert(lhs.m_value.array != nullptr);
                    assert(rhs.m_value.array != nullptr);
-                    return *lhs.m_value.array < *rhs.m_value.array;
+                    return (*lhs.m_value.array) < *rhs.m_value.array;


This is a fix from upstream: nlohmann/json#590

bocon13 · 2021-11-09T18:23:12Z

common/producerstatetable.h

@@ -1,6 +1,7 @@
 #pragma once

 #include <memory>
+#include <vector>


This is missing import. std::vector is used below.

bocon13 · 2021-11-09T18:26:53Z

common/dbconnector.cpp

-    if (key.find_first_of(" \t") != string::npos)
-    {
-        SWSS_LOG_ERROR("EXISTS failed, invalid space or tab in single key: %s", key.c_str());
-        throw runtime_error("EXISTS failed, invalid space or tab in single key");
-    }


@mint570 @rhalstea I don't think we have any whitespace in our keys. Can/should we revert this change? #Closed

bocon13 · 2021-11-09T18:32:34Z

@qiluo-msft this PR is cleaned up now. Please take a look

qiluo-msft · 2021-11-09T19:46:29Z

common/status_code_util.h

+namespace swss {
+
+enum class StatusCode {
+  SWSS_RC_SUCCESS,


SWSS_RC_SUCCESS

Which repoes will use the constants in this file? if only swss, we can move this file to swss repo. #Closed

Also p4rt (pins-infra repo)

qiluo-msft · 2021-11-09T19:47:58Z

common/status_code_util.h

+  SWSS_RC_UNKNOWN,
+};
+
+static std::map<StatusCode, std::string> statusCodeMapping = {


Also add const ? #Closed

qiluo-msft · 2021-11-09T19:48:25Z

common/status_code_util.h

+    {"SWSS_RC_UNKNOWN", StatusCode::SWSS_RC_UNKNOWN},
+};
+
+inline std::string statusCodeToStr(const StatusCode& status) {


inline

also add static #Closed

qiluo-msft · 2021-11-09T19:49:11Z

common/status_code_util.h

+
+enum class StatusCode {
+  SWSS_RC_SUCCESS,
+  SWSS_RC_INVALID_PARAM,


The repo is using 4-space indentation. Please follow with similar code. #Closed

donNewtonAlpha · 2021-11-16T16:47:52Z

@qiluo-msft @prsunny @bocon13
Hi Prince, Qi,
All the tests have passed on this PR, can we get this merged?
Don

qiluo-msft · 2021-11-17T20:11:36Z

Please resolve the conflicts. Possibly by rebase your PR to latest master branch.

donNewtonAlpha · 2021-11-17T20:24:18Z

@qiluo-msft This now shows able to merge the tests are re-running.

qiluo-msft · 2021-11-17T21:29:00Z

Please check the build failure

saiaclschema_ut.cpp:1:10: fatal error: gmock/gmock.h: No such file or directory
 #include <gmock/gmock.h>
          ^~~~~~~~~~~~~~~

donNewtonAlpha · 2021-11-17T21:36:56Z

I was just looking at that but it doesn't make sense to me. In azure-pipelines.yml we definitely add that. Is there something else we need to do? sudo apt-get install -y python3-pip sudo pip3 install pytest sudo apt-get install -y python sudo apt-get install cmake libgtest-dev sudo apt-get install cmake libgtest-dev libgmock-dev cd /usr/src/gtest && sudo cmake . && sudo make displayName: "Install dependencies" - script: |

…

On Wed, Nov 17, 2021 at 4:29 PM Qi Luo ***@***.***> wrote: Please check the build failure saiaclschema_ut.cpp:1:10: fatal error: gmock/gmock.h: No such file or directory #include <gmock/gmock.h> ^~~~~~~~~~~~~~~ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#545 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJ4SJM7QHIVPIDBOIOB4DTUMQNCNANCNFSM5GQYHZUA> .

StephenWangGoogle · 2021-11-17T21:51:23Z

I was just looking at that but it doesn't make sense to me. In azure-pipelines.yml we definitely add that. Is there something else we need to do? sudo apt-get install -y python3-pip sudo pip3 install pytest sudo apt-get install -y python sudo apt-get install cmake libgtest-dev sudo apt-get install cmake libgtest-dev libgmock-dev cd /usr/src/gtest && sudo cmake . && sudo make displayName: "Install dependencies" - script: |
…
On Wed, Nov 17, 2021 at 4:29 PM Qi Luo @.***> wrote: Please check the build failure saiaclschema_ut.cpp:1:10: fatal error: gmock/gmock.h: No such file or directory #include <gmock/gmock.h> ^~~~~~~~~~~~~~~ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#545 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJ4SJM7QHIVPIDBOIOB4DTUMQNCNANCNFSM5GQYHZUA .

Hmm, this file is not added by this PR. Is the tip of the tree broken?

donNewtonAlpha · 2021-11-17T22:09:19Z

556 was merged first. The tests etc including the gmock dependency were the rebase I had to do. On Wed, Nov 17, 2021 at 4:51 PM StephenWangGoogle ***@***.***> wrote:

…

I was just looking at that but it doesn't make sense to me. In azure-pipelines.yml we definitely add that. Is there something else we need to do? sudo apt-get install -y python3-pip sudo pip3 install pytest sudo apt-get install -y python sudo apt-get install cmake libgtest-dev sudo apt-get install cmake libgtest-dev libgmock-dev cd /usr/src/gtest && sudo cmake . && sudo make displayName: "Install dependencies" - script: | … <#m_-3355289809722603875_> On Wed, Nov 17, 2021 at 4:29 PM Qi Luo *@*.***> wrote: Please check the build failure saiaclschema_ut.cpp:1:10: fatal error: gmock/gmock.h: No such file or directory #include <gmock/gmock.h> ^~~~~~~~~~~~~~~ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#545 (comment) <#545 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJ4SJM7QHIVPIDBOIOB4DTUMQNCNANCNFSM5GQYHZUA . Hmm, this file is not added by this PR. Is the tip of the tree broken? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#545 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJ4SJKRYAEB2PNUGDH7B43UMQPWNANCNFSM5GQYHZUA> .

bocon13 · 2021-11-17T22:26:23Z

Hmm, this file is not added by this PR. Is the tip of the tree broken?

Yes, #556 seems to have broken master. Somehow gmock is missing from the buildimage container.

Update: master and 202012 share a tag, so our builder container reverted and we lost the dependency.
https://github.com/Azure/sonic-buildimage/blob/5a56659b61ccbc7eec4e18bdadc0a151b33d4b28/.azure-pipelines/docker-sonic-slave-template.yml#L91

donNewtonAlpha · 2021-11-17T22:54:06Z

Probably what needs to be done is the sonic-buildimage commit that adds in gmock should be backported to all active branches Don

…

On Wed, Nov 17, 2021 at 5:26 PM bocon13 ***@***.***> wrote: Hmm, this file is not added by this PR. Is the tip of the tree broken? Yes, #556 <#556> seems to have broken master. Somehow gmock is missing from the buildimage container. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#545 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJ4SJO7X6XJINOXWOXR26TUMQTZVANCNFSM5GQYHZUA> .

donNewtonAlpha · 2021-11-17T23:18:47Z

/azpw run

mssonicbld · 2021-11-17T23:18:49Z

/AzurePipelines run

azure-pipelines · 2021-11-17T23:19:00Z

Azure Pipelines successfully started running 1 pipeline(s).

donNewtonAlpha · 2021-11-18T00:28:06Z

/azpw run

mssonicbld · 2021-11-18T00:28:08Z

/AzurePipelines run

azure-pipelines · 2021-11-18T00:28:19Z

Azure Pipelines successfully started running 1 pipeline(s).

bocon13 · 2021-11-18T04:21:16Z

Probably what needs to be done is the sonic-buildimage commit that adds in gmock should be backported to all active branches Don

Asked for the backport here:
sonic-net/sonic-buildimage#8950 (comment)

StephenWangGoogle · 2021-11-18T16:48:39Z

/azpw run

donNewtonAlpha · 2021-11-18T18:04:05Z

/azpw run

mssonicbld · 2021-11-18T18:04:06Z

/AzurePipelines run

azure-pipelines · 2021-11-18T18:04:19Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenWangGoogle · 2021-11-18T19:12:47Z

@qiluo-msft can you approve again? The rebase removed the approval

donNewtonAlpha · 2021-11-18T22:00:57Z

/azpw run

mssonicbld · 2021-11-18T22:00:58Z

/AzurePipelines run

azure-pipelines · 2021-11-18T22:01:11Z

Azure Pipelines successfully started running 1 pipeline(s).

donNewtonAlpha · 2021-11-19T01:56:48Z

@qiluo-msft @prsunny all tests have passed can we get this merged now?

bocon13 force-pushed the pr2-batch-ops branch from 7662145 to a14f4d5 Compare October 22, 2021 16:31

bocon13 previously approved these changes Oct 22, 2021

View reviewed changes

bocon13 dismissed their stale review via 00a46d3 October 22, 2021 16:36

bocon13 force-pushed the pr2-batch-ops branch from a14f4d5 to 00a46d3 Compare October 22, 2021 16:36

mint570 reviewed Oct 22, 2021

View reviewed changes

prsunny requested a review from qiluo-msft October 23, 2021 00:43

bocon13 mentioned this pull request Oct 25, 2021

PINS Upstream Tracking for MVP sonic-net/SONiC#841

Open

zhangyanzhao requested review from prsunny and yxieca October 25, 2021 19:11

donNewtonAlpha force-pushed the pr2-batch-ops branch from 00a46d3 to d7ed943 Compare October 30, 2021 02:11

donNewtonAlpha changed the title ~~PINS: Pr2 batch ops~~ P4Runtime : Status Code additions Nov 8, 2021

donNewtonAlpha force-pushed the pr2-batch-ops branch from f768a4d to ce1b78a Compare November 9, 2021 18:18

Removing Batch OPs pending further review

0596f17

signed-of-by:Don Newton<don@opennetworking.org>

donNewtonAlpha force-pushed the pr2-batch-ops branch from ce1b78a to 0596f17 Compare November 9, 2021 18:19

bocon13 reviewed Nov 9, 2021

View reviewed changes

common/producerstatetable.cpp Outdated Show resolved Hide resolved

bocon13 reviewed Nov 9, 2021

View reviewed changes

common/producerstatetable.h Outdated Show resolved Hide resolved

Remove Redis commands

d097326

bocon13 reviewed Nov 9, 2021

View reviewed changes

qiluo-msft reviewed Nov 9, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into pr2-batch-ops

fa9a1fc

donNewtonAlpha dismissed qiluo-msft’s stale review via fa9a1fc November 17, 2021 20:20

qiluo-msft approved these changes Nov 19, 2021

View reviewed changes

qiluo-msft merged commit 1dfe06f into sonic-net:master Nov 19, 2021

stephenxs mentioned this pull request Nov 23, 2021

[sonic-swss] Advance submodule sonic-net/sonic-buildimage#9288

Merged

5 tasks

P4Runtime : Status Code additions #545

P4Runtime : Status Code additions #545

Conversation

donNewtonAlpha commented Oct 22, 2021

bocon13 commented Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented Oct 23, 2021

bocon13 commented Oct 25, 2021

bocon13 commented Oct 27, 2021 • edited Loading

qiluo-msft commented Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

bocon13 Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

bocon13 Nov 9, 2021 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bocon13 commented Nov 9, 2021

qiluo-msft Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donNewtonAlpha commented Nov 16, 2021

qiluo-msft commented Nov 17, 2021

donNewtonAlpha commented Nov 17, 2021

qiluo-msft commented Nov 17, 2021

donNewtonAlpha commented Nov 17, 2021 via email

StephenWangGoogle commented Nov 17, 2021

donNewtonAlpha commented Nov 17, 2021 via email

bocon13 commented Nov 17, 2021 • edited Loading

donNewtonAlpha commented Nov 17, 2021 via email

donNewtonAlpha commented Nov 17, 2021

mssonicbld commented Nov 17, 2021

azure-pipelines bot commented Nov 17, 2021

donNewtonAlpha commented Nov 18, 2021

mssonicbld commented Nov 18, 2021

azure-pipelines bot commented Nov 18, 2021

bocon13 commented Nov 18, 2021

StephenWangGoogle commented Nov 18, 2021

donNewtonAlpha commented Nov 18, 2021

mssonicbld commented Nov 18, 2021

azure-pipelines bot commented Nov 18, 2021

StephenWangGoogle commented Nov 18, 2021

donNewtonAlpha commented Nov 18, 2021

mssonicbld commented Nov 18, 2021

azure-pipelines bot commented Nov 18, 2021

donNewtonAlpha commented Nov 19, 2021

bocon13 commented Oct 22, 2021 •

edited

Loading

bocon13 commented Oct 27, 2021 •

edited

Loading

qiluo-msft commented Nov 8, 2021 •

edited

Loading

bocon13 Nov 9, 2021 •

edited

Loading

bocon13 Nov 9, 2021 •

edited by qiluo-msft

Loading

qiluo-msft Nov 9, 2021 •

edited

Loading

qiluo-msft Nov 9, 2021 •

edited

Loading

qiluo-msft Nov 9, 2021 •

edited

Loading

qiluo-msft Nov 9, 2021 •

edited

Loading

bocon13 commented Nov 17, 2021 •

edited

Loading