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.

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

Reject invalid MeterConfigs in Stratum-bfrt #826

Merged
merged 3 commits into from Dec 9, 2021

Conversation

pudelkoM
Copy link
Member

P4RT meters are trTCM according to RFC 2698.
Not all P4 MeterConfigs possible are actually valid. We don't want to solely rely on the SDE to reject those, therefore we introduce logic in Stratum to catch them early and with helpful error messages.

@codecov
Copy link

codecov bot commented Sep 20, 2021

Codecov Report

Merging #826 (3fbc063) into main (2832b4e) will decrease coverage by 0.00%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #826      +/-   ##
==========================================
- Coverage   78.56%   78.55%   -0.01%     
==========================================
  Files         334      334              
  Lines       30057    30077      +20     
==========================================
+ Hits        23613    23628      +15     
- Misses       6444     6449       +5     
Impacted Files Coverage Δ
stratum/hal/lib/p4/utils.cc 66.05% <68.42%> (+0.49%) ⬆️
stratum/hal/lib/barefoot/bfrt_table_manager.cc 25.26% <100.00%> (+0.11%) ⬆️
stratum/p4c_backends/fpm/parser_field_mapper.cc 92.02% <0.00%> (+0.26%) ⬆️

stratum/hal/lib/p4/utils.cc Outdated Show resolved Hide resolved
@pudelkoM pudelkoM force-pushed the bfrt-validate-meter-config branch 2 times, most recently from 1d3ab72 to 6d74db9 Compare September 21, 2021 17:26
Copy link
Contributor

@pierventre pierventre left a comment

Choose a reason for hiding this comment

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

other than that looks good to me

stratum/hal/lib/p4/utils.cc Outdated Show resolved Hide resolved
@bocon13 bocon13 added this to the 2021-12 Release milestone Nov 18, 2021
Copy link
Member

@bocon13 bocon13 left a comment

Choose a reason for hiding this comment

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

This looks good after addressing @pierventre's comment

@bocon13 bocon13 merged commit e196cb6 into main Dec 9, 2021
@bocon13 bocon13 deleted the bfrt-validate-meter-config branch December 9, 2021 16:47
@pudelkoM
Copy link
Member Author

pudelkoM commented Dec 9, 2021

I'm totally ok with merging, one remaining thought was that the check should happen in p4_service to cover all backends.
But that opens up the gate to all kinds of validations, p4 pipeline aware and not.
Maybe in the future...

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.

None yet

3 participants