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

SONiC QOS Scheduler, WRED, Queue Yangs #7281

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

AshokDaparthi
Copy link
Contributor

@AshokDaparthi AshokDaparthi commented Apr 9, 2021

Why I did it

Created SONiC Yang model for QOS
Tables: SCHEDULER, WRED_PROFILE, QUEUE table.
How I did it

Defined Yang models for QOS based on Guideline doc:
https://github.com/Azure/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md
and
https://github.com/Azure/sonic-utilities/blob/master/doc/Command-Reference.md
How to verify it

sonic_yang_models package build

Dependent pull requests:
#7752 - To modify platfrom files
sonic-net/sonic-utilities#1626 - DB migration
sonic-net/sonic-swss#1754 - swss change to remove ABNF format

@lguohan lguohan added the YANG YANG model related changes label Apr 10, 2021
@anshuv-mfst
Copy link

Hi @neethajohn - could you please take a look, thanks.


leaf scheduler {
type leafref {
path "/sch:sonic-scheduler/sch:SCHEDULER/sch:SCHEDULER_LIST/sch:name"; //Reference to SCHEDULER table
Copy link
Collaborator

@nazariig nazariig Apr 28, 2021

Choose a reason for hiding this comment

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

@AshokDaparthi how do you expect to handle key reference notation from swss-schema.md?

It seems that current YANG model won't be able to handle existing config_db.json:
Example:

"QUEUE": {
    "Ethernet4|0": {
        "scheduler": "[SCHEDULER|scheduler.0]"
    },
    "Ethernet4|1": {
        "scheduler": "[SCHEDULER|scheduler.0]"
    },
    "Ethernet4|2": {
        "scheduler": "[SCHEDULER|scheduler.0]"
    },
    "Ethernet4|3": {
        "wred_profile": "[WRED_PROFILE|AZURE_LOSSLESS]",
        "scheduler": "[SCHEDULER|scheduler.1]"
    }
}

"SCHEDULER": {
    "scheduler.0": {
        "type": "DWRR",
        "weight": "14"
    },
    "scheduler.1": {
        "type": "DWRR",
        "weight": "15"
    }
}

"WRED_PROFILE": {
    "AZURE_LOSSLESS": {
        "red_max_threshold": "2097152",
        "wred_green_enable": "true",
        "ecn": "ecn_all",
        "green_min_threshold": "1048576",
        "red_min_threshold": "1048576",
        "wred_yellow_enable": "true",
        "yellow_min_threshold": "1048576",
        "green_max_threshold": "2097152",
        "green_drop_probability": "5",
        "yellow_max_threshold": "2097152",
        "wred_red_enable": "true",
        "yellow_drop_probability": "5",
        "red_drop_probability": "5"
    }
}
root@sonic:/home/admin# redis-cli -n 4 HGETALL 'QUEUE|Ethernet4|3'
1) "scheduler"
2) "[SCHEDULER|scheduler.1]"
3) "wred_profile"
4) "[WRED_PROFILE|AZURE_LOSSLESS]"

Which means that for scheduler/wred we expect to match:

  1. scheduler -> scheduler.0
  2. wred -> AZURE_LOSSLESS

and not:

  1. scheduler -> SCHEDULER|scheduler.0
  2. wred -> WRED_PROFILE|AZURE_LOSSLESS

Copy link
Collaborator

Choose a reason for hiding this comment

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

@praveen-li can you please elaborate what should be the appropriate YANG model for SONiC ABNF key reference?

How do we expect to handle this one?

"BUFFER_PORT_EGRESS_PROFILE_LIST": {
    "Ethernet8": {
        "profile_list": "[BUFFER_PROFILE|egress_lossless_profile],[BUFFER_PROFILE|egress_lossy_profile]"
    },
root@sonic:/home/admin# redis-cli -n 4 HGETALL 'BUFFER_PORT_EGRESS_PROFILE_LIST|Ethernet8'
1) "profile_list"
2) "[BUFFER_PROFILE|egress_lossless_profile],[BUFFER_PROFILE|egress_lossy_profile]"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nazariig, @praveen-li - We can discuss this over review call, i am not sure pyang will take care leafref validation even if we give ABNF format. Else we should convert to leafref to pattern. But we will miss in this case leafref check's in YANG. As of now i am not sure is there anyway to resent in YANG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nazariig - Back end changes to remove table ABNF format (example "[BUFFER_PROFILE|" )from field value is merged in latest master.

@@ -114,4 +114,15 @@ module sonic-types {
pattern "percentage|used|free|PERCENTAGE|USED|FREE";
}
}

container operation {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is being discussed in another pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed operation

leaf yellow_max_threshold {
type uint64;
units bytes;
must "(/stypes:operation/stypes:operation = 'DELETE') or (current() >= current()/../yellow_min_threshold)" {
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 operation check needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed operation based checks

lguohan pushed a commit to sonic-net/sonic-utilities that referenced this pull request Sep 2, 2021
Qos tables in config db and app db used ABNF format i.e "[TABLE_NAME|name] to refer fieldvalue other qos tables.

Example:
Config DB:
"Ethernet92|3": {
"scheduler": "[SCHEDULER|scheduler.1]",
"wred_profile": "[WRED_PROFILE|AZURE_LOSSLESS]"
},
"Ethernet0|0": {
"profile": "[BUFFER_PROFILE|ingress_lossy_profile]"
},
"Ethernet0": {
"dscp_to_tc_map": "[DSCP_TO_TC_MAP|AZURE]",
"pfc_enable": "3,4",
"pfc_to_queue_map": "[MAP_PFC_PRIORITY_TO_QUEUE|AZURE]",
"tc_to_pg_map": "[TC_TO_PRIORITY_GROUP_MAP|AZURE]",
"tc_to_queue_map": "[TC_TO_QUEUE_MAP|AZURE]"
},

AppDB:
"BUFFER_QUEUE_TABLE:Ethernet88:3-4": {
"profile": "[BUFFER_PROFILE_TABLE:egress_lossless_profile]"
},

1#This format is not consistent with other DB schema followed in sonic.
2# Added db_migrator.py case to  change from old format in config_db and appl_db  to new format. 
3#Modified the test case 

Dependent pull requests: 
sonic-net/sonic-buildimage#7752  - To modify platfrom files 
sonic-net/sonic-buildimage#7281 - Yang model 
sonic-net/sonic-swss#1754    - swss change to remove ABNF format
@zhangyanzhao
Copy link
Collaborator

Depends on sonic-net/sonic-utilities#1626

@zhangyanzhao
Copy link
Collaborator

Dependency sonic-net/sonic-utilities#1626 is merged

@smaheshm
Copy link
Contributor

smaheshm commented Sep 9, 2021

@AshokDaparthi Can you update the PR with suggestions. I see couple of open items, do they still need to get resolved? Are you waiting for inputs from the community?

@AshokDaparthi
Copy link
Contributor Author

@smaheshm - Main issues blocking this PR is comments in sonic-queue.yang, Which needs DB format change. Below are PR raised sonic-net/sonic-utilities#1626
sonic-net/sonic-swss#1754
#7752
sonic-net/sonic-mgmt#3824 There is problems to pipeline testing because of all are interlinked changes and as per last call sonic-mgmt test cases needs both format in same code base based on release. It needs rework and find way to pass different config_db.json from yaml files with both new and old formats.

neethajohn pushed a commit that referenced this pull request Sep 28, 2021
Depends on sonic-net/sonic-utilities#1626
Depends on sonic-net/sonic-swss#1754

QOS tables in config db used ABNF format i.e "[TABLE_NAME|name] to refer fieldvalue to other qos tables.

Example:
Config DB:
"Ethernet92|3": {
"scheduler": "[SCHEDULER|scheduler.1]",
"wred_profile": "[WRED_PROFILE|AZURE_LOSSLESS]"
},
"Ethernet0|0": {
"profile": "[BUFFER_PROFILE|ingress_lossy_profile]"
},
"Ethernet0": {
"dscp_to_tc_map": "[DSCP_TO_TC_MAP|AZURE]",
"pfc_enable": "3,4",
"pfc_to_queue_map": "[MAP_PFC_PRIORITY_TO_QUEUE|AZURE]",
"tc_to_pg_map": "[TC_TO_PRIORITY_GROUP_MAP|AZURE]",
"tc_to_queue_map": "[TC_TO_QUEUE_MAP|AZURE]"
},

This format is not consistent with other DB schema followed in sonic.
And also this reference in DB is not required, This is taken care by YANG "leafref".

Removed this format from all platform files to consistent with other sonic db schema.
Example:
"Ethernet92|3": {
"scheduler": "scheduler.1",
"wred_profile": "AZURE_LOSSLESS"
},

Dependent pull requests:
#7752 - To modify platfrom files
#7281 - Yang model
sonic-net/sonic-utilities#1626 - DB migration
sonic-net/sonic-swss#1754 - swss change to remove ABNF format
@smaheshm
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AshokDaparthi
Copy link
Contributor Author

@smaheshm - Could you please help to see why it failing, i ran sonic-yang-models, and sonic-yang-mgmt python wheels. it success for me.

@smaheshm
Copy link
Contributor

smaheshm commented Oct 4, 2021

@smaheshm - Could you please help to see why it failing, i ran sonic-yang-models, and sonic-yang-mgmt python wheels. it success for me.

looks like some process crashed, seen in other tests as well. I don't know if anyone's working on it. For I'd suggest update your branch and re-trigger the pipeline.

@smaheshm smaheshm closed this Oct 4, 2021
@smaheshm smaheshm reopened this Oct 4, 2021
@zhangyanzhao
Copy link
Collaborator

Build failure need some help from people who can check the logs, looks like impacted by warmreboot test. Rajesh will re-push today

@smaheshm
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AshokDaparthi
Copy link
Contributor Author

@smaheshm - How man times i restart i see same error kvmtest fails. Could you please help to see why it fails.

@AshokDaparthi
Copy link
Contributor Author

@smaheshm, @lguohan - checks are passed finally, could you please review or merge it. since it already approved once.

@smaheshm smaheshm merged commit a99d78d into sonic-net:master Oct 18, 2021
dgsudharsan added a commit to dgsudharsan/sonic-buildimage that referenced this pull request Oct 29, 2021
dgsudharsan added a commit to dgsudharsan/sonic-buildimage that referenced this pull request Oct 29, 2021
malletvapid23 added a commit to malletvapid23/Sonic-Utility that referenced this pull request Aug 3, 2023
Qos tables in config db and app db used ABNF format i.e "[TABLE_NAME|name] to refer fieldvalue other qos tables.

Example:
Config DB:
"Ethernet92|3": {
"scheduler": "[SCHEDULER|scheduler.1]",
"wred_profile": "[WRED_PROFILE|AZURE_LOSSLESS]"
},
"Ethernet0|0": {
"profile": "[BUFFER_PROFILE|ingress_lossy_profile]"
},
"Ethernet0": {
"dscp_to_tc_map": "[DSCP_TO_TC_MAP|AZURE]",
"pfc_enable": "3,4",
"pfc_to_queue_map": "[MAP_PFC_PRIORITY_TO_QUEUE|AZURE]",
"tc_to_pg_map": "[TC_TO_PRIORITY_GROUP_MAP|AZURE]",
"tc_to_queue_map": "[TC_TO_QUEUE_MAP|AZURE]"
},

AppDB:
"BUFFER_QUEUE_TABLE:Ethernet88:3-4": {
"profile": "[BUFFER_PROFILE_TABLE:egress_lossless_profile]"
},

1#This format is not consistent with other DB schema followed in sonic.
2# Added db_migrator.py case to  change from old format in config_db and appl_db  to new format. 
3#Modified the test case 

Dependent pull requests: 
sonic-net/sonic-buildimage#7752  - To modify platfrom files 
sonic-net/sonic-buildimage#7281 - Yang model 
sonic-net/sonic-swss#1754    - swss change to remove ABNF format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
YANG YANG model related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants