-
Notifications
You must be signed in to change notification settings - Fork 659
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
add bgp comm regex-match #1043
add bgp comm regex-match #1043
Conversation
Major YANG version changes in commit 38d0ccd: |
@dplore based on the fact it's not reproducible locally and that random threads are being killed, I'm guessing it's because since all the models were running in parallel, the build machine went OOM and decided to kill one of the pyangbind threads. Making ygnmi run after pyangbind (empirically these two were the ones conflicting with one another) seems to resolve this issue. But if this happens let me know and I'll make the processes more serial. In the future I think it makes sense to move to one of the high memory private worker pool nodes. It doubles the build cost but will give us 4x more memory. Please review openconfig/models-ci#98 and then I will fix up cloudbuild.yaml. |
Based on results in #1043 it suggests an OOM issue.
Based on results in #1043 it suggests an OOM issue.
@dplore fixed up now. For any other affected PRs, they will need to merge from the main branch to see the new results. |
Last call, this will merge on March 11th. |
The fundamental request here is "use full ieee extended regex".
NOTE: regular expressions are generally the worst performing policy element in policy engines across all vendors. Vendors spend a lot of energy avoiding such performance hits or letting customers shoot themselves in the foot too much. My recommendation would be for OC to support some form of the IETF restrictions for maximal compatibility across vendors. From https://datatracker.ietf.org/doc/html/draft-ietf-idr-bgp-model:
|
Thanks for this feedback, it's very helpful. I've added a constraint and reference to the IETF draft. |
Co-authored-by: Rob Shakir <[email protected]>
@robshakir can you re-review/approve? I fixed the community-member union type to point to the intended bgp-community-regex type which introduced the constaints on top of the posix extended regex type. (The earlier approved commit was still referencing the 'plain' posix extended regex type) |
Based on results in openconfig#1043 it suggests an OOM issue.
* (M) release/models/bgp/openconfig-bgp-errors.yang * (M) release/models/bgp/openconfig-bgp-policy.yang * (M) release/models/bgp/openconfig-bgp-types.yang * (M) release/models/types/openconfig-types.yang Adds posix-regex type and bgp-community-regex type and changes the bgp community-member list to include use the new posix community type. This is a breaking change due to changing an existing union type. Also clarifies the linkbw community type description.
Change Scope
Rather than using various implementation specific regex implementations
The result is a vendor neutral method for writing regex for BGP communities which can be tested. Testing is possible because the regex implementation is standard and implemented in many open source development and testing tools versus the proprietary regex that are present today.
This is an incremental step towards an end state for bgp community set modeling described in #883
Tree Representation
Since this is only a type and description change, there is no change to the OC tree. Affected paths are:
/routing-policy/defined-sets/bgp-defined-sets/community-sets/community-set/config/community-member
/routing-policy/defined-sets/bgp-defined-sets/ext-community-sets/ext-community-set/config/ext-community-member
Platform Implementations
Regex based pattern matching for community is widely implemented but most use a proprietary format for the regex. References are included for comparison of BGP community regex matching behavior.