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

Implement acceptable interface identifier range for P4RT #653

Merged
merged 4 commits into from
Jun 18, 2022

Conversation

earies
Copy link
Contributor

@earies earies commented Jun 8, 2022

  • (M) release/models/p4rt/openconfig-p4rt.yang
    • Add range statement to interface identifier in conformance to P4RT
      specification v1.3.0
    • Description cleanup to map OpenConfig terminology to P4RT
      specification terminology

Per https://p4.org/p4-spec/p4runtime/main/P4Runtime-Spec.html#sec-translation-of-port-numbers, the 'SDN Port' is the equivalent attribute to the OpenConfig P4RT interface 'id'. This change limits the acceptable uint32 range for configurable values per specification.

* (M) release/models/p4rt/openconfig-p4rt.yang
  - Add range statement to interface identifier in conformance to P4RT
    specification v1.3.0
  - Description cleanup to map OpenConfig terminology to P4RT
    specification terminology
@OpenConfigBot
Copy link

OpenConfigBot commented Jun 8, 2022

Compatibility Report for commit 138b239:
yanglint@SO 1.10.17

Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

This LGTM. One question we could have is whether to change the type here to have enumerated values for the special cases, but given that these are actually invalid values - then I think I prefer this change as-is.

@dplore @rsgcp @xw-g -- any objections?

@earies
Copy link
Contributor Author

earies commented Jun 17, 2022

This LGTM. One question we could have is whether to change the type here to have enumerated values for the special cases, but given that these are actually invalid values - then I think I prefer this change as-is.

@dplore @rsgcp @xw-g -- any objections?

Thx @robshakir - yeah, if all options were available for config or state of all possible values in a different construct/location then I would say yes but since this restriction is on the interface id itself it's locked into the range for 'SDN Ports'

@dplore
Copy link
Member

dplore commented Jun 17, 2022

Does an SDN controller ever need to address the SDN_PORT_RECIRCULATE or SDN_PORT_CPU? Reading from the spec it seems like this is explicitly defined and permitted? Seems like the change should permit those values.

@earies
Copy link
Contributor Author

earies commented Jun 17, 2022

Does an SDN controller ever need to address the SDN_PORT_RECIRCULATE or SDN_PORT_CPU? Reading from the spec it seems like this is explicitly defined and permitted? Seems like the change should permit those values.

This restriction is solely for the interface id node - e.g. to define an identifier that falls within the valid SDN_PORT range

RECIRCULATE and CPU would not be values ever assigned to a interface in this context

@dplore
Copy link
Member

dplore commented Jun 18, 2022

Ok, I think you are saying /interfaces/interface is not expected to include CPU or RECIRCULATE entities and therefore we don't need to allow those IDs?

@earies
Copy link
Contributor Author

earies commented Jun 18, 2022

Ok, I think you are saying /interfaces/interface is not expected to include CPU or RECIRCULATE entities and therefore we don't need to allow those IDs?

Correct

@dplore
Copy link
Member

dplore commented Jun 18, 2022

/gcbrun

@dplore
Copy link
Member

dplore commented Jun 18, 2022

Hi @wenovus we are seeing ygot CI check failing here?

@wenovus
Copy link
Contributor

wenovus commented Jun 18, 2022

Hi @wenovus we are seeing ygot CI check failing here?

Is the PR in sync with master?

@dplore
Copy link
Member

dplore commented Jun 18, 2022

@earies please rebase/merge with master to pick up changes that should fix the CI issue. :)

@earies
Copy link
Contributor Author

earies commented Jun 18, 2022

@earies please rebase/merge with master to pick up changes that should fix the CI issue. :)

thx @dplore @wenovus - branch sync'd and passing CI now

@dplore dplore merged commit db7a87d into openconfig:master Jun 18, 2022
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.

5 participants