-
Notifications
You must be signed in to change notification settings - Fork 157
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 third generic media tuning key to search for (medium+lane) #538
base: master
Are you sure you want to change the base?
Add third generic media tuning key to search for (medium+lane) #538
Conversation
a1f121f
to
cfa926a
Compare
cfa926a
to
ed5ab2f
Compare
@bobbymcgonigle can you review this PR if it meets the requirement? https://github.com/sonic-net/sonic-platform-daemons/pull/533/files |
@bobbymcgonigle there is a regex match for media key here:- https://github.com/sonic-net/sonic-platform-daemons/blob/master/sonic-xcvrd/xcvrd/xcvrd_utilities/media_settings_parser.py#L183 If you specify your SI settings as below:-
Basically the media type is a regular expression it just how you represent that regular expression your platform media_settings.json |
This does not cover all usecases. I think it's important to distinguish between media type and medium. Probably best explained with this example I have on one of my devices here. Let's print the keys searched without my change:
How can we tell here if these are copper or optic ? They both have the same media_key (aside from length). with this change:
We can easily apply the correct tunings now that we know one is optical and one is copper. This also future proofs any new specs that come along; we don't have to maintain a constant list of form factor types, media types etc. Without this change we wouldn't be able to tell which are the correct tunings to apply without knowing all part numbers. |
@bobbymcgonigle The
|
@bobbymcgonigle can you explain what you mean by media type and medium? "distinguish between media type and medium" |
Medium is Copper or Fiber Really we don't need to support all these different media types when the parent characteristic (medium) covers them all. |
@bobbymcgonigle can this closed as we discussed we don't need this change? |
Hi Prince, took some time to figure out all edge cases we've missed. I've discovered a couple that would make BASE-CR not work like we expected. I've described it all out in a full proposal for the third key; I'll email this document today! |
Description
This change searches for a third generic key that covers medium type + lane speed. This will cover all Arista usecases as at Arista out tuning values are based off medium type + lane speed. Of course this will work for all vendors if they choose to do so. The consequence of this is that on bringup Arista can upload a single media_settings.json which will cover every possible speed and lane combination. Any users of our SKUs will not need to ever ask for updates to cover new optics, speeds etc.
Note if there is no xcvr present we will assume copper; this will cover internal phy to external phy connection, as well as any backplane links.
The new flow will look like: (Addition to current flow in green)
Motivation and Context
The current implementation of the the dynamic media tuning feature requires that you know either:
This is too specific. For example consider the following case. I am using QSFP-DDs, but last minute before deployment I change vendors and use QSFPs - now I need the vendor like Arista to upstream a new PR that includes different or even the same tuning values but with a new vendor/media key to match against. Same scenario if we change from DACs to optics last minute.
We can make this more generic by adding a third key that is a combination of physical medium (copper or optical) and lane speed (E.g. 400g-8 is 50000, 100g-4 is 25000).
This key can be easily derived in xcvrd because
This means that we can now have 1 media_settings.json file that can cover all scenarios. It does the check after Vendor+Media key - so you can still have very specific values if you want to take precedence over the generic.
How Has This Been Tested?
Added a new media_settings.json for Arista QuicksilverP with the following entries per port:
Changing speeds and doing xcvr swap I can see that the different and correct values are applied in APPL_DB.
I also added test cases and made sure other testcases pass tests/test_xcvrd.py unit tests.
Additional Information (Optional)