-
Notifications
You must be signed in to change notification settings - Fork 555
add SAI definitions for optical circuit switch #2229
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Nathan Ni <[email protected]>
|
Need to add new SAI objects and types for OCS in SAI extension header files. Without the following change, the SAI OCS object/api will not be processed by parser to generating meta data. saiextensions.h saitypesextensions.h |
| * @objects SAI_OBJECT_TYPE_OCS_PORT | ||
| */ | ||
| SAI_OCS_CROSS_CONNECT_ATTR_B_SIDE_PORT_ID, | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add operational status so user know if a connection is up or down, without looking at the corresponding port status?
/**
* @brief Cross connect operation status
*
* @type sai_ocs_cross_connect_oper_status_t
* @flags READ_ONLY
*/
SAI_OCS_CROSS_CONNECT_ATTR_OPER_STATUS,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For SAI layer as the physical information, cross-connect is more about configuration and the configurational result success/failure. For upper layer, like swss, if necessary, it can use port status and other alarm information to have a logic result about cross-connect operational status.
| /** | ||
| * @brief Default status for any port which is with no configuration | ||
| */ | ||
| SAI_OCS_PORT_STATUS_BLOCKED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This default value (SAI_OCS_PORT_STATUS_BLOCKED) conflicts to the default of override_state_t, SAI_OCS_PORT_OVERRIDE_STATE_NORMAL??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SAI_OCS_PORT_STATUS_BLOCKED is a value of OCS port operational status while SAI_OCS_PORT_OVERRIDE_STATE_NORMAL is a value of OCS port configuration. When OCS port is created, the default override state configuration is SAI_OCS_PORT_OVERRIDE_STATE_NORMAL. OCS port operational status is updated by OCS driver/SDK, dynamically based on rules (by checking port override state configuration and OCS cross-connect on the port)..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SAI_OCS_PORT_STATUS_BLOCKED is a value of OCS port operational status while SAI_OCS_PORT_OVERRIDE_STATE_NORMAL is a value of OCS port configuration. When OCS port is created, the default override state configuration is SAI_OCS_PORT_OVERRIDE_STATE_NORMAL. OCS port operational status is updated by OCS driver/SDK, dynamically based on rules (by checking port override state configuration and OCS cross-connect on the port)..
got it.
| * @brief Overrides the status imposed by the programmed cross-connects | ||
| * | ||
| * @type sai_ocs_port_override_state_t | ||
| * @flags CREATE_AND_SET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In latest SAI version, for CREATE_AND_SET attributors, it is required to have default value flag:
@default SAI_OCS_PORT_OVERRIDE_STATE_NORMAL
Otherwise parser and compile will be error-out. All CREATE_AND_SET attributes need to add @default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, updated the code.
| SAI_OCS_PORT_OVERRIDE_STATE_POWERED_OFF, | ||
|
|
||
| /** | ||
| * @brief Default, state to be determined by presence of cross-connect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is port state NORMAL is determined connection present or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OCS port override state is a configurable attribute of OCS port. it is defined as CREATE_AND_SET. It can be configured independently, no matter whether cross-connect exists on the port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
understood. But the description seems not accurate.
| /** | ||
| * @brief Start of attributes | ||
| */ | ||
| SAI_OCS_CROSS_CONNECT_ATTR_START, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All new acronyms (non-English words), such as OCS need to be add the in the meta dictionary meta/aspell.en.pws. compile will fail without this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, updated the code. Thanks
| * List of center frequency values. Use int32 for representation of decimal value with 3 fraction digits | ||
| * | ||
| * @type sai_s32_list_t | ||
| * @flags READ_ONLY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To convert sai_s32_list_t -> decimal, how does the application code (ex. SWSS) know that it should have 3 decimal digits, without hard code to SAI_OCS_CROSS_CONNECT_FACTORY_DATA_ATTR_FREQUENCY_THZ? For decimal -> sai_s32_list_t, how do we validate that the decimal string is 3 precision digits in redis/NBI?
The decimal precision info should be part of the meta data, so the conversion can done generically. One way is to use sai_s32_t and add a new tag @precision n and so the application can retrieve the number of decimal digits from meta data. The proposal to introduce a new type sai_double_t is dropped due to NAN issue.
Same comments for all decimal representation below.
| * @brief End of attributes | ||
| */ | ||
| SAI_OCS_CROSS_CONNECT_ATTR_END, | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add custom rang for vendor extension (see DASH SAI). same for port attributes.
/** Custom range base value */
SAI_OCS_CROSS_CONNECT_ATTR_CUSTOM_RANGE_START = 0x10000000,
/** End of custom range base */
SAI_OCS_CROSS_CONNECT_ATTR_CUSTOM_RANGE_END
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the custom range can be defined for vendor specific extensions, updated the code.
| /** | ||
| * @brief Unique identifier for a port | ||
| * | ||
| * @type sai_u8_list_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for port name why not using @type char?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In SAI attribute value, char data is defined as: char chardata[32], to support port name with length > 32, sai_u8_list_t is used instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
32 should be long enough for a port name? char is much easy to use than u8_list.
| * | ||
| * @brief This module defines SAI OCS | ||
| */ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add the following to compatible with SAI extension common practice:
*
* @warning This module is a SAI experimental module.
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks
| /** | ||
| * @brief A side port | ||
| * | ||
| * @type sai_object_id_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why you don't use port name (a string) in the port attributes? sai_object_id is auto-generated when SAI object is created. Do you want to enforce creating cross-connect after creating ports? This would need extra logic in SWSS? In any case, I think port name A and B are useful info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using SAI object id as type is for reference of OCS ports in OCS cross-connect, it can also enforce that OCS cross-connect must be provisioned between existing OCS ports. OCS ports can be created by swss based device specific on template/profile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is certain valid design choice. However, In config DB cross-connect uses port name. so we need logic (SWSS) to find our the port's sai_object_id. As sai_object_id is dynamically generated at run time, so it is different on each restart. This make debug a little hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood the point, yes, using this proposal, SONiC SWSS can maintain the association between port sai_object_id and port name. Port name is defined as a SAI OCS port attribute. so in ASIC DB port name can be read via port's port sai_object_id. In OCS vendor SAI lib, association of port sai_object_id and port name can also be maintained (during the processing of SAI creating OCS ports.
Signed-off-by: Nathan Ni <[email protected]>
Signed-off-by: Nathan Ni <[email protected]>
Signed-off-by: Nathan Ni <[email protected]>
mdemerchant
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this proposal needs discussion with more OCS vendors to make sure it is not too technology specific. It would be very useful to have a discussion within the OCP OCS sub-project since a large number of vendors are participating.
| * OCS ports are referenced to OCS cross-connect by their SAI object IDs in cross-connect attributes. | ||
| ## 4.2. Cross-Connect Configuration | ||
| * To have fast switching performance, SAI bulk create/remove APIs are used for management of OCS cross-connect configurations. | ||
| * OCS cross-connect can not be provisioned between two A side ports or two B side ports. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requirement restricts OCS technology choices. While most OCS devices available today are limited to A to B connections, not all devices have this limitation. If using bidi optics the ability to connect true any to any port seems like it would be quite valuable, ideally the SAI API should be generalized to support that functionality.
| ## 4.2. Cross-Connect Configuration | ||
| * To have fast switching performance, SAI bulk create/remove APIs are used for management of OCS cross-connect configurations. | ||
| * OCS cross-connect can not be provisioned between two A side ports or two B side ports. | ||
| * One OCS port can only involved in one OCS cross-connect; when switching existing OCS cross-connect, e.g. switch from 1A-1B to 1A-2B, the upstream application need to use SAI API to remove 1A-1B and then create 1A-2B. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The described behavior seems like it would be performance limiting if you are trying to make bulk changes. You would first have to bulk delete existing cross connects, then bulk create the new desired cross connects which requires two operations. If the behavior is that the switching engine simply replaces the existing connection if you re-use a port then it would be able to do it in one operation.
| # 5. OCS Cross-Connects Factory Data | ||
| OCS cross-connect factory data are read-only information about insertion loss measurements of all possible cross-connects. | ||
| * Factory data are measured during the manufacturing calibration phase. | ||
| * The measurement is for every possible optical path (cross-connect) with an optical spectrum (central frequency) in specific environment (temperature). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no definition given to standardize what data is actually present, only the format of how the data is presented. Is it useful without some standard expectation of what measurements are actually made? What if one vendor decides to measure in O Band and one in C Band. Or one vendor makes a single wavelength measurement and one decides to measure at ten different wavelengths on each path?
| /** | ||
| * @brief First element that the beam hits is powered off, for testing | ||
| */ | ||
| SAI_OCS_PORT_OVERRIDE_STATE_POWERED_OFF, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think POWERED_OFF is a good choice of state name. It implies specific OCS technology. Wouldn't something more generic like DISABLED be better? It seems the only operational impact of the port being in this state is that you just can't use it to create a cross connection.
What is the actual use case for wanting this state anyway as distinct from the FORCE_BLOCKED state? What this will do optically seems undefined and will vary from device to device.
| /** | ||
| * @brief Insertion loss is within 0.5dB of target | ||
| */ | ||
| SAI_OCS_PORT_STATUS_TUNED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defining this as loss within 0.5dB of target seems like a strange definition and potentially complex to implement for vendors. Why not just define it as when the device has finished switching? For devices that don't have on-going control loops this is simple and for devices that do have on-going control loops they can implement their own threshold of when it's close enough to guarantee their optical performance specs.
| /** | ||
| * @brief If there is a hardware failure | ||
| */ | ||
| SAI_OCS_PORT_STATUS_FAILED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this state intended only to represent a permanent HW failure or simply some kind of failure to set up the last requested connection for this port? What about failure mechanisms where a particular cross connect path connect be achieved but other paths using the port still work fine? The object definition ties failures to ports but not all failure mechanisms are port related, depending on the OCS technology.
I think representing failures in a technology agnostic way needs something considerably more sophisticated in order to convey useful information to the upper layer SW.
| * @type sai_u8_list_t | ||
| * @flags READ_ONLY | ||
| */ | ||
| SAI_OCS_PORT_ATTR_PHYSICAL_MAPPING, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add something to the markdown file or comments here documenting what this attribute actually is. It doesn't seem to be in the port creation example in the markdown file either.
| * @brief List of OCS cross connect factory data attributes. Inventory data for all possible cross-connects, | ||
| * factory insertion loss measurements | ||
| */ | ||
| typedef enum _sai_ocs_cross_connect_factory_data_attr_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there examples of comparable data structures used for other types of switches in SAI? Maybe I'm misunderstanding the proposed flow but it seems like the SAI implementation wouldn't actually do anything with this data, not even read it from hardware. Some higher level platform specific SW would have to read it from device storage and parse it, then pass it into SAI to create all the objects at initialization. Then later higher level management SW can get the data via those SAI objects. I can see it makes that second access device independent but it just seems kind of an odd implementation. It seems very different from how general device inventory information is handled.
| /** | ||
| * @brief OCS methods table retrieved with sai_api_query() | ||
| */ | ||
| typedef struct _sai_ocs_api_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that SAI has singular and bulk operations because historically it started with singular and added bulk later. But if the OCS is a greenfield application is it necessary/useful to include the singular operations?
… PORT override state configuration Signed-off-by: Nathan Ni <[email protected]>
Add API definitions for Optical Circuit Switch, mainly including: