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

Enum reordering #1249

Closed

Conversation

shri-khare
Copy link
Contributor

No description provided.

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 9, 2021 19:14
@kcudnik
Copy link
Collaborator

kcudnik commented Jun 10, 2021

what's wrong with current order? why this change?

@tushar-ty
Copy link
Collaborator

what's wrong with current order? why this change?

enum type values like SAI_NATIVE_HASH_FIELD_SRC_IPV4 should not change from one spec version to another as that can break any warm upgrade (ISSU).

B/w v1.6.x and v1.8.x some enums are inserted in the middle vs at the end.

subtype enums should always be added to the end of the list.

@rlhui
Copy link
Collaborator

rlhui commented Jun 15, 2021

will you be able to add description of this PR. Currently it says "No description provided."

@shri-khare
Copy link
Contributor Author

While trying to fix the sanity checks failing here, I accidentally ended up posting another pull request #1250. That has the latest changes with all the sanity checks passing. Am closing this request thus in favor of pull request 1250.

@shri-khare shri-khare closed this Jun 15, 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.

4 participants