Skip to content

Make the fault storage backend swappable, relational or not #650

Description

@bburda

Summary

We want a storage backend other than SQLite to be added without touching the rest of the fault
manager. PostgreSQL is the concrete case being worked on, but the goal is wider than one
relational database: a document store should be able to plug in the same way.

FaultStorage already covers the fault data itself, and its method signatures are plain CRUD.
Nothing about SQL, cursors or the connection type leaks into the interface, so a non-relational
backend is not blocked by the interface shape. What blocks it is that several parts of the
storage layer around the interface still assume SQLite or a local file path, and that the
guarantees a backend has to provide are nowhere written down.

What is in the code today:

  1. The backend is picked by a hardcoded if/else in FaultManagerNode::create_storage()
    (fault_manager_node.cpp). Adding a backend means editing the node. There is no
    registration point for a backend that lives in another package.

  2. An unknown storage_type logs an error, falls back to InMemoryFaultStorage and the node
    keeps running. A typo in the parameter, or a backend that did not register, gives a fault
    manager that looks healthy and drops all fault history on restart. Startup should fail
    instead.

  3. database_path is a filesystem path. create_storage() creates the parent directory and
    :memory: is a special value. A network database needs a connection string, and there is
    currently no parameter to put host, port, user or database name in.

  4. The audit log is behind no interface at all. FaultAuditLog includes <sqlite3.h> and owns
    its own sqlite handle. When audit_log.database_path is left empty and storage_type is not
    sqlite, create_audit_log() sets the path to :memory:. The startup line does print
    :memory:, but nothing warns that the audit log will not survive a restart, so audit logging
    looks enabled while it is not persisted.

  5. There is no conformance suite an out-of-tree backend can run. test_rosbag_storage_parity.cpp
    is a typed test over InMemoryFaultStorage and SqliteFaultStorage, compiled into this
    package's own test binary. A backend in another package cannot register itself into it, and
    the parity coverage is only rosbag retention, not the rest of the FaultStorage contract.

  6. Some FaultStorage methods have default implementations that do not meet the contract written
    above them. store_rosbag_files() defaults to a plain loop, and its own comment says that
    default is not all-or-nothing. A backend that does not override it compiles and looks correct,
    then can leave rows pointing at a bag that was already removed.

What a backend actually has to guarantee

These hold today only because both in-tree backends are transactional and strongly consistent.
None of them is stated as a requirement anywhere, so a new backend can miss one and still compile,
run and look correct. This is the part that decides whether a given store can back the fault
manager at all.

  • Multi-record atomicity is required, not optional. BEGIN IMMEDIATE is used in
    store_rosbag_files(), store_snapshots() (insert plus cap eviction), delete_rosbag_recording(),
    delete_rosbag_files() and the healing path of report_fault_event(). The comment on
    store_rosbag_files() already says it must be all-or-nothing, because the caller treats a throw
    as "no row was written" and removes the recording. A store without multi-record transactions
    cannot satisfy this as written.
  • Two queries are aggregates. get_total_rosbag_storage_bytes() is a dedupe-then-sum
    (SUM(MAX(size_bytes) GROUP BY file_path)), because a recording shared by a burst must count
    once. get_max_capture_id() is a MAX over every snapshot. A key-value store would have to
    maintain these as counters.
  • One query is a join. list_rosbags_for_entity() joins the rosbag records to the faults and
    expands the reporting_sources JSON array. A document store needs either denormalisation or a
    second round trip.
  • Ordering needs a monotonic tiebreaker. A burst stamps one created_at_ns on every record it
    writes, so ties are guaranteed, not rare. SQLite breaks them by rowid and the in-memory backend
    by its own seq counter. A backend without an equivalent orders differently, and the parity
    tests would go flaky instead of failing honestly.
  • Read-your-writes is assumed. The capture path writes and then reads back immediately. A
    backend reading at eventual consistency breaks this.

Proposed solution

Keep FaultStorage as the data interface and close the gaps around it:

  • Replace the if/else in create_storage() with a registry, so a backend can be contributed from
    another package.
  • Fail startup on an unknown storage_type instead of falling back to in-memory.
  • Add a backend-neutral connection parameter next to database_path, so a backend that is not
    file based can be configured. database_path keeps working for SQLite.
  • Put the audit log behind an interface as well, so the selected backend can provide it. Until
    that is done, warn when audit logging is enabled but resolves to :memory:.
  • Export the parity tests as a conformance suite any backend can instantiate, and extend it from
    rosbag retention to the whole FaultStorage contract.
  • Make the contract-critical methods pure virtual, so a backend cannot silently inherit a default
    that breaks a documented guarantee.
  • Write the guarantees above down as the backend contract, next to the interface, so an author
    can tell up front whether their store can meet them. Where a guarantee is expensive outside
    SQL, say whether it is negotiable. The aggregates and the join are the two places worth
    revisiting, since both could be served by a maintained counter or a denormalised field instead
    of a query.

Additional context

We would rather extract this interface from two working implementations than design it up front,
because a guessed interface tends to fit only the backend it was guessed from. So the order is the
PostgreSQL PR first, this work after it lands. A document store is a useful second reader of the
contract even before anyone writes one, because it is where the aggregate and join assumptions
show up.

Background discussion: #649

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions