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

[CBF] Added configuration templates to generate configs for CBF #8689

Merged
merged 4 commits into from
Oct 30, 2021

Conversation

Cosmin-Jinga-MS
Copy link
Contributor

@Cosmin-Jinga-MS Cosmin-Jinga-MS commented Sep 6, 2021

[build_templates]: Added default configuration file for CBF
[rules]: Added loading rule for CBF config

Why I did it

The CBF default config is required to load default start-up config on CBF capable platforms

How I did it

Added the default config files among the existing ones, mirroring the QOS implementation
Unit tests are in:
sonic-net/sonic-utilities#1799

How to verify it

Deploy SBI on a CBF capable platform(e.g. SODA)

Description for the changelog

Default CBF config files

Signed-off-by: v-cjinga@microsoft.com

[build_templates]: Added default configuration file for CBF

[rules]: Added loading rule for CBF config

Why I did it
 The CBF default config is required to load default start-up config on CBF capable platforms
How I did it
 Added the default config files among the existing ones, mirroring the QOS implementation
How to verify it
 Deploy SBI on a CBF capable platform(e.g. SODA)

Signed-off-by: v-cjinga@microsoft.com
@yozhao101
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yozhao101
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rlhui rlhui requested a review from abdosi October 5, 2021 16:53
@Cosmin-Jinga-MS
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Cosmin-Jinga-MS
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -0,0 +1,82 @@
{
"DSCP_TO_TC_MAP": {
Copy link
Contributor

Choose a reason for hiding this comment

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

DSCP_TO_TC_MAP is already used by qos_config.j2.

If the intention is to use the same mapping, no need to add here. QoS config generation will create the config for DSCP_TO_TC_MAP. Having in two places will result in race condition where the last config generation will overwrite the DSCP_TO_TC_MAP.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this DSCP_TO_FC_MAP ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a mistake I've made while updating the dscp mapping to cover all 64 associations. CBF has to have its own separate mapping. I've updated it to DSCP_TO_FC_MAP, as it should be.

@abdosi
Copy link
Contributor

abdosi commented Oct 21, 2021

can you please provide what does FC and CBF stands for ?

@abdosi
Copy link
Contributor

abdosi commented Oct 21, 2021

can you please provide what does FC and CBF stands for ?

Ok, I think we are referring Forwarding Class and Class Based Forwarding.

Bit this create confusion what is the difference between TC and FC ?

@Cosmin-Jinga-MS
Copy link
Contributor Author

@abdosi FC allows traffic engineering as described in https://github.com/Azure/SONiC/pull/796/files while TC allows splitting packets in queues for QoS

@smaheshm
Copy link
Contributor

@Cosmin-Jinga-MS Going forward we required all configs to have a corresponding yang model. (e.g. PR for QoS #7375 )

Changes look good to me.

When do you think you can create a yang model PR for:
"DSCP_TO_FC_MAP"
"EXP_TO_FC_MAP"

@smaheshm
Copy link
Contributor

@Cosmin-Jinga-MS Going forward we required all configs to have a corresponding yang model. (e.g. PR for QoS #7375 )

Changes look good to me.

When do you think you can create a yang model PR for: "DSCP_TO_FC_MAP" "EXP_TO_FC_MAP"

@Cosmin-Jinga-MS Created #9108 Can you assign this to yourself.

Copy link
Contributor

@smaheshm smaheshm left a comment

Choose a reason for hiding this comment

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

Looks good.

@smaheshm smaheshm changed the title Updated CBF config packaging [CBF] Added configuration templates to generate configs for CBF Oct 28, 2021
@@ -0,0 +1,82 @@
{
"DSCP_TO_FC_MAP": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the same as DSCP_TO_TC_MAP, or do they need to be same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mappings used for CBF are different from the ones used in QoS. It's intended we have a separate table in this case;
DSCP_TO_TC_MAP for qos
DSCP_TO_FC_MAP for cbf

@Cosmin-Jinga-MS
Copy link
Contributor Author

@Cosmin-Jinga-MS Going forward we required all configs to have a corresponding yang model. (e.g. PR for QoS #7375 )
Changes look good to me.
When do you think you can create a yang model PR for: "DSCP_TO_FC_MAP" "EXP_TO_FC_MAP"

@Cosmin-Jinga-MS Created #9108 Can you assign this to yourself.

I don't seem to have the rights to change assignees on that issue.
I'll start looking into creating the yang model today.

@smaheshm smaheshm merged commit dfc1697 into sonic-net:master Oct 30, 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.

6 participants