Drive identifiers sometimes not an array

[sosodev]

I noticed that chassis.Drives() was often returning a truncated slice of drives. A little bit of debugging revealed that GetDrive was failing to parse one of our drives that lists its Identifiers as an object instead of an array. Most of the drives do not do this but the returned drives slice was quite small because chassis.Drives() returns early when it encounters an error for any drive in the list.

  "Identifiers": {
    "DurableName": "5CD2E4D40DC20100",
    "DurableNameFormat": "EUI"
  },

Is this something you have encountered before? I don't know how to parse the variable type besides using an interface and type assertions but that seems hacky.

[stmcginnis]

I have not run in to that, but it definitely is not following the spec. It explicitly states "type": "array", so this looks like another case (unfortunately) that will have to go back to the vendor to get them to fix their implementation.

We could certainly change those instances to just capture the error but continue on through the list. I would have thought in most cases things would either fail right away or be fine, but this appears to be at least one case where maybe there are some legitimately formatted results.

If you step through debugging, does it actually return some valid drive objects before hitting the malformed one? If so, feel free to propose a change to not return early from that loop. I think ideally then we should do the same for all instances of that pattern where we are gathering up a collection of objects, so take it however far you want to. If you don't have the time/inclination to change them all, then I can follow up to get the rest.

Or if not at all, I can put it on my list to update sometime in the near future. :)

Thanks for all the detail @sosodev!

[sosodev]

Thanks for pointing out that this is vendor jank. Yeah, it was hitting some valid drive entities before the malformed one. I'll start working on a PR to collect errors rather than return early. I suppose we don't want to change the signature of those funcs, right? Any idea what should be done with the collected errors?

[stmcginnis]

Good question. Maybe something like fmt.Errorf("errors retrieving items: %v", collectedErrors)? (not saying exactly that, but hopefully you get the idea)

[sosodev]

Hmm, I was just thinking about this today when I realized a slight problem. Collecting and returning the errors merged into a single error does subtly change the function signature. Previously any errors in the loop were ignored and only critical errors were returned. Existing code that uses these functions would need to be modified to handle the new return case of collection + soft error.

[stmcginnis]

I was thinking of defining a new error type that could have multiple "sub-errors" added to it. So it can be one error returned that would format a decent message from Error().

[sosodev]

I agree that a new error type seems like the way to go from the implementation side. What I meant though was that existing user code would need to be refactored from something like

drives, err := chassis.Drives()
if err != nil {
    return err
}

to

drives, err := chassis.Drives()
if err != nil && len(drives) == 0 {
    return err
}

It's a subtle difference but a sweeping change from ignoring errors to returning them could make user code significantly more brittle

[stmcginnis]

Checking whether there was no error and whether or not there were drives returned is something that should be done today for most of these calls. It's quite possible that everything was successful, but the system does not have any objects to return.

[sosodev]

Going to go ahead and close this. FWIW we've pushed the vendor to fix their jank and it will be patched soon hopefully. But if anybody else is reading this and needs a fix check out my jank-friendly branch.