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

[yang] Unable to apply config from sonic-telemetry_client.yang #16356

Open
faraazbrcm opened this issue Aug 31, 2023 · 3 comments
Open

[yang] Unable to apply config from sonic-telemetry_client.yang #16356

faraazbrcm opened this issue Aug 31, 2023 · 3 comments
Assignees
Labels
Triaged this issue has been triaged YANG YANG model related changes

Comments

@faraazbrcm
Copy link
Contributor

faraazbrcm commented Aug 31, 2023

Description

sonic-telemetry_client.yang defines a Table level container with one container Global for static key(This is fine), but it also has two lists TELEMETRY_CLIENT_DS_LIST and TELEMETRY_CLIENT_SUB_LIST with same key count, this pattern is not recommended. As per the guideline we can only split the tables with lists with distinct number of keys (not with pattern and type).

Even sonic-utilities is failing to apply the configs.

Steps to reproduce the issue:

  1. Create JsonPatch. Sample JsonPatch:
[{"op": "add", "path": "/TELEMETRY_CLIENT/Subscription_A/paths", "value": "xyz"}]
  1. Using GCU with JsonPatch above
admin@sonic:~$ sudo config apply-patch  -v ./p5.json
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "add", "path": "/TELEMETRY_CLIENT/Subscription_A/paths", "value": "[192.168.1.1:8080](http://192.168.1.1:8080/)"}]
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating all JsonPatch operations are permitted on the specified fields
Patch Applier: Validating target config does not have empty tables, since they do not show up in ConfigDb.
Patch Applier: Sorting patch updates.
Patch Sorter - Strict: Validating patch is not making changes to tables without YANG models.
libyang[1]: Default value "" in the list key "port" is ignored. (/sonic-snmp:sonic-snmp/SNMP_AGENT_ADDRESS_CONFIG/SNMP_AGENT_ADDRESS_LIST)
libyang[1]: Default value "" in the list key "vrf_name" is ignored. (/sonic-snmp:sonic-snmp/SNMP_AGENT_ADDRESS_CONFIG/SNMP_AGENT_ADDRESS_LIST)
Patch Sorter - Strict: Validating target config according to YANG models.
Patch Sorter - Strict: Sorting patch updates.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: Path token not found.
  model: OrderedDict([('@name', 'TELEMETRY_CLIENT_DS_LIST'), ('key', OrderedDict([('@value', 'prefix')])), ('ordered-by', OrderedDict([('@value', 'user')])), ('leaf', [OrderedDict([('@name', 'prefix'), ('type', OrderedDict([('@name', 'string'), ('pattern', OrderedDict([('@value', 'DestinationGroup_.*')]))])), ('__isleafList', False)]), OrderedDict([('@name', 'dst_addr'), ('type', OrderedDict([('@name', 'ipv4-port')])), ('__isleafList', False)])])])
  token_index: 2
  path_tokens: ['TELEMETRY_CLIENT', 'Subscription_A', 'paths']
  config: {'report_interval': '200', 'paths': 'xyz'}

Describe the results you received:

Failed application of JsonPatch, unable to write the configs

Describe the results you expected:

Successful application of JsonPatch

Additional Information

The SONiC YANG guideline currently lacks specific rules concerning multiple LIST elements within a table-level container. At present, sonic-utility selects the first LIST found within such a container. This approach works correctly only when the number of keys for each LIST differs, as seen in sonic-interface. However, it fails when the LISTs have the same number of keys, as is the case in sonic-telemetry_client.yang.

To safeguard against future application issues, I've updated the guideline to include a new point specifically addressing TABLE keys when the content is divided into multiple lists. The changes are available in this PR.

@faraazbrcm faraazbrcm changed the title [yang] sonic-telemetry_client.yang does not align with ConfigDB Guidelines [yang] Unable to apply config from sonic-telemetry_client.yang Sep 13, 2023
@prgeor prgeor added YANG YANG model related changes Triaged this issue has been triaged labels Sep 13, 2023
@prgeor
Copy link
Contributor

prgeor commented Sep 13, 2023

@qiluo-msft can you please check the yang guidelines

@qiluo-msft
Copy link
Collaborator

@faraazbrcm Could you check which branches are impacted?

@wen587
Copy link
Contributor

wen587 commented Oct 18, 2023

Create a modification PR on this: #16861

qiluo-msft pushed a commit that referenced this issue Dec 16, 2023
### Why I did it
Github issue: #16356. The YANG definition breaks GCU feature.

We can either update sonic_yang and GCU's search algorithm to enable the same key count case or simply update YANG model to solve the issue.

The pros for update YANG model are it could solve the issue directly and we don't need to handle the complicate search algorithm in sonic_yang and GCU. This is the only YANG model that has this issue.

### How I did it
Combine two list into one. The previous YANG validation unit tests are still applicable.
#### How to verify it
Unit test and E2E test
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this issue Jan 4, 2024
### Why I did it
Github issue: sonic-net#16356. The YANG definition breaks GCU feature.

We can either update sonic_yang and GCU's search algorithm to enable the same key count case or simply update YANG model to solve the issue.

The pros for update YANG model are it could solve the issue directly and we don't need to handle the complicate search algorithm in sonic_yang and GCU. This is the only YANG model that has this issue.

### How I did it
Combine two list into one. The previous YANG validation unit tests are still applicable.
#### How to verify it
Unit test and E2E test
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this issue Feb 2, 2024
### Why I did it
Github issue: sonic-net#16356. The YANG definition breaks GCU feature.

We can either update sonic_yang and GCU's search algorithm to enable the same key count case or simply update YANG model to solve the issue.

The pros for update YANG model are it could solve the issue directly and we don't need to handle the complicate search algorithm in sonic_yang and GCU. This is the only YANG model that has this issue.

### How I did it
Combine two list into one. The previous YANG validation unit tests are still applicable.
#### How to verify it
Unit test and E2E test
mssonicbld pushed a commit that referenced this issue Feb 2, 2024
### Why I did it
Github issue: #16356. The YANG definition breaks GCU feature.

We can either update sonic_yang and GCU's search algorithm to enable the same key count case or simply update YANG model to solve the issue.

The pros for update YANG model are it could solve the issue directly and we don't need to handle the complicate search algorithm in sonic_yang and GCU. This is the only YANG model that has this issue.

### How I did it
Combine two list into one. The previous YANG validation unit tests are still applicable.
#### How to verify it
Unit test and E2E test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Triaged this issue has been triaged YANG YANG model related changes
Projects
None yet
Development

No branches or pull requests

4 participants