Skip to content

fix(device): read the subsystem dict, not its keys - #1321

Open
BombermaG wants to merge 1 commit into
simplyblock:R26.3from
BombermaG:fix/device-probe-subsystem-R26.3
Open

BombermaG wants to merge 1 commit into
simplyblock:R26.3from
BombermaG:fix/device-probe-subsystem-R26.3

Conversation

@BombermaG

Copy link
Copy Markdown
Collaborator

subsystem_get() returns a single subsystem dict, but the probe passed it to list.extend(),
which appends the dict's keys instead of the dict.
Listener and namespace were therefore always reported missing, even on a perfectly healthy device.

device_repair() calls this probe twice: once to decide what to rebuild, and once
afterwards to verify. Both calls got the same wrong answer, so:

  • the early return that skips an intact stack without spending an attempt (if not missing) could never be reached;
  • _def_create_device_stack() is additive and correctly rebuilt nothing;
  • the verification probe then reported the same phantom gaps, so a successful no-op was recorded as a failed attempt.

Five of those and the device is marked retries_exhausted, which disables automatic repair for it permanently - including for a real failure later.

Self-repair therefore never confirmed success on any device since it was introduced in 6872469.

R26.3 only. main does not have the self-repair feature yet, so it does not have this bug.

subsystem_get() returns one subsystem dict, but the probe passed it to
list.extend(), which appends the dict's keys. The first element was then
the string 'nqn' instead of the dict, so listener and namespace were always
reported missing, even on a healthy device. Self-repair uses this probe
twice: once to decide what to rebuild, once to check the result. It rebuilt
nothing, called itself failed, and spent an attempt on every run. After five
attempts the device is marked retries_exhausted and gets no more automatic
repair.

@mxsrc mxsrc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! With this in mind we can simplify further:
The whole _get_subsys is not required anymore, and subsys can simply be assigned the result of subsystem_get(...). It raises on errors, so the _safe wrapper is not required anymore. The list type is a left-over of the previous filtered subsystem_list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants