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

[portorch]: Skip to create port if the lane set isn't available in ASIC #1923

Merged
merged 7 commits into from
Oct 12, 2021

Conversation

Pterosaur
Copy link
Contributor

@Pterosaur Pterosaur commented Sep 22, 2021

Signed-off-by: Ze Gan ganze718@gmail.com

What I did
To support partial ports in orchagent by skipping the failure of adding port.

Why I did it
In SONiC KVM scenario, we don't always enable all ports in the qemu configuration.

How I verified it
Start a SONiC KVM with partial ports enabled. The qemu xml configuration like the following that I only enabled two ports.
image

This PR is related with: sonic-net/sonic-sairedis#938

@Pterosaur Pterosaur force-pushed the fix_partial_ports branch 3 times, most recently from 43ed544 to 7eb4931 Compare September 22, 2021 16:41
@prsunny
Copy link
Collaborator

prsunny commented Sep 22, 2021

Can you provide details in description?

@Pterosaur Pterosaur force-pushed the fix_partial_ports branch 2 times, most recently from 2761376 to 5e481d8 Compare September 23, 2021 02:42
@Pterosaur
Copy link
Contributor Author

Can you provide details in description?

Sure, I'm testing it. If this PR is available, I will add more description about it.

Signed-off-by: Ze Gan <ganze718@gmail.com>
Signed-off-by: Ze Gan <ganze718@gmail.com>
@Pterosaur Pterosaur changed the title Skip to create port if the lane set isn't available in ASIC [portorch]: Skip to create port if the lane set isn't available in ASIC Sep 23, 2021
@Pterosaur Pterosaur marked this pull request as ready for review September 23, 2021 14:24
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
@Pterosaur Pterosaur marked this pull request as draft October 2, 2021 02:29
Signed-off-by: Ze Gan <ganze718@gmail.com>
Signed-off-by: Ze Gan <ganze718@gmail.com>
@lguohan
Copy link
Contributor

lguohan commented Oct 6, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Pterosaur Pterosaur marked this pull request as ready for review October 8, 2021 15:08
@sonic-net sonic-net deleted a comment from azure-pipelines bot Oct 11, 2021
@sonic-net sonic-net deleted a comment from azure-pipelines bot Oct 11, 2021
@Pterosaur
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Pterosaur
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Pterosaur Pterosaur merged commit fd0cafe into sonic-net:master Oct 12, 2021
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…onic-net#1923)

#### What I did
Fixing issue sonic-net#1908 

#### How I did it
- When the given patch produces empty-table, reject it since ConfigDb cannot show empty tables. Achieved that by:
  - Adding a validation step before patch-sorting
- The patch-sorter should not produce steps which result in empty-tables because it is not possible in ConfigDb, we should instead delete the whole table. Achieved that by:
  - Adding a new validator to reject moves producing empty tables
  - No need to add a generator for deleting the whole table, current generators take care of that. They do by the following:
    - The move to empty a table is rejected by `NoEmptyTableMoveValidator`
    - The previous rejected move is used to generate moves using `UpperLevelMoveExtender` which produces either
      - Remove move for parent -- This is good, we are removing the table
      - Replace move for parent -- This later on will be extended to a Delete move using `DeleteInsteadOfReplaceMoveExtender`

The generators/validators are documented in the `PatchSorter.py`, check the documentation out.

#### How to verify it
unit-test

#### Previous command output (if the output of a command-line utility has changed)
```sh
admin@vlab-01:~$ sudo config apply-patch empty-a-table.json-patch -v
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "replace", "path": "/DEVICE_METADATA", "value": {}}]
Patch Applier: Validating patch is not making changes to tables without YANG models.
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config according to YANG models.
... sonig-yang-mgmt logs
Patch Applier: Sorting patch updates.
... sonig-yang-mgmt logs
Patch Applier: The patch was sorted into 11 changes:
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/bgp_asn"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/buffer_model"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/default_bgp_status"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/default_pfcwd_status"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/deployment_id"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/docker_routing_config_mode"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/hostname"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/hwsku"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/mac"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/platform"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost"}]
Patch Applier: Applying changes in order.
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: After applying patch to config, there are still some parts not updated
admin@vlab-01:~$ 
```

The above error occurs because post the update, the whole `DEVICE_METADATA` table is deleted, not just showing as empty i.e. `"DEVICE_METADATA": {}`

#### New command output (if the output of a command-line utility has changed)
```
admin@vlab-01:~$ sudo config apply-patch empty-a-table.json-patch -v
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "replace", "path": "/DEVICE_METADATA", "value": {}}]
Patch Applier: Validating patch is not making changes to tables without YANG models.
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config does not have empty tables, since they do not show up in ConfigDb.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: Given patch is not valid because it will result in empty tables which is not allowed in ConfigDb. Table: DEVICE_METADATA
admin@vlab-01:~$ 
```
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.

3 participants