Finding drive vid in lsblk vendor #1094
Conversation
prasant123
requested review from
safronovD,
Andrzej-Zukowski,
lordjun,
alexemc,
katarzyna-kulpa,
rafalw82 and
Kacper-Michon-DELL
There was a problem hiding this comment.
I'm good with the change as it was discussed with @mishoyama
pkg/base/linuxutils/lsblk/lsblk.go
Outdated
| @@ -178,7 +173,7 @@ func (l *LSBLK) SearchDrivePath(drive *api.Drive) (string, error) { | |||
| vid := drive.VID | |||
| pid := drive.PID | |||
| for _, l := range lsblkOut { | |||
| if strings.EqualFold(l.Serial, sn) && strings.EqualFold(l.Vendor, vid) && | |||
| if strings.EqualFold(l.Serial, sn) && strings.Contains(l.Vendor, vid) && | |||
There was a problem hiding this comment.
how about the pid handling ? lsblk returned "HGST HUS728T8TALE6L0", but the drive CR value is from drivemgr, which is "HGST HUS728T8TAL".
and also is there possibility that Serialnumber need same handling with Vendor, as Serialnumber may also contain some trailing space.
There was a problem hiding this comment.
We are observing failure only for the Vendo ID. Since the serial number reported by both HAL and lsblk are found the same, we should be good with the existing implementation. We need a quick resolution due to the time constraint, however I agree that we need to have a relook at this area of code in the next release cycle
There was a problem hiding this comment.
do you mean for the pid I mentioned above, for different length strings, strings.EqualFold will return true in the case ?
There was a problem hiding this comment.
I think we should only ignore the trailing space in this JIRA. It is not safe to use "string.contains(A, B)" as it will also return TRUE for many other cases. For example: A = "testing " B="ing"
| @@ -161,11 +161,6 @@ func (l *LSBLK) GetBlockDevices(device string) ([]BlockDevice, error) { | |||
| // Receives an instance of drivecrd.Drive struct | |||
There was a problem hiding this comment.
one general comments is to create an issue first and link the issue to the PR. and put [ISSUE-xxx] in PR title.
pkg/base/linuxutils/lsblk/lsblk.go
Outdated
| @@ -178,7 +173,7 @@ func (l *LSBLK) SearchDrivePath(drive *api.Drive) (string, error) { | |||
| vid := drive.VID | |||
| pid := drive.PID | |||
| for _, l := range lsblkOut { | |||
| if strings.EqualFold(l.Serial, sn) && strings.EqualFold(l.Vendor, vid) && | |||
| if strings.EqualFold(l.Serial, sn) && strings.Contains(l.Vendor, vid) && | |||
| strings.EqualFold(l.Model, pid) { | |||
There was a problem hiding this comment.
Need code change here to handle the PID.
In our test, for the same disk, the VID and PID format is different from lsblk and drive CR.
l.Vendor is "ATA " while vid (drive.VID) is “ATA”
l.Model is "HGST HUS728T8TALE6L0" while pid (drive.PID) is “HGST HUS728T8TAL”
There was a problem hiding this comment.
Agree!
Updated the changes, please take a look
| strings.EqualFold(l.Model, pid) { | ||
| lvid := strings.TrimSpace(l.Vendor) | ||
| if strings.EqualFold(l.Serial, sn) && strings.EqualFold(lvid, vid) && | ||
| strings.HasPrefix(l.Model, pid) { |
There was a problem hiding this comment.
basically, it is ok.
just noted that: if pid is null string(""), then strings.HasPrefix will also return true. but in our case, pid seems to not able to be null. we may need re-look at this code in later release as well.
|
Please go ahead to merge the code, Prasant. Thanks. |
* Finding drive vid in lsblk vendor * addressing review comments * Initializing device ---------
* Finding drive vid in lsblk vendor * addressing review comments * Initializing device --------- Signed-off-by: Andrzej Zukowski <[email protected]>
* [Issue-1084] Addjust scheduler extender algorithm Signed-off-by: Andrzej Zukowski <[email protected]> * [Issue-1084] Fixes after review Signed-off-by: Andrzej Zukowski <[email protected]> * [Issue-1091] 1.5.1 version (#1093) Signed-off-by: Andrzej Zukowski <[email protected]> * Finding drive vid in lsblk vendor (#1094) * Finding drive vid in lsblk vendor * addressing review comments * Initializing device --------- Signed-off-by: Andrzej Zukowski <[email protected]> * [ISSUE-1096] Fix lint and unit test error for resolution to atl-22498 (#1098) * [Issue #1096] - Fix lint error for bug atl-22498 Signed-off-by: Eddie Pavkovic <[email protected]> * [Issue #1096] - Put code back that returned device if path set - to fix unit test Signed-off-by: Eddie Pavkovic <[email protected]> * [ISSUE-1096] - Put back code from previous PR and removed invalid test case Signed-off-by: Eddie Pavkovic <[email protected]> --------- Signed-off-by: Eddie Pavkovic <[email protected]> Signed-off-by: Andrzej Zukowski <[email protected]> * [ISSUE-1096] Change component version to 1.6.0 Signed-off-by: Andrzej Zukowski <[email protected]> --------- Signed-off-by: Andrzej Zukowski <[email protected]> Signed-off-by: Eddie Pavkovic <[email protected]> Co-authored-by: PrasantDell <[email protected]> Co-authored-by: eddiepavkovic <[email protected]>
Purpose
Resolves #1095
PR checklist
Testing
Provide test details