Skip to content

Bugfix/connection fail reporting - #326

Open
graepaul wants to merge 14 commits into
developmentfrom
bugfix/connection_fail_reporting
Open

graepaul wants to merge 14 commits into
developmentfrom
bugfix/connection_fail_reporting

Conversation

@graepaul

@graepaul graepaul commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Move connection failure handling into the connection managers. ConnectionManager._fail_connect() records one CRITICAL event, clears the connection and sets EXECUTION_FAILURE. connect_decorator is the backstop: an unexpected exception, or a result that is still UNSET with no connection, becomes a failure with a CRITICAL event, so no failed connection is silent.
  • Save connection results separately from collector results. A connection created by a plugin is written to <plugin>/<collector>/connection/
  • Add DataPluginResult.connection_result, a snapshot of the connection result for the plugin run, and put connection result in DataPluginResult
  • Do not retry a failed connection. A manager whose last attempt ended in ERROR or EXECUTION_FAILURE is not reconnected by later plugins. A manager that is UNSET, OK or WARNING with no live connection (for example after a disconnect) is still reconnected.
  • disconnect() keeps the connection result instead of resetting it to UNSET. Add safe_disconnect(), which logs instead of raising, so one failing disconnect cannot stop the rest from closing.
  • preserve_connection now defaults to True on DataPlugin.collect() / run(). A manager the plugin created is always closed; a manager passed in by the caller stays open unless preserve_connection=False. The executor no longer force-sets it.
  • FileSystemLogHook no longer overwrites earlier logs. The first result in a directory goes to result.json / events.json, later ones to result(1).json / events(1).json, and so on. The data-model file is not numbered.
    _close_connections() are split out of run_queue, and connections are always closed even if a post-action or collator raises.
  • Move the Redfish, SSH proxy and OOB SSH managers onto _fail_connect(), so each failed target produces an event.
  • Add TypeUtils.filter_to_model_fields() to replace three copies of the global-arg filtering.

Result format example

OsPlugin against a remote host over SSH, all steps successful (scraper_logs_....)

scraper_logs_<dt>/
├── nodescraper.log
├── nodescraper.csv
├── system_info.json
├── os_discovery/
│   └── result.json                      # OS-discovery connection (new location)
├── os_plugin/
│   ├── in_band_connection_manager/
│   │   └── result.json                  # shared connection, saved under the plugin that opened it
│   ├── os_collector/
│   │   ├── result.json
│   │   ├── events.json
│   │   ├── osdatamodel.json
│   │   └── command_artifacts.json
│   └── os_analyzer/
│       ├── result.json
│       └── osdatamodel.json
└── memory_plugin/
    ├── memory_collector/         # No connection result here because preserve_connection = True, if False then it will be there
    │   ├── result.json
    │   ├── events.json
    │   ├── memorydatamodel.json
    │   └── command_artifacts.json
    └── memory_analyzer/
        ├── result.json
        ├── events.json
        └── memorydatamodel.json

Typical result

{
  "status": "OK",
  "message": "task completed successfully",
  "task": "InBandConnectionManager",
  "parent": "os_discovery",
  "details": {},
  "start_time": "2026-10-27T07:21:44.196857",
  "end_time": "2026-10-27T07:21:45.691677"
}

Console Log result

+-------------------------+--------+-----------------------------+
| Connection              | Status | Message                     |
+-------------------------+--------+-----------------------------+
| InBandConnectionManager | OK     | task completed successfully |
+-------------------------+--------+-----------------------------+

+----------+--------+-------------------------------------+
| Plugin   | Status | Message                             |
+----------+--------+-------------------------------------+
| OsPlugin | OK     | Plugin tasks completed successfully |
+----------+--------+-------------------------------------+

On a failed connection, the same result.json is written with "status": "EXECUTION_FAILURE" and the reason in message, next to an events.json holding the CRITICAL event. Dependent plugins report NOT_RAN and do not reconnect.

Failed Example:

+-------------------------+-------------------+--------------------------------------------------------------------------------+
| Connection              | Status            | Message                                                                        |
+-------------------------+-------------------+--------------------------------------------------------------------------------+
| InBandConnectionManager | EXECUTION_FAILURE | task failed to run (1 errors; critical: SSH connection to 'system-name         |
|                         |                   | -lab-123-56' failed: SSH network error: [Errno 21] Is a directory:             |
|                         |                   | '/home/notadir')                                                               |
+-------------------------+-------------------+--------------------------------------------------------------------------------+


+--------------+---------+----------------------+
| Plugin       | Status  | Message              |
+--------------+---------+----------------------+
| OsPlugin     | NOT_RAN | Plugin tasks not ran |
| MemoryPlugin | NOT_RAN | Plugin tasks not ran |
+--------------+---------+----------------------+

The failed example will have a result like below found in a folder like ~/node-scraper/scraper_logs_2026_10_27-08_12_41_AM/os_plugin/in_band_connection_manager/events.json

[
  {
    "id": "2258a79e-abc9-4e0b-a8eb-de24b79dec06",
    "timestamp": "2026-10-27T15:12:42.071014Z",
    "reporter": "NODE_SCRAPER",
    "category": "SSH",
    "description": "SSH connection to 'system-name-lab-123-56' failed: SSH network error: [Errno 21] Is a directory: '/home/notadir'",
    "data": {
      "task_name": "InBandConnectionManager",
      "task_type": "CONNECTION_MANAGER",
      "parent": "OsPlugin",
      "session_id": "676dc412-bda8-4a62-be6e-bd6b18258057"
    },
    "priority": "CRITICAL"
  }
]

Behaviour changes

  • preserve_connection defaults to True. External callers that pass connection_manager= and relied on it being closed after run() must close it themselves or pass preserve_connection=False.
  • Repeat results in one directory are numbered instead of merged into events.json.
  • ConnectionManager.disconnect() no longer resets the result to UNSET.

Test plan

  • pytest test/unit
  • pytest test/functional (if applicable)
  • pre-commit run --all-files

Checklist

  • Added/updated tests (or explained why not)
  • Updated docs/README if behavior changed
  • No secrets or credentials committed

console_log=True,
)
self.connection = None
self.result.status = ExecutionStatus.EXECUTION_FAILURE

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.

i think we should also set self.result.message here otherwise it will be empty for a failed SSH or Refish connection

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have added a section which includes message + event description instead of only the event descriptions

https://github.com/amd/node-scraper/pull/326/changes#diff-e9ae472ff94c6628c26eee61d1d452bc63b8f2c17cee86530d824fb6caec6901R183-R216

Comment on lines 256 to +266
@@ -244,12 +263,10 @@ def discover_os_info(self) -> None:
discover_and_write_os_family(inband_connection, self.system_info, self.logger)
except Exception as e:
self.logger.error(
"Error occurred during OS discovery with InBandConnectionManager: %s",
str(e),
"Error occurred during OS discovery with InBandConnectionManager: %s", str(e)

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.

there might be another issue here: result.status is not set to OK its left as UNSET in the decorator here: nodescraper/interfaces/connectionmanager.py when shell is successful and SSH returns, so maybe we need to fix that first. Then we need to update this code and let this: discover_and_write_os_family(inband_connection, self.system_info, self.logger) run after a successful connection

@graepaul graepaul Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Most connection managers do not set a status or a message however since we have the connect_decorator it will always either get an exception resulting in log_event call and the result being set to EXECUTION_FAILURE here

connection_manager.result.status = ExecutionStatus.EXECUTION_FAILURE
connection_manager.result.message = f"Connection failed: {str(exception)}"

However, if there is a connection pass then the connect_decorator will eventually call result.finalize() which will reach _update_status() here, this will then use the event's here to determine what self.status should be, if there are ERROR or CRITICAL events the status is ERROR and if there are WARNING events then the status is WARNING, otherwise the status is set to OK.

So if connect is called it will always cause a result of one of
NOT_RAN
OK
WARNING
ERROR
EXECUTION_FAILURE

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants