Skip to content

Conversation

@sudhanshu112233shukla
Copy link
Contributor

Description

This PR implements support for applying the same table multiple times in a pipeline while maintaining backward compatibility with existing JSON formats.

Fixes #1286

Key Changes

  • Added a new table_apply.h and table_apply.cpp to implement the TableApply class that represents a specific application of a table in the control flow
  • Modified P4Objects to support table_applies with unique IDs and names
  • Updated the JSON format to version 3.0, adding a new "table_applies" array to each pipeline
  • Moved the "next_tables" field from table objects to table_apply objects
  • Added methods to get and manage TableApply objects in P4Objects
  • Added comprehensive unit tests to verify the new functionality
  • Updated documentation in JSON_format.md to explain the new format

Backward Compatibility

This implementation maintains full backward compatibility with existing JSON formats (version 2.x). The code checks the JSON version and only processes table_applies for version 3.0+. Legacy JSON files will continue to work without modification.

Testing

  • Added unit tests that verify both the new functionality and backward compatibility
  • Created test_table_apply.cpp to test the TableApply class and its integration with P4Objects
  • All existing tests continue to pass

Documentation

  • Updated JSON_format.md with details about the new "table_applies" objects and version 3.0 changes
  • Added explanatory comments in the code to clarify the implementation

@jafingerhut
Copy link
Contributor

I have not looked at the code changes in detail yet.

It would be great if you were able to get all CI build & tests passing before we go through code review. Feel free to ask questions about how to do that if you get stuck, but I would start by looking at the section "Some checks were not successful" on the PR's web page here: #1301, and clicking on the names of tests that are failing to see the logs. Try to determine why the tests are failing, and in some cases it is helpful if you can reproduce the failures on your local development system, too.

@sudhanshu112233shukla
Copy link
Contributor Author

Sure sir, I’ll do it. I will follow your instructions and reach out if I face any difficulties. I will check the workflow as well.

@sudhanshu112233shukla sudhanshu112233shukla force-pushed the feature/tables_applyP4object branch from 7c9a1bf to f8dd62b Compare June 11, 2025 13:28
@jafingerhut
Copy link
Contributor

It is not clear to me why the CI tests all show status "Exepcted - Waiting for status to be reported" when the last commit to this PR was yesterday. It seems either they never started, or they started and are "hung" somehow. It would be nice to see the CI test results for these proposed changes before reviewing them in detail.

@sudhanshu112233shukla One way to "kick" the CI process is to add another commit to the PR. That usually causes CI tests to be started again. In this particular case, I noticed that your PR removed the file Dockerfile. That is not required for your changes, is it? If not, that file should not be removed by the PR. Pushing another commit to this PR that restores the existence of that file in the repo would be one way to make CI tests run again.

Alternately, any simple inconsequential whitespace-only change in a README or a comment is another way.

@sudhanshu112233shukla
Copy link
Contributor Author

It is not clear to me why the CI tests all show status "Exepcted - Waiting for status to be reported" when the last commit to this PR was yesterday. It seems either they never started, or they started and are "hung" somehow. It would be nice to see the CI test results for these proposed changes before reviewing them in detail.

@sudhanshu112233shukla One way to "kick" the CI process is to add another commit to the PR. That usually causes CI tests to be started again. In this particular case, I noticed that your PR removed the file Dockerfile. That is not required for your changes, is it? If not, that file should not be removed by the PR. Pushing another commit to this PR that restores the existence of that file in the repo would be one way to make CI tests run again.

Alternately, any simple inconsequential whitespace-only change in a README or a comment is another way.

Sure sir I'll recover the file.

@fruffy
Copy link
Contributor

fruffy commented Jun 12, 2025

It is not clear to me why the CI tests all show status "Exepcted - Waiting for status to be reported" when the last commit to this PR was yesterday. It seems either they never started, or they started and are "hung" somehow. It would be nice to see the CI test results for these proposed changes before reviewing them in detail.

@sudhanshu112233shukla One way to "kick" the CI process is to add another commit to the PR. That usually causes CI tests to be started again. In this particular case, I noticed that your PR removed the file Dockerfile. That is not required for your changes, is it? If not, that file should not be removed by the PR. Pushing another commit to this PR that restores the existence of that file in the repo would be one way to make CI tests run again.

Alternately, any simple inconsequential whitespace-only change in a README or a comment is another way.

I assume security feature since it is a first-time contributor or someone without write permissions (using a fork) editing CI workflow files. Let me check our settings but I do not think we can manually run them here...

@jafingerhut
Copy link
Contributor

I assume security feature since it is a first-time contributor or someone without write permissions (using a fork) editing CI workflow files. Let me check our settings but I do not think we can manually run them here...

@sudhanshu112233shukla Do you need the changes to the github workflow test.yaml file in order to run CI on your changed version of the code? I did not try to understand them all, but some of them appeared to be copying log or other output files around.

If they are helpful for debugging build failures, but are not otherwise needed for build to succeed, I'd recommend keeping a local copy of those changes on your system, but removing them from this PR, to see if doing so will enable the CI tests to run.

@sudhanshu112233shukla
Copy link
Contributor Author

sudhanshu112233shukla commented Jun 16, 2025

@jafingerhut Sure sir I will do it...

@sudhanshu112233shukla sudhanshu112233shukla force-pushed the feature/tables_applyP4object branch 2 times, most recently from 2071348 to 7197ef2 Compare June 18, 2025 12:18
@sudhanshu112233shukla sudhanshu112233shukla force-pushed the feature/tables_applyP4object branch 2 times, most recently from 029a25d to d0a333f Compare July 14, 2025 12:23
sudhanshu112233shukla and others added 18 commits July 17, 2025 08:19
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
…cker"

This reverts commit c0276ba.

Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
sudhanshu112233shukla and others added 6 commits July 17, 2025 08:19
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
Signed-off-by: sudhanshu shukla <[email protected]>
Signed-off-by: sudhanshu112233shukla <[email protected]>
…ild improvements from both branches

Signed-off-by: sudhanshu112233shukla <[email protected]>
@sudhanshu112233shukla sudhanshu112233shukla force-pushed the feature/tables_applyP4object branch from 68eb865 to 05348a8 Compare July 17, 2025 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generalize BMv2 JSON file format to enable the same identical table to be applied multiple times in a single pipeline

3 participants