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.

Sign up for GitHub

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

Jump to bottom

Migrate STF scripts to the STF parser library. Modernize stale run-bmv2-test.py code. #4040

Merged
merged 4 commits into from
Aug 9, 2023
Merged

Migrate STF scripts to the STF parser library. Modernize stale run-bmv2-test.py code. #4040

merged 4 commits into from
Aug 9, 2023

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Jun 19, 2023 •
edited
Loading

This PR is in preparation to fix several open issues with the BMv2 STF testing framework. The first step is to use a proper parser.

  • Simplify bmv2stf.py and use stfparser.py instead.
  • Subclass the generic STFParser and STFLexer with BMv2-specific instructions which are validated using the lexer/parser.
  • Augment the STFParser grammar to support arbitrary identifiers (using unicode, for example)
  • Clean up the run-bmv2-test.py and use common utilities available in the compiler.
    • Remove duplicated utilities and instead rely on testutils in the tools folder.

Fixes #1489. Fixes #3514.

@jafingerhut
Copy link
Contributor

Out of curiosity, is part of the goal an attempt to make STF tests more "time order aware", e.g. to be able to add table entries, send one or more packets in, expect one or more packets out, and then add more table entries later that will not be added until after the previous round of packet-in & packet-out checking? Because my understanding is that current STF tests always do the following:

  • all table adds, regardless of the order they appear in the STF file relative to 'packet' and 'expect' lines.
  • all send packets, regardless of the order they appear in the STF file relative to table operations and 'expect' lines
  • all packet expects, regardless of the order they appear in the STF file relative to 'packet' and table operation lines

That seems fine to me to limit the functionality of STF tests to that order of operations, personally. If you want something fancier, we have PTF tests for fancier control of the order of events.

@fruffy
Copy link
Collaborator Author

fruffy commented Jun 19, 2023 •
edited
Loading

The motivation is actually simpler than that.
Currently, there are two STF parsers in the compiler. One uses yacc and ply and defines a proper grammar, the other is an ad-hoc implementation parsing STF instructions as needed.

The ad-hoc implementation is harder to extend and does not support all types of instructions. It is also very specialized to BMv2. The STF grammer implementation on the other hand can be made more general to support different targets. Having the grammar explicit also makes it easier to implement some missing control-plane commands and match types. I am also learning to deal with lex/yacc. :)

This PR gets rid of the ad-hoc implementation and replaces it with the parser implementation.

@mihaibudiu
Copy link
Contributor

But the STF test limitations will remain.

@fruffy
Copy link
Collaborator Author

fruffy commented Jun 19, 2023

But the STF test limitations will remain.

Yes, this is not intended to solve the timing problem. Although once you have the parsed instructions, this should also be implementable.

STF is just easier to bootstrap compared to PTF, in particular when implementing a new target.

@ChrisDodd
Copy link
Contributor

I thought the main limitation with the stf parser library is that it did not support expanding environment variable references in the stf script? The bmv2 implementation is also somewhat broken in that it does not implement wait commands to wait for packets to complete processing before processing more script lines.

@fruffy
Copy link
Collaborator Author

fruffy commented Jun 20, 2023

I thought the main limitation with the stf parser library is that it did not support expanding environment variable references in the stf script? The bmv2 implementation is also somewhat broken in that it does not implement wait commands to wait for packets to complete processing before processing more script lines.

The current approach used by run-bmv2-test.py also does not support environment variable expansion afaik. This PR mostly just consolidates the two parsers as first step. Existing limitations are not fixed.

@fruffy fruffy force-pushed the fruffy/stf_refactor branch 6 times, most recently from 090286e to a0841e5 Compare August 2, 2023 11:23
@fruffy fruffy marked this pull request as ready for review August 2, 2023 14:51
fruffy
fruffy commented Aug 2, 2023
View reviewed changes
testdata/p4_14_samples/counter1.stf
@@ -3,9 +3,6 @@ add tab1 ethernet.dstAddr:0xa1a2a3a4a5a6 act(port:2) = A
add tab1 ethernet.dstAddr:0xb1b2b3b4b5b6 act(port:3) = B
add tab1 ethernet.dstAddr:0xc1c2c3c4c5c6 act(port:5) = C

expect 2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests were never working correctly, this was caught by the refactor. STF has a limitation in that it does not catch packets that were not expected to be forwarded if no expect is present.
See also:
p4lang/behavioral-model#1205

backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake
@@ -62,12 +62,6 @@ p4tools_add_xfail_reason(
p4tools_add_xfail_reason(
"testgen-p4c-bmv2"
"Exception"
# Running simple_switch_CLI: Exception Unexpected key field &
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests all pass now because we parse key and action names correctly.

fruffy
fruffy commented Aug 2, 2023
View reviewed changes
tools/stf/stf_lexer.py
@@ -195,6 +196,12 @@ def t_ID(self, t):
# print t, "pos:", t.lexpos, "col:", self.lexer.colno
return t

@TOKEN(quoted_identifier)
def t_quoted_ID(self, t):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a better way to lex an identifier within quotes? The idea here is that any character can be used.

jafingerhut
jafingerhut approved these changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

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

LGTM

@jafingerhut
Copy link
Contributor

I am not sure why, but I see "Merging is blocked" for this PR, even though I have approved it. Perhaps p4lang/p4c rules have been configured so that 2 approving reviews are required?

@fruffy
Copy link
Collaborator Author

fruffy commented Aug 9, 2023

I am not sure why, but I see "Merging is blocked" for this PR, even though I have approved it. Perhaps p4lang/p4c rules have been configured so that 2 approving reviews are required?

@jnfoster or @pkotikal need to approve because this PR touches some Testgen code.

@fruffy fruffy merged commit d31444b into main Aug 9, 2023
15 checks passed
@fruffy fruffy deleted the fruffy/stf_refactor branch August 9, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkotikal pkotikal pkotikal approved these changes

@jafingerhut jafingerhut jafingerhut approved these changes

@jnfoster jnfoster Awaiting requested review from jnfoster

@mihaibudiu mihaibudiu Awaiting requested review from mihaibudiu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unexpected key field for BMV2 example Migrate bmv2stf to use the STF parsing library
5 participants
@fruffy @jafingerhut @mihaibudiu @ChrisDodd @pkotikal