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

Remove unused chassis-id for P4RT #1020

Merged
merged 2 commits into from
Dec 27, 2023
Merged

Conversation

earies
Copy link
Contributor

@earies earies commented Dec 14, 2023

  • (M) p4rt/openconfig-p4rt.yang
    • Remove unused chassis-id

Change Scope

Remove unused/unnecessary chassis ID previously defined for a P4RT use-case

Platform Implementations

N/A: The chassis ID is not used in the P4RT APIs directly and does not have a
surrounding use-case. This PR is for cleanup purposes.

  * (M) p4rt/openconfig-p4rt.yang
    - Remove unused chassis-id
@earies earies requested a review from a team as a code owner December 14, 2023 00:19
@OpenConfigBot
Copy link

OpenConfigBot commented Dec 14, 2023

Major YANG version changes in commit 74404e5:
openconfig-p4rt.yang: 0.4.0 -> 1.0.0

@sulrich
Copy link
Contributor

sulrich commented Dec 14, 2023

a couple of quick comments:

  • i believe this was added to address some application. (ref: PR Add p4RT id to component/chassis/ #625).
  • in either event, shouldn't we still go through the process of marking these as deprecated for a cycle or so and then remove? this would give implementations some time to track to these proposed changes.

@dplore
Copy link
Member

dplore commented Dec 14, 2023

With some experience working with this, Google discovered that this path is not needed by Google after all.

Deprecating is ok. However, I do plan for the next release this month to be OC 3.0 which will include a couple of breaking changes that were committed in the last month.

@earies
Copy link
Contributor Author

earies commented Dec 14, 2023

I considered slow deprecation here but

  • This node should not be in use by anyone today (although I know EOS has implemented it natively) - I would be very curious to hear of it's real world usage to date
  • There was never any description or prescription as to it's use and imo has no true association to P4RT (It's a integer based identifier for a system much like a hostname being a string identifier) - At most, I think if there were such a use for this type of identifier that it would be a new user-configurable system identifier alternate to/in parallel to a hostname
  • If we roll this into a 0.5.0 and cut a global 2.7.0 (or 2.x.x variant) then cut a 3.0.0 then what is the purpose of letting this sit should there be no recommendation to implement or use.

The proper thing to do (and follow strict protocol) would be to slow deprecate as @sulrich suggests however the bigger concern here imo is keeping around nodes that possibly should have never been merged in the first place. It creates a tremendous amount of unnecessary complexity to document, explain and provide guidance for such cases.

My vote would be for immediate removal on this one

@dplore
Copy link
Member

dplore commented Dec 14, 2023

This will be reviewed at the Dec 19, 2023 OC Operators meeting.

Google was the originator for this leaf. Historically we have set and get this data using a non-OC models internally for some time and wanted to upstream it as a public OC model for use across many types of devices. It is in a file related to p4rt because the operational use case at Google was to use this ID by our p4rt controller. It it true it is not actually part of p4rt (which doesn't use yang or gNMI) nor does it map directly to any p4rt entity. It did serve as a machine readable identifier for a group of p4rt nodes (integrated circuits) that are aggregated into a single chassis.

However, we have made changes which no longer require this field so it is no longer operationally necessary.

Given this and the strong likelihood that it was only ever used by Google, it seems ok to me to delete it. We plan to release OpenConfig v3.0.0 this month which will include some breaking changes, so it is convenient timing from a release perspective.

If we achieve rough consensus on deleting this leaf at the Dec 19, 2023 OC Operators meeting, I believe we should move forward and remove this leaf.

I do welcome input on any operational impact this may cause which could lead to keeping the leaf in the model and/or setting it to deprecate status.

@dplore dplore self-assigned this Dec 27, 2023
@dplore dplore merged commit af9d6b6 into openconfig:master Dec 27, 2023
romeyod pushed a commit to romeyod/aftNextHop that referenced this pull request Sep 19, 2024
* (M) p4rt/openconfig-p4rt.yang
    - Remove unused chassis-id
@earies earies deleted the p4rt-chassis-id branch October 15, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants