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.
Redfish identifies an account by a string, and an implementation is free
to use the account name, which this one does. The handler converted the
last element of the path to an integer, so every per user read, update and
delete answered "invalid literal for int() with base 10: 'root'" as an
unexpected error, with a traceback to match. Confluent offered the id
itself, listing the account as "root", and then could not accept it back.
Take the element as given. Everything below already compares ids as
strings, and the input parsing already keeps a non numeric uid, so only
this conversion stood in the way. nodebmcpassword goes through exactly
this path, reading users/all for the id and then writing to that account,
so it could not work at all on such a bmc.
ipmi users really are numbered slots, so the conversion is right there and
stays, but say so when it fails rather than letting a ValueError surface
as an unexpected error.
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 loop decoded the first three bytes of a name and never consumed them,
so any sensor or fru name a bmc encodes as 6 bit packed ascii spins at
full speed, appending the same four characters until the process runs out
of memory. Measured at about 10 MB a second, so a bmc using an encoding
the spec gives its own worked example of costs a pinned core and, before
long, the daemon.
Consume each group, and decode a trailing group of one or two bytes rather
than dropping it, since those carry a character each and the name would
otherwise come back short.
The arithmetic was already right, it was only never reached a second time.
Verified by encoding names per the packing and reading them back: exact for
every length except those leaving three characters in a three byte group,
where the byte count cannot say whether three or four were meant and a
trailing space is unavoidable.
A bmc may keep its sensor data records on the sensor device instead of in
a repository, and this one does, so it had no sensors, no health and only
a partial inventory over ipmi.
The records themselves are identical, version 0x51 and the same types, so
everything that decodes them is reused as is. Only the fetching differs:
a command of its own, a reservation of its own, and records held per lun
rather than in one place. The luns to ask, and a change indicator to
cache on, come from Get Device SDR Info.
The fetch is written out rather than shared with the repository one. The
loops are alike, but nothing available here has a repository to test
against, and the price of factoring them together is that a mistake would
land on every bmc that works today rather than only on those that do not
work at all.
Records are cached in memory and on disk exactly as repository records
are, keyed on the change indicator, and a device that offers no such
indicator is read afresh each time rather than cached wrongly.
Names are stripped of the nulls that pad a fixed width field. This bmc
pads every name out to sixteen bytes, and a name carrying them cannot be
matched by a caller asking for a sensor by name.
On the bmc this was written for: 163 sensors and 9 frus, against the 163
the device reports it has. The 54 temperatures and 36 fans it now reads
match what the same machine reports over redfish, to within the precision
each side gives.
The oem lookup answers whether it found a handler for the vendor, and that
answer was being stored as whether the lookup had been done at all. On
anything the map does not name, which is every bmc that is not a Lenovo,
the flag stayed false and each oem_init issued another Get Device ID and
built another handler.
Almost everything goes through oem_init, so this is a round trip added to
almost every operation. Where those calls are close together it is far
worse than that: reading the sensor data records asks for the event
constants once per record, so a run of 172 records fired 176 Get Device ID
commands back to back, which was enough to make the bmc stop answering and
the read fail with a timeout. The same sequence now takes 3 commands.
Settling for the generic handler is an answer. The device id cannot
change within a session, so asking again buys nothing, and the handler it
throws away each time is the one holding the sensor names it had cached.
A bmc that keeps its sensor data records on the sensor device rather than
in a repository was answered with a bare NotImplementedError. With no
text of its own it reached the user as "Unexpected Error:
NotImplementedError" and was logged with a traceback, for a capability the
library had simply never implemented rather than anything having gone
wrong.
Say so instead, in all three places that give up: the two branches for a
bmc without an sdr repository, and the version check that only understands
records of version 0x51, which now names the version it was given.
This makes the inventory usable on such a bmc as a side effect. The
system fru is gathered before the records are, and an unsupported
operation is tolerated where an unexpected error was not, so nodeinventory
answers with the board, chassis and product data instead of one line of
error.
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.
Reading the alert destinations of a bmc that has none reported "Unknown
code 0x80 encountered", which is the fallback text for a completion code
the library has no name for. 0x80 on this parameter is not a failure, it
is the platform saying it does not have alert destinations, and the
redfish side of the same resource has said so in words for a while.
The lan parameter fetch already knew how to tell those apart, so build the
alert reads on it rather than on a raw command that raises on any non-zero
code, and raise UnsupportedFunctionality with something to read. Both the
count and an individual destination are covered, so a platform that offers
one and not the other says the same thing instead of failing differently.
Splitting the completion code handling out of the parameter fetch is what
makes that reuse possible; the interpretation of the payload, and every
answer it gives, is unchanged.
The oem hook for the destination count was passing its byte through ord(),
which raises TypeError on the bytearray it is given. No handler in tree
implements the hook, so it had never been called; hand it the integer.
A bmc that does not implement a lan configuration parameter says so in the
completion code and answers with no data at all. The helper reached
straight into the payload, so such a parameter raised IndexError, and with
it went the whole of nodeconfig over ipmi: the bmc group, the plain, the
detailed, the extended and the advanced reads all ended in "bytearray
index out of range". It also took the attribute enumeration with it, so
the client then rejected names it should have accepted.
The guard that was there caught an exception carrying the completion code,
but oldraw_command reports the code in its response rather than raising on
it, so nothing was ever caught. Read the code from the response instead:
parameter not supported and parameter out of range mean the platform does
not have it, and anything else is a real failure that should say what it
was rather than be mistaken for absence.
The address configuration method was looked up in a table with no regard
for whether it had been read at all, so a bmc that does not report it
would have traded the IndexError for a KeyError. Answer None when it is
absent, as the address above it already does, and name the value when it
is present but unfamiliar.
A console whose bmc had gone away reported "Unexpected error - None", and
the api answered 504 with an error of None. The redfish plugin took the
text for an unreachable target from the strerror of the socket error it
caught, guarded by a hasattr that is always true: every OSError has a
strerror attribute, and it is None on most of the ones a bmc going away
produces, TimeoutError and gaierror among them. Ask for the text the same
way as everywhere else instead, which also keeps the errno on the errors
that do carry one.
The same applies to an unreachable target raised with no message at all,
so use the same helper there, on both transports.
Underneath that, give the node error messages a default to fall back on
rather than carrying whatever they were handed. Each subclass already had
one, in an __init__ that an explicit None went straight past; making it a
class attribute the base class applies means it holds however the message
was built, and removes five copies of the same constructor.
Also repair an affluent handler that put a closing parenthesis in the
wrong place, passing its error text to Queue.put_nowait as a second
argument. Any OSError there other than "no route to host" raised
TypeError from inside the except clause instead of reporting anything.
The receive loop treated only WSMsgType.CLOSE as the end of a session, but
aiohttp reports a peer that has gone away as CLOSED, and it does so
immediately and for every subsequent call. Everything that was not CLOSE
fell to an else branch that printed a line and went round again, so a
console whose bmc restarted became a full speed loop writing one line per
iteration: measured at 2.7 million iterations a second, and observed
filling 15 GB of log in a quarter of an hour while the daemon stopped
answering requests.
Treat every message that is not data as the end of the session, clear the
connected flag and report the disconnect once. A session that ended any
other way than a clean close is recorded in the trace log, unbuffered so
that it survives a daemon that does not, rather than printed.
Both websocket console plugins carried the same loop. While here, give
the openbmc one the parts tsmsol already had: text frames are data rather
than a surprise, and the client session is closed when the upgrade fails
and when the console does, instead of being leaked.
nodefirmware <node> disks reported the bmc version, and so did adapters and
misc. The generic handler takes a category and ignores it, and the ipmi plugin
does not filter either, so every category answered with the one entry ipmi can
report.
Apply the rule R13 established for a redfish inventory that does not categorise
itself: the bmc's own firmware is system firmware, so it answers for core and
for nothing else. Filtering in the handler that produces the entry leaves an oem
handler that does categorise its own firmware free to answer for more.
There is no ipmi command for a bmc hostname, so get_hostname falls back to the
DCMI management controller identifier. A bmc that does not implement the DCMI
group at all rejects that with "invalid command", which was handed to the caller
as if the request had been bad: nodeconfig <node> bmc read seven fields
correctly and then reported "Error: Invalid command", and the api answered 500
Unexpected error.
Route every DCMI request through one helper that turns "invalid command" and
"command disabled or unavailable" into UnsupportedFunctionality, so the
identifier, the asset tag and the hostname all report a platform that cannot do
this rather than a fault. Where the caller asked about a hostname, say that
rather than naming DCMI.
The whole group read still ends in one error line, because an operation a
platform cannot perform and one that failed are the same message to the client.
That is worth separating, but not here.
The unhandled tails of handle_request, handle_configuration and handle_alerts
raised a bare Exception('Not implemented'), so asking for a resource the
transport has no code for was reported as an unexpected error and logged with a
traceback. management_controller/location over ipmi is one such resource: R5
implemented it for redfish only.
Raise UnsupportedFunctionality naming the resource instead, which the plugins
already treat as its own case rather than a fault, and give decode_alert over
redfish the same treatment. Any resource added to the tree without an
implementation on one transport now reports that plainly.
nodereseat printed "Error: " and nothing else against a bmc that refused the
credentials. The redfish plugin reports it properly, but the message it emits
re-raises TargetEndpointBadCredentials with no arguments when a single node is
addressed, and the enclosure plugin renders that with str(e), which is empty.
Both hardwaremanagement plugins already had a helper for exactly this, one copy
each. Keep one in confluent.exceptions instead, teach it to fall back to the
description a confluent exception carries by class before falling back to the
exception name, and use it in the enclosure plugin too.
get_error_body had the mirror image of the same bug, joining the class
description and the message unconditionally and so answering "Bad Credentials -"
with a separator and nothing after it. The apierrorstr property beside it
already gets this right, so use it.
The exit callback opened the pid file unguarded, so when it was already gone the
atexit handler raised FileNotFoundError and python reported an exception ignored
in an atexit callback. The removal of the debug socket immediately above is
guarded, so this was an oversight rather than an intent. Verified by stopping
the service with the pid file deleted first.
terminate() called sys.exit(0) from a signal handler while the asyncio loop was
running. The SystemExit escaped run_forever, and closing the loop afterwards
then failed with "Cannot close a running event loop", so every clean stop wrote
a cascade of tracebacks and left a pending task behind.
Ask the loop to stop instead: deliver the signals through add_signal_handler,
which is the signal safe route, and set an event the main coroutine waits on so
that it returns and asyncio can unwind itself. Flush configuration on the way
out, as the client requested shutdown has always done.
That client requested shutdown went the same way, calling sys.exit from inside a
request coroutine, and it is the route the systemd unit uses to stop the
service. It now asks for the same orderly stop through a hook the running
service registers, keeping the old behaviour when nothing is registered.
Measured on all three routes, with redfish and ipmi sessions to a bmc in flight:
under a third of a second and no tracebacks, where before each one wrote a
cascade.
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.
Asking for the mac addresses of a node whose inventory does not describe any,
which is every node reached over ipmi, printed absolutely nothing and exited
successfully, leaving no way to tell an empty answer from a broken command. Name
what was asked for instead. The exit code stays successful, since an inventory
that does not mention something is a valid answer rather than a failure.
get_ntp_enabled returns None to mean the platform cannot tell us, and that went
to the caller as the literal text "None", which says nothing at all. Report it as
unsupported instead, which the tooling already renders plainly.
A discrete sensor reports no value, and the unit was appended regardless, so a
watchdog came out as "Watchdog:% " and an event log sensor as "SEL:". The unit
belongs to a reading, so only print it when there is one. On the platform this
was seen on the units field is itself meaningless for such a sensor, carrying a
percent sign and a trailing space from the sdr.
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.
A stray trailing comma made the update detail a one element tuple, so a
firmware error printed as a python tuple. A missing status printed the whole
response dict. A failure that named no node was dropped entirely, which is how
a service data request that the server refused came out as silence and a
success exit code, and nodestorage, nodelicense and nodesupport exited zero
even when they had reported an error.
nodeconsole crashed decoding an absent screenshot, and again on the terminal
calls behind a pipe, where a log replay crashed too; refuse the terminal only
modes cleanly and dump the log when there is no terminal to replay into.
nodedefine raised a ValueError on an argument without an equals sign, and
firmware for a category the target does not describe printed usage as though
the question had been malformed.
On the server side the readability check was applied to the path a download is
saved to, so asking for service data or saved licences at a path that does not
exist yet failed claiming the destination was not readable.
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.
handle_users iterated get_users with async for while the same call is awaited
a few lines below, so listing the users collection, and creating a user,
raised a TypeError about a coroutine having no __aiter__. list_inventory in
the redfish plugin had the mirror of it, awaiting an async generator.
get_extended_bmc_configuration is called with hideadvanced but the ipmi chain
never accepted it, so the extra and extra_advanced resources raised a
TypeError; thread the argument through instead.
A user slot the bmc refuses to describe no longer takes the whole user list
with it: one MegaRAC slot answered Invalid data field for good after an
account was deleted, which broke every user operation.
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.
Fifteen error kinds beyond the two async ones report nothing on this tree
today and have something in it to bite on, so turning them on costs no
findings and keeps it that way.
invalid-syntax is the one that closes a gap rather than covering ground
another check already holds: CI compiles under a modern interpreter, which
accepts syntax the el8 and sles15 interpreters cannot parse. Checking
against python-version rejects it instead, which makes that setting load
bearing for the first time.
Kinds whose subject matter this tree does not contain stay off, among them
everything reached only through typing: the module is never imported, so
TypeVar and namedtuple naming and stale `# type: ignore` have nothing to
find here.
Every rule added here is at zero once the previous commit lands, so it costs
no cleanup: the point is that a future patch cannot introduce one without the
ruff job failing. They are the rest of pyflakes' format-string checks,
flake8-2020, most of bugbear, the pylint warnings that describe bugs rather
than style, four flake8-async rules for blocking calls in coroutines, and
some RUF, LOG, PGH, PIE, ISC and EXE rules in the same spirit. Each was
confirmed to fire on a synthetic violation, so none is silently inert under
the py37 target.
Rules are named by group wherever the group is already clean, and each prefix
stops short of a rule that is not: PLW150 rather than PLW15, which would pull
in PLW1510. Bugbear is listed rule by rule apart from B02 and B03, since
B006, B007 and B018 are all left out on purpose.
bin/confluentsrv.py is a python2 script that setup.py never lists in
scripts, so no package has ever installed it, and both the systemd unit and
the sysvinit script start bin/confluent instead. It had also drifted out of
step with what it calls: main.run takes the argument vector and this passed
none.
confluentsrv.spec goes with it. It is a PyInstaller spec whose only input
is c:/Python27/Scripts/confluentsrv.py, a path that has never existed in
this tree, left over from the Windows compatibility work.
WebConnection requires a port, and every other caller passes one, twice in
this very file. Following a relay URL during discovery raised TypeError
instead.
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.
ConfluentTargetNotFound takes the node as its first argument, and every
other caller passes it. The two inventory plugins constructed it with no
arguments at all, so asking for a component that is not in the inventory
map raised TypeError instead of returning the 404 the path was written to
return.
Both now follow the pattern used a few lines further down for volumes and
name the component that was not found.
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.
The loop resolver takes only host and port positionally, so these three calls
raised "BaseEventLoop.getaddrinfo() takes 3 positional arguments but 5 were
given" every time they ran.
get_ipaddr and _find_service have no handler above them, so link local XCC
discovery and a targeted SSDP search both died outright. The snoop copy sits
under an except Exception, which swallowed it and left the MGTIFACE reply
unanswered instead.
Each is the only thing keeping its rule group from being selectable whole.
userutil.py imported ctypes with a star; the names it uses are POINTER,
byref, c_char_p, c_int, c_int32, c_uint and cdll. The oem lookup loop had an
else with no break, so the else always ran. The alert parameter table wrapped
int in a lambda that only forwards to it. And the watchdog interval passed 0
where os.environ.get documents a string, which worked because int(0) is 0.
get_sensor_names and get_sensor_descriptions reach get_psu_count for any
sensor whose table entry carries elementsfun, and get_psu_count is a
coroutine. As plain generators they could not await it, so range() was handed
the coroutine object and enumeration died with "'coroutine' object cannot be
interpreted as an integer".
Every DW612S has such entries, so nodesensors returned nothing for the
enclosure. get_sensor_descriptions was doubly broken: the Lenovo handler
already iterated it with async for, which a plain generator cannot satisfy.
Verified against a DW612S SMM (FPC variant 38). Before, descriptions raised at
the async for and readings raised partway through enumeration; after, both
return all 34 sensors, 19 of which are the PSU entries that never enumerated.
RUF006 catches a create_task whose result is discarded. The loop holds only a
weak reference, so such a task can be collected while still pending and the
work disappears without a trace.
Selected last, once the three existing offenders are gone, so the tree stays
clean under it from this commit on.
Markus Hilger
Jarrod Johnson