get_webclient falls off its end when /api/login answers anything but 200, so
it returned None. wc() passes that back, and thirty of the thirty-four call
sites use it unchecked, so a refused login arrived as "'NoneType' object has
no attribute 'grab_json_response'" from wherever it landed.
Raised where the failure is known, and with the status: 404 is firmware with
no web api, or a Redfish-only capture of one, while 401 is credentials it
will not take. Neither was distinguishable before.
The other four call sites are the inventory reads, and they always did check.
They answer partially when the web interface is out of reach, which is why
nodeinventory still says something useful. They ask through wc_if_available
now, so that tolerance is stated rather than resting on a None.
Two places where the code that exists to explain a failure fails instead, and
the caller is shown the second failure rather than the first.
_do_web_request builds its message from the response body when that body is
not the JSON error document the spec asks for. An html 404 page is exactly
that, the body is bytes, and str + bytes raised TypeError. The status and the
body never reached anyone.
LenovoFirmwareConfig raised a bare Exception when python-lxml and
python-eficompressor are absent. Confluent has no handler for one, so a
missing dependency showed as "Unexpected Error" and hid a message that
already said what to install.
Neither changes what fails, only what the caller is told.
Same function, one line above. EthernetInterfaces is optional, and when it is
absent the None went into a request and raised TypeError from the url library
rather than saying what was missing.
Kept beside the ambiguous case because the two are one question asked twice.
Dedicated plus shared is how BMCs are built, so several NICs is ordinary.
_get_bmc_nic_url only reaches the count when the address the session came in
on matches none of them, which is what a tunnel or a NAT does, and it then
raised the bare PyghmiException base class.
Confluent has no handler for the base class, so it fell through to the
generic one and showed "Unexpected Error" plus a traceback, for a machine
doing nothing wrong. UnsupportedFunctionality now, which the redfish plugin
already reports plainly and which stays inside PyghmiException.
The message said "does not have exactly one interface" without saying how
many, which ones, or what to do. Every caller takes a name, so it lists the
candidates, and the empty case reads differently from the ambiguous one.
A Redfish service may publish no Systems collection, and power and cooling
equipment does exactly that. sysurl is then None, and get_power and
get_bootdev handed it straight to a request, raising TypeError from inside
the url library.
sysinfo already guarded the same field. Both now ask through _system_url and
get a refusal naming what is missing. DMTF publish three services of this
shape, which is why they sit commented out in inventory-dmtf.yaml.
PCIeDevices and PCIeFunctions are optional, and get_health indexed both
without checking. _get_adp_urls in the same file already spells it
.get('PCIeDevices', []), so these two were the outliers.
Power and cooling equipment has no PCIe at all. The KeyError escaped the
health read and reached the user as "Unexpected Error" with a traceback
behind it. Reproduces offline against DMTF's public-rackmount1 mockup.
A read only ever looked at the manager's log services, and fell back to the
system's when the manager published none. A platform that keeps an event log
in both places had the second one invisible: an AMI MegaRAC keeps power unit
and thermal events in a chassis log that nothing read, 103 records that no
command could reach.
Clearing deliberately does not follow. It stays where it was, so a log that
only a read reaches is never destroyed by one, and clearing a platform that
keeps its only event log on the system still works.
The name test now ignores spacing, since a build that calls its post code log
"BIOS POST Code Log" was read as an event log and merged 2719 post codes in.
Reading an event log skipped every log service whose id or name said journal,
dump, post code, host logger or crash, on every implementation. Those names
are bmcweb's: the AMI and Lenovo bmcs call theirs SEL, EventLog, AuditLog and
PlatformLog. bmcweb does need the distinction, keeping its event log under the
system while a clear would destroy its dumps, so it gets a handler that names
the words and generic names none, reading whatever a platform publishes.
Generic added an UpdateParameters part to every multipart firmware push,
because the specification has one carried. Only the AMI firmware was seen to
insist on it, so name it in that handler and let generic send what the caller
passed.
The deadline was set when monitoring began, so a flash that kept
answering for longer than the grace period had already spent it by the
time the reboot it covers arrived, and reported a working update as a
failure.
Firmware for a supply or a fan matched no fragment and so was called
core, and nothing could ever answer for misc.
Only the collections that mean one thing are matched by url. A Storage
resource is the subsystem, so its firmware is the controller rather than
a drive, and a Processor is a cpu as readily as an accelerator, so that
one is decided by asking the processor what it is.
The labels are worked out across the whole inventory, since a platform may give
every entry the same Name, so an entry that came back empty would be asked for a
name it does not have and take the naming of the others with it.
Some will not take an image without being told which kind it is, and the
only way to find out was to attempt an update and read the error, which
writes to the bmc before it gets that far.
This bmc gives all three of its firmware entries the same Name, "Software
Inventory", and puts what they actually are in the description. The first
entry took that name, and the two after it fell back to their ids, so
nodefirmware answered with "Software Inventory", "cpld_active" and
"d1dc9e4b" for what are the host, cpld and bmc images.
Decide the labels across the collection rather than one entry at a time,
so a name the platform repeats can be recognised as no name at all. Where
that happens, use a description that does tell them apart, and the id when
even that is shared. A platform whose names are already distinct keeps
exactly the names it had.
The labels are what a caller addresses an entry by, so this also turns
inventory/firmware/all/d1dc9e4b into inventory/firmware/all/bmc_image.
Processor inventory carried a single field, the model, so a platform that
does not give one had a processor in the listing with nothing in it, and
the client, which skips empty values, showed no processor at all. This
bmc names the manufacturer, the socket and the core and thread counts, and
gives no model.
Carry those, along with the speed, serial and part number where a platform
offers them, and treat a processor as missing only when the bmc says its
state is absent, rather than whenever it does not describe a state.
Reading the identify state says plainly when a platform describes no
indicator, but writing it went ahead and patched IndicatorLED regardless.
This bmc has neither that property nor the boolean that replaced it, and
answered the write with an internal service error, which reached the user
as one and the log as a traceback.
Ask the same question the read asks. With neither property present there
is nothing to write, so say so in the same words instead of finding out
from the bmc.
This bmc advertises a Bios resource on its system and answers 404 for it.
Confluent followed the link and passed the bmc's complaint on as an
unexpected error, so a nodeconfig read printed every bmc setting and then
ended with "The requested resource of type named 'Bios' was not found",
and the system half of the configuration was a 500 saying the same.
There is already a good answer for a system that offers no bios settings,
and a link that is advertised and not served is the same thing as far as a
caller is concerned, so give it the same one. The result is checked once
and remembered, including the negative, so this costs one request on the
first ask and nothing after.
A caller asking for fans or energy got nothing from any bmc that serves
the Sensors collection. Those sensors were filed under their redfish
reading type, Rotational for a fan, while the categories are named after
the ipmi sensor types the rest of the code uses, so nothing matched.
Power appeared to work only by coincidence, Power and Current happening to
be spelled the same in both vocabularies.
Translate the reading type as the sensor is mapped, so a sensor means the
same thing whether it came from the Sensors collection, from the older
Thermal and Power documents, or from ipmi. On the bmc this was found on,
fans go from nothing to the 24 tachometers, and temperature and power
already agreed with what the same hardware reports over ipmi.
The fan controls stay out, and cannot be brought in. Their reading type
is Percent, which is also what a battery state of health reports, and this
bmc fills in no PhysicalContext to tell them apart, so there is nothing to
classify them by that would not also drag in unrelated percentages.
The redfish event log was taken from every log service the manager
advertises, whatever those turned out to be. On a bmc that keeps its
systemd journal there, nodeeventlog answered with a thousand lines of
kernel probe failures and daemon chatter, and the log the user asked for
was never read at all, because this implementation keeps it under the
system. Clearing was worse: of the services it did find, the ones with a
clear action were the dumps, so a clear destroyed diagnostic data, left
the event log untouched, and reported success.
Judge a log service before reading it. A service whose id or name says
journal, dump, post code, host logger or crash is not an event log, and
both reading and clearing skip it, so a clear can no longer take out
something that was never asked for.
If that leaves the manager with no event log at all, look under the
system, where such an implementation keeps it. Only then: a bmc that has
one under the manager is served exactly as before, from the same requests,
so this cannot change what an implementation that already worked reports.
The list of services was also being extended in place, and it belongs to
whatever the url cache is holding, so an extra log added by an oem handler
accumulated on every call within the cache window.
The web session helper sent X-CSRF-Token. MegaRAC checks X-CSRFTOKEN, so the
login succeeded and then every request answered Invalid Authentication, which is
why this helper has never worked. Confirmed both ways against a bmc: the same
request answers 401 with the old spelling and 200 with the new one.
IndicatorLED is deprecated in redfish in favour of the boolean
LocationIndicatorActive, so a platform that only implements the newer property
reported no identify state and could not be told to light up. Read either one,
preferring the older where both appear, and write the boolean when that is what
the resource offers. The boolean has no way to express blinking, so a request to
blink lights it steadily.
The led resource shares the same reader, so it gains this as well.
The lookup fell back to the generic handler whenever it was given the service
root, which is the early call during connection setup, so a bmc that names its
vendor only in the manager document was served by two different handlers on
one connection. Read the manager during that early call too.
Deleting a user gave up after one attempt and reported why the fallback of
blanking the name failed rather than why the delete did. MegaRAC reports a
timeout for a delete that a second ask completes, so retry, check whether the
account went away, and keep the original error.
Attaching media judged a device free by ConnectedVia, which describes how the
device is wired to the host rather than whether anything is in it. Every
device on this bmc reports a fixed value there, so nothing was ever selected
and the attach reported success having done nothing. Judge by whether an image
is loaded, fall back to the properties when an advertised insert action is not
served, and say so when no device would take the image.
The firmware category was passed to the library and ignored, so core,
adapters, disks and misc all returned the same full list. Classify by what
each entry is related to, and answer only for core when a platform says
nothing about where its firmware belongs.
reseat_bay reached for a hardcoded Nvidia action on Chassis_0, so it failed
with a not found for that url instead of saying reseat is unsupported.
Five resources called methods that do not exist on the redfish client, so
each answered with an internal error naming the missing attribute: the leds,
the management controller identifier, the domain name, the remote kvm licence
and the alert destinations. Implement the first four from the manager network
protocol, the graphical console and the chassis indicator.
Alert destinations stay unimplemented on purpose. Redfish describes where to
send events with EventService subscriptions, which is a different model from
the numbered PET destinations this resource was built around, so say so and
drop the code that could never run.
The location resource fetched its data and discarded it, so a read produced
no output whatsoever.
Reads are cached for thirty seconds and a write did not invalidate anything,
so setting a boot device or the identify state and then reading it back
reported the value from before the write for up to half a minute. A write can
change documents other than the one written, an action url not being the
resource it acts on, so drop the cache rather than one entry.
A bare UnsupportedFunctionality() left the user with an error containing no
text at all, or with no output and a success exit code, so asking a platform
for something it does not implement looked like nothing had happened.
Name what is unsupported at each raise, treat it as its own case in the
plugins so it reads as a limitation rather than an unexpected error and does
not log a traceback, and fall back to naming the exception when an exception
still arrives with nothing to say. The generic redfish
get_extended_bmc_configuration was also declared without async while the
caller awaits it.
Three things stopped a redfish firmware update on MegaRAC. The AMI handler
opened by asking the bmc to preserve fourteen named settings, and a build that
knows a different set rejects the whole request, which aborted the update
before anything was uploaded; send only the keys the bmc advertises. The
multipart push carried the image alone, and the specification has it carry an
UpdateParameters part too, which this firmware enforces. AMI also wants an
OemParameters part naming the kind of image, and nothing was supplying one.
The kind of image is asked for rather than worked out from the file. The
extension is vendor habit rather than format, and the leading bytes answer just
as confidently about an image they have never seen, while being wrong means a
bmc flashed with a bios image. So nodefirmware takes --type, it travels as far
as the handler that wants it, and where the bmc publishes the types it accepts,
an unknown one is refused with the list, as is asking with none. A platform
that reads the kind of firmware out of the image itself refuses the option
rather than dropping it, so nobody aims an update somewhere they did not mean
to. A parameter file still wins, since it can carry more than the image type.
Updating the bmc takes the bmc, and the task being watched, away for minutes.
That is the update working rather than the monitoring failing, so wait a
bounded while for it to answer again instead of reporting a successful flash
as an error.
MegaRAC refuses a PATCH with no If-Match header, so setting the bmc hostname,
ntp, the bmc network configuration, a location and ejecting media all failed
with a precondition error. set_identify already passed etag=*; do the same at
the call sites that did not, including the firmware push busy flag.
get_identify indexed the sysinfo method object rather than awaiting it, so
reading the identify state raised a KeyError naming a bound method on every
redfish system. Read the indicator from the chassis that owns the physical
led, since some implementations leave the copy on the system stale, and say
plainly when a platform does not report one.
The generic get_description took no fishclient while every other handler and
the caller pass one, so the description resource raised a TypeError on any
non Lenovo bmc.
Four calls reached a base method with an argument list it does not accept,
so they raised TypeError about the argument count.
Three of them would have failed either way, since the base only raises
UnsupportedFunctionality. What changes there is that the failure becomes
the intended, catchable one rather than an argument count error the caller
cannot interpret. get_diagnostic_data grew an autosuffix argument
everywhere except the ipmi generic handler, which is the handler used for
unrecognized hardware. The redfish generic handler already had it. The two
storage super() calls dropped the cfgspec they were given.
The fourth is a real fallback rather than a message: the XCC user_delete
dropped the fishclient it receives from redfish/command.py, so deleting a
uid the XCC does not list raised TypeError instead of attempting the
generic Redfish delete.
The retry around the firmware progress poll named socket.socket, which is not
an exception class, so the moment the request it guards actually failed Python
raised "catching classes that do not inherit from BaseException is not
allowed" in place of the error.
socket.error is OSError, which is what a failed poll raises and what the retry
below was written for.
Taken from fix/asyncio-port-critical, limited to what pyrefly reports.
The SMM handler still used the httplib style connect/request/getresponse that
the async webclient does not have, so nothing was ever sent. It goes through
grab_response_with_status now, with allow_redirects=False to preserve httplib
behaviour, hence the new webclient parameter. That also fixes a login passing
its headers as urlencode's second positional argument.
Delta PDU's logout was a plain function both callers awaited, so the power
paths raised TypeError. XCC's set_system_configuration was the last
synchronous implementation of a method every caller awaits.
The DedicatedSpareDrives payload was built as a set containing a list
containing a dict comprehension with a constant key, so it collapsed to a
single entry and then raised TypeError on the unhashable list. Build a
list of drive references, the same shape as the Drives list just above it.
Each of these loops rebinds the name that holds the iterable. They work
today because the iterable is evaluated once before the loop starts, but
the name is then gone, so any later use reads a loop item instead of the
collection.
- nodeinventory: `for arg in args` / `for arg in arg.split(',')`.
- confignet (common and debian copies): iname holds the comma separated
interface list and is then reused for each interface in it.
- xcc _get_agentless_firmware: adata holds the adapter query response and
is then reused for each adapter.
No behaviour change, just distinct names for distinct things.
Apply ruff's safe autofixes.
The changes are mechanical and behaviour-preserving. Issues fixed:
- F401: remove unused imports.
- F841: drop unused local variables and assignments, including discarded
await/return values, unused "except ... as e" bindings, and unused
"with ... as name" targets.
- F541: remove the f prefix from f-strings that contain no placeholders.
- E711: compare against None with "is"/"is not" instead of "=="/"!=".
- E712: test truthiness directly instead of comparing to True.
- E713: use "x not in y" instead of "not x in y".
- E714: use "is not" instead of "not ... is".
- E731: convert lambdas bound to a name into def statements.
- W291/W293: trim trailing whitespace on touched lines.
Jarrod Johnson
Markus Hilger
Jarrod Johnson