Fix ambiguous bay/disk labels on Joyent-S10G5

Review Request #2471 — Created Dec. 10, 2019 and submitted


The Joyent S10G5 platform contains two SAS expanders to drive the front and rear disks. The SES implementation on both of these expanders encodes a simplistic element descriptor string for the array device elemnts that uses the format "Slot##". The result is that front disks get labels "Slot00 - Slot23" and the rear disks get labels "Slot00-Slot11" and so you have essentially duplicate labels for the first 12 disks between the front and rear disk backplanes.

This ticket is to cover changes to the ses enumerator to add custom logic to dynamically assign less ambiguous labels to the bay and disk nodes when we detect we're on the Joyent S10G5 platform. The logic will look at the product ID of the SAS expanders (which are different between the front and rear) and create labels "Front Slot 0" through "Front Slot 23" "Rear Slot 0" through "Rear Slot 11" for the front and rear bay/disks nodes, respectively.

See ticket for testing details:

  • 0
  • 0
  • 3
  • 1
  • 4
Description From Last Updated
    1. Thanks for looking at this and sorry for the long delay in responding - I've been out the last few weeks. I've restructured this change quite a bit and retested it across the different variants of the SMCI 4u36 that we have deployed internally.

  2. Can we structure this so it's not quite so hardcoded to be just Joyent here? I think you may want an array of things which are overriden. As this likely impacts not just the Joyent model, but also the underlying SMCI system that this is built upon (assuming is accurate).

    Maybe having a top-level array of products and methods to call? I think that'd make this a little bit cleaner and more extensible as this shouldn't be specific to a single SKU.

    1. Yeah, I restructured this code quite a bit to both make it more extensible and also make sure we can apply it to all the variants of the SMCI 4U36.

  3. I believe you want to initialize nvl to NULL. If topo_mod_nvalloc fails, we will always call nvlist_free() on it, but there's no guarantee that the failing function will zero out the nvl pointer. Certainly the existing code doesn't today.
  4. Maybe this should look like the case at +3668 (or vice versa) as I don't believe there's any change in resource mappings between them.
    1. I left this as-is. Maybe I'm not understanding the concern here?

    2. I think this was trying to say using topo_mod_seterrno() and goto err, rather than handling it differently. Though it was some time ago and my original message was pretty vague.

  5. cstyle wants the != bits on the previous line.
  1. Ship It!
Review request changed

Status: Closed (submitted)