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

Add support for NVMe disks in LSHW #565

Merged
merged 3 commits into from
Aug 30, 2017
Merged

Add support for NVMe disks in LSHW #565

merged 3 commits into from
Aug 30, 2017

Conversation

defect
Copy link
Contributor

@defect defect commented Aug 1, 2017

Turns out that (at least some) NVMe flash storage is listed as storage in the LSHW output instead of memory like some other flash storage devices. This PR updates the diskMatcher to look for flash devices under both names.

@byxorna @qx-xp @roymarantz @michaeljs1990

@byxorna
Copy link
Contributor

byxorna commented Aug 1, 2017

Plz post screenies of the hardware summary of a lshw report containing both pcie and scsi disks?

@defect
Copy link
Contributor Author

defect commented Aug 8, 2017

@byxorna
Copy link
Contributor

byxorna commented Aug 8, 2017

Would be nice to aggregate the pcie disk capacity on overview page, so people dont have to do subtraction to figure out how much flash nvme there is in the box

@defect
Copy link
Contributor Author

defect commented Aug 16, 2017

@byxorna Most flash drives (at least the ones we've been using) don't have their storage capacity in the lshw output for some stupid reason. Hence the sketchy flashSize parameter: https://github.com/tumblr/collins/blob/master/conf/reference/lshw_reference.conf#L7

I don't think it's worth implementing an aggregate flash capacity when it's going to be wrong in most cases anyway?

@byxorna
Copy link
Contributor

byxorna commented Aug 16, 2017

The delta between total storage and scsi storage makes it look like we know how large the pcie storage is, but are just too lazy to display it. Id rather display the pcie storage count, and separately address that number being incorrect in some installs, personally

@defect
Copy link
Contributor Author

defect commented Aug 25, 2017

@byxorna Done

@@ -250,6 +250,13 @@
<td>SCSI Storage</td>
<td>@aa.lshw.totalStorage.toHuman</td>
</tr>
@if(aa.lshw.hasFlashStorage != true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this logic backwards, or am i crazy? shouldnt it be @if(aa.lshw.hasFlashStorage)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, yup. Pushed a commit from testing stuff 🤦‍♂️ One sec, i'll fix it.

Copy link
Contributor

@gtorre gtorre left a comment

Choose a reason for hiding this comment

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

If it works, 👍

@defect defect merged commit f964b6c into tumblr:master Aug 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants