Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Verónica López <veronica.lopez@supabase.io>
Signed-off-by: Verónica López <veronica.lopez@supabase.io>
Verolop
force-pushed
the
fix/topology-health
branch
from
September 25, 2026 22:37
8f1b9e4 to
4388a45
Compare
Contributor
Author
|
/e2e |
🔬 Go Test Coverage ReportSummary
Status✅ PASS DetailShow New Coverage |
|
✅ E2E tests success — View run |
graveland
reviewed
Sep 25, 2026
graveland
left a comment
graveland
left a comment
Contributor
There was a problem hiding this comment.
I added some comments, but it wasn't a very thorough review and I don't have the context for the overall change, so take them with a grain of salt!
| type TopoServerStatus struct { | ||
| // HealthCheckedAt is the completion time of the latest direct etcd health probe. | ||
| // +optional | ||
| HealthCheckedAt *metav1.Time `json:"healthCheckedAt,omitempty"` |
Contributor
There was a problem hiding this comment.
What's going to be reading this? We'd be doing a PUT to the api server every 30s for every multigrescluster, so that could become problematic? If we're also publishing metrics, we'd be alerting from the metrics instead of by reading k8s .statuses?
I'd suggest only updating on transitions instead of a timestamp?
If we want a timestamp to make a decision in another controller, we could store an xsync.Map or something like that to store the timestamp per project in memory so other controllers would still be able to use the timestamp without impacting the API server?
Contributor
There was a problem hiding this comment.
If we need .status.healthCheckAt I'd suggest to keep polling at 30s, then immediately updating on transitions and otherwise only write it to the API server every 5m or something like that?
Also- we'd want to add a predicate on the MultigresCluster's Owns(&TopoServer{}) to ignore updates to the TopoServer when the only thing that changes is the healthCheckedAt field.
| checked time.Time, | ||
| members []TopologyMemberMetrics, | ||
| ) { | ||
| DeleteTopologyHealth(name, namespace) |
Contributor
There was a problem hiding this comment.
There's a race between deleting and adding the metrics back again. If a scrape happens between the delete + add, it could cause prom to consider it stale. To fix it though would likely require a custom prom.Collector with a mutex
Contributor
There was a problem hiding this comment.
... or add the new reason, then delete the old- there might be a couple reasons in one scrape, but that might be good enough?
| description: "Member {{ $labels.target_namespace }}/{{ $labels.member }} restarted at least three times in ten minutes. Check termination reasons, memory pressure, and quorum health." | ||
| runbook_url: "https://github.com/multigres/multigres-operator/blob/main/docs/monitoring/runbooks/TopologyHealth.md" | ||
|
|
||
| - alert: MultigresFailoverUnavailable |
Contributor
There was a problem hiding this comment.
I think this one, and others will trigger for every new cluster before they become ready?
| ) (ctrl.Result, error) { | ||
| resourceAge := time.Since(cluster.CreationTimestamp.Time) | ||
| if resourceAge < topoUnavailableGracePeriod { | ||
| r.markTopologyFailed(ctx, cluster, "TopologyUnavailable", cause, logger) |
Contributor
There was a problem hiding this comment.
I think this sets the phase to degraded during creation, where it currently sits in progressing
| l.V(1).Info("reconcile complete", "duration", time.Since(start).String()) | ||
| r.Recorder.Event(cluster, "Normal", "Synced", "Successfully reconciled MultigresCluster") | ||
| return ctrl.Result{}, nil | ||
| return ctrl.Result{RequeueAfter: clusterHealthInterval}, nil |
Contributor
There was a problem hiding this comment.
I had a change I was planning to make the reconciles not the default 10h from the controller-runtime, but to have reconciles maybe hourly +- a random jitter. Every 30s will become a problem with a lot of multigrescluster objects though.
Verolop
marked this pull request as draft
September 27, 2026 23:14
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
2 participants
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.
Description
This PR adds direct health monitoring for managed etcd and reports topology quorum and failover readiness in cluster status. Topology outages currently surface through downstream reconcile errors, while pod readiness and stale conditions can leave the cluster reporting healthy.
The new checks distinguish topology and orchestrator failures from SQL availability, with dedicated alerts for lost failover protection and resource pressure.
Main changes
QuorumAvailableand the latest probe time on TopoServer. AddTopologyQuorumAvailableandFailoverReadyto MultigresCluster, accounting for managed global and cell-local topology, registration results, and orchestrator readiness for every desired shard. Treat old topology observations and shard status from earlier generations as unknown.TopologyReadyon registration failures and refresh cluster status and metrics before returning errors or requeuing. Keep SQL availability separate from failover readiness.Testing
Unit tests for the changed packages and the TopoServer and MultigresCluster integration suites pass. Coverage includes partial connectivity, stale observations, topology failures, metric cleanup, and status updates that preserve maintenance reservations.
Live three-member etcd tests verify quorum with one member down, quorum loss with two down, and a complete outage. They also verify that NOSPACE blocks writes while reads succeed, that the probe reports the alarm, and that health recovers after the alarm is cleared. Prometheus fixtures verify alert timing, recovery, missing measurements, and memory pressure before an OOM. Probe tests pass with the race detector. Changed packages pass lint, the observability overlay renders, Prometheus validates the scrape configuration, and
git diff --checkis clean.