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.

Sign up for GitHub

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

Jump to bottom

generic_config_updater: Add Rack: Issues #2007

Closed
renukamanavalan opened this issue Jan 13, 2022 · 2 comments · Fixed by #2008
Closed

generic_config_updater: Add Rack: Issues #2007

renukamanavalan opened this issue Jan 13, 2022 · 2 comments · Fixed by #2008
Labels
generic-config-updater

Comments

@renukamanavalan
Copy link
Contributor

Description

Applying a generic patch for addRack fails for 2 reasons

  1. The new interfaces should be ordered to be added at the start before addition of any other object.
    Vice versa, for remove rack, the interface objects removal should be ordered to go as last.
  2. Do NOT split update/add of a single object into many as shown below.
    This blocks BGP coming up for this new IP.
    Sample:
    i/p:
    {
        "op": "add",
        "path": "/BGP_NEIGHBOR/10.0.0.33",
        "value": {
            "admin_status": "up",
            "asn": "64001",
            "holdtime": "10",
            "keepalive": "3",
            "local_addr": "10.0.0.32",
            "name": "ARISTA01T0",
            "nhopself": "0",
            "rrclient": "0"
        },
    {
        "op": "add",
        "path": "/BGP_NEIGHBOR/fc00::42",
        "value": {
            "admin_status": "up",
            "asn": "64001",
            "holdtime": "10",
            "keepalive": "3",
            "local_addr": "fc00::41",
            "name": "ARISTA01T0",
            "nhopself": "0",
            "rrclient": "0"
        }
    },

Patch Applier: Applying 16 changes in order:
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33", "value": {"admin_status": "up"}}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/asn", "value": "64001"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/holdtime", "value": "10"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/keepalive", "value": "3"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/local_addr", "value": "10.0.0.32"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/name", "value": "ARISTA01T0"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/nhopself", "value": "0"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/rrclient", "value": "0"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/fc00::42", "value": {"admin_status": "up"}}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/fc00::42/asn", "value": "64001"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/fc00::42/holdtime", "value": "10"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/fc00::42/keepalive", "value": "3"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/fc00::42/local_addr", "value": "fc00::41"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/fc00::42/name", "value": "ARISTA01T0"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/fc00::42/nhopself", "value": "0"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/fc00::42/rrclient", "value": "0"}]
},

Steps to reproduce the issue

  1. Create two config_db.json one with & one w/o a T0
  2. Load the config w/o T0; Ensure BGP is started for all (config bgp startup all)
  3. Use jsonpatch.JsonPatch.from_diff to create a patch (w/o to with) for addRack
  4. Use "config apply-patch " to apply the created patch
  5. Note: BGP session for the added port don't come up and never.

Describe the results you received

Describe the results you expected

Additional information you deem important (e.g. issue happens only occasionally)

Output of show version

(paste your output here)
@ghooo
Copy link
Contributor

ghooo commented Jan 13, 2022 •
edited
Loading

  1. This seems like a dependency issue:
    /BGP_NEIGHBOR/10.0.0.32 is actually referring to INTERFACE/Ethernet64|10.0.0.32/31. So if we can get the correct dependency from yang models, we always add the INTERFACE first, then the BGP_NEIGHBOR. And in case of deletion we will delete the BGP_NEIGHBOR first then the INTERFACE
  2. What if we turn-up the BGP_NEIGHBOR admin up at the end, would this work? if so I think this is the same problem as Generic_patch: AddRack: Port should be turned as admin up as last change #1930 where modification of BGP_NEIGHBOR requires it go down first then up at the end.

@renukamanavalan @qiluo-msft thoughts?

@ghooo ghooo mentioned this issue Jan 14, 2022
Merged
@ghooo
Copy link
Contributor

ghooo commented Jan 15, 2022

The problem is in splitting adding the neighbor in multiple steps:

Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33", "value": {"admin_status": "up"}}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/asn", "value": "64001"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/holdtime", "value": "10"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/keepalive", "value": "3"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/local_addr", "value": "10.0.0.32"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/name", "value": "ARISTA01T0"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/nhopself", "value": "0"}]
Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/rrclient", "value": "0"}]

The reason is all fields in a neighbor are create-only field, i.e. they can only be created when the parent is created but they cannot be updated.

So if we start with

Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33", "value": {"admin_status": "up"}}]

Then directly add the other fields e.g.:

Patch Applier: * [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.33/asn", "value": "64001"}]

This change does not take effect. Hence marking the fields with create-only solves the problem.

ghooo added a commit that referenced this issue Jan 21, 2022
@ghooo
[GCU] Mark children of bgp_neighbor as create-only (#2008)
01dfb9c 
#### What I did

Fixes #2007 

Most of the children of `/BGP_NEIGHBOR/*` except `admin_status` are create-only field i.e. they can only be created with the neighbor but cannot be modified later.

Validated each attribute is read-only by the following steps:
* Delete a neighbor
* Add the neighbor back without the attribute under test e.g. `holdtime`
* show running config for the neighbor
* show neighbor config using `show ip bgp neighbor <ip>`
* Add just the attribute under test e.g. `holdtime`
* show running config for the neighbor -- we can see the attribute is added
* show neighbor config using `show ip bgp neighbor <ip>` -- we can see the attribute change did not take effect

Example for `holdtime`:
```sh
admin@vlab-01:~$ sudo config apply-patch remove-bgp-neighbor.json -i '' 
.
.
.
Patch applied successfully.
admin@vlab-01:~$ sudo config apply-patch remove-bgp-neighbor.json -i ''
.
.
.
Error: can't remove a non-existent object '10.0.0.57'
admin@vlab-01:~$ sudo config apply-patch add-bgp-neighbor-without-holdtime.json -i ''
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.57", "value": {"admin_status": "up", "asn": "64600", "keepalive": "3", "local_addr": "10.0.0.56", "name": "ARISTA01T1", "nhopself": "0", "rrclient": "0"}}]
.
.
.
Patch applied successfully.
admin@vlab-01:~$ show runningconfiguration all | grep 10.0.0.57 -A8
        "10.0.0.57": {
            "admin_status": "up",
            "asn": "64600",
            "keepalive": "3",
            "local_addr": "10.0.0.56",
            "name": "ARISTA01T1",
            "nhopself": "0",
            "rrclient": "0"
        },
admin@vlab-01:~$ show ip bgp neighbors 10.0.0.57
.
.
. 
  Hold time is 180, keepalive interval is 3 seconds
.
.
. 
admin@vlab-01:~$ sudo config apply-patch add-holdtime.json -i ''
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.57/holdtime", "value": "10"}]
.
.
. 
Patch applied successfully.
admin@vlab-01:~$ show runningconfiguration all | grep 10.0.0.57 -A10
        "10.0.0.57": {
            "admin_status": "up",
            "asn": "64600",
            "holdtime": "10",
            "keepalive": "3",
            "local_addr": "10.0.0.56",
            "name": "ARISTA01T1",
            "nhopself": "0",
            "rrclient": "0"
        },
        "10.0.0.59": {
admin@vlab-01:~$ show ip bgp neighbors 10.0.0.57
BGP neighbor is 10.0.0.57, remote AS 64600, local AS 65100, external link
.
.
. 
  Hold time is 180, keepalive interval is 3 seconds
.
.
. 
admin@vlab-01:~$ 
```

Also added a validation to `create-only` fields to reject moves that add their parents without them, because we would have to delete their parents again later and add it back. There is no point.
Example assume we have 2 fields marked with create-only namely x,y and they are under c. 
The patch would be:
```
{"op":"add", "path":"/a/b/c", "value":{"x":"value_x", "y":"value_y"}}
```
The generated moves would be:
```
{"op":"add", "path":"/a/b/c", "value":{"x":"value_x"}}
{"op":"remove", "path":"/a/b/c"}
{"op":"add", "path":"/a/b/c", "value":{"x":"value_x", "y":"value_y"}}
```

There is no point of the first 2 moves, because the `y` is create only and it will require the object to be deleted again then added. 


#### How I did it
Marked the fields as create only

#### How to verify it
unit-test

#### Previous command output (if the output of a command-line utility has changed)

#### New command output (if the output of a command-line utility has changed)
@ghooo ghooo mentioned this issue Jan 25, 2022
Merged
4 tasks
judyjoseph pushed a commit that referenced this issue Jan 31, 2022
@ghooo @judyjoseph
[GCU] Mark children of bgp_neighbor as create-only (#2008)
e433a02 
#### What I did

Fixes #2007 

Most of the children of `/BGP_NEIGHBOR/*` except `admin_status` are create-only field i.e. they can only be created with the neighbor but cannot be modified later.

Validated each attribute is read-only by the following steps:
* Delete a neighbor
* Add the neighbor back without the attribute under test e.g. `holdtime`
* show running config for the neighbor
* show neighbor config using `show ip bgp neighbor <ip>`
* Add just the attribute under test e.g. `holdtime`
* show running config for the neighbor -- we can see the attribute is added
* show neighbor config using `show ip bgp neighbor <ip>` -- we can see the attribute change did not take effect

Example for `holdtime`:
```sh
admin@vlab-01:~$ sudo config apply-patch remove-bgp-neighbor.json -i '' 
.
.
.
Patch applied successfully.
admin@vlab-01:~$ sudo config apply-patch remove-bgp-neighbor.json -i ''
.
.
.
Error: can't remove a non-existent object '10.0.0.57'
admin@vlab-01:~$ sudo config apply-patch add-bgp-neighbor-without-holdtime.json -i ''
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.57", "value": {"admin_status": "up", "asn": "64600", "keepalive": "3", "local_addr": "10.0.0.56", "name": "ARISTA01T1", "nhopself": "0", "rrclient": "0"}}]
.
.
.
Patch applied successfully.
admin@vlab-01:~$ show runningconfiguration all | grep 10.0.0.57 -A8
        "10.0.0.57": {
            "admin_status": "up",
            "asn": "64600",
            "keepalive": "3",
            "local_addr": "10.0.0.56",
            "name": "ARISTA01T1",
            "nhopself": "0",
            "rrclient": "0"
        },
admin@vlab-01:~$ show ip bgp neighbors 10.0.0.57
.
.
. 
  Hold time is 180, keepalive interval is 3 seconds
.
.
. 
admin@vlab-01:~$ sudo config apply-patch add-holdtime.json -i ''
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.57/holdtime", "value": "10"}]
.
.
. 
Patch applied successfully.
admin@vlab-01:~$ show runningconfiguration all | grep 10.0.0.57 -A10
        "10.0.0.57": {
            "admin_status": "up",
            "asn": "64600",
            "holdtime": "10",
            "keepalive": "3",
            "local_addr": "10.0.0.56",
            "name": "ARISTA01T1",
            "nhopself": "0",
            "rrclient": "0"
        },
        "10.0.0.59": {
admin@vlab-01:~$ show ip bgp neighbors 10.0.0.57
BGP neighbor is 10.0.0.57, remote AS 64600, local AS 65100, external link
.
.
. 
  Hold time is 180, keepalive interval is 3 seconds
.
.
. 
admin@vlab-01:~$ 
```

Also added a validation to `create-only` fields to reject moves that add their parents without them, because we would have to delete their parents again later and add it back. There is no point.
Example assume we have 2 fields marked with create-only namely x,y and they are under c. 
The patch would be:
```
{"op":"add", "path":"/a/b/c", "value":{"x":"value_x", "y":"value_y"}}
```
The generated moves would be:
```
{"op":"add", "path":"/a/b/c", "value":{"x":"value_x"}}
{"op":"remove", "path":"/a/b/c"}
{"op":"add", "path":"/a/b/c", "value":{"x":"value_x", "y":"value_y"}}
```

There is no point of the first 2 moves, because the `y` is create only and it will require the object to be deleted again then added. 


#### How I did it
Marked the fields as create only

#### How to verify it
unit-test

#### Previous command output (if the output of a command-line utility has changed)

#### New command output (if the output of a command-line utility has changed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
generic-config-updater
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[GCU] Mark children of bgp_neighbor as create-only ghooo/sonic-utilities
2 participants
@ghooo @renukamanavalan