Conversation
A timeout on CLIENT LIST left the old master's clients connected for good: the error was only logged, and the next reconcile finds a correctly configured replica, so replicaOf never runs for that pod again. Mark the pod before demoting it and clear the mark once the disconnect went through. CLIENT LIST plus one kill per client becomes a single CLIENT KILL LADDR. Signed-off-by: Michael J. <7890659+krangerich@users.noreply.github.com>
… disconnecting Disconnecting the old master's clients right after the role label patch is too early. The pod is still in the master service's endpoint slices at that point, so a client that reconnects lands straight back on the read-only instance and keeps failing with -READONLY. Defer the disconnect until the pod has left those endpoint slices, driven by an endpoint slice watch on the pod lifecycle controller. Node data planes converge separately, so this removes the window that preceded the endpoint slice write rather than every possible reconnect. The wait is bounded at 30s and falls through to the disconnect, so a stalled EndpointSlice controller cannot leave a demotion unfinished. The pending marker now carries the RFC3339 demotion time rather than "true", because bounding the wait needs a deadline that survives an operator restart. It also clears when a marked pod is promoted again, which REPLTAKEOVER did not do. Requires new RBAC: discovery.k8s.io/endpointslices get, list and watch. Apply it together with the image, the endpoint slice watch cannot start without it. Signed-off-by: Dimosthenis Schizas <dimos@hackthebox.eu>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new CLIENT KILL ... LADDR address construction does not correctly format IPv6 host:port values, which can prevent disconnects on IPv6 clusters.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves planned failover behavior by deferring client disconnection until the demoted pod has actually left the master Service’s EndpointSlices, reducing the chance that reconnecting clients immediately land back on the now read-only instance.
Changes:
- Adds a “pending client disconnect” marker (with RFC3339 timestamp) and a reconcile loop that waits (up to 30s) for the demoted pod to disappear from the master Service EndpointSlices before disconnecting clients.
- Watches EndpointSlice changes and indexes marked pods to efficiently trigger reconciles when endpoints change.
- Adds required RBAC permissions for
discovery.k8s.io/endpointslicesacross manifests and Helm templates, and centralizes master Service naming viaMasterServiceName.
File summaries
| File | Description |
|---|---|
| manifests/dragonfly-operator.yaml | Adds EndpointSlice RBAC permissions. |
| config/rbac/role.yaml | Adds EndpointSlice RBAC permissions. |
| charts/dragonfly-operator/templates/roles.yaml | Adds EndpointSlice RBAC permissions (namespaced role). |
| charts/dragonfly-operator/templates/clusterroles.yaml | Adds EndpointSlice RBAC permissions (cluster role). |
| internal/resources/const.go | Introduces PendingClientDisconnectAnnotationKey. |
| internal/resources/resources.go | Adds MasterServiceName(df) helper and uses it when creating the master Service. |
| internal/controller/util.go | Adds helpers for pending disconnect timestamp parsing and EndpointSlice endpoint targeting. |
| internal/controller/util_test.go | Adds tests for new util helpers. |
| internal/controller/dragonfly_pod_lifecycle_controller.go | Adds EndpointSlice watch + pod field index to trigger reconcile for pending disconnects. |
| internal/controller/dragonfly_pod_lifecycle_controller_test.go | Tests the EndpointSlice→pending-pods enqueue mapping. |
| internal/controller/dragonfly_instance.go | Implements deferred disconnect logic gated on master Service EndpointSlices and replaces per-client kills with CLIENT KILL ... LADDR. |
| internal/controller/dragonfly_instance_demotion_test.go | Adds demotion test ensuring clients are not killed while still in master Service endpoints. |
| internal/controller/dragonfly_instance_pending_disconnect_test.go | Adds tests for wait/timeout/promotion/failed-disconnect retry behavior. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+201
to
+203
| func clientListenerAddress(podIp string) string { | ||
| return sanitizeIp(podIp) + ":" + strconv.Itoa(resources.DragonflyPort) | ||
| } |
Author
There was a problem hiding this comment.
Dragonfly formats LADDR unbracketed itself — Connection::LocalBindStr() returns StrCat(le.address().to_string(), ":", le.port()) and CLIENT KILL matches it by string equality — so bracketing here would stop matching on IPv6 rather than fix it.
Comment on lines
+171
to
+180
| { | ||
| name: "ipv6", | ||
| podIp: "fd00::1", | ||
| want: "fd00::1:6379", | ||
| }, | ||
| { | ||
| name: "bracketed ipv6", | ||
| podIp: "[fd00::1]", | ||
| want: "fd00::1:6379", | ||
| }, |
Author
There was a problem hiding this comment.
The unbracketed expectation is correct: it matches what Dragonfly's LocalBindStr() produces, which is what CLIENT KILL ... LADDR compares against.
This was referenced Sep 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #575. Its commit is the first of the two here and is not part of this change; this diff shrinks to a single commit once #575 merges.
#575 makes the client disconnect durable, but it still fires immediately after the role label patch, while the pod is still listed in the master Service's endpoint slices, so a reconnecting client lands straight back on the read-only instance. This defers the disconnect until the pod has actually left those endpoint slices.
Changes (second commit only)
"true", since bounding the wait needs a deadline that survives an operator restart.REPLTAKEOVERdid not do.discovery.k8s.ioendpointslicesget/list/watch, inconfig/rbac,manifests/and both Helm role templates. Apply it together with the image, the watch cannot start without it.Notes
TestReplicaOfDoesNotDisconnectClientsWhileStillInTheMasterServicefails against fix(controller): don't lose the client disconnect when it fails #575 alone and passes with the second commit.Fixes #324