fix(ssacli): keep an unassigned drive available for LV creation (ARTESCA-18120) - #94
fix(ssacli): keep an unassigned drive available for LV creation (ARTESCA-18120)#94ezekiel-alexrod wants to merge 2 commits into
Conversation
ssacli answers "Status: OK" for a healthy drive whether or not it belongs to an array, and the parser maps that to PDStatusUsed. "Drive Type: Unassigned Drive", the only field that says the drive is free, promoted it to PDStatusUnassignedGood only when the status was not already Used, so it never fired. Every drive came back used, IsAvailable reported false, and CreateLV rejected every request with "unavailable drives". The three fields that decide a status, the raw "Status:" label, "Drive Type:" and the lsblk verdict on "Disk Name", are now collected while parsing and turned into a PDStatus once the block has been read, the way the storcli2 getter decides from raw values. Deciding line by line let whichever field came last win, which is what a reviewer warned about when the old guard went in. Precedence: a device that carries data is used, then a plain OK on an unassigned drive is available, then the label decides. So "Failed", "Offline", "Predictive Failure" and any label this parser does not model keep the status they map to, and a free drive with a leftover filesystem stays used. That error message also named the drive by Slot.String(), which orders the parts enclosure:bay:port, so it reported "1:1:1I" for the drive known everywhere else, and by ssacli itself, as "1I:1:1". It now uses the drive ID, like the size check next to it. Refs: ARTESCA-18120
| if !pd.IsAvailable() { | ||
| unavailableDrives = append(unavailableDrives, pd.Slot.String()) | ||
| // pd.ID, not pd.Slot.String(): the ID is what the caller passed in | ||
| // and what the vendor CLI understands, while Slot.String() orders | ||
| // the parts differently (enclosure:bay:port). | ||
| unavailableDrives = append(unavailableDrives, pd.ID) | ||
| } |
There was a problem hiding this comment.
Hmm to me that's an issue to me the string of Slot should be represented from larger level to the lower one I mean "::".
In other word it means in most setup the first part of the slot is always the same, the middle one also for small setup and the last one may change
And it's what ssacli disk ID look like so both should match to me, even if the wording are not the same.
Since we have enclosure:bay:port and SSACLI ID is port:box:bay in my opinion
- SSACLI
portshould be inenclosure - SSACLI
boxshould be inbay - SSACLI
bayshould be inport
I know it's not intuitive but we need those to match otherwise every time we will try to print the Slot it will be confusing.
Or maybe we have to change the order in our lib so that it's like this port:enclosure:bay so for SSACLI we would have
- SSACLI
port=>port - SSACLI
box=>enclosure - SSACLI
bay=>bay
And for megaraid we would have
- megaraid
enclosure=>enclosure - megaraid
slot=>bay
This way to me it would be better (I'm not sure that change is that huge tbh)
There was a problem hiding this comment.
Went with the second option, and it is a three line change: ParseSlot already reads port:enclosure:bay and Format() already writes it, String() was the only outlier. It now renders the same address.
ParseSlot(String()) used to scramble the slot: 1I:1:1 came back as port=1, enclosure=1, bay=1I. No adapter has to move, ssacli already fills Port, Enclosure and Bay in that order and the others go through ParseSlot.
Done in e7b12ab, with a test covering both slot shapes and the round trip.
Slot.String() ordered the parts enclosure:bay:port while Slot.Format() and ParseSlot both use port:enclosure:bay, so a printed slot could not be fed back as a drive id: 1I:1:1 came out as 1:1:1I. String() now renders the same address as Format(). Every adapter already fills the parts in that order. The ssacli getter maps port, box and bay onto Port, Enclosure and Bay, and the storcli2, perccli2 and megaraid getters build their slots through ParseSlot, so nothing else has to move. Refs: ARTESCA-18120
2 participants
Problem
On an HPE Smart Array controller, creating a data volume fails on every drive,
so the zero touch install stops right after it decides the controller is empty:
Root cause
ssacli answers
Status: OKfor a healthy drive whether or not it belongs to anarray, and the parser maps that to
PDStatusUsed. The only field that says adrive is free is
Drive Type: Unassigned Drive, and the case handling itpromoted the drive to
PDStatusUnassignedGoodonly when the status was notalready
Used, so it never fired.IsAvailable()requiresPDStatusUnassignedGood, soValidateRAIDCreationrefused every request.Reproduced on the parsed fixture for the drive that failed on the node:
The error message also named that drive
1:1:1I.unavailableDrives()usedSlot.String(), which orders the parts enclosure:bay:port, while the siblingsize check a few lines below already used the drive ID.
Fix
The three fields that decide a status, the raw
Status:label,Drive Type:and the lsblk verdict on
Disk Name, are collected while parsing and turnedinto a
PDStatusonce the block has been read, the way the storcli2 getterdecides from raw values. Deciding line by line let whichever field came last
win, which is what a reviewer warned about on #10 when the old guard went in.
Precedence: a device that carries data is used, then a plain OK on an unassigned
drive is available, then the label decides. So
Failed,Offline,Predictive Failureand any label this parser does not model keep the status they map to,and a free drive with a leftover filesystem stays used, which is what #19 asked
for.
The unavailable drive message now names the drive by its ID, like #13 did for
every other place a drive is identified.
Testing
Table-driven cases on the status decision, each with an ssacli fixture: free
drive available, data drive used, failed unassigned drive still failed,
predictive failure on an unassigned drive still used, free drive carrying a
filesystem still used. The last two cover the holes that the first version of
this fix opened.
go build ./...,go vet ./...andgo test -race ./... -count=1are clean.golangci-lintreports 22 findings on this module, the same set as onmain,none of them on a line this PR touches. They come from a newer linter than the
v2.4.0this repo pins, which does not run on my machine, so I could notreproduce the CI lint locally.
Out of scope
A drive ssacli calls
Failedthat still carries data is reported as used,because the lsblk verdict wins over the label. That masking is the current
behaviour and it deserves its own ticket.
See: https://scality.atlassian.net/browse/ARTESCA-18120