Added route-advertise to bgp_af module

[stalabi1]

SUMMARY

I updated the sonic_bgp_af module to include route-advertise.

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

sonic_bgp_af

ADDITIONAL INFORMATION

bgp_af_regression_report.pdf

show_running_configuration_bgp_output.txt

bgp_af_log_output (6).txt

[kerry-meyer]

Corresponding to the addition of the "route_advertise_list" list to the argspec, the following line needs to be added to the "TEST_KEYS" list in the "plugins/module_utils/network/sonic/config/bgp_af/bgp_af.py" file:

{'route_advertise_list': {'advertise_afi_safi': ''}},

The TEST_KEYS list is needed for diff generation by the "get_diff" utility function. For each "list" in the configuration portion of the argspec, an entry is needed in the TEST_KEYS list to specify the key or keys that uniquely identify a given element of the list. In this case, the "advertise_afi_safi" key (ipv4 or ipv6) identifies the two possible elements in the "route_advertise_list' list.

A reqression test case to verify this functionality should also be provided: To do this, add configuration (with or without a route map) for both address families to confirm that this works correctly. Please also add a test case to delete the route_advertise configuration for one of the two configured AFs.

[kerry-meyer]

Test comments:

In the "tests/regression/roles/sonic_bgp_af/defaults/main.yml" file, please add the following case:

• Change the referenced route map for an advertise list entry.

• Please append to this PR the log output for a passing regression run. I would like to verify that the configuration changes that add/delete an AF entry from the advertise list within a given AS actually work as intended. I am concerned that the current cases may not really be working as expected because the key for the list items wasn't registered. It may be necessary to add new test cases that only change 'advertise list' properties to really test this. But before making any changes for this purpose, let's analyze the "before" and "after" outputs in the log for the test cases that do these types of add/delete.

[kerry-meyer]

All of the changes applied by the most recent commit look good to me, but I see a problem in the log output. Is the currently posted test log "with" or "without" the added TEST_KEY entry? If it's "with", I must be missing something in my analysis of the diffs: I see a problem happening in test_case_03 that needs to be investigated. It looks like, for the route_advertise testing, test case 03 is intended to add IPv6 route advertisements for IPv6 to both AS configurations. But, instead, it looks like it is replacing the already present IPv4 advertisement with the new IPv6 advertisement and also adding the new one a second time. From the "before" and "after" sections of the log for that test case:

Here's an example from bgp_as 52:

Before:

                        "afi": "l2vpn",
                        "dampening": null,
                        "max_path": null,
                        "network": null,
                        "redistribute": null,
                        "route_advertise_list": [
                            {
                                "advertise_afi": "ipv4",
                                "route_map": "rmap_reg1"
                            }

After

                        "afi": "l2vpn",
                        "dampening": null,
                        "max_path": null,
                        "network": null,
                        "redistribute": null,
                        "route_advertise_list": [
                            {
                                "advertise_afi": "ipv6",
                                "route_map": "rmap_reg2"
                            },
                            {
                                "advertise_afi": "ipv6",
                                "route_map": "rmap_reg2"

[kerry-meyer]

It looks like an additional code block is needed in the "get_modify_requests" function in the "plugins/module_utils/network/sonic/config/bgp_af/bgp_af.py" function to handle the case of adding or modifying an entry in the "route advertise" list.

[kerry-meyer]

The new commit to provide the revised "modify" handling for the route advertise list looks fine to me except for a minor change for which I am posting a comment (to get rid of an unnecessary additional input parameter).

The test log, though, doesn't look right. Every "gathered" instance show a value of "null" for 'route_advertise_list'.

The log seems to show the correct commands in the "invocation" section, but they must not be getting converted to correct curl commands. I'm still analyzing the code to see if I can figure out what's wrong, but wanted to go ahead and post this observation now so you can also take a look at the code to figure out the cause of the problem.

[kerry-meyer]

There is also a failing sanity test. Please fix this or respond to this comment with an explanation of why it can't be fixed.

[kerry-meyer]

I see two remaining problems that need to be fixed before merging this PR:

1. The "config" handling for adding (merging) a second "route advertise" AF is replacing the existing AF instead of just adding the new one. The logic in the config file for handling this case needs to be fixed. (I'll take a look at the existing logic to see if I can spot the problem and will post an update comment if I find it.)

2. The sanity check failures are better, but there's still one left: Try changing the "author" for the httpapi sonic.py plugin to your name (first last) or, if that doesn't work, to your Github ID.

[kerry-meyer]

The change set looks good with the most recent changes.

Approved.