[show] Add bgpraw to show run all

[wen587]

What I did

Add bgpraw output to show runningconfiguration all

Requirements:
1. current `show runningconfig` will print all the ConfigDB in a json format, we need to add a new key-value into the json output "bgpraw" with a long string value
2. The long string value should be the output of `vtysh -c "show run"`. It is normally multiline string, may include special characters like \". Need to make sure the escaping properly
3. We do not need to insert the key-value into ConfigDB is not existing there
4. If ConfigDB already has the key-value, we do not need to override it by vtysh command output
5. Not break multi-asic use

How I did it

Generate bgpraw output then append it to show runnningconfiguration all's output

How to verify it

Mannual test

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

admin@vlab-01:~$ show run all
{
    "ACL_TABLE": {
......
    "WRED_PROFILE": {
        "AZURE_LOSSLESS": {
            "ecn": "ecn_all",
            "green_drop_probability": "5",
            "green_max_threshold": "2097152",
            "green_min_threshold": "1048576",
            "red_drop_probability": "5",
            "red_max_threshold": "2097152",
            "red_min_threshold": "1048576",
            "wred_green_enable": "true",
            "wred_red_enable": "true",
            "wred_yellow_enable": "true",
            "yellow_drop_probability": "5",
            "yellow_max_threshold": "2097152",
            "yellow_min_threshold": "1048576"
        }
    }
}

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

admin@vlab-01:~$ show run all
{
    "ACL_TABLE": {
......
    "WRED_PROFILE": {
        "AZURE_LOSSLESS": {
            "ecn": "ecn_all",
            "green_drop_probability": "5",
            "green_max_threshold": "2097152",
            "green_min_threshold": "1048576",
            "red_drop_probability": "5",
            "red_max_threshold": "2097152",
            "red_min_threshold": "1048576",
            "wred_green_enable": "true",
            "wred_red_enable": "true",
            "wred_yellow_enable": "true",
            "yellow_drop_probability": "5",
            "yellow_max_threshold": "2097152",
            "yellow_min_threshold": "1048576"
        }
    },
    "bgpraw": "Building configuration...\n\nCurrent configuration......end\n"
}

[qiluo-msft]

sudo

Could you check if really need sudo? #Closed

[arlakshm]

I think if we use rvtysh we dont need need sudo. I think if we use sudo then the user has have RW privilages

[ganglyu]

Should we check the output of run_command?

[wen587]

You mean the format of output or bgpraw in output?

[ganglyu]

geoffrey@vm-04:~$ python3
Python 3.6.9 (default, Nov 25 2022, 14:10:45)
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.

import json
json.loads("")
Traceback (most recent call last):
File "", line 1, in
File "/usr/lib/python3.6/json/init.py", line 354, in loads
return _default_decoder.decode(s)
File "/usr/lib/python3.6/json/decoder.py", line 339, in decode
obj, end = self.raw_decode(s, idx=_w(s, 0).end())
File "/usr/lib/python3.6/json/decoder.py", line 357, in raw_decode
raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

[wen587]

Maybe that should be checked in sonic-cfggen?
sonic-cfggen guarantees that --print-data could be json.dump.

[qiluo-msft]

display_cmd

run_command will internally call click.echo() or sys.exit(), so there will be side-effect on corner cases. #Closed

[arlakshm]

as comments

[arlakshm]

I think if we use rvtysh we dont need need sudo. I think if we use sudo then the user has have RW privilages

[arlakshm]

if the bgp container is down, bgpraw will contain error messge. Do we want to store that or make output['bgpraw'] empty?

[qiluo-msft]

Empty running-config value for bgp down is a good suggestion.

[wen587]

The run_command was calling subprocess. The output will be empty though subprocess calling is error.
So, the bgpraw will be empty if bgp container is down.

[qiluo-msft]

stdout

suggest function return (stdout, rc). After calling this function, check rc first. #Closed

[wen587]

@qiluo-msft If we follow the original behavior, we don't need to check the rc. As it will be empty string by default if rc is non-zero.

def run_command(command, display_cmd=False, return_cmd=False):
    if display_cmd:
        click.echo(click.style("Command: ", fg='cyan') + click.style(command, fg='green'))

    # No conversion needed for intfutil commands as it already displays
    # both SONiC interface name and alias name for all interfaces.
    if clicommon.get_interface_naming_mode() == "alias" and not command.startswith("intfutil"):
        clicommon.run_command_in_alias_mode(command)
        raise sys.exit(0)

    proc = subprocess.Popen(command, shell=True, text=True, stdout=subprocess.PIPE)

    while True:
        if return_cmd:
            output = proc.communicate()[0]                   <===============
            return output

[qiluo-msft]

bgpraw

return (stdout, rc). After calling this function, check rc first. If rc non zero, output['bgpraw'] should be empty string. #Closed

[wen587]

Same as above, it will be empty string by default if rc is non-zero.

[yxieca]

This change causes build failure in 202205 branch. @wen587 will create separate PR for 202205 branch.

[yxieca]

@wen587 #2609 caused test failure when advancing submodule head. I reverted it for now.

[liuh-80]

Cherry-pick PR merged: #2639

[stephenxs]

Hi @wen587 @qiluo-msft @prsunny
I would like to understand the expected behaviour if BGP docker is not running. eg. during config reload or feature bgp is disabled?
We see it returns Error response from daemon: Container 34ab09feab75c467ec13dc1aa419ed2e0b15b9a35b2914396fcacd7d17f2492d is not running but I'm not sure whether there should be more user friendly output and the rest configuration should be displayed even if docker bgp is down.
Would you please clarify it?
Thanks.

@liat-grozovik @dgsudharsan FYI

[wen587]

Hi @stephenxs , it does print the rest config in this PR. I saw it is another PR break this behavior: #3148
It is calling run_bgp_show_command which will fail if bgp down.

[stephenxs]

Hi @stephenxs , it does print the rest config in this PR. I saw it is another PR break this behavior: #3148 It is calling run_bgp_show_command which will fail if bgp down.

Thank you @wen587 for clarifying it. I'll check PR #3148