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

[TAM]: Remove mandatory flag on the attribute TAM DSCP #2133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Pterosaur
Copy link
Collaborator

@Pterosaur Pterosaur commented Jan 23, 2025

In some transport protocols, such as GENetlink, the attribute DSCP is meaningless.
So, remove the mandatory flag on the attribute TAM DSCP and use the default value 0 to indicate this protocol doesn't support the field DSCP.

@Pterosaur Pterosaur changed the title [TAM]: Remove mandatory flag on the attribute TAM DESP [TAM]: Remove mandatory flag on the attribute TAM DSCP Jan 23, 2025
@Pterosaur
Copy link
Collaborator Author

/azpw run

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 24, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Pterosaur Pterosaur force-pushed the remove_mandatory_tam_dscp branch from 68ddcdb to 65ef6d5 Compare January 24, 2025 21:24
@Pterosaur
Copy link
Collaborator Author

/azp run

Copy link

Commenter does not have sufficient privileges for PR 2133 in repo opencomputeproject/SAI

@Pterosaur
Copy link
Collaborator Author

/azp run

Copy link

Commenter does not have sufficient privileges for PR 2133 in repo opencomputeproject/SAI

@Pterosaur
Copy link
Collaborator Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@r12f
Copy link
Contributor

r12f commented Jan 27, 2025

makes sense! Thanks Ze!

@Pterosaur
Copy link
Collaborator Author

@kcudnik Could you please help to merge this PR?

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 27, 2025

was this already discussed on SAI community meeting and approved ?

@Pterosaur
Copy link
Collaborator Author

Pterosaur commented Jan 27, 2025

was this already discussed on SAI community meeting and approved ?

Not yet, but this is proposed by a vendor and we both think it's reasonable and straightforward. Because the original API design was already adapted the GENetlink few years ago, and it is obvious that GENetlink doesn't have DSCP field. Right now, vendors need to implement this scenario and this issue is revealed.

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 27, 2025

agree, and this change is backward compatible, but i think every change to api should be discussed on team meeting, is this time sensitive to merge this soon ?

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.

5 participants