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

[sonic-netns-exec]: use "$@" to reflects all positional parameters as they were set initially #4375

Merged
merged 2 commits into from
Apr 7, 2020

Conversation

SuvarnaMeenakshi
Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi commented Apr 5, 2020

Signed-off-by: SuvarnaMeenakshi sumeenak@microsoft.com

fixes #4367
- What I did
Issue:
sonic-netns-exec fails to execute below command in swss.sh:

_function clean_up_tables()_
{
    sonic-netns-exec "$NET_NS" sonic-db-cli $1 EVAL "
    local tables = {$2}
    for i = 1, table.getn(tables) do
        local matches = redis.call('KEYS', tables[i])
        for j,name in ipairs(matches) do
            redis.call('DEL', name)
        end
    end" 0
}

This command fails with error " redis.exceptions.ResponseError: value is not an integer or out of range" .

Root cause:

When sonic-netns-exec executes the above function, argument passed to sonic-db-cli is NOT executed as a single script.

The argument is passed as separate keywords to sonic-db-cli, as below:

['EVAL', 'local', 'tables', '=', "{'PORT_TABLE*'}", 'for', 'i', '=', '1,', 'table.getn(tables)', 'do', 'local', 'matches', '=', "redis.call('KEYS',", 'tables[i])', 'for', 'j,name', 'in', 'ipairs(matches)', 'do', "redis.call('DEL',", 'name)', 'end', 'end', '0']

- How I did it
To make sure that the parameters are passed as they were set initially, fix sonic-netns-exec to use double quoted "$@", where "$@" is "$1" "$2" "$3" ... "${N}"

After fix, the argument passed to sonic-db-cli is as below:

Argument passed to sonic-db-cli:

['EVAL', "\n    local tables = {'PORT_TABLE*'}\n    for i = 1, table.getn(tables) do\n        local matches = redis.call('KEYS', tables[i])\n        for j,name in ipairs(matches) do\n            redis.call('DEL', name)\n        end\n    end", '0']

- How to verify it
Load VS image and create vs testbed with T0 topology.
Do a config reload -y
Run test_announce_routes.py - this was failing before fix as vlan1000 interface was down, after fix this passes.
- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

command with quotes can be executed.

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>
@lguohan
Copy link
Collaborator

lguohan commented Apr 5, 2020

the focus of the description should be why you need to quote here, why the command does not work without quote.

@lguohan
Copy link
Collaborator

lguohan commented Apr 5, 2020

btw, which command in swss.sh is the offending command?

@SuvarnaMeenakshi
Copy link
Contributor Author

btw, which command in swss.sh is the offending command?

in swss.sh clean_up_tables():
sonic-netns-exec "$NET_NS" sonic-db-cli $1 EVAL "
local tables = {$2}
for i = 1, table.getn(tables) do <… >

@SuvarnaMeenakshi
Copy link
Contributor Author

the focus of the description should be why you need to quote here, why the command does not work without quote.

Updated description and fix.

@lguohan
Copy link
Collaborator

lguohan commented Apr 6, 2020

@SuvarnaMeenakshi , can you use markdown format to format you description? it is really difficult to read and understand.

@SuvarnaMeenakshi
Copy link
Contributor Author

@SuvarnaMeenakshi , can you use markdown format to format you description? it is really difficult to read and understand.

Updated description.

@abdosi
Copy link
Contributor

abdosi commented Apr 6, 2020

btw, which command in swss.sh is the offending command?

in swss.sh clean_up_tables():
sonic-netns-exec "$NET_NS" sonic-db-cli $1 EVAL "
local tables = {$2}
for i = 1, table.getn(tables) do <… >

@lguohan: Instead of passing script as string may be using using lua script file will be cleaner here. We can pass file name instead of string.

…mpty or not.

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>
@lguohan lguohan changed the title [sonic-netns-exec]: Fix the script to make sure that any [sonic-netns-exec]: use "$@" to reflects all positional parameters as they were set initially Apr 7, 2020
@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Apr 7, 2020

The first line of the code in PR description looks weird.

@lguohan lguohan merged commit 2a59551 into sonic-net:master Apr 7, 2020
abdosi pushed a commit that referenced this pull request Apr 15, 2020
… they were set initially (#4375)

sonic-netns-exec fails to execute below command in swss.sh:

    sonic-netns-exec "$NET_NS" sonic-db-cli $1 EVAL "
    local tables = {$2}
    for i = 1, table.getn(tables) do
        local matches = redis.call('KEYS', tables[i])
        for j,name in ipairs(matches) do
            redis.call('DEL', name)
        end
    end" 0

This command fails with error " redis.exceptions.ResponseError: value is not an integer or out of range" .

Root cause:

When sonic-netns-exec executes the above function, argument passed to sonic-db-cli is NOT executed as a single script.

The argument is passed as separate keywords to sonic-db-cli, as below:

['EVAL', 'local', 'tables', '=', "{'PORT_TABLE*'}", 'for', 'i', '=', '1,', 'table.getn(tables)', 'do', 'local', 'matches', '=', "redis.call('KEYS',", 'tables[i])', 'for', 'j,name', 'in', 'ipairs(matches)', 'do', "redis.call('DEL',", 'name)', 'end', 'end', '0']

- How I did it
To make sure that the parameters are passed as they were set initially, fix sonic-netns-exec to use double quoted "$@", where "$@" is "$1" "$2" "$3" ... "${N}"

After fix, the argument passed to sonic-db-cli is as below:

Argument passed to sonic-db-cli:

['EVAL', "\n    local tables = {'PORT_TABLE*'}\n    for i = 1, table.getn(tables) do\n        local matches = redis.call('KEYS', tables[i])\n        for j,name in ipairs(matches) do\n            redis.call('DEL', name)\n        end\n    end", '0']

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>
tiantianlv pushed a commit to SONIC-DEV/sonic-buildimage that referenced this pull request Apr 24, 2020
… they were set initially (sonic-net#4375)

sonic-netns-exec fails to execute below command in swss.sh:

    sonic-netns-exec "$NET_NS" sonic-db-cli $1 EVAL "
    local tables = {$2}
    for i = 1, table.getn(tables) do
        local matches = redis.call('KEYS', tables[i])
        for j,name in ipairs(matches) do
            redis.call('DEL', name)
        end
    end" 0

This command fails with error " redis.exceptions.ResponseError: value is not an integer or out of range" .

Root cause:

When sonic-netns-exec executes the above function, argument passed to sonic-db-cli is NOT executed as a single script.

The argument is passed as separate keywords to sonic-db-cli, as below:

['EVAL', 'local', 'tables', '=', "{'PORT_TABLE*'}", 'for', 'i', '=', '1,', 'table.getn(tables)', 'do', 'local', 'matches', '=', "redis.call('KEYS',", 'tables[i])', 'for', 'j,name', 'in', 'ipairs(matches)', 'do', "redis.call('DEL',", 'name)', 'end', 'end', '0']

- How I did it
To make sure that the parameters are passed as they were set initially, fix sonic-netns-exec to use double quoted "$@", where "$@" is "$1" "$2" "$3" ... "${N}"

After fix, the argument passed to sonic-db-cli is as below:

Argument passed to sonic-db-cli:

['EVAL', "\n    local tables = {'PORT_TABLE*'}\n    for i = 1, table.getn(tables) do\n        local matches = redis.call('KEYS', tables[i])\n        for j,name in ipairs(matches) do\n            redis.call('DEL', name)\n        end\n    end", '0']

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>
@SuvarnaMeenakshi SuvarnaMeenakshi deleted the netns_fix branch December 14, 2020 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mellanox] SONiC fails to start on t0
4 participants