Repository navigation
networking-calico: modernize DB access and improve resync performance - #12881
Conversation
…d resync
Three load-bearing changes plus the cleanup they enable, in service of
CORE-11889 (eventlet hub rollback errors) and the QueuePool timeouts
observed at scale=1000+ in the resync scale test.
1. Modernize transaction management. Replace
``session.begin(subtransactions=True)`` (deprecated in SQLAlchemy 1.4,
removed in 2.0) with neutron_lib's ``db_api.CONTEXT_READER.using()``
at every Neutron DB read site -- both the postcommit-hook paths in
the mech driver and the read paths in the resync runner. All
callsites only read from the Neutron DB (writes go to etcd), so a
reader context is sufficient; nested reader contexts compose
correctly under oslo.db enginefacade. This fixes the eventlet
hub-rollback AssertionError traces in neutron-server.log, and is the
strongest current hypothesis for the intermittent QueuePool-timeout
failures we saw in the resync scale test (the deprecated path is
suspected of not reliably returning connections to the pool).
2. Bulk-fetch per-item DB data during resync. Replace the O(N)
DB round-trip pattern in WorkloadEndpointSyncer and PolicySyncer with
a handful of bulk queries at the start of each syncer's resync,
cached for the duration of the pass.
* PolicySyncer: a single ``get_security_group_rules(filters={
"security_group_id": [all_sg_ids]})`` call replaces N per-SG
calls. The rules are grouped by ``security_group_id`` in
``self._rules_by_sg`` and consumed by
``neutron_to_etcd_write_data``.
* WorkloadEndpointSyncer: ``_prefetch_bulk_port_data()`` populates
``self._bulk`` with IPAllocation, FloatingIP, Network,
SecurityGroup-port-binding, QoS bandwidth + packet-rate rule, and
Subnet rows, indexed appropriately.
``get_extra_port_information()`` checks ``self._bulk`` first and
uses the cache in O(1); if not populated (e.g. called from a
postcommit hook on a single port) it falls back to the original
per-port code path unchanged. ``add_port_qos()`` is modified to
read from the same bulk cache when available.
* A subnet missing from the bulk cache falls back to a per-subnet
``get_subnet()``, mirroring ``add_port_gateways``.
Both syncers are restricted to the scope's port / SG IDs to avoid
over-fetching for narrow resyncs. Expected impact: at 1000 ports
the WorkloadEndpoint resync DB cost previously measured at ~29s on
a mock SQLite DB drops to ~0.68s -- a ~42x speedup, scaling O(1) in
DB round-trips vs O(N) before, so the ratio grows for larger N.
Lens will show the real DevStack number once this lands.
3. Close admin-context sessions in ``_loop_writing_port_statuses``.
That loop creates its own admin context via
``ctx.get_admin_context()`` and previously never explicitly closed
the resulting session. Under newer eventlet + SQLAlchemy versions,
GC of the unclosed session triggers a rollback on the eventlet hub
greenlet, which raises ``AssertionError: do not call blocking
functions from the mainloop`` and produces alarming (but
functionally benign) ERROR traces. Add
``_close_session_safely()`` and call it after each port-status
update plus once on loop exit so the connection is returned to the
pool promptly. (PR projectcalico#12658's removal of ``periodic_resync_thread``
means the only background thread that needs this is the port-status
loop; the one-shot resync worker exits the process when done, so
the GC-on-hub concern doesn't apply there.)
4. Cleanup enabled by (1) + (3). Move the MySQL-driver sanity check
to start of day -- read ``cfg.CONF.database.connection`` directly,
validate the driver, raise on the first call to
``CalicoMechanismDriver.initialize()`` or
``networking_calico.resync.cli.main()`` rather than on first DB
access. Eliminate ``_txn_from_context()`` wrappers entirely
(their only remaining job was the lazy mysql check + a one-line
wrap around ``db_api.CONTEXT_READER.using``). Drop the
``txn_from_context`` parameter plumbed through every
``ResourceSyncer`` constructor. Call sites in ``mech_calico.py``,
``syncer.py``, and ``scope.py`` use ``db_api.CONTEXT_READER.using``
directly. Net: -138 / +63 lines in this cleanup alone.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI triage — CalicoRecommendation: Could not retrieve failure details for this run. Workflow header below: Workflow 07dc7a7f-a0d2-463f-a603-16fa52e71e85verdict: failed Failed: Check Python / Check Python files |
In neutron-server's startup, ML2's mechanism_manager.initialize() runs
during plugin __init__, before anything has imported oslo.db's
enginefacade -- which is what would otherwise side-effect-register the
[database] option group with oslo.config. As a result the start-of-day
_check_mysql_driver() added in the previous commit raises
NoSuchOptError on the first access to cfg.CONF.database.connection:
oslo_config.cfg.NoSuchOptError: no such option database in group [DEFAULT]
This wasn't caught by the unit tests because their cfg.CONF is a
MagicMock and auto-creates attributes on access.
Register oslo_db.options.database_opts in group "database" ourselves
before the lookup. Idempotent (re-registering the same opt is a
no-op), and decoupled from Neutron's startup ordering. The registered
options bind to whatever values were parsed from neutron.conf at the
earlier common_config.init() call, so the value of [database] connection
is correctly observed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…access The modernization commit wrapped every Neutron-DB access in db_api.CONTEXT_READER.using(context), on the theory that "we only read from Neutron, we write to etcd." That premise is incomplete: Neutron helpers transitively invoked from our code do occasionally write to the Neutron DB as an opportunistic side-effect. The first one to surface in CI was get_security_group_rules(), which calls _ensure_default_security_group() to lazily create a per-project default SG -- requiring CONTEXT_WRITER inside what we declared as a READER: TypeError: Can't upgrade a READER transaction to a WRITER mid- transaction oslo.db's enginefacade strictly separates READER and WRITER scopes; the old session.begin(subtransactions=True) idiom we replaced was permissive about read/write nesting, which is why this never surfaced previously. Flip every CONTEXT_READER.using site to CONTEXT_WRITER.using -- mech_calico.py (8 postcommit/SG-update wrappers), syncer.py (3 wrappers around get-all / update / create in the resync compare loop), and scope.py (the expand() IPAllocation query, for symmetry). WRITER is a strict superset of READER capabilities, so reads still work, and the occasional write-by-side-effect from Neutron helpers goes through. No realistic cost: postcommit hooks run AFTER Neutron's main writer transaction has committed, so the WRITER lock is uncontended; the resync worker is a dedicated process with no parallel writers; and expand() is part of the same resync run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…utron
The PolicySyncer's resync() previously populated self._rules_by_sg from
a Neutron query BEFORE delegating to the base class -- i.e., before the
base class's etcd-first read. That subverts the etcd-first ordering's
CAS-based race protection against concurrent dynamic SG-rule updates:
T1: PolicySyncer.resync reads SG rules from Neutron
T2: security_groups_rule_updated handler rewrites the NetworkPolicy
in etcd at mod_revision R2 with the new rules
T3: base class reads etcd, sees R2
T4: compare loop's CAS-update succeeds at R2 and overwrites the
dynamic update with the stale rules from T1
Move the bulk-rule fetch into get_from_neutron, which the base class
calls AFTER get_from_etcd. That puts the rule-snapshot strictly later
than the etcd-snapshot, so the same race now resolves in our favour:
either we see the updated rule too, or our CAS update fails at the
mod_revision we recorded (R2-1, since we now read etcd before the
dynamic write).
Mirrors the pattern WorkloadEndpointSyncer already uses for its bulk
port-data prefetch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ndpointSyncer._bulk The bulk-prefetch cache is safe to read without locks today because the resync runs in a different OS process from the API / RPC forks that handle dynamic postcommit hooks -- CalicoStartupResyncWorker gets its own WorkloadEndpointSyncer instance, distinct from the postcommit-handlers' instance, so a postcommit can never transitively read the resync's _bulk dict. That property comes from the architecture, not from anything _bulk itself enforces. If a future refactor ever shares a single syncer across resync and postcommit paths in the same process, this becomes load-bearing: a concurrent postcommit could read stale bulk-cached data and rewrite the WEP with it. Expand the comment on self._bulk to spell this out, so the constraint isn't only carried in committer heads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…drop _close_session_safely _loop_writing_port_statuses previously called the local _close_session_safely() helper after every iteration, to force the SQLAlchemy connection-fairy's rollback-and-release to happen inside this greenlet rather than wherever Python finalisation happened to fire it. Without that, the rollback's socket I/O could end up on the eventlet hub greenlet and trip ``AssertionError: do not call blocking functions from the mainloop`` (CORE-11889). Replace that pattern with the modern enginefacade equivalent: wrap each iteration's DB work in ``db_api.CONTEXT_WRITER.using(admin_context)``. That is a deterministic transaction scope -- enter opens a writer transaction, exit commits-or-rolls-back AND returns the connection to the pool, all inside the with-block, inside this greenlet. No opaque finalisation involved at all, and no manual cleanup helper required. CONTEXT_WRITER is right because _try_to_update_port_status calls ``self.db.update_port_status(...)`` which writes. WRITER also permits arbitrary reads, matching the same "WRITER is a strict superset of READER" reasoning the modernization commit established for the postcommit-hook wrappings. Delete the _close_session_safely helper -- no remaining callers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The three db_query_*_bulk side-effect helpers each did their own inline ``from types import SimpleNamespace`` at function scope. Stdlib import; move it up to sit with the other top-level imports. Pure cosmetic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…antics The earlier comments framed the post-resync reset of self._bulk (endpoints.py) and self._rules_by_sg (policy.py) as protecting a "subsequent postcommit-hook call" from reading the cache. In production that's not actually true: _do_startup_resync() builds a fresh syncer instance via Scope(self.db).run() (no driver=), and the instance is GC'd as Scope.run() returns -- so the driver's own endpoint_syncer / policy_syncer (which IS what postcommit hooks use) never has the bulk cache populated in the first place. Where the reset DOES matter is the test framework, which reuses the driver's syncer instances via _trigger_resync(driver=self.driver). Without the reset, the second of two _trigger_resync() calls (or a postcommit hook after a _trigger_resync()) would read stale cached data on the same long-lived instance. Cheap to leave the reset in place and useful defence against any future architecture that shares syncer instances across paths -- but the comments should describe the actual reason, not a misleading one. Also clarify that for self._rules_by_sg specifically, the only path that benefits from the reset is a second resync on the same instance: the postcommit-hook SG write path (write_sgs_to_etcd) ignores the cache entirely and always does a fresh DB query. Pure comment update -- no functional change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
….using, drop _close_session_safely" This reverts commit 016fa9e. The CONTEXT_WRITER.using wrapping broke CI on the QoS-responsiveness DevStack test. ``Ml2Plugin.update_port_statuses`` is decorated with neutron_lib's ``@retry_db_errors``, which explicitly refuses to run inside an existing transaction (it manages its own writer scope so it can restart on retryable conflicts): ERROR networking_calico.plugins.ml2.drivers.calico.mech_calico [...] Unexpected error updating port status for [...]: RuntimeError: Method <function Ml2Plugin.update_port_statuses at 0x...> cannot be called within a transaction. The status updates therefore never landed, the VM never reached ACTIVE, and the test timed out trying to delete it. The framing in 016fa9e -- "wrap each iteration in a deterministic transaction scope" -- was wrong for this loop. ``update_port_statuses`` already manages its own transaction; what we actually needed was just deterministic session cleanup AFTER the call, which is exactly what ``_close_session_safely`` provided. Bring it back. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
While reviewing the latest CI run's The traceback: This is direct fallout of wrapping The functional concern would be: if our outer transaction rolled back after the notification fired, an OVO subscriber would be told about an SG that doesn't actually exist. In our paths the outer transaction always commits (postcommit hooks don't raise to roll the outer transaction back), so by the time anything acts on the OVO notification, the SG is committed. Frequency: 1 occurrence per project, on the first SG access. The whole 24h CI window had a single occurrence (admin project's default SG creation). I'm accepting this as a known quirk of the nested-write pattern we needed for For completeness on the other log noise:
|
…/test/test_plugin_etcd.py Co-authored-by: Pedro Coutinho <coutinhop@users.noreply.github.com>
…/test/test_plugin_etcd.py Co-authored-by: Pedro Coutinho <coutinhop@users.noreply.github.com>
…_statuses Commit 00716fd added an assertion that ``_loop_writing_port_statuses`` calls ``_close_session_safely`` twice -- once from the inner ``finally`` after each port-status update, once from the outer ``finally`` on loop exit. The assertion referenced ``m_close.call_count``, but the patcher that would have bound that name was omitted, so the test errored out with NameError before reaching the assertion. Add ``mock.patch.object(mech_calico, "_close_session_safely") as m_close`` to the same ``with`` group as the existing queue / try-update mocks, so the reviewer's intended assertion runs and passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR projectcalico#12881 replaced the legacy ``_txn_from_context`` wrapper (using ``session.begin(subtransactions=True)``) around every mech-driver postcommit / handler body with ``db_api.CONTEXT_WRITER.using(...)``. Under concurrent VM creation in the Calico/OpenStack ST that change started failing with: ERROR neutron.plugins.ml2.managers Mechanism driver 'calico' failed in update_port_postcommit: sqlalchemy.exc.PendingRollbackError: ... Original exception was: (pymysql.err.IntegrityError) (1062, "Duplicate entry '...' for key 'default_security_group.PRIMARY'") Root cause: upstream Neutron's ``get_security_group_rules`` is decorated with ``@db_api.retry_if_session_inactive``, which only retries when no outer transaction is active. The decorator catches the LP#1194579-era (https://bugs.launchpad.net/neutron/+bug/1194579) race in ``_ensure_default_security_group`` (where two concurrent workers both try to lazy-create the same default SG and one loses on the table's unique constraint) by rolling back the session and retrying -- after which the SG already exists, so the retry succeeds. Wrapping our postcommit body in ``CONTEXT_WRITER.using`` makes ``is_session_active(context.session)`` True for the inner call, disabling the retry and letting the ``IntegrityError`` propagate. The session is left in needs-rollback state, the next inner DB op fails with ``PendingRollbackError``, our postcommit raises, Neutron returns 500 to Nova, the spawn retries hit the same race, and after the retry budget the VM goes to ERROR. The old ``subtransactions=True`` wrapper was effectively a no-op outside an outer transaction in modern SQLAlchemy: no enginefacade writer/reader was opened, ``is_session_active`` returned False, and the upstream retry decorator did its job. The fix is documented in Neutron's contributor devref (``doc/source/contributor/internals/db_layer.rst``): "If the database layer (SQLAlchemy, oslo.db) returns a database exception, the current active transaction should end. ... any further database command should be executed in a new context." "[retry_if_session_inactive] can be used along with a transaction decorator but the retry decorator must be declared before the context one. If we first declare the database context (writer or reader) and then the retry decorator, the retry context would be always called from inside an active transaction making it useless." And ``effective_neutron.rst`` calls out PendingRollbackError as a specific failure mode of "nesting transaction blocks, and the innermost block raises an error without proper rollback." Mirror what every other in-tree ML2 mechanism driver does: drop the outer writer/reader context entirely. OVN's ``mech_driver.create_port_postcommit`` / ``update_port_postcommit`` have no wrapper at all; l2pop's ``update_port_postcommit`` calls helpers like ``l2pop_db._get_active_network_ports`` that use raw ``context.session.query(...)`` and rely on SQLAlchemy autobegin. OVS / linuxbridge inherit the no-op base. Inner calls into ``self.db.get_*`` (and ``policy_syncer.write_sgs_to_etcd``, ``endpoint_syncer.write_endpoint``, etc., which call those) are already ``@retry_if_session_inactive``-decorated and manage their own transactions. Drop ``db_api.CONTEXT_WRITER.using(...)`` from the eight sites in mech_calico (``update_network_postcommit`` QoS branch, ``handle_qos_policy_update``, ``create_subnet_postcommit``, ``update_subnet_postcommit``, ``create_port_postcommit``, ``update_port_postcommit``, ``update_floatingip``, ``security_groups_rule_updated``), and the now-unused ``db_api`` import. Comments that previously claimed the wrapper "ensures that a change ... can't be processed by another controller process while we're writing the effects of this call into etcd" were never actually true: a Neutron writer transaction in our session does not row-lock the port, does not order against other workers' sessions, and does not delay our etcd write past any other worker's etcd write. The re-read that the comments accompanied IS a useful best-effort hedge for fast-paired updates being processed out of order -- it picks up the other update's changes if that update has already committed to the Neutron DB by the time we re-read -- but it is not watertight against multi-worker races on the etcd write itself. Comments updated to describe this honestly. A separate concern surfaced by this audit: dynamic etcd writes in mech_calico are not CAS-guarded (``mod_revision=None`` in ``endpoints.write_endpoint``), and we no longer run a periodic resync, so any persistent drift caused by an out-of-order multi-worker race is not corrected until the next neutron-server restart's startup resync. CAS-on-dynamic-writes with retry-on-conflict is the follow-up to make this watertight; tracked separately. The 3 ``db_api.CONTEXT_WRITER.using`` sites in ``syncer.ResourceSyncer.resync`` are deliberately left in place for now: the resync runs in its own OS process (no concurrent racers in the same neutron-server), and its compare loop interacts more intricately with the bulk-prefetch caches. Worth a separate look as follow-up. Verified locally: tox-caracal passes (185 tests). ST verification pending. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reproduces the as-first-reviewed diff of projectcalico#12881 ('networking-calico: modernize DB access and improve resync performance' by @nelljerram) for the Council of Claudes benchmark. base(fork)=2308be04bc974cb653c995f06a991681dd9ddf87 anchor=3474867132a6371966aa9c7bea1e91c513c75f05
|
Docs at tigera/docs#2800 |
networking-calico: modernize DB access and improve resync performance (cherry picked from commit d23155d) Conflicts resolved for release-v3.32: resync/cli.py stays deleted (no calico-resync CLI on v3.32). In mech_calico.py, kept v3.32's periodic resync code where this commit edited the one-shot resync it replaces, and dropped the contextlib import as this commit does.



Why
Three interlocking improvements to the OpenStack mech driver's Neutron-DB access:
neutron-server.logon Noble, caused by SQLAlchemy connection-fairy finalisation happening on the eventlet hub greenlet.QueuePooltimeouts observed at scale=1000+ in the resync scale test, where the deprecatedsubtransactions=Truepath failed to return connections to the pool reliably.What
1. Modernize transaction management
Replace
session.begin(subtransactions=True)(deprecated in SQLAlchemy 1.4, removed in 2.0) with neutron_lib'sdb_api.CONTEXT_WRITER.using()at every Neutron-DB access point in the postcommit-hook paths in the mech driver and the read paths in the resync runner.We use
CONTEXT_WRITER(notCONTEXT_READER) on those paths, even on apparent "read" paths, because some Neutron helpers transitively write to the DB -- most notably_ensure_default_security_group(), which lazily creates the per-project default SG inside what looks like a pure read. oslo.db's enginefacade strictly forbids upgrading a READER to a WRITER mid-transaction, so a READER scope would break those paths. No realistic cost: postcommit hooks run AFTER Neutron's main writer transaction has committed; the resync runs in a dedicated worker process.2. Bulk-fetch per-item DB data during resync
Replace the O(N) DB round-trip pattern in both
WorkloadEndpointSyncerandPolicySyncerwith bulk queries at the start of each syncer'sget_from_neutron(), cached for the duration of the pass.get_security_group_rules(filters={"security_group_id": [all_sg_ids]})call replaces N per-SG calls. Rules are grouped inself._rules_by_sgand consumed byneutron_to_etcd_write_data. Performed insideget_from_neutron, after the base class's etcd read, so the cache reflects a Neutron snapshot strictly later than the etcd snapshot -- preserving the etcd-first ordering's CAS-based race protection against concurrent dynamic SG-rule updates._prefetch_bulk_port_data()populatesself._bulkwith IPAllocation, FloatingIP, Network, SecurityGroup-port-binding, QoS bandwidth + packet-rate rule, and Subnet rows, indexed appropriately.get_extra_port_information()consultsself._bulkin O(1) when populated; otherwise (e.g. postcommit-hook single-port writes) it falls through to the original per-port path unchanged.add_port_qos()reads from the same bulk cache when available.get_subnet(), mirroringadd_port_gateways().Both syncers restrict the bulk-fetch to the scope's port / SG IDs to avoid over-fetching narrow resyncs.
3. Close admin-context sessions in the port-status loop
_loop_writing_port_statusescreates its own admin context and previously never explicitly closed the resulting session. Under newer eventlet + SQLAlchemy versions, the connection-fairy's finalisation (rollback + return-to-pool) can fire on the eventlet hub greenlet at unpredictable moments, tripping the "do not call blocking functions from the mainloop" assertion (CORE-11889).Add a
_close_session_safely()helper and call it after each port-status update (plus once on loop exit) so the connection is returned to the pool deterministically inside the worker greenlet, not via opaque finalisation.4. Cleanup enabled by (1)
cfg.CONF.database.connectiondirectly and raise on the first call toCalicoMechanismDriver.initialize()ornetworking_calico.resync.cli.main(), instead of lazily on first DB access.[database]option group before that read, since ML2'smechanism_manager.initialize()runs before anything else has imported oslo.db's enginefacade (which is what would otherwise side-effect-register it)._txn_from_context()wrappers entirely. After (1), they were one-line wrappers arounddb_api.CONTEXT_WRITER.usingplus a lazy mysql check that has now moved to start of day. Drop thetxn_from_contextparameter plumbed through everyResourceSyncerconstructor. Call sites inmech_calico.py,syncer.py, andscope.pyusedb_api.CONTEXT_WRITER.usingdirectly.Release Note