Add gpu support #537

jyundt · 2017-04-17T14:57:18Z

This pull request adds support for capturing GPU information in collins. Currently, it is hardcoded for NVIDIA GPUs.

I wasn't clear if the definitions in AsssetMeta were still in use, so I added GPU_COUNT and GPU_DESCRIPTION to both AssetMeta.scala and the asset_meta table.

Updating the lshw parser to capture GPU information, WIP

This renders the proper number of GPUs and also correctly shows the description if the Server has multiple types or an onboard graphics card.

Only add NVIDIA GPUs to asset list. Previously all display adapters (including the onboard VGA adapters) were listed as GPUs.

michaeljs1990 · 2017-04-17T15:06:35Z

Since we couldn't find a great way to separate the onboard graphics card from the external GPUs (The lshw xml is identical) we do have it hard coded currently. I am not sure how this should be handled since if we just allow it to parse all the GPUs every box would show up with a count of one and then you get into the case of checking NUM_NVIDIA_GPUS and NUM_AMD_GPUS and whatever other flavor you have.

The whitelist was an easy fix for us but possibly not a great way to handle it for a wider audience.

cc @byxorna @roymarantz @defect

Create two new tests to verify GPU functionality works as expected.

byxorna · 2017-04-20T18:55:13Z

@jyundt @michaeljs1990 a common pattern is to make a configuration array of strings to match against LSHW output to identify external graphics adapters (by product, vendor, etc). Up to you if you wanna make it opt-in or opt-out. I would love this to be configurable via production.conf, with a sane default like matching on Manufacturer field with "AMD","NVIDIA"

byxorna

Generally looks good, thanks for the diff! Comments inline

byxorna · 2017-04-20T18:56:42Z

conf/evolutions/collins/2.sql

@@ -58,6 +58,9 @@ INSERT INTO asset_meta VALUES (31, 'LLDP_VLAN_NAME', -1, 'LLDP VLANE Name', 'VLA
 INSERT INTO asset_meta VALUES (32, 'INTERFACE_NAME', -1, 'Interface Name', 'Name of physical interface, e.g. eth0');
 INSERT INTO asset_meta VALUES (33, 'INTERFACE_ADDRESS', 0, 'IP Address', 'Address on interface, e.g. 10.0.0.1');

+INSERT INTO asset_meta VALUES (34, 'GPU_COUNT', -1, 'GPU Count', 'Number of physical GPUs in asset');


so, this should actually be a new migration; once an evolution is committed, we never update it. You should be able to upgrade your DB schema by only running the newest evolutions. Please move these into a $(n+1).sql, thanks

byxorna · 2017-04-20T18:57:28Z

app/collins/models/AssetMeta.scala

@@ -198,6 +198,10 @@ object AssetMeta extends Schema with AnormAdapter[AssetMeta] with AssetMetaKeys
    val NicName = Value(32, "INTERFACE_NAME")
    // DO NOT USE - Deprecated
    val NicAddress = Value(33, "INTERFACE_ADDRESS")
+
+    //I know this says Deprecated, but I'm unsure of where to define Gpu vars


did this not compile without this? I would like to not add magic incantations places if it isnt strictly necessary :)

byxorna · 2017-04-20T18:58:49Z

app/collins/models/LshwHelper.scala

+      if (descr.isEmpty) {
+        seq
+      } else {
+        Gpu(descr, "", "") +: seq


Why are these last 2 params ""?

byxorna · 2017-04-20T18:59:56Z

app/collins/models/LshwHelper.scala

@@ -86,6 +92,41 @@ object LshwHelper extends CommonHelper[LshwRepresentation] {
    )
  }

+  protected def reconstructGpu(meta: Map[Int, Seq[MetaWrapper]]): FilteredSeq[Gpu] = {


i assume this is copy+paste from another reconstruct function? its really hard to read :/

Yeah, I modeled reconstructGpu and collectGpus off of the existing CPU/memory/disk/NIC functions. I wasn't sure how consistent you'd want the new GPU code to be compared to the previous CPU/memory/disk/NIC stuff.

ideally this would be made more legible to make support burden easier going forward

Will do, I didn't change this with my most recent commit (d019c27) but I'll get this cleaned up.

byxorna · 2017-04-20T19:01:56Z

app/collins/util/parsers/LshwParser.scala

@@ -79,6 +82,13 @@ class LshwParser(txt: String) extends CommonParser[LshwRepresentation](txt) {
      Cpu(cores, threads, speed, asset.description, asset.product, asset.vendor)
  }

+  val gpuMatcher: PartialFunction[NodeSeq, Gpu] = {
+    case n if ((n \ "@class" text) == "display" && (n \\ "vendor" text).contains("NVIDIA Corporation")) => {


yikes, we cant have this. We should check a config parameter that has a list of valid GPU vendors instead

byxorna · 2017-04-20T19:02:20Z

conf/evolutions/collins/7.sql

@@ -2,9 +2,9 @@

 # --- !Ups

-UPDATE asset_meta SET priority = -1 WHERE id=33;
+UPDATE asset_meta SET priority = -1 WHERE id=35;


👎
dont modify existing evolutions

byxorna · 2017-04-20T19:03:10Z

test/collins/util/parsers/LshwParserSpec.scala

@@ -492,5 +492,18 @@ class LshwParserSpec extends mutable.Specification {
      }
    } // wonky opterons

+    "Parse GPU server" in {


we should have many new GPU-enabled LSHW fixtures to test this functionality. Can you commit a bunch of sample LSHWs from different hw to have a more robust test suite?

I'm working on this, but it will probably have to be a separate PR.

We (and by "we" I mean @michaeljs1990) found that our lshw XML files broke all of the existing model tests in LshwHelperSpec. In order to get the new GPU code to pass the tests we had to "add" GPUs to the existing XML test files.

Unfortunately it looks like there is something unique about our hardware. But with the exception of the model tests, everything else seems to work fine with our lshw XML data.

Hmm... ok. I would definitely like any new functionality (especially that which has the potential to break existing functionality) to have unit test coverage before landing. Are you cool with that??

Sorry, I should have clarified: I added two additional tests for LshwHelperSpec and LshwParserSpec to test the new GPU functionality. These new tests (and corresponding lshw XML files) work as expected.

However, for some reason the lshw XML output from all of our new GPU servers will fail all of the existing collins tests. I validated this on the current master branch and add_gpu_support branch.

I've been poking around with this, but there appears to be something 'funky' with our hardware, not the recent changes (or the tests).

As a sanity check, I grabbed lshw output from a Dell device with one gpu (not one of our regular GPU servers) and added two new tests (there are four GPU tests total). Everything works as expected: 7212e38

I'm reasonably confident that there is something off with our GPU servers / our lshw output.

Looks like the tests are failing because you are asserting a Vector()==List(). @jyundt

Expected: LshwRepresentation(Vector(Cpu(8,16.... Actual: LshwRepresentation(List (Cpu(8,16...

I think if you sort that out (and ensure that equals() on Cpu and Gpu classes work with value equality and not object identity), it should work!

I noticed the vector vs list discrepancy previously, but I just stumbled onto the problem: our CPU frequency is different per processor unlike the existing XML test cases.

I manually edited <size units="Hz">###</size> from one of our sample XML files and all tests are now passing from our funky gpu servers.

Interestingly enough, I think this problem would have bubbled up previously if lshw-b0216.xml was used as a test case for the LshwHelperSpec test (instead of just LshwParserSpec).

Thanks for the help, lemme take another look at this.

@byxorna I poked around this a bit more and I think our usage of dynamic frequency scaling is causing this behavior. The reported CPU speed is "changing" in the lshw XML files depending on the load of the system.

Additionally, it looks like the current default behavior is to capture CPU information from the first socket and persist that information for all sockets.

As a result of these two behaviors, the existing LshwHelperSpec tests will always fail.

TBH, I'm not sure what is "expected behavior" for this. Should collins assume the max clock speed for CpuSpeedGhz for all sockets? Should collins honor the data in the lshw XML files and permit different clock speeds per socket?

I'm leaning towards opening a separate GH issue for this because it is unrelated to any of the GPU changes.

Thoughts?

I think putting more assumptions into collins to address [mis]configuration of servers during lshw upload is a road we do not want to go further down. IMHO, speed stepping should be disabled by genesis (if it isnt already) before lshw collection, as a workaround. This definitely merits more discussion though, because of the assumptions baked into collins. Could you open an issue and tag me in it? Thanks!

Good call with disabling speed stepping, I hadn't thought of that.

New GH issue: #550

* Update evolutions migration to avoid hardcoding index values. * Remove additional hardcoded GPU AssetMeta references. * Add initial gpu whitelist config file functionality.

jyundt

Thanks for the feedback! As per your comments I removed any references to the deprecated "static" AssetMeta references and corrected the evolutions SQL migration scripts.

I also added initial functionality for a "whitelist" for gpuVendors defined in the collins config file.

Let me know your thoughts?

jyundt · 2017-04-26T17:56:54Z

test/collins/util/parsers/LshwParserSpec.scala

@@ -492,5 +492,18 @@ class LshwParserSpec extends mutable.Specification {
      }
    } // wonky opterons

+    "Parse GPU server" in {


I'm working on this, but it will probably have to be a separate PR.

We (and by "we" I mean @michaeljs1990) found that our lshw XML files broke all of the existing model tests in LshwHelperSpec. In order to get the new GPU code to pass the tests we had to "add" GPUs to the existing XML test files.

Unfortunately it looks like there is something unique about our hardware. But with the exception of the model tests, everything else seems to work fine with our lshw XML data.

jyundt · 2017-04-26T17:57:04Z

app/collins/models/LshwHelper.scala

@@ -86,6 +92,41 @@ object LshwHelper extends CommonHelper[LshwRepresentation] {
    )
  }

+  protected def reconstructGpu(meta: Map[Int, Seq[MetaWrapper]]): FilteredSeq[Gpu] = {


Yeah, I modeled reconstructGpu and collectGpus off of the existing CPU/memory/disk/NIC functions. I wasn't sure how consistent you'd want the new GPU code to be compared to the previous CPU/memory/disk/NIC stuff.

michaeljs1990 · 2017-04-26T20:23:25Z

https://github.com/tumblr/collins/blob/master/support/ruby/collins-client/lib/collins/asset.rb#L206 will want to add gpus to this as well for consistency and so it's easy to access from the ruby client. Not sure if other stuff will need to be updated as well.

michaeljs1990 · 2017-05-05T07:34:08Z

Not sure what this is about... but I was testing this PR inside the provided dockerfile for this repo and even adding the additional config needed for gpuVendors to the production.conf file it will not intake GPUs but does show the additional field in hardware.

Likely doing something really dumb but should likely figure out exactly what that dumb thing is :/

jyundt · 2017-05-05T15:21:17Z

Not sure what this is about... but I was testing this PR inside the provided dockerfile for this repo and even adding the additional config needed for gpuVendors to the production.conf file it will not intake GPUs but does show the additional field in hardware.

I think I found the culprit, the docker specific validations.conf file is missing the GpuConfig.

I'll validate and update my PR.

michaeljs1990 · 2017-05-09T17:13:26Z

FWIW we ended up having to deploy this which just happens to be the current master branch with this deployed on-top of it. It's working well so far with the exception that we had to install the NVIDIA driver for our genesis iso to get access to some extra information about the GPUs in /proc/sys by default the lshw returns "NVIDIA Corporation" for product and vendor.

Our current plan is spicing up the lshw xml for product by injecting the actual model name found in /proc/sys/**/information (can't remember exact location right now).

I should say anything older than the Titan X models do return same product information.

michaeljs1990 · 2017-05-16T07:58:57Z

Something strange is happening on update. I checked by uploading a valid XML file with GPUs and then changing the name of a few things such as the NIC and GPU product. The NIC is updated fine and has a new name however it seems GPU is never updated.

The farthest I have been able to trace it back is to LshwHelper.updateAsset however it seems to have the correct XML at this point.

Related to tumblr#547

byxorna · 2017-05-18T15:34:26Z

@jyundt is your latest update fixing the odd behavior that @michaeljs1990 pointed out? Is this diff ready for review now? I want to make sure people dont spend time reviewing a WIP

Thanks for the diff, this is gonna be awesome!!!

byxorna

Nice update! Looking forward to landing this :)

byxorna · 2017-05-18T15:35:43Z

app/collins/models/AssetMeta.scala

@@ -206,9 +206,15 @@ object AssetMeta extends Schema with AnormAdapter[AssetMeta] with AssetMetaKeys
    val BaseProduct = findOrCreateFromName("BASE_PRODUCT")
    val BaseVendor = findOrCreateFromName("BASE_VENDOR")
    val BaseSerial = findOrCreateFromName("BASE_SERIAL")
+    val GpuCount = findOrCreateFromName("GPU_COUNT")


Can this be computed on reconstruction, instead of storing this? It would make it easier to compute how many NVIDIA vs AMD GPUs there are (if in a mixed env)

Good call, I removed the extraneous GPU_COUNT

byxorna · 2017-05-18T15:36:11Z

app/collins/models/AssetMeta.scala

@@ -206,9 +206,15 @@ object AssetMeta extends Schema with AnormAdapter[AssetMeta] with AssetMetaKeys
    val BaseProduct = findOrCreateFromName("BASE_PRODUCT")
    val BaseVendor = findOrCreateFromName("BASE_VENDOR")
    val BaseSerial = findOrCreateFromName("BASE_SERIAL")
+    val GpuCount = findOrCreateFromName("GPU_COUNT")
+    val GpuDescription = findOrCreateFromName("GPU_DESCRIPTION")


I think we should have both vendor and description? I believe pci enumeration has fields for both vendor and description?

I added product and vendor. It's a bit confusing because lshw refers to the "label" or "description" of the PCI device as the "product".

byxorna · 2017-05-18T15:37:43Z

app/views/asset/show_overview.scala.html

@@ -204,6 +204,14 @@
          <td>@{if (aa.lshw.hasHyperthreadingEnabled) "Yes" else "No"}</td>
        </tr>

+          <th colspan="3">GPU</th>


whitespace is off here

Fixed, good catch.

byxorna · 2017-05-18T15:38:30Z

app/views/asset/show_hwdetails.scala.html

+    @aa.lshw.gpus.zipWithIndex.map { case(gpu,id) =>
+      <tr>
+        <th>@id</th>
+        <td>@gpu.description</td>


add vendor column here, and maybe sort by id, so the GPUs show up in enumeration order?

Added Description (internally called "product") and Vendor columns.

byxorna · 2017-05-18T15:40:46Z

app/collins/util/config/GpuConfig.scala

+  override val namespace = "gpu"
+  override val referenceConfigFilename = "gpu_reference.conf"
+
+  def gpuVendors= getStringSet("gpuVendors")


I would like the default values to be reflected here, instead of relying on the config to include something. I.e. the default unpopulated config should include nvidia, instead of the user being forced to set this config on every installation

Added default value of "NVIDIA Corporation".

byxorna · 2017-05-18T15:41:37Z

conf/test_base.conf

@@ -282,6 +282,10 @@ lshw {
  lshw.defaultNicCapacity=10000000000
 }

+gpu {
+  gpuVendors = ["NVIDIA Corporation"]


This is a stutter. gpu.gpuVendors? Maybe gpu.supportedVendorStrings or something to be more explicit

Good call, renamed gpuVendors -> supportedVendorStrings

- Split 'description' -> 'product' and 'vendor' based on PCI enumeration - Remove static GPU_COUNT - Cleanup hwdetails view to include GPU product, GPU vendor - Modify hwdetails view to only display GPUs info if GPUs are present - Whitelist NVIDIA GPUs by default - Rename GPU whitelist config param to avoid stutter

jyundt · 2017-05-19T14:10:03Z

@jyundt is your latest update fixing the odd behavior that @michaeljs1990 pointed out? Is this diff ready for review now? I want to make sure people dont spend time reviewing a WIP

Yes, I incorporated the change from @michaeljs1990 with f301fd9.

My most recent commit (d019c27) should incorporate all of the requested changes except cleaning up reconstructGpu (it's still messy).

New lshw output provided from a Dell device with one GPU.

byxorna · 2017-05-20T09:12:53Z

app/collins/util/LshwRepresentation.scala

@@ -105,6 +113,7 @@ case class LshwRepresentation(
        (cpuCoreCount == other.cpuCoreCount) &&
        (cpuThreadCount == other.cpuThreadCount) &&
        (cpuSpeed == other.cpuSpeed) &&
+        (gpuCount == other.gpuCount) &&


i am not sure where this method gets used, but comparing GPU counts (as opposed to deep comparison of desc/vendor/product) could produce odd behavior when you change GPUs and update LSHW, but the same number of GPUs are present. Would collins not update the meta attributes in the DB if the old reconstructed LSHW equals the newly submitted one?

I think what you have in this diff is fine, but we should test this behavior (induct with GPU A, change to GPU B, induct, ensure LSHW is updated properly) and open an issue if this is a problem

Good catch, this was something I intended to remove but missed.

byxorna · 2017-05-20T09:13:28Z

app/collins/util/config/GpuConfig.scala

+  override val namespace = "gpu"
+  override val referenceConfigFilename = "gpu_reference.conf"
+
+  def supportedVendorStrings= getStringSet("supportedVendorStrings", Set("NVIDIA Corporation"))


What do you think about moving the default string to the reference config?

TBH i feel like the reference config is a confusing layer of indirection. When trying to figure out how collins gets some default config setting, you have to unwind the magic config loading behavior (reference configs, included base configs, environment configs, then application.conf) to understand what gets what from where. I personally prefer (and recommended) to have default config directly in the code

byxorna · 2017-05-20T09:15:35Z

conf/evolutions/collins/14.sql

+
+# --- !Ups
+
+INSERT INTO asset_meta (name, priority, label, description) VALUES ('GPU_PRODUCT', -1, 'GPU Product', 'GPU product (description of GPU)');


doesnt this need a GPU_DESCRIPTION as well? I see we extract description from LSHW, but it doesnt seem like it ever gets persisted

I omitted GPU_DESCRIPTION because I didn't see any value in storing it in the database. Based on all of the lshw dumps I could find, the description for any display adapter (including onboard adapters) will be "VGA compatible controller".

But I don't really feel too strongly about it and could add it if you think there is value in persisting this information.

Yea, that is fair. If its not a useful field, lets ignore it. I was thinking it could contain some more useful information, but apparently not! 💃

byxorna

This looks great to me! Thanks so much for this!

Few comments inline, namely around making sure you persist GPU_DESCRIPTION if you are extracting it. More tests/validation would be great, but this looks like it is close. Ill add in more people to review now.

byxorna

Marking as approved to remove the blocker :)

byxorna · 2017-05-23T10:35:55Z

I think this is good to land. @jyundt can you open a new diff for gh-pages to document the new configuration bits for the GPU section?

Ref tumblr#537

jyundt · 2017-05-23T14:31:19Z

I think this is good to land. @jyundt can you open a new diff for gh-pages to document the new configuration bits for the GPU section?

#551, let me know if you'd like anything documented/worded differently.

byxorna · 2017-05-23T15:00:58Z

@jyundt looks great! Lets wait for @defect and @roymarantz to review before we land?

defect · 2017-05-29T15:14:19Z

One teeny tiny comment, but looks good to me :)

byxorna · 2017-05-30T07:58:28Z

@jyundt ill land this unless you have any last change you need to make, cool?

jyundt · 2017-05-31T13:06:47Z

No, no additional changes. Thanks for your help with this!

Jacob Yundt and others added 4 commits April 14, 2017 13:46

Adding initial GPU support

34da274

Updating the lshw parser to capture GPU information, WIP

Adding (missing) Gpu.scala file

f9fbeca

Render All GPUs (#1)

50a520f

This renders the proper number of GPUs and also correctly shows the description if the Server has multiple types or an onboard graphics card.

Filter NVIDIA GPUs

9e3e600

Only add NVIDIA GPUs to asset list. Previously all display adapters (including the onboard VGA adapters) were listed as GPUs.

michaeljs1990 mentioned this pull request Apr 17, 2017

Add GPU support #536

Closed

Add gpu support tests (#3)

7b3a826

Create two new tests to verify GPU functionality works as expected.

byxorna suggested changes Apr 20, 2017

View reviewed changes

byxorna assigned jyundt Apr 20, 2017

Jacob Yundt and others added 3 commits April 20, 2017 16:33

Cleanup of DB migration scripts.

a3cc729

Add gpu support fixup

148b990

* Update evolutions migration to avoid hardcoding index values. * Remove additional hardcoded GPU AssetMeta references. * Add initial gpu whitelist config file functionality.

Minor typo fix for hwdetails view.

ea915fc

jyundt commented Apr 26, 2017

View reviewed changes

Update ruby collins-client with GPU functionality

0412dac

Update docker/validations.conf to include GpuConfg

63a25f1

Incorporate Dynamic Enum fix

f301fd9

Related to tumblr#547

byxorna suggested changes May 18, 2017

View reviewed changes

Add two additional GPU tests

7212e38

New lshw output provided from a Dell device with one GPU.

byxorna reviewed May 20, 2017

View reviewed changes

byxorna requested review from defect and roymarantz May 20, 2017 09:18

byxorna approved these changes May 20, 2017

View reviewed changes

Remove unnecessary gpuCount.

262c2ea

jyundt mentioned this pull request May 23, 2017

CPU frequency is inconsistently collected and persisted #550

Open

jyundt pushed a commit to jyundt/collins that referenced this pull request May 23, 2017

Update GH pages to include GPU config information.

d81a560

Ref tumblr#537

jyundt mentioned this pull request May 23, 2017

Update GH pages to include GPU config information. #551

Merged

byxorna merged commit d662de2 into tumblr:master May 31, 2017


		# --- !Ups

		INSERT INTO asset_meta (name, priority, label, description) VALUES ('GPU_PRODUCT', -1, 'GPU Product', 'GPU product (description of GPU)');

Add gpu support #537

Add gpu support #537

Conversation

jyundt commented Apr 17, 2017

michaeljs1990 commented Apr 17, 2017 • edited Loading

byxorna commented Apr 20, 2017

byxorna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyundt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeljs1990 commented Apr 26, 2017

michaeljs1990 commented May 5, 2017

jyundt commented May 5, 2017

michaeljs1990 commented May 9, 2017 • edited Loading

michaeljs1990 commented May 16, 2017

byxorna commented May 18, 2017

byxorna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyundt commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byxorna left a comment

Choose a reason for hiding this comment

byxorna left a comment

Choose a reason for hiding this comment

byxorna commented May 23, 2017

jyundt commented May 23, 2017

byxorna commented May 23, 2017

defect commented May 29, 2017

byxorna commented May 30, 2017

jyundt commented May 31, 2017

michaeljs1990 commented Apr 17, 2017 •

edited

Loading

michaeljs1990 commented May 9, 2017 •

edited

Loading