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.
These __main__ blocks called coroutines as if they were functions, so they did
nothing at all. Single calls go through asyncio.run; sshutil, proxmox and
vcenter needed an _selftest coroutine. Two were invisible to pyrefly because
repr() and list() count as using the result: vcenter's get_vm_serial needs an
await, and proxmox's get_vm_inventory is an async generator.
lldp called _extract_neighbor_data twice, once correctly, so the bare call is
dropped. xcc3.remote_nodecfg is not a self test: every other handler defines
it as a coroutine and selfservice.py awaits it.
Catches unawaited coroutines and awaits on non-awaitables, which ruff and
compileall cannot see. Only those two kinds are enabled, and in pyrefly.toml
rather than a --only flag so CI, local runs and editors agree; a full check
reports thousands of errors on this largely unannotated tree.
search-path is what lets imports resolve, and without it a large share of the
findings, including everything in the SMM handler, goes unreported. Pyrefly
walks *.py only and a glob does not lift that, so extensionless tools are
named individually. No baseline: it matches by file, kind and column, so a new
mistake at the same indentation as an old one would pass unnoticed.
Advisory until the existing findings are dealt with.
lxml refuses a str carrying an encoding declaration, and the SMM declares one
when it answers /data/login, so _webconfigcreds has raised ValueError on the
first thing it does after logging in ever since the switch to lxml. stdlib
ElementTree took the same input, which is why it went unnoticed. fromstring
already means to take either shape, so encode there. Confirmed against a
DW612S.
compileall only ever compiles *.py. Handed anything else, even by name on the
command line, it skips the file and still exits 0, so the job has been
checking 218 of the 298 Python files in this tree and reporting success for
the rest. Everything without the extension went unchecked: the whole of
confluent_client/bin and confluent_server/bin, the osdeploy deploy scripts,
the loose misc utilities and the setup.py.tmpl templates.
Those files now go to py_compile, which compiles what it is given. The list
is built from python shebangs, read with the shell builtin rather than by
forking head and grep per file, plus four patterns for the files that carry
no shebang at all and so cannot be detected: the setup templates, configbmc,
add_local_repositories and misc/filterpasswd. It comes to the same 298 files
ruff.toml arrives at through extend-include, and wants keeping in step with
it.
The shebang test matches python anywhere in the line rather than after a
slash or space, because several tools use /usr/libexec/platform-python.
Adds the checks whose findings were cleared in the two preceding commits
(F401, F541, E701, E711, E712, E713, PLC0414) plus three that were already
at zero and cost nothing to lock in: E401, B015 and B023.
B905 is deliberately left out even though it also reads as clean: it only
reports on py310+, and satisfying it would mean adding a keyword the oldest
interpreters this tree runs on cannot parse.
Hand written rather than autofixed, since three of the four need the
surrounding code read to be sure they are equivalent:
- confetty: `powerstate == None` -> `is None`.
- nodeconfig: `setmode != True` / `!= False` -> `not setmode` / `setmode`.
Safe because setmode only ever holds None, True or False, and the two
lines above each test normalise None away first.
- pam: split two `if cond: stmt` one-liners.
- imgutil: `from shutil import copytree as copytree`, an alias that renames
nothing. Not a re-export marker, this is a script.
Entirely mechanical, produced by `ruff check --fix --select F401,F541,E713`
and reviewed rather than taken on faith: deleting an import is only safe if
nothing imports it for its side effects or re-exports it. None of the 19
removed names is referenced anywhere in its file, none appears in any string
literal, and none of the touched files uses eval, exec, globals() or
__import__, so there is no dynamic lookup that could reach them.
The enforced rule set is deliberately narrow: undefined names, statements
in impossible positions, duplicate definitions, invalid escapes and a
couple of bugbear checks that only fire on genuine defects. No style
rules, and the tree is clean under it as of the preceding commits.
Discovery needs help. Ruff only walks *.py, and about a quarter of the
Python here has no extension: every node* CLI tool, the server bin tools,
the osdeploy scripts (some of which carry no shebang either) and the
setup.py templates. extend-include lists them, and *.sh is excluded so
the shell scripts sharing those directories are not parsed as Python.
The CI job pins both the action and the ruff version, since there is no
pyproject.toml for the action to read a version from and an unpinned
`latest` would let a new ruff release fail an unchanged branch.
The guard evaluated the `next` builtin and discarded it, which does
nothing, so a pending node with no handler fell through to
None.NodeHandler(...). The AttributeError was caught by the enclosing
except and logged as "Unexpected error during discovery", turning a node
that should have been quietly skipped into a spurious error in the log.
load_plugins() used `plugin` as the loop variable for plugin file names,
which shadows `import confluent.plugin as plugin` for the whole function.
Nothing in the function needed the module, so this was latent rather than
broken, but the next line that does need it would have failed oddly.
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.
Three names were defined twice in the same scope, so the first definition
was unreachable:
- lenovo OEM handler: two set_user_access methods, the second silently
replacing the first. That made the SMM privilege update dead code. The
conditions are mutually exclusive (is_fpc returns None once has_xcc is
true), so merge both into the surviving method.
- redfish plugin handle_cert_authorities and prepfish
disable_host_interface: byte identical copies, drop the redundant one.
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.
Every one of these raises NameError if its code path is reached:
- nodeapply: run_automation accumulated into an exitcode that only existed
in run(), so any automation error crashed instead of being reported. It
now keeps and returns its own, tracked separately from the exit code of
the ssh commands: the early exit after the spawn loop tests that one,
and folding automation failures into it would exit with children already
running and their pipes abandoned. Both are reported at the real exits.
- nodeconsole: redraw() reads firstnodename, which was local to
do_screenshot(); promote it to a module global like the other drawing
state.
- nodedeploy: the redeploy path appended to a lockednodes list that did not
exist yet. The block that follows re-reads the same lock state and acts
on it, so drop the dead duplicate.
- samples/nodeattrib_from_switch.py, misc/filterpasswd: missing import sys.
- xcc3: fixuuid was never imported. xcc imports xcc3, so take a local copy
the way the smm handler does instead of creating an import cycle.
- httpapi: the async session call still passed the WSGI-era env and an
extra argument to handle_async(), which has taken only querydict since
the aiohttp port. Calling it correctly exposed that handle_async()
registers an AsyncSession before raising on the discontinued long poll
path, so every request to it would leak a session that is never reaped.
It now only creates one when there is a websocket handler to yield it to.
- messages: the InputFirmwareUpdate.filename property checked
self.filebynode[node] with no node in scope. __init__ already validates
every expanded path and nodefile() rechecks per node, so drop the checks.
- pam: drop the python2 branches referencing unicode and raw_input. The
server has been python3 only since the asyncio port.
- cooltera: the sensor-name listing referenced a nonexistent sensors dict.
The available sensors depend on the model, which is only known after
reading the device, so list them from the same status data the readings
use.
- deltapdu, eatonpdu, geist: the not-implemented response in update() used
node outside the loop, unlike retrieve() in the same files and unlike
raritan/enlogic.
- confluentdbgcli: stray self. on a module-level socket connect.
This allows a user to opt into pcrs if they understand what they are doing.
Some PCRs are sensitive to firmware updates and some are sensitive to boot loader, kernel, boot config, or initramfs. All of these are an opportunity for an unsuspecting update to remove access to the boot volume. There are update processes that can be put into place to make this work,
but it is up to the OS update process to address that, and
OS update processes are likely not to address that at this time.
reply_dhcp4 logs the insecure mode remediation hint on every DHCP
discover it refuses. A node in this state never receives a reply, so it
retries for as long as it is powered on and the same message repeats
every few seconds.
Rate limit it per hardware address the way the neighbouring boot attempt
messages already do, reusing the ignoremacs window that check_reply uses
for the missing profile hint.
The per-MAC 90 second log throttle in proxydhcp has been inert: the
`skiplogging = True` reset sat in relay_proxydhcp, where it is a dead
local, while the loop in proxydhcp only ever assigns False. Once the
first packet is handled the flag stays False for the life of the
process, so every retransmitted boot request logs again even though
ignoredisco is updated to suppress it.
Reset the flag at the top of each loop iteration instead, next to the
timestamp check it belongs to, and drop the dead assignment.
reply_dhcp4 declines to answer a PXE boot request unless
deployment.useinsecureprotocols is set to firmware or always, but
proxydhcp had no such check. A node left at the default of never was
therefore still offered a TFTP bootfile and a plain http boot.ipxe URL
whenever the request arrived on port 4011 rather than port 67, so the
attribute silently did nothing in ProxyDHCP deployments alongside an
independent DHCP server.
Apply the same gate, including the UEFI HTTP boot exemption, and log the
same remediation hint. The node attributes are now fetched once and
passed through to get_deployment_profile instead of being looked up
again there.
Requests whose architecture could not be determined are ignored rather
than falling through to the reply. opts_to_dict stops parsing before the
client architecture option whenever the message type is not a request,
and such a packet would otherwise reach the iPXE branch and be handed a
plain http boot.ipxe URL without ever passing the gate.
The default 'cp' is /lbin/cp, but that fails, use full path to the cp that works.
Perform the hmac registration of api key that was missing.
Remove assumption that the ip will be ipv6, wrapping it only if a : is present in address.
The rsync push carried no preservation flags, so files arrived with their
special permission bits explicitly disabled and a setuid/setgid entry could
only be honored by the permissions= chmod on the client side.
Preservation was turned on once before in e52a9ff70f ("Have syncfiles
attempt to preserve more") and rolled back the same day in c0287e93ed
("Roll back rsync ownership"), because rsync also applied the staging copy's
attributes to the parent directories it merely traversed on the way to the
synced files, clobbering the permissions of system directories such as /etc.
Naming every staged file explicitly through --files-from and adding
--no-implied-dirs confines preservation to the content actually being
synchronized, leaving traversed directories alone and creating missing ones
with default attributes.
Two details follow from the way the staging tree is built. Files are staged as
symlinks, so rsync reads their attributes through to the real file, but
directories are staged as directories and need the source attributes copied
onto them for the otherwise empty ones that have to be named explicitly.
Ownership is mapped from the account the daemon runs as to root, since that
account generally does not exist on the node and would otherwise arrive as a
meaningless numeric id.
--xattrs from that earlier attempt is deliberately left out: with --copy-links
rsync reads xattrs off the symlink rather than its referent, so it transfers
nothing here while adding a failure mode on hosts without xattr support.
chown() clears the setuid bit of a file on Linux (and its setgid bit, if
the file is group-executable), even when run by root and even when the
owner/group are unchanged. Since the owner/group chown ran after the
permissions chmod, any syncfiles entry combining owner=/group= with a
setuid/setgid permissions= value silently lost the special bits.
get_syncresult() caught the sync task's exception, logged a repr server
side and returned 200 OK with a null body. The node then called
.get('options') on that null resulted in:
c1: 'NoneType' object has no attribute 'get'
and syncfileclient still exited 0 as if syncing had succeeded.
Return the error to the requestor as a 500 with an error payload. On
the node, unwrap the body that grab_url_with_status raises for a
non-success status, print it once and exit non-zero. Only a failure the
server deliberately reported for this sync is terminal. Anything else,
such as a dropped connection, is re-raised so the existing retry loop
handles it as before. The same case now reports
c1: Error performing syncfiles: Syncing failed due to unreadable files: /etc/dangling.conf
c1: 'syncfileclient' exited with code 1
nodeattrib/nodegroupattrib -e replaced '.' with '_' in the attribute name
before handing it to the server, not just when looking up the environment
variable. Any attribute with a dot in it was therefore rejected, e.g.
$ export info_note=test
$ nodeattrib -e gpu1 info.note
Traceback (most recent call last):
File "/opt/confluent/bin/nodeattrib", line 97, in <module>
exitcode=client.updateattrib(session,args,nodetype, noderange, options, argassign)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/confluent/lib/python/confluent/client.py", line 688, in updateattrib
key, os.environ[key.upper()])
~~~~~~~~~~^^^^^^^^^^^^^
File "<frozen os>", line 714, in __getitem__
KeyError: 'INFO_NOTE'
$ export INFO_NOTE=test
$ nodeattrib -e gpu1 info.note
Error: Bad Request - info_note attribute on node gpu1 is invalid
Keep the attribute name intact and derive the environment variable name
from it separately. A missing environment variable now reports which
variable names were looked for instead of raising a bare KeyError
traceback.
Markus Hilger
Jarrod Johnson
Jarrod Johnson