Skip to content

[multi-DB] Part 4: add sonic-db-cli to replace redis-cli#54

Merged
qiluo-msft merged 6 commits intosonic-net:masterfrom
dzhangalibaba:multidb_cli
Nov 5, 2019
Merged

[multi-DB] Part 4: add sonic-db-cli to replace redis-cli#54
qiluo-msft merged 6 commits intosonic-net:masterfrom
dzhangalibaba:multidb_cli

Conversation

@dzhangalibaba
Copy link
Copy Markdown
Collaborator

  • add sonic-db-cli wrapper to replace redis-cli later when applying multiDB
    • sonic-db-cli CONFIG_DB keys * = redis-cli -p 6379 -n 4 keys *
  • package in swsssdk, it will be installed in /usr/local/bin/sonic-db-cli when swsssdk pkg is installed
  • cover both host and docker environment

admin@ASW-7005:~$ sonic-db-cli LOGLEVEL_Dx
More than three arguments are required. 'usage: sonic-db-cli 'dbname' 'redis commands''

admin@ASW-7005:~$ sonic-db-cli LOGLEVEL_DB x
(error) ERR unknown command x, with args beginning with:

admin@ASW-7005:~$ sonic-db-cli LOGLEVEL_DB1 keys *
Invalid database name input : 'LOGLEVEL_DB1'

admin@ASW-7005:~$ sonic-db-cli LOGLEVEL_DB keys *

  1. "SAI_API_IPMC_GROUP:SAI_API_IPMC_GROUP"
  2. "SAI_API_VLAN:SAI_API_VLAN"
  3. "SAI_API_LAG:SAI_API_LAG"
  4. "vxlanmgrd:vxlanmgrd"
  5. "teamsyncd:teamsyncd"
  6. "vlanmgrd:vlanmgrd"

Signed-off-by: Dong Zhang [email protected]

@dzhangalibaba
Copy link
Copy Markdown
Collaborator Author

@qiluo-msft move sonic-db-cli into swsssdk pkg

qiluo-msft
qiluo-msft previously approved these changes Oct 23, 2019
@qiluo-msft qiluo-msft self-requested a review October 23, 2019 02:06
@qiluo-msft qiluo-msft dismissed their stale review October 23, 2019 02:07

Need to double review


# Determine whether stdout is on a terminal
if os.isatty(1):
DOCKER_EXEC_FLAGS += "t"
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Oct 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOCKER_EXEC_FLAGS [](start = 4, length = 17)

Not needed any more? #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

print >> sys.stderr, msg
logger.error(msg)
else:
client = redis.StrictRedis(db=dbid, host=dbhostname, port=dbport)
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Oct 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client [](start = 8, length = 6)

This try-except is a working solution.
How about SonicV2Connector.get_redis_client() ? Is it much easier? #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use SonicV2Connector() as well, updated.

@dzhangalibaba
Copy link
Copy Markdown
Collaborator Author

dzhangalibaba commented Oct 23, 2019

BTW do we have any app using SonicV2Connector who has no root role? If the app is not executed via root, we cannot use unix_socket_path by default since redis.sock requiring root access. For safety purpose, do we want to make use_unix_socket_path to FALSE by default. There are too many scripts using SonicV2Connector(). And today, they are all using TCP + port mode. #Resolved

@qiluo-msft
Copy link
Copy Markdown
Contributor

qiluo-msft commented Oct 23, 2019

All app using SonicV2Connector are running by root. Python/script could use TCP port as default, which is current situation. I believe if you use unixsocket, it is still working fine.


In reply to: 545566509 [](ancestors = 545566509)

logger.addHandler(logging.NullHandler())

if len(sys.argv) < 3:
msg = "More than three arguments are required. 'usage: sonic-db-cli dbname rediscommands'"
Copy link
Copy Markdown
Contributor

@jleveque jleveque Nov 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"More than three arguments are required" is not quite an accurate message (more correctly, it requires at least two arguments (the first arg in argv is the program name)). I suggest removing that message altogether, and just print the usage message, like:

Usage: sonic-db-cli <db_name> <redis_commands>

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about Usage: sonic-db-cli <db_name> <cmd> [cmd [arg [arg ...]]]
redis-cli -h will give an excellent sample. #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated. @qiluo-msft @jleveque

admin@ASW-7005:~$ sonic-db-cli XXX
'Usage: sonic-db-cli <db_name> [cmd [arg [arg ...]]]'. See 'sonic-db-cli -h' for detail example.

admin@ASW-7005:~$ sonic-db-cli -h
Example 1: sonic-db-cli CONFIG_DB keys *
Example 2: sonic-db-cli APPL_DB HGETALL VLAN_TABLE:Vlan10
Example 3: sonic-db-cli APPL_DB HGET VLAN_TABLE:Vlan10 mtu

print "Example 2: sonic-db-cli APPL_DB HGETALL VLAN_TABLE:Vlan10"
print "Example 3: sonic-db-cli APPL_DB HGET VLAN_TABLE:Vlan10 mtu"
elif argc < 3:
msg = "'Usage: sonic-db-cli <db_name> <cmd> [cmd [arg [arg ...]]]'. See 'sonic-db-cli -h' for detail example."
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
msg = "'Usage: sonic-db-cli <db_name> <cmd> [cmd [arg [arg ...]]]'. See 'sonic-db-cli -h' for detail example."
msg = "'Usage: sonic-db-cli <db_name> <cmd> [arg [arg ...]]'. See 'sonic-db-cli -h' for detail example."

Sorry for the wrong sample above #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected


logger = logging.getLogger(__name__)
logger.setLevel(logging.INFO)
logger.addHandler(logging.NullHandler())
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need logging? Is it a performance issue? #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is necessary. There is no logging in original redis-cli shell script. I think we can remove it. Definitely it will add overhead to run this python script.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@qiluo-msft qiluo-msft merged commit bc3964b into sonic-net:master Nov 5, 2019
@dzhangalibaba dzhangalibaba deleted the multidb_cli branch November 22, 2019 08:42
abdosi pushed a commit that referenced this pull request Jan 6, 2020
* [multi-DB] Part 4: add sonic-db-cli to replace redis-cli
* use SonicV2Connector instead and fix previous if condition typo
* remove unused import
* use_unix_socket_path set to False by default to avoid unnecessary failed
* update Usage message and add example under -h
* remove logging and update usage msg
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.

5 participants