Closing a console gives up its claim on the session, and that talks to the
bmc, so let it fail the same way the console's own deactivate is already
allowed to.
kg was left as the caller passed it in both the register key and the reuse
check, so the mismatch fixed for the name and password still applied to it.
The count for a new socket was taken before binding it and before the io
task was known to be up. Take it last, once nothing is left that can still
fail.
Telling a keepalive that the session is gone can end up back in logout,
because reporting it is how a console gives up its claim. The inner pass
finished by clearing the register of keepalives while the outer was still
walking it, so the next entry was looked up on None. Two entries is what an
XCC has, the console's and the oem handler's.
Take the callbacks and give up the register before notifying anyone.
The check for sharing a live session compared the credentials a session
keeps encoded against the strings every caller passes, so it never matched
and each caller built another session beside the one it could not see.
Normalise both sides. Three commands to one bmc went from three sessions on
three sockets to one, and from five sessions open on the bmc to three.
Not from the port to asyncio: upstream compares the same two things the same
way.
One session is now routinely handed to a console and a command at once, and
logout closed it for both, leaving whoever was left holding one that
answered as though it had been lost. Count the holders and give up a claim
instead, unless the session is no longer usable, which logout is told by
sessionok.
A console had no way to give a claim back: close deactivated its sol payload
and left the session alone, which was right when closing meant closing it
for everybody and is a leak now. Both of its exits release it.
__init__ opened by checking for an initialized attribute, meaning it had
been handed a session someone else was establishing. That attribute is only
ever set further down in the same method, so the check cannot be true, and
the port lost the return that made it work upstream. Waiting for someone
else's login is done in __new__ now, so this is dead code claiming to
protect something.
logout decremented the count of sessions on a socket twice over, and
_mark_broken again for the case logout had not, so it went negative and kept
falling. _assignsocket picks the least used socket and refuses one at
MAX_BMCS_PER_SOCKET, and both of those read that number.
initting_sessions exists so a caller can share a session already on its way,
but the entry was added only once the login had finished, leaving the login
itself uncovered. Two callers asking at once each built a session, both on
the socket the other had not claimed yet, and replies route by bmc address
and local port, so only the last to transmit was ever answered. That is the
console session failing about one attempt in three.
Register before the login and remove the entry in a finally. The two old
removals keyed on the encoded credentials while the register is keyed on the
caller's strings, so they never matched and an entry outlived its session.
A session handed over mid login is now waited for rather than returned as
one that answers as though it had been lost.
A session that failed raised with no message at all, so a failed console
read "IpmiException: None". Record the reason wherever a session is marked
broken.
The asyncio rework mistakenly follows up a long waiting acquire with blanking and starting over.
Now use 'True' as a sentinel value to trigger a fetch, otherwise, assume srp is ripe for processing.
Immediately discard srp after unpack and replace with sentinal value.
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.
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.
nodeconsole spawned a task per input byte from the stdin reader callback
and kept no reference to it. Two of those tasks overlap as soon as one
parks in relay_keypresses waiting on the VNC connection, so keystrokes
can reach the node out of order and the escape sequence state (buffer,
inputcontext, modkeys) is mutated by more than one task at a time. With
the first keystroke relaying slowly, typing abcdef arrives as bcdefa.
Those tasks were also unreferenced, which asyncio documents as
collectable while still pending, so a keypress could be dropped.
Queue the bytes in the reader callback and process them from one
long-lived task instead. Keep a reference to the watch_input task as
well, since collecting that one takes the whole input handler with it.
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.
If someone wants to seal to a PCR
explicitly to prevent booting rescue, the PCR is likely to
extend differently during install.
Leave the volume sealed to the tpm without any PCRs until first boot.
Then wipe the bindings without PCR specified, and seal according to user preferred values.
Jarrod Johnson
Markus Hilger
Jarrod Johnson