Add Redis Sentinel support to prefect-redis#22377
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 989a3144b0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Merging this PR will not alter performance
Comparing Footnotes
|
989a314 to
4b28891
Compare
|
hi team, any chance this gets accepted? it's an easier implementation than redis cluster, so hopefully it's not too much to review |
99faae6 to
d4865da
Compare
desertaxle
left a comment
There was a problem hiding this comment.
Sorry for the delayed review @fatih-acar! This is looking pretty good, but I wonder if we can slim down this PR a bit by delegating more URL parsing to redis-py.
| default_port=_DEFAULT_SENTINEL_PORT if is_sentinel else _DEFAULT_PORT, | ||
| ) | ||
| path_segments = [segment for segment in parts.path.split("/") if segment] | ||
| query = parse_qs(parts.query, keep_blank_values=True) |
There was a problem hiding this comment.
Can we funnel the remaining query params through redis-py’s parse_url instead of owning all of this parsing ourselves? Right now, this parser only consumes the auth/TLS-specific keys and drops other Redis URL options for Sentinel URLs.
I'd recommend looking at Docket’s Sentinel parser, which already treats remaining query params as redis-py connection options. That seems like the right contract here and should reduce the custom parsing surface and prevent this path from drifting.
There was a problem hiding this comment.
Good catch (and sorry for that), this mostly slipped because of the previous tls_insecure flags that are used in another codebase from where the implementation was based on... I've removed the flags and made sure the query params are passed through as much as possible
| socket_connect_timeout=resolved_socket_connect_timeout, | ||
| protocol=resolved_protocol, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Changing how these clients are built means we'll need to change how the cache is cleared. The addition of aclose_redis_client and close_redis_client in this PR makes me think that there's extra shutdown required that our caching implementation needs to be updated to handle.
There was a problem hiding this comment.
Thanks, adapted this part as well.
d4865da to
95360d3
Compare
… teardown Per review on PrefectHQ#22377: - parse_redis_url now consumes only the prefect-specific query keys and funnels every remaining query parameter through redis-py's parse_url on a synthetic standalone URL, so standard connection options (socket_timeout, max_connections, health_check_interval, ...) get redis-py's own type conversion and validation instead of being dropped; db/username/password stay governed by the path and userinfo. This mirrors docket's _redis_sentinel parser. - close_all_cached_connections tears down cached Sentinel-backed clients with the daemon-aware aclose_redis_client so the per-daemon Redis clients are closed too. - Drop the Docket section from the prefect-redis docs page; the main self-hosted docs remain the reference for PREFECT_SERVER_DOCKET_URL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@desertaxle thanks for the review, no worries about the delay :) |
Build on the Redis Cluster URL groundwork merged in PrefectHQ#22211 to add native Redis Sentinel support. Cluster URLs stay gated (detection-only) exactly as in PrefectHQ#22211; this change enables the redis+sentinel:// and rediss+sentinel:// schemes across every connection point: the server messaging client, the RedisLockManager, the RedisDatabase block, and the Docket services URL. - New prefect_redis.connection module: a shared URL parser (parse_redis_url) and client builder (build_redis_client) that resolve the current master through the listed Sentinel daemons and follow failover automatically. Data-node connections get pinned TCP keepalive so a silently-dead master is noticed promptly while Sentinel completes failover. - client.py dispatches redis+sentinel:// URLs to the Sentinel builder while leaving PrefectHQ#22211's cluster NotImplementedError gate intact. - RedisLockManager and RedisDatabase accept a connection_url that is authoritative over the scalar host/port fields; RedisDatabase round-trips Sentinel URLs through from_connection_string and block_info. - Document the Sentinel schemes on PREFECT_SERVER_DOCKET_URL and bump the pydocket floor to >=0.22.0 for its Sentinel URL support. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Tolerate pre-connection_url pickles in RedisLockManager.__setstate__ - Honor tls_insecure/tls_ca_file on single-node messaging URLs and retain them verbatim in RedisDatabase.from_connection_string - Detect Sentinel schemes without urlsplit (IPv6 member lists) and case-insensitively; same for cluster URLs - Forward caller socket timeouts to the Sentinel daemon connections - Lift the 0-15 database index cap (only negative indexes are invalid) - Close Sentinel daemon clients deterministically in read_path/write_path via new close_redis_client/aclose_redis_client helpers Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… teardown Per review on PrefectHQ#22377: - parse_redis_url now consumes only the prefect-specific query keys and funnels every remaining query parameter through redis-py's parse_url on a synthetic standalone URL, so standard connection options (socket_timeout, max_connections, health_check_interval, ...) get redis-py's own type conversion and validation instead of being dropped; db/username/password stay governed by the path and userinfo. This mirrors docket's _redis_sentinel parser. - close_all_cached_connections tears down cached Sentinel-backed clients with the daemon-aware aclose_redis_client so the per-daemon Redis clients are closed too. - Drop the Docket section from the prefect-redis docs page; the main self-hosted docs remain the reference for PREFECT_SERVER_DOCKET_URL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Remove the tls_insecure/tls_ca_file and sentinel_ssl/sentinel_tls_* query parameters: TLS options are expressed with redis-py's own connection options (ssl_cert_reqs, ssl_ca_certs, ssl_check_hostname, ...), which reach single-node URLs via Redis.from_url and Sentinel URLs via the query-param funnel. Daemon TLS now follows the scheme exactly like docket's parser, and TLS clients inherit redis-py's default cert verification instead of a custom "optional" profile. The parser now consumes only sentinel_username/sentinel_password and funnels everything else to redis-py, shrinking the custom parsing surface. Also drop the unused redis_client_from_url wrapper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Verified against a real TLS Sentinel topology (3 daemons, master + 2 replicas, self-signed CA): the funneled ssl_* query options previously reached only the data-node connections, so a private CA passed as ?ssl_ca_certs= (or ?ssl_cert_reqs=none) left every Sentinel daemon connection failing certificate verification during discovery and master resolution raised MasterNotFoundError. On rediss+sentinel:// URLs the daemon connections now reuse the URL's ssl_* options, so one CA covers the whole topology; plaintext daemons are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
95360d3 to
2e26e73
Compare
] Match the TLS query-parameter model of the prefect-redis Sentinel integration (PrefectHQ/prefect#22377): drop the custom tls_insecure, tls_ca_file, sentinel_ssl, sentinel_tls_insecure and sentinel_tls_ca_file query parameters in favor of the redis-py-native ssl_cert_reqs, ssl_check_hostname and ssl_ca_certs. On a rediss+sentinel:// URL the ssl_* options are now shared with the Sentinel daemon connections, so a single ?ssl_cert_reqs=none (or ?ssl_ca_certs=/path/ca.pem for a private CA) covers the whole topology instead of requiring a separate flag per side. The same URL now parses identically for INFRAHUB_CACHE_URL and Prefect's PREFECT_REDIS_MESSAGING_URL. Verified against a live TLS Redis Sentinel cluster: parse consistency with the prefect-redis parser, cache and lock over rediss+sentinel, and transparent master failover. - Rewrite parse_redis_url to funnel all non-sentinel-auth query params through redis-py and share ssl_* with the daemons on TLS schemes. - Update unit tests, the INFRAHUB_CACHE_URL field description, the regenerated configuration reference, and the high availability guide. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…saging [#9449] Point prefect and prefect-redis at PrefectHQ/prefect#22377 (fatih-acar fork, feat/redis-sentinel-on-22211) so prefect-redis discovers the Redis master through Sentinel and follows failover, removing the previous "Prefect has no Sentinel support" workarounds. - Task result storage switches from prefect.blocks.redis.RedisStorageContainer (redis-py from_url, no Sentinel) to prefect_redis.RedisDatabase, which parses redis+sentinel:// URLs; the worker default block slug follows to redis-database/. - build_cache_connection_string passes a configured cache URL through unchanged (single-node or Sentinel) instead of reducing a Sentinel URL to a best-effort first-member connection on 6379. - Prefect messaging follows failover via a redis+sentinel:// PREFECT_REDIS_MESSAGING_URL; the HA guide and k8s messaging values are updated accordingly. Pulling prefect#22377 also moves prefect core to a main-based build, which requires fastapi>=0.139.0 (0.136.3 -> 0.139.0) and pydocket 0.23.0. Validated against a live TLS Redis Sentinel cluster: prefect-redis messaging publish/consume and the RedisDatabase result-storage block both round-trip over rediss+sentinel://, and the existing unit suites pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Supersedes #22248, which targeted the
prefect-redis-cluster-client-foundationbranch from #22211; that branch has since merged tomain, auto-closing the old (draft) PR. This re-opens the work againstmain.This PR adds native Redis Sentinel support to
prefect-redis, building on the Redis Cluster URL groundwork merged in #22211. Cluster URLs stay detection-only (gated behindNotImplementedError) exactly as #22211 left them; this PR enables theredis+sentinel://andrediss+sentinel://schemes across every connection point: the server messaging client,RedisLockManager, theRedisDatabaseblock, and the Docket services URL.Details
prefect_redis.connectionmodule — a shared URL parser (parse_redis_url) and client builder (build_redis_client) that resolve the current master through the listed Sentinel daemons and follow failover automatically. Data-node (master) connections get pinned TCP keepalive so a silently-dead master is noticed promptly while Sentinel completes failover.client.pydispatchesredis+sentinel://URLs to the Sentinel builder while leaving Add Redis Cluster URL and key groundwork #22211's clusterNotImplementedErrorgate intact.RedisLockManagerandRedisDatabaseaccept aconnection_urlthat is authoritative over the scalar host/port fields;RedisDatabaseround-trips Sentinel URLs throughfrom_connection_stringandblock_info.PREFECT_SERVER_DOCKET_URLdocuments the Sentinel schemes; thepydocketfloor is bumped to>=0.22.0for its Sentinel URL support.URL grammar follows the
redis-sentinel-urlconvention:The Sentinel schemes accept a comma-separated member list and a master group name. The parser consumes only
sentinel_username/sentinel_password(authentication to the Sentinel daemons); every other query param is funneled through redis-py's ownparse_urland applied to the data-node connections as a standard connection option (socket_timeout,max_connections,ssl_cert_reqs, ...), exactly like a standaloneredis://URL and matching Docket's Sentinel parser. Onrediss+sentinel://thessl_*options also apply to the Sentinel daemon connections, so a single private CA (?ssl_ca_certs=...) covers the whole topology.Testing
prefect-redissuite passes (243 tests, incl. the new connection/integration tests).get_async_redis_client, andRedisLockManagerall automatically follow the Sentinel-promoted replica (~3.5s); data written before the crash survives via replication; the restarted old master rejoins as a replica.tls-replication yes):?ssl_ca_certs=verifies discovery and data connections end-to-end,?ssl_cert_reqs=none&ssl_check_hostname=falseworks for unverified setups, a missing CA is correctly rejected, and failover under TLS completes in ~3s.RedisDatabaseblock against a real Prefect server: block saved/loaded through the REST API, flow runs recordedCOMPLETED, andread_path/write_pathsurvive a master failover (Prefect task retries ride the promotion window).🤖 Generated with Claude Code