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

2 enum reordering #1250

Closed

Conversation

shri-khare
Copy link
Contributor

@shri-khare shri-khare commented Jun 15, 2021

Consider a scenario where an application (adapter host) using SAI spec X
(say) warmboots to an application using new SAI spec Y (say).

If an enum is extended between X and Y with fields inserted "in between"
rather than "at the end" of the enum, the old fields can get "renumbered".
However, the SAI implementation (adapter) continues to return old enums
for previously returned objects thereby breaking the warmboot.

Fix this by moving the newly inserted fields "to the end"
(this could mean at the end of _END block) to keep the enum field values
the same.

Problem:

Consider a scenario where an application (adapter host) using SAI spec X
(say) warmboots to an application using new SAI spec Y (say).

If an enum is extended between X and Y with fields inserted "in between"
rather than "at the end" of the enum, the old fields can get "renumbered".
However, the SAI implementation (adapter) continues to return old enums
for previously returned objects thereby breaking the warmboot.

Fix this by moving  the newly inserted fields "to the end"
(this could mean at the end of _END block) to keep the enum field values
the same.

Specifically, below patch was first introduced in v1.7.0 and inserted enum fields 'in betweeen':

opencomputeproject@1aeb1c8

Thus,

- In 1.6.5: SAI_ACL_ENTRY_ATTR_FIELD_DSCP is 4126, SAI_ACL_ENTRY_ATTR_ACTION_SET_DSCP is 8214 etc.
- In 1.8.1: SAI_ACL_ENTRY_ATTR_FIELD_DSCP is 4134, SAI_ACL_ENTRY_ATTR_ACTION_SET_DSCP is 8216 etc.

If the application created ACL entry with SAI_ACL_ENTRY_ATTR_FIELD_DSCP
(4126) prior to warmboot, even after warmboot, a get query on that ACL
entry returns 4126 which no longer means SAI_ACL_ENTRY_ATTR_FIELD_DSCP.

This patch fixes it by moving all the newly introduced ACL fields to the
end.

Signed-off-by: zecheng@fb.com
Signed-off-by: Shrikrishna Khare <skhare@fb.com>
Problem:

Consider a scenario where an application (adapter host) using SAI spec X
(say) warmboots to an application using new SAI spec Y (say).

If an enum is extended between X and Y with fields inserted "in between"
rather than "at the end" of the enum, the old fields can get "renumbered".
However, the SAI implementation (adapter) continues to return old enums
for previously returned objects thereby breaking the warmboot.

Fix this by moving  the newly inserted fields "to the end"
(this could mean at the end of _END block) to keep the enum field values
the same.

Specifically, below patch was first introduced in v1.7.0 and inserted enum fields 'in betweeen':

opencomputeproject@21bdb48

- In 1.6.5: SAI_NATIVE_HASH_FIELD_L4_SRC_PORT is 7, SAI_NATIVE_HASH_FIELD_L4_DST_PORT is 8.
- In 1.8.1: SAI_NATIVE_HASH_FIELD_L4_SRC_PORT is 15, SAI_NATIVE_HASH_FIELD_L4_DST_PORT is 16.

If the application configured SAI_NATIVE_HASH_FIELD_L4_SRC_PORT (7)
prior to warmboot, even after warmboot, a query returns 7 which no
longer means SAI_NATIVE_HASH_FIELD_L4_SRC_PORT in 1.8.1.

This patch fixes it by moving all the newly introduced ACL fields to the
end.

Signed-off-by: zecheng@fb.com
Signed-off-by: Shrikrishna Khare <skhare@fb.com>
@shri-khare shri-khare marked this pull request as draft June 15, 2021 16:02
@shri-khare shri-khare marked this pull request as ready for review June 15, 2021 16:05
@shri-khare shri-khare mentioned this pull request Jun 15, 2021
Copy link
Collaborator

@kcudnik kcudnik left a comment

Choose a reason for hiding this comment

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

you need to discuss this on weekly SAI meeting, this seems like a lot of shuffling and seems not binary backward compatible

@kcudnik
Copy link
Collaborator

kcudnik commented Aug 26, 2021

Closing since #1259 was merged

@kcudnik kcudnik closed this Aug 26, 2021
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.

2 participants