Fixes #30399: Fix connector teardown regressions from the source-owns-BaseConnection refactor#30402
Fixes #30399: Fix connector teardown regressions from the source-owns-BaseConnection refactor#30402IceS2 wants to merge 8 commits into
Conversation
…asticsearch/OpenSearch close_on_failure closes only the BaseConnection owner, so connector-specific teardown that lived in a close() override was skipped when test_connection raised in __init__. Move the teardown onto the owner by registering it with _on_close inside _get_client, so it runs on both the normal close and the close_on_failure path: - MicroStrategy: register close_api_session (server-side session was leaked). - Elasticsearch/OpenSearch: register removal of the SSL staging dir that holds the client private key (the key was left on disk). The source close() overrides are dropped (base close() now covers owner dispose plus metadata.close()), along with their now-unused shutil/Path imports.
… overrides tableau, looker and powerbi override close() without chaining super().close(), so the owned BaseConnection is never disposed on the normal path. Chain the base close() (which disposes the owner and closes the metadata client): - looker: keep compute_percentile, replace metadata.close() with super().close(). - tableau: drop the direct client.sign_out() (the owner already registers it via _on_close, so keeping both would sign out twice) and chain super().close(). - powerbi: move the file-client tmp-file cleanup onto the owner (_on_close in _get_client) and drop the override so it inherits the base close().
The Airflow 2.x BackendConnection engine is airflow.settings.engine, reached via settings.Session().get_bind() -- it is borrowed, not built by OpenMetadata. The owner registered _on_close(engine.dispose) unconditionally for any Engine client, so closing the owner (including on every Automation test-connection) tore down the host Airflow process's connection pool. Only register dispose for engines this connection built, identified by identity against settings.engine.
The dashboard base was the only service base that stopped setting self.connection_obj after the source-owns refactor. connection_obj is a kept, deprecated seam for out-of-tree custom connectors and Collate; the eight other bases still set it. Set connection_obj to the client and read it in the legacy test_connection fallback, matching the siblings (same object, behavior unchanged).
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e SSL context get_ssl_context (Elasticsearch) and _handle_ssl_context_by_value (OpenSearch) write the cert/key files and can raise while parsing them. Registering the _on_close cleanup after that call left the staged private key on disk when the SSL build failed, because _build_client only unwinds teardowns registered before the failure point. Register the cleanup as the staging material is acquired.
|
Thanks — addressing both findings: greptile P1 — SSL failure leaves staged secrets: Valid, fixed in 15e7046. The gitar — cleanup may delete a shared cert dir: Real observation, but pre-existing and out of scope for this regression PR. The prior source |
🔴 Playwright Results — 2 test failure(s)✅ 4526 passed · ❌ 2 failed · 🟡 31 flaky · ⏭️ 102 skipped
Genuine Failures (failed on all attempts)❌
|
…nostic The test faked airflow via sys.modules and re-imported the connection module to force the 2.x path, but IS_AIRFLOW_3 is computed at import time -- in CI the real airflow 3.x module was already cached, so the test hit the env-var backend branch and raised instead of returning the borrowed engine. Exercise the dispose guard directly: patch `settings` and `_get_connection` so the result is independent of the Airflow version, using real airflow when installed and a minimal fake only when it is absent.
…sal tests Refines the previous test rewrite so it verifies real behavior instead of a stubbed _get_connection: - borrowed case exercises the real Airflow 2.x backend path (patch IS_AIRFLOW_3 to False and provide a settings whose Session().get_bind() returns the process-global engine), asserting it is NOT disposed; - OM-built case uses the real SQLite backend delegate to build a real engine and asserts it IS disposed (spying Engine.dispose). The only patched surface is airflow itself (IS_AIRFLOW_3 + settings); the connection module's dispatch, _get_backend_engine_from_session and the dispose guard all run for real. Real airflow is used when installed; a minimal fake only enables import when absent.
Code Review ✅ Approved 1 resolved / 1 findingsFixes ingestion connector teardown regressions by registering cleanup callbacks on the owning BaseConnection and protecting Airflow's global engine from disposal. No issues found. ✅ 1 resolved✅ Edge Case: Staging-dir cleanup may delete a shared cert directory
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
| certificates = connection.sslConfig.certificates | ||
| if isinstance(certificates, SslCertificatesByValues): | ||
| staging_dir = certificates.stagingDir | ||
| self._on_close(lambda: _cleanup_staging_dir(staging_dir)) |
There was a problem hiding this comment.
When two connectors use by-value TLS certificates without overriding the shared /tmp/openmetadata-certs staging directory, closing or failing initialization of the Elasticsearch connection recursively deletes that directory, removing the other active connector's certificate and private-key files and causing later TLS setup or reconnects to fail.
|
Fixes #30399
Four follow-up regressions from the source-owns-BaseConnection refactor (#29870 / #30094 / #30148 / #30133 / #30174).
Key finding:
close_on_failure(self._connection)in each base__init__disposes only the owner (it unwinds the owner's_on_closeteardowns); it never runs a sourceclose()override. So any connector teardown that lived in aclose()override was skipped whentest_connectionraised in__init__. The fix pattern is to move connector teardown onto the owner — register it with_on_closeinside_get_client— so it fires on both the normalsuper().close()and theclose_on_failurepath, then drop the redundant override.close_api_session/ staging-dir removal on the owner in_get_client; delete the sourceclose()overridesclose()overrides never chainedsuper().close()→ the ownedBaseConnectionwas never disposed on the normal pathsuper().close(); tableau drops its directsign_out()(the owner already registers it via_on_close, so keeping both signs out twice); powerbi movesfile_client.delete_tmp_filesonto the ownerairflow.settings.engine— the borrowed process-global engine returned byBackendConnection— on close and on every Automation test-connection, tearing down the host Airflow pooldisposefor engines this connection built (isinstance(client, Engine) and client is not settings.engine)self.connection_obj, breaking the kept, deprecated seam for out-of-tree custom dashboard connectorsconnection_objand read it in the legacytest_connectionfallback (same object, behavior unchanged)One commit per fix (splittable).
Greptile Summary
Fixes connector teardown ownership regressions.
BaseConnection.connection_objseam.Confidence Score: 4/5
The PR is not yet safe to merge because Elasticsearch cleanup can delete TLS files belonging to another active connector using the shared default staging directory.
The newly registered owner callback recursively removes a shared staging directory rather than only the files owned by the closing connection.
ingestion/src/metadata/ingestion/source/search/elasticsearch/connection.py
Important Files Changed
connection_objalias while preserving owned-connection test behavior.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Build Elasticsearch connection] --> B[Stage by-value TLS files] B --> C[Register owner cleanup] C --> D{Close or initialization failure} D --> E[Unwind BaseConnection callbacks] E --> F[Remove configured staging directory]Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/source-owns..." | Re-trigger Greptile