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.
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.
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.
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.
run_handler scheduled the coroutine that serves an async HTTP request and
dropped the returned task. The event loop only keeps a weak reference, so the
task could be collected while still pending, leaving the request unanswered
and "Task was destroyed but it is pending!" in the log.
The session already outlives the request in _asyncsessions, so it holds the
task in a set and discards it from a done callback.
local_node_trust_setup() called get_cluster_list() and sign_host_key() without
awaiting them, so "osdeploy initialize -l" aborted with "TypeError: cannot
unpack non-iterable coroutine object" before doing any work.
Both awaits have to land together: sign_host_key() is called in a loop that
unlinks the existing ssh_host_*_key-cert.pub before writing the new one, so
fixing only the unpack would delete every host certificate and then fail.
It has been observed there are times where an ethernet switch is partially working with MLD snoop/IGMP snoop. A workaround for the unreliable behavior seems to be to reassert multicast joins ever so often.
Give it a try to restart the SSDP sockets every minute.
The plugin was written against the http.client based SecureHTTPConnection, and
when that went away the reference was pointed at the aiohttp WebConnection,
which shares the name and nothing else. Nothing in it could run: the transport
called an async request() without awaiting it and then reached for a
getresponse() the new class does not have, and three PDUClient methods that
were never coroutines were awaited by the entry points.
Two transports now, both local to this plugin. https is aiohttp and stays on
the event loop, since the cert verifier records new fingerprints through
tasks.spawn. http is http.client in a thread, with its own socket so it can
still ask for a smaller segment size before connect: aiohttp only takes a
socket factory from 3.12 on, newer than el9, el10, ubuntu 24.04 or Leap 16
ship. That side has no cert to verify and its credentials arrive already read,
so the thread touches nothing.
connect() establishes and authenticates, wc is just the accessor now, and
logout() no longer sends a session id it never obtained. update() reports an
unsupported element instead of raising NameError.
On the https side cookies follow aiohttp's domain rules and the one POST with
a body goes out as text/plain, where http.client replayed every cookie and
sent no content type. The http side is as before, and neither can be settled
without an Eaton PDU on the bench. Both transports were exercised against a
stand-in: login, outlet read and set, sensors, logout, and a clamped segment
size on the plaintext path.
virEventRunDefaultImpl waits for an event that an idle domain need not
produce, so the thread could outlive a deactivation that reported success, and
every later activation was refused while it did. Registering a timeout is what
makes it return: measured, a thread with nothing registered was still running
four seconds after being asked to stop, and with a half second timer it came
out at once.
Deactivation dropped its reference once the wait expired, whether or not the
thread had stopped, so the next activation started a second one and revived
the first by setting run_console again. The reference is cleared only when the
thread is really gone, and activation refuses with 0x80 while one is alive.
That makes the wait a courtesy rather than a correctness measure, so it drops
to a second.
Activation started an event thread whichever way the base handler had just
answered, so a refusal started one anyway and an already active console got a
second. activated alone cannot tell the two refusals apart, being true
already on the already active path, so the value from before the call decides.
Deactivation joined that thread on the event loop, where it could stall every
other session and its own response. The wait moves off the loop and is
bounded, and the thread is a daemon.
The console awaits its output handler, but both sample BMCs supplied a plain
function, and both dropped the send_data coroutine. virshbmc additionally
receives its stream callback on a libvirt thread, so the send goes through
run_coroutine_threadsafe against the loop captured at activation, called
asyncloop because the class already has a loop method it uses as a thread
target.
ServerConsole asked the session layer to retry, which a ServerSession cannot
do: it never runs Session.__init__, so it has no timeout, and its _timedout
does nothing. IpmiServer.logout was synchronous and an argument short while
_cleanup awaits logout(False). The boot options handler answered, then read an
unbound name and answered again with 0xff.
bmc.py, serversession.py, fakebmc.py and virshbmc.py were byte identical to
upstream pyghmi: the async port went through the session layer beneath them
and left the server side alone. So every response created a coroutine and
dropped it, and the overrides the now async parent awaits returned None.
Running fakebmc bound no socket, spun a core, and answered nothing.
Everything that sends a response is a coroutine now, and so is the dispatch
that reaches it; the hooks a subclass implements stay ordinary functions, so
an out of tree Bmc is unaffected unless it overrides the payload handlers.
Two things had to leave their constructors, both being coroutines: assigning
the server socket, into bind(), which is why listen() is no longer a
classmethod, and answering the open session request, into
send_open_session_response.
Verified against upstream with ipmitool over nine commands, with identical
output. SOL is not covered: fakebmc reports the payload disabled on both.
Housekeeper dates from when this work was a blocking select loop. Under
asyncio the event loop is already that place, and a thread around it takes a
captured loop reference, a scheduling step before the thread starts, and a
daemon flag, only to sit waiting on a coroutine that never returns with no way
to stop it. A task runs on the right loop by construction and cancels. The
loop is looked up before the coroutine is built, so calling this without one
raises rather than stranding it.
Nothing in this tree used the class. Out of tree callers need
start_housekeeping() instead, and gain the ability to stop it.
TsmConsole created an aiohttp ClientSession and never closed it, and leaked it
again when ws_connect failed. Neither was reachable before the connection path
was repaired. It is closed on both paths, and starts as None so that closing
before a connect does not trip over a missing attribute.
A task per read swallowed failures, could deliver keystrokes out of order, and
at end of input returned with the reader still registered, so a level
triggered selector called it again for the same EOF. The reader queues now,
one consumer sends in order, and it is gathered with the main loop so a
failure reaches the caller.
Three faults in the same few lines. The redfish Command lost its constructor
for an async create, so building one raised TypeError, which the except below
reported as TargetEndpointUnreachable. await_redirect is defined nowhere in
this repository's history, so that call raised too; create performs the
session setup it was meant to trigger. And oem is a coroutine method rather
than an attribute, with its web connection coming from get_wc, which is what
performs the login that sets csrftok.
ConsoleSession grew an async create and lost its constructor, and ShellSession
inherits that. sockapi was updated for the console branch but not the shell
branch immediately below it, so opening a shell session raised TypeError. It
is the only place in the tree that builds one.
Command.eventloop called wait_for_rsp with no timeout. With nothing waiting or
being kept alive there is no deadline to derive one from, so it returns
without suspending and the loop runs flat out, measured at over 100000
iterations in two tenths of a second.
MAX_IDLE gives it something to wait on, as a ceiling rather than a fixed
delay: real deadlines still shorten it and an arriving packet still ends it
early.
The Console was never connected, so main_loop ran against a session that had
never been established. Input arrived on a thread that called send_data and
dropped the coroutine; the thread is gone and the loop watches stdin with
add_reader instead. The output handler was a plain function that
Console._print_data awaits.
Command lost its constructor for an async create classmethod, so the utility
raised TypeError before connecting. The onlogon callback it was built around
is gone as well: create establishes the session itself, so both the callback
and the eventloop that waited for it are unnecessary.
docommand awaited nothing, so every operation produced a coroutine that was
printed and dropped, and three of its calls are async generators. Each BMC is
handled in turn now rather than only the last.
Console.main_loop drives Session.wait_for_rsp, which is a coroutine, so it
spun without ever waiting for a packet. It is a coroutine now, and pyghmicons
runs its main under asyncio.run. pyghmiutil had the same shape around
Command.eventloop.
sockapi sent its collective refusal without awaiting tlvdata.send. redfish
handle_sensors returned a coroutine from most branches and None from the short
ones, so the caller's await raised TypeError; it is a coroutine throughout
now. console send_payload waited for a response without awaiting the wait.
Two suppressions are pyrefly limitations rather than bugs: Session defines an
async __new__, and the keepalive registry holds coroutine functions in an
untyped dict.
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.
Markus Hilger
Jarrod Johnson
Jarrod Johnson