[ssdutil] Inform if the disk is not SSD#1957
[ssdutil] Inform if the disk is not SSD#1957sujinmkang wants to merge 6 commits intosonic-net:masterfrom
Conversation
| log = logger.Logger(SYSLOG_IDENTIFIER) | ||
|
|
||
|
|
||
| def get_disk_type(diskdev): |
There was a problem hiding this comment.
better to validate if the listed diskdev is valid as listed by "lsblk" otherwise the utility will crash. you can use
lsblk -l -n | grep 'disk'
to get the list of all disk names available in the host
ssdutil/main.py
Outdated
| proc = subprocess.Popen(cmd, shell=True, text=True, stdout=subprocess.PIPE) | ||
| outs = proc.stdout.readlines() | ||
| for out in outs: | ||
| if out.split()[0] is diskdev_name: |
There was a problem hiding this comment.
what if diskdev_name is one of ['sda', 'sdb', 'sdc'], but you seem to be checking only the first element
|
This pull request introduces 1 alert when merging d17a02f into 2e462ef - view on LGTM.com new alerts:
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@prgeor, can you look at the new changes? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
alexrallen
left a comment
alexrallen
left a comment
There was a problem hiding this comment.
Hey @sujinmkang while looking this over I had a few comments / questions. Please take a look, thanks.
ssdutil/main.py
Outdated
| if args.device: | ||
| disk_device = args.device | ||
| else: | ||
| disk_device = get_default_disk() |
There was a problem hiding this comment.
Why not call get_default_disk() from within argparse? i.e. default=get_default_disk() that would remove the necessity of this code.
| if diskdev_name not in out: | ||
| return DISK_INVAILD | ||
| cmd = "cat /sys/block/{}/queue/rotational".format(diskdev_name) | ||
| proc = subprocess.Popen(cmd, shell=True, text=True, stdout=subprocess.PIPE) |
There was a problem hiding this comment.
Forking to cat here seems unnecessary. Is there a reason you chose not to use native python io here?
There was a problem hiding this comment.
It is also more pythonic to do this and add error handling in order to handle the DISK_INVALID case rather than forking lsblk as well.
| cmd = "lsblk -l -n |grep {}".format(host_mnt) | ||
| proc = subprocess.Popen(cmd, shell=True, text=True, stdout=subprocess.PIPE) | ||
| out = proc.stdout.readline() | ||
| if host_mnt in out: |
There was a problem hiding this comment.
Is this check necessary? grep should take care of this. Perhaps check if the result is not empty (or grep return code)?
| """Check default disk""" | ||
| default_device = DEFAULT_DEVICE | ||
| host_mnt = '/host' | ||
| cmd = "lsblk -l -n |grep {}".format(host_mnt) |
There was a problem hiding this comment.
You may be able to do this cleaner with df
admin@r-lionfish-16:~$ df --output=source /host
Filesystem
/dev/sda3
4 participants
sonic-net/sonic-buildimage#9407
What I did
Inform that the disk is not SSD for the platform with non-SSD disk
How I did it
Check disk type and exit the ssdutil command
How to verify it
Check
show platform ssdhealthcommand on the platform with non SSD diskPrevious 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)