From 6fab886f456f32f71d87271324fffddade381b6b Mon Sep 17 00:00:00 2001 From: as51340 Date: Wed, 14 Oct 2026 08:04:49 +0200 Subject: [PATCH] feat: Make Ready the summary of every other condition Ready was True as soon as a MAIN was elected, so it stayed True through a roll, a scale-down, a resize or a rejected license. kstatus, and with it Flux health checks, decides on Ready alone for a custom resource, so a GitOps step waiting on a MemgraphCluster passed while the operator was still working. Ready is now True only when the cluster serves (a MAIN behind a reachable coordinator leader) and LicenseApplied, Converged and Updated are all True for the current generation. Otherwise it carries the reason of the first thing that is not, in that order: not serving (NoMainElected, WorkloadsNotReady, CoordinatorUnreachable, NoCoordinatorLeader, ApplyFailed), then the license, membership, and the roll, its message naming the condition the reason came from. A True decided for an older generation makes it Unknown/Reconciling. The new True reason is Reconciled; MainElected is gone from the API. Whether the cluster serves is now a pass-local verdict, recorded where Ready used to be set and folded into Ready by summarizeReady at the end of Reconcile, so it never reaches the API. A pass that ends before it observes the cluster keeps a not-serving Ready. status.main still says a MAIN is serving throughout. The envtest suite now creates the default license Secret every cluster reads, as an install following the README has, since without it LicenseApplied is pending and Ready can never be True. The quickstart waits on Ready. --- CLAUDE.md | 1 + README.md | 6 +- api/v1alpha1/memgraphcluster_types.go | 40 +++-- docs/external-access.md | 2 +- docs/flags.md | 2 +- docs/monitoring.md | 2 +- .../controller/memgraphcluster_controller.go | 138 ++++++++++++--- .../memgraphcluster_controller_test.go | 158 +++++++++++++++--- 8 files changed, 283 insertions(+), 66 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 791d969..ba4fe1f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -78,6 +78,7 @@ Test philosophy: assert external behavior, never internal call ordering or priva - Both StatefulSets use `updateStrategy: OnDelete`, so **nothing but the operator ever restarts a workload pod**. A pod-template change no reconcile acts on takes effect never, which is what the `Updated` condition exists to report. A run-time flag change is deliberately *not* a pod-template change (see Memgraph flags above). `RollingUpdate` cannot express the required order (it sweeps highest ordinal to lowest, and `partition` is a descending cutoff, not a set), so a MAIN on any ordinal but 0 would be restarted mid-sweep and each such restart buys another failover. - The operator never promotes a MAIN outside bootstrap and the scale-down handover: a roll deletes the MAIN's pod and lets the Raft coordinators promote. That is only safe on a Memgraph reporting an unreachable MAIN as `role=main, health=down` — a release that vacates the `main` row makes `planner.Plan` believe there is no MAIN and race the failover. - **The shrink is the one write with an expectation** (`internal/expectations`, decided 2026-10-09). The operator reads its StatefulSets from the informer cache, and `replicaCounts` applies `max(declared, cached spec.replicas)`, so a pass reading the StatefulSet from before `shedRetiredPods` lowered it would grow it back (and shrink it again once it found the member gone, the pod recreated in between). `shedRetiredPods` records the UID and generation the apply returned; `replicaCounts` marks a role whose cached StatefulSet is the same UID at a lower generation, and the pass ends before applying anything, waiting on the StatefulSet event (`Owns`). It is in-memory on purpose: a restarted operator lists into a fresh cache that already holds every write it made, so only the writing process can be ahead of its cache. Every other cached read is safe without one (the apply restates desired state, pod deletes carry a UID precondition, `rollout`'s `Stale` covers status, membership is read live over Bolt); a new write whose stale read would undo it, or a decision pushed into Memgraph from a cached count, needs one too. +- **Conditions** (decided 2026-10-14): four, all normal-True. `Converged` (membership, storage, settings), `Updated` (the roll) and `LicenseApplied` each answer one question; `Ready` is their summary, because kstatus (and with it Flux health checks) decides on `Ready` alone for a custom resource. `Ready` is True only when the cluster serves (a MAIN behind a reachable leader) and the other three are True **for the current generation**; otherwise it carries the reason of the first that is not, in the order not serving, `LicenseApplied`, `Converged`, `Updated`, the message prefixed with the condition's name, and a True decided for an older generation makes it `Unknown/Reconciling`. Whether the cluster serves is a pass-local verdict (`conditionServing`, set where `Ready` used to be) that `summarizeReady` folds into `Ready` at the end of `Reconcile` and removes, so it never reaches the API; a pass that did not observe the cluster keeps a not-serving `Ready`. `status.main` is what says a MAIN is serving through a roll, scale or resize. Detail goes into reasons, never a new condition per activity (cass-operator's fourteen is the shape avoided). - Log messages follow Kubernetes style: capital first letter, no trailing period, past tense, object type named (see AGENTS.md for examples). - **External access** (`spec.externalAccess`, `type: LoadBalancer | Gateway`) is operator-owned on both sides: the external objects *and* the announced `bolt_server` of every exposed member. Both modes build the same Services, one shared by the coordinators plus one per data instance (selected by the StatefulSet pod-name label); with `LoadBalancer` they are the way in, with `Gateway` they are ClusterIPs behind one operator-owned Gateway (`gatewayClassName` required, listeners `coordinators-bolt` on 7687 and `data--bolt` on `dataPortBase + n`, default 9000) fed by one TCPRoute each. The address is discovered, never declared: external-dns hostname annotation (on the Service, or in Gateway mode on the TCPRoute, then the Gateway), then the reported hostname, then the IP, pod DNS until any exists (`Converged=False/ExternalAddressPending`). `{ordinal}` in per-instance annotation values is substituted with the pod ordinal and CEL requires it on a data hostname annotation. Only the bolt address ever leaves the cluster network; the operator itself always dials coordinators on pod DNS (`resources.CoordinatorEndpoints`), never through the LoadBalancer or Gateway. External objects follow the data pods the operator *runs* (applied count), are pruned by label once the spec stops describing them, and removing the block reverts every `bolt_server` to pod DNS. The Gateway API CRDs are never bundled: `main.go` discovers once at start (`controller.GatewayAPIServed`) whether `Gateway` and `TCPRoute` are served at `v1`, watches the kinds only if so, and a cluster asking for `Gateway` without that gets `Converged=False/ApplyFailed` naming what is missing. TCPRoute is `v1` only from Gateway API v1.6 (Envoy Gateway ≥ v1.9); older releases serve it as `v1alpha2` alone and v1.6 stops serving `v1alpha2`, so no single version works on both sides — the operator requires ≥ v1.6. The e2e suite gets LoadBalancer addresses from MetalLB and a Gateway from Envoy Gateway (`hack/kind-metallb.sh`, `hack/kind-envoy-gateway.sh`, run by `make setup-test-e2e`); envtest loads the Gateway API CRDs from the `sigs.k8s.io/gateway-api` module. - **TLS** (`spec.tls`) is presence-based per mode and takes one shared `kubernetes.io/tls` Secret per mode for both roles, fixed keys, no key-name knobs: a StatefulSet has one pod template, and Memgraph verifies no member hostname, so per-pod identities buy nothing. `bolt` (`secretName`; `tls.crt`/`tls.key` mounted read-only at `/etc/memgraph/ssl`, `--bolt-cert-file`/`--bolt-key-file`) also moves 9091 to https because Memgraph serves metrics from the Bolt server context. Everything downstream is derived, never a knob: the operator dials `bolt+ssc` (TLS, unverified — verification would force pod-DNS SANs and a CA onto a user-facing cert and buys nothing while Bolt is unauthenticated; Bolt auth is the trigger to revisit) and falls back to the other scheme once, because a roll turning TLS on or off leaves coordinators in the old mode while the spec says the new one (`memgraph.dialSchemes`); the ServiceMonitor switches to https with `insecureSkipVerify`. Probes are TCP and untouched. Rotation is not the operator's: the Secret is mounted without `subPath` so the kubelet refreshes it, and Memgraph picks it up on `RELOAD BOLT SERVER TLS` until the core reloads by mtime (planned upstream); the operator watches no Secret and never restarts for an in-place update, and a `secretName` change is an ordinary roll. `intraCluster` (`secretName`; `tls.crt`/`tls.key`/`ca.crt` mounted read-only at `/etc/memgraph/intra_cluster_tls`, the three `--cluster-*-file` flags always together) is mTLS proving membership, not identity: Memgraph verifies the chain and demands a client cert but checks no hostname, so one shared cert is the shape (nine of thirteen surveyed operators do the same). Its presence is pinned by a CEL transition rule on `MemgraphClusterSpec`, not on `TLSSpec`, because a rule on the block does not fire when the block is added or removed whole: a member with it cannot talk to one without, so a one-pod-at-a-time roll deadlocks on lag. `secretName` changes roll normally; a CA cut-over needs a bundled `ca.crt` in both Secrets, documented, not detected. Renewal is `RELOAD INTRA_CLUSTER TLS` per instance until the core reloads by mtime (Raft already does). diff --git a/README.md b/README.md index 601e059..f1ccece 100644 --- a/README.md +++ b/README.md @@ -83,15 +83,15 @@ memgraph 3 2 instance_0 True True 4m12s ``` - **`MAIN`** is the data instance the coordinators elected as MAIN — the one that accepts writes. It is observed, not decided by the operator, so it changes on failover. -- **`READY`** is True once a MAIN is elected, i.e. the cluster serves writes. +- **`READY`** is True once the cluster both serves and runs its spec: a MAIN is elected, and `Converged`, `Updated` and `LicenseApplied` are all True. Otherwise its reason says what is unfinished, the cluster not serving first; a MAIN keeps serving through a roll, a scale or a resize, so `MAIN` is what says whether writes are accepted. - **`CONVERGED`** is True once every declared coordinator and data instance is registered and reported healthy, and both StatefulSets run the declared number of replicas. `kubectl get mgc -n memgraph -o wide` adds how many of each role's declared members the cluster actually has registered (`REGISTERED-COORDINATORS`, `REGISTERED-DATA`), which is what a scale is watched through. -To block a script or a GitOps step on the cluster being usable: +To block a script or a GitOps step on the cluster being done (Flux and other tools built on kstatus read the same condition): ```sh -kubectl wait --namespace memgraph --for=condition=Converged \ +kubectl wait --namespace memgraph --for=condition=Ready \ memgraphcluster/memgraph --timeout=10m ``` diff --git a/api/v1alpha1/memgraphcluster_types.go b/api/v1alpha1/memgraphcluster_types.go index c9564b2..830e76f 100644 --- a/api/v1alpha1/memgraphcluster_types.go +++ b/api/v1alpha1/memgraphcluster_types.go @@ -179,16 +179,22 @@ const ( RetentionPolicyDelete StorageRetentionPolicy = "Delete" ) -// Condition types reported on MemgraphCluster status. Both use normal-True -// polarity: True is the healthy state. Ready answers "is the cluster serving" -// (a MAIN is elected and reachable); Converged answers "does the cluster match -// the declared topology" (every declared coordinator and data instance is -// registered, and both StatefulSets run the declared number of replicas). A -// cluster can be Ready but not Converged — a MAIN still serves while a lost -// replica registration is being restored, or while a scale finishes. +// Condition types reported on MemgraphCluster status. All four use +// normal-True polarity: True is the healthy state. Converged, Updated and +// LicenseApplied each answer one question; Ready is their summary, the one +// condition tools that know nothing about Memgraph read (kstatus, and with it +// Flux health checks, decides on Ready alone for a custom resource). const ( - // ConditionReady is True when a MAIN data instance is elected and the - // coordinator leader is reachable. + // ConditionReady is True when the cluster serves and runs its spec: a + // data instance is MAIN behind a reachable coordinator leader, and + // LicenseApplied, Converged and Updated are all True for the current + // generation. Otherwise it carries the reason of the first thing that is + // not, in that order: the cluster not serving (NoMainElected, + // WorkloadsNotReady, CoordinatorUnreachable, NoCoordinatorLeader, + // ApplyFailed), then LicenseApplied, Converged and Updated, with the + // message naming the condition the reason came from. A MAIN keeps serving + // through most of what makes Ready False — a roll, a scale, a resize — and + // status.main names it throughout. ConditionReady = "Ready" // ConditionConverged is True when the observed cluster matches the declared @@ -265,9 +271,8 @@ const ( // registered but an exposed one still announces its in-cluster address, // because the LoadBalancer in front of it has not been given an external // address yet. The message names the Service being waited on. Registration - // itself is complete and in-cluster clients are served, so Ready is - // unaffected; what is missing is the cloud provisioning the operator cannot - // hurry. It persisting means the LoadBalancer is not being provisioned at + // itself is complete and in-cluster clients are served; what is missing + // is the cloud provisioning the operator cannot hurry. It persisting means the LoadBalancer is not being provisioned at // all — no cloud controller or no address pool answers the Service. ReasonExternalAddressPending = "ExternalAddressPending" @@ -395,16 +400,19 @@ const ( // clears it. ReasonLicenseRejected = "LicenseRejected" - // ReasonMainElected is set when a data instance is observed as MAIN. - ReasonMainElected = "MainElected" - // ReasonNoMainElected is set when the cluster is reachable but no data // instance has yet been promoted to MAIN. ReasonNoMainElected = "NoMainElected" // ReasonReconciling is set on a condition no pass has decided yet, so - // every condition is present from the first status write on. + // every condition is present from the first status write on. Ready also + // carries it while one of the conditions it summarizes still describes an + // earlier generation of the spec. ReasonReconciling = "Reconciling" + + // ReasonReconciled is set on Ready when the cluster serves and every other + // condition is True for the current generation. + ReasonReconciled = "Reconciled" ) // InitializeConditions sets every condition the resource reports to Unknown, diff --git a/docs/external-access.md b/docs/external-access.md index 39ef7bb..0b294bb 100644 --- a/docs/external-access.md +++ b/docs/external-access.md @@ -143,7 +143,7 @@ kubectl get mgc memgraph -n memgraph -o jsonpath='{.status.externalAccess}' | jq | `False` | `ApplyFailed` | The exposure cannot be served on this cluster at all; the message says what to install. Waiting does not clear it. | | `True` | `AllInstancesRegistered` | Every member is registered at the address in `status.externalAccess`. | -So `kubectl wait --for=condition=Converged` on an exposed cluster means "reachable at the addresses in status", which is what a client outside the cluster wants to wait for. `Ready` is untouched by any of this: a MAIN is serving and in-cluster clients work throughout. +So `kubectl wait --for=condition=Converged` on an exposed cluster means "reachable at the addresses in status", which is what a client outside the cluster wants to wait for. `Ready` carries the same reason until then, since it summarizes `Converged`, while a MAIN serves and in-cluster clients work throughout (`status.main` names it). ## Lifecycle diff --git a/docs/flags.md b/docs/flags.md index 9ee6ccc..7abd2c4 100644 --- a/docs/flags.md +++ b/docs/flags.md @@ -94,7 +94,7 @@ Run-time settings are applied on the pass that finds the cluster registered, bef - `Converged=False` with reason **`SettingsPending`** names the ready pods that did not answer. The usual cause is a Bolt endpoint lagging its pod's readiness by a few seconds; the pass retries on a delay. A pod that is not ready is not dialed and not reported — it is the roll's or the kubelet's business, and the flag file has its settings for when it comes up. - `Converged=False` with reason **`SettingsRejected`** names the pod, the exact `SET DATABASE SETTING` and Memgraph's error, verbatim: a value the setting's validator refuses (`Boolean value supports only 'false' or 'true'`, `Unsupported log level`), or `Unknown setting name` on a Memgraph that does not have the setting at run time. It is retried every pass and only a changed flag clears it. The operator never escalates to a restart on its own: a value an instance refuses at run time it would refuse at startup too, and crash-loop on. -`Ready` is unaffected by either: a refused log level does not make a MAIN stop serving. `Updated` is the roll's, as before. +`Ready` carries the same reason, since it summarizes `Converged`, but a refused log level does not make a MAIN stop serving: `status.main` still names it. `Updated` is the roll's, as before. ## Removing a flag diff --git a/docs/monitoring.md b/docs/monitoring.md index 6da9f80..118a0c3 100644 --- a/docs/monitoring.md +++ b/docs/monitoring.md @@ -64,7 +64,7 @@ Converged=False reason=ApplyFailed restart the operator ``` -while `Ready` and everything else about the cluster are unaffected. Installing the CRD later takes an operator restart, which is documented here rather than detected, exactly as for the Gateway API. Dropping the block clears the condition. +and `Ready` the same reason, while the cluster itself serves as before. Installing the CRD later takes an operator restart, which is documented here rather than detected, exactly as for the Gateway API. Dropping the block clears the condition. ## `spec.monitoring.grafanaDashboard` diff --git a/internal/controller/memgraphcluster_controller.go b/internal/controller/memgraphcluster_controller.go index 5341e09..661bd33 100644 --- a/internal/controller/memgraphcluster_controller.go +++ b/internal/controller/memgraphcluster_controller.go @@ -237,6 +237,7 @@ func (r *MemgraphClusterReconciler) Reconcile(ctx context.Context, req ctrl.Requ cluster.InitializeConditions() result, reconcileErr := r.reconcileCluster(ctx, cluster) + summarizeReady(cluster) if equality.Semantic.DeepEqual(orig.Status, cluster.Status) { return result, reconcileErr @@ -713,7 +714,7 @@ func (r *MemgraphClusterReconciler) applyDesired( applyErr := fmt.Errorf("applying %T %s: %w", obj, obj.GetName(), err) msg := truncateMessage(applyErr.Error()) setStatus(cluster, lastObserved(cluster), - notReadyCondition(memgraphcomv1alpha1.ReasonApplyFailed, msg), + notServingCondition(memgraphcomv1alpha1.ReasonApplyFailed, msg), notConvergedCondition(memgraphcomv1alpha1.ReasonApplyFailed, msg)) return nil, applyErr } @@ -1333,7 +1334,7 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( log.Info("Waited for workload pods to become ready before registration") msg := "Waiting for all workload pods to become ready" setStatus(cluster, lastObserved(cluster).exposedAt(exposure), - notReadyCondition(memgraphcomv1alpha1.ReasonWorkloadsNotReady, msg), + notServingCondition(memgraphcomv1alpha1.ReasonWorkloadsNotReady, msg), notConvergedCondition(memgraphcomv1alpha1.ReasonWorkloadsNotReady, msg)) return ctrl.Result{RequeueAfter: requeueWhilePending}, nil } @@ -1362,7 +1363,7 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( "No coordinator reported a leader, so the cluster has no Raft quorum" } setStatus(cluster, lastObserved(cluster).exposedAt(exposure), - notReadyCondition(reason, msg), + notServingCondition(reason, msg), notConvergedCondition(reason, msg)) return ctrl.Result{RequeueAfter: requeueWhilePending}, nil } @@ -1395,14 +1396,14 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( log.Info("Deferred retirement because no surviving data instance is caught up with MAIN") msg := "Waiting for a surviving data instance that is reachable and caught up with MAIN " + "before moving MAIN off the instance being retired" - setStatus(cluster, latest, readyOrNot(latest.main), + setStatus(cluster, latest, servingOrNot(latest.main), notConvergedCondition(memgraphcomv1alpha1.ReasonNoCaughtUpSurvivor, msg)) return ctrl.Result{RequeueAfter: requeueWhilePending}, nil } if err := r.shedRetiredPods(ctx, cluster, topology, replicas); err != nil { return ctrl.Result{}, err } - setStatus(cluster, latest, readyOrNot(latest.main), + setStatus(cluster, latest, servingOrNot(latest.main), notConvergedCondition(memgraphcomv1alpha1.ReasonRetirementInProgress, retiring)) // The shrink was applied, not yet observed back: the next pass sees the // lowered count, finds nothing retiring, and reports convergence. @@ -1446,7 +1447,7 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( // Reported even when the delete fails, for the reason a rejected apply // is: the next pass has to explain an absence it caused, and a restart // nobody announced looks like the cluster losing a pod on its own. - setStatus(cluster, latest, readyOrNot(latest.main), converged, + setStatus(cluster, latest, servingOrNot(latest.main), converged, notUpdatedCondition(decision.Reason, decision.Message)) if err := r.restartPod(ctx, cluster, decision); err != nil { return ctrl.Result{}, err @@ -1457,7 +1458,7 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( case rollout.Wait: log.Info("Deferred the next pod restart", "reason", decision.Message) - setStatus(cluster, latest, readyOrNot(latest.main), converged, + setStatus(cluster, latest, servingOrNot(latest.main), converged, notUpdatedCondition(decision.Reason, decision.Message)) return ctrl.Result{RequeueAfter: requeueWhilePending}, nil } @@ -1468,7 +1469,7 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( updated := trueCondition(memgraphcomv1alpha1.ConditionUpdated, memgraphcomv1alpha1.ReasonAllPodsUpdated, "All workload pods run the pod template the spec describes") - setStatus(cluster, latest, readyOrNot(latest.main), converged, updated) + setStatus(cluster, latest, servingOrNot(latest.main), converged, updated) if !applied.done() || !claims.done() { // A setting is still owed to some pod: a Bolt endpoint lagging its // pod's readiness clears on its own, and a rejection is retried for @@ -1480,8 +1481,8 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( } // Report the in-progress state before mutating the cluster: a MAIN already - // serving stays Ready while a lost registration is restored; a fresh - // bootstrap has no MAIN yet, so Ready is False until one is elected. A + // serving keeps serving while a lost registration is restored; a fresh + // bootstrap has no MAIN yet, so it does not serve until one is elected. A // retirement in flight is named as such — it is the more specific operation, // and the one whose pending members a user wants to see. A pending leadership // yield is more specific still: it is the one step whose outcome nobody can @@ -1499,19 +1500,19 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( yielded) } inProgress := notConvergedCondition(reason, message) - setStatus(cluster, latest, readyOrNot(latest.main), inProgress) + setStatus(cluster, latest, servingOrNot(latest.main), inProgress) // A command the leader rejects is reported on the resource before the error is // returned, for the reason a rejected apply is: the command is retried forever, // so without it the conditions keep saying registration is in progress while - // the rejection only ever reaches the operator's log. Ready is left describing - // what the cluster was last observed doing — a MAIN that is serving keeps - // serving through a registration the coordinator refuses. + // the rejection only ever reaches the operator's log. The serving verdict is + // left describing what the cluster was last observed doing — a MAIN that is + // serving keeps serving through a registration the coordinator refuses. for _, command := range commands { if err := command.Run(ctx, leader); err != nil { commandErr := fmt.Errorf("executing registration command %q: %w", command, err) msg := truncateMessage(commandErr.Error()) - setStatus(cluster, latest, readyOrNot(latest.main), + setStatus(cluster, latest, servingOrNot(latest.main), notConvergedCondition(memgraphcomv1alpha1.ReasonRegistrationFailed, msg)) return ctrl.Result{}, commandErr } @@ -1808,7 +1809,7 @@ func waitForFlagClasses( msg := "Waiting for a coordinator leader to say whether " + strings.Join(keys, ", ") + " are startup flags before restarting any pod for them" logf.FromContext(ctx).Info("Deferred the next pod restart", "reason", msg) - setStatus(cluster, latest, readyOrNot(latest.main), converged, + setStatus(cluster, latest, servingOrNot(latest.main), converged, notUpdatedCondition(memgraphcomv1alpha1.ReasonFlagsUnclassified, msg)) return ctrl.Result{RequeueAfter: requeueWhilePending} } @@ -2406,15 +2407,106 @@ func observedMain(observed []memgraph.Instance) string { return "" } -// readyOrNot builds the Ready condition from whether a MAIN is elected: the +// conditionServing is a pass's verdict on whether the cluster serves: a data +// instance is MAIN behind a reachable coordinator leader. Every return that +// observed the cluster, or failed trying, records it among the conditions like +// any other, and summarizeReady folds it into Ready and removes it, so it never +// reaches the API. +const conditionServing = "Serving" + +// reasonMainElected is the serving verdict's reason when a data instance is +// MAIN. Like the verdict, it never reaches the API. +const reasonMainElected = "MainElected" + +// servingReasons are the reasons a serving verdict can be False with, and so +// the reasons Ready carries when the cluster does not serve. +var servingReasons = map[string]bool{ + memgraphcomv1alpha1.ReasonApplyFailed: true, + memgraphcomv1alpha1.ReasonWorkloadsNotReady: true, + memgraphcomv1alpha1.ReasonCoordinatorUnreachable: true, + memgraphcomv1alpha1.ReasonNoCoordinatorLeader: true, + memgraphcomv1alpha1.ReasonNoMainElected: true, +} + +// servingOrNot builds the serving verdict from whether a MAIN is elected: the // cluster serves writes exactly when a data instance is MAIN. -func readyOrNot(main string) metav1.Condition { +func servingOrNot(main string) metav1.Condition { if main == "" { - return notReadyCondition(memgraphcomv1alpha1.ReasonNoMainElected, + return notServingCondition(memgraphcomv1alpha1.ReasonNoMainElected, "No data instance has been promoted to MAIN yet") } - return trueCondition(memgraphcomv1alpha1.ConditionReady, - memgraphcomv1alpha1.ReasonMainElected, "Data instance "+main+" is MAIN") + return trueCondition(conditionServing, reasonMainElected, "Data instance "+main+" is MAIN") +} + +// summarizeReady replaces the pass's serving verdict with the Ready condition +// it and the other three conditions add up to. +func summarizeReady(cluster *memgraphcomv1alpha1.MemgraphCluster) { + conditions := &cluster.Status.Conditions + var serving *metav1.Condition + if verdict := apimeta.FindStatusCondition(*conditions, conditionServing); verdict != nil { + serving = verdict.DeepCopy() + apimeta.RemoveStatusCondition(conditions, conditionServing) + } + ready := readyCondition(serving, *conditions, cluster.Status.Main, cluster.Generation) + ready.ObservedGeneration = cluster.Generation + apimeta.SetStatusCondition(conditions, ready) +} + +// readyCondition is Ready from the pass's serving verdict, nil when the pass +// ended before it observed the cluster, and the conditions the pass leaves: +// True only when the cluster serves and LicenseApplied, Converged and Updated +// are True for the current generation, and otherwise the reason of the first +// of those that is not, in that order. Not serving goes first because it is +// what clients feel; the license next because an expired one is what stops +// the cluster converging at all. +// +// A pass that did not observe the cluster keeps the previous Ready when that +// said the cluster was not serving, or was not decided: nothing since has +// shown otherwise. Otherwise the cluster served at the last observation, and +// Ready follows the other conditions, which such a pass can still change. +func readyCondition( + serving *metav1.Condition, + conditions []metav1.Condition, + main string, + generation int64, +) metav1.Condition { + previous := apimeta.FindStatusCondition(conditions, memgraphcomv1alpha1.ConditionReady) + switch { + case serving == nil && previous != nil && previous.Status != metav1.ConditionTrue && + (servingReasons[previous.Reason] || previous.Reason == memgraphcomv1alpha1.ReasonReconciling): + return *previous + case serving != nil && serving.Status != metav1.ConditionTrue: + return metav1.Condition{ + Type: memgraphcomv1alpha1.ConditionReady, Status: serving.Status, + Reason: serving.Reason, Message: serving.Message, + } + } + for _, condType := range []string{ + memgraphcomv1alpha1.ConditionLicenseApplied, + memgraphcomv1alpha1.ConditionConverged, + memgraphcomv1alpha1.ConditionUpdated, + } { + condition := apimeta.FindStatusCondition(conditions, condType) + switch { + case condition == nil || condition.ObservedGeneration != generation: + return metav1.Condition{ + Type: memgraphcomv1alpha1.ConditionReady, Status: metav1.ConditionUnknown, + Reason: memgraphcomv1alpha1.ReasonReconciling, + Message: condType + " has not been decided for the current spec yet", + } + case condition.Status != metav1.ConditionTrue: + message := condition.Message + if message == "" { + message = "not decided yet" + } + return metav1.Condition{ + Type: memgraphcomv1alpha1.ConditionReady, Status: condition.Status, + Reason: condition.Reason, Message: truncateMessage(condType + ": " + message), + } + } + } + return trueCondition(memgraphcomv1alpha1.ConditionReady, memgraphcomv1alpha1.ReasonReconciled, + "Data instance "+main+" is MAIN and the cluster runs the spec it declares") } // maxConditionMessage bounds a condition message well under the API's own @@ -2433,9 +2525,9 @@ func trueCondition(condType, reason, message string) metav1.Condition { return metav1.Condition{Type: condType, Status: metav1.ConditionTrue, Reason: reason, Message: message} } -func notReadyCondition(reason, message string) metav1.Condition { +func notServingCondition(reason, message string) metav1.Condition { return metav1.Condition{ - Type: memgraphcomv1alpha1.ConditionReady, Status: metav1.ConditionFalse, Reason: reason, Message: message, + Type: conditionServing, Status: metav1.ConditionFalse, Reason: reason, Message: message, } } diff --git a/internal/controller/memgraphcluster_controller_test.go b/internal/controller/memgraphcluster_controller_test.go index 6765657..f589e47 100644 --- a/internal/controller/memgraphcluster_controller_test.go +++ b/internal/controller/memgraphcluster_controller_test.go @@ -153,6 +153,21 @@ var _ = Describe("MemgraphCluster Controller", func() { GatewayAPI: true, ServiceMonitorAPI: true, } + + // The license Secret every cluster reads by default, holding the license + // the fake's pods start with, as an install following the README has: + // without it LicenseApplied is pending and Ready can never be True. + // Specs about the license name Secrets of their own. + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: memgraphcomv1alpha1.DefaultSecretName, Namespace: resourceNamespace}, + Data: map[string][]byte{ + memgraphcomv1alpha1.DefaultLicenseSecretKey: []byte(startupLicense), + memgraphcomv1alpha1.DefaultOrganizationSecretKey: []byte(startupOrganization), + }, + } + if err := k8sClient.Create(ctx, secret); !apierrors.IsAlreadyExists(err) { + Expect(err).NotTo(HaveOccurred()) + } }) reconcileCluster := func(name string) reconcile.Result { @@ -169,6 +184,20 @@ var _ = Describe("MemgraphCluster Controller", func() { Expect(k8sClient.Get(ctx, types.NamespacedName{Name: name, Namespace: resourceNamespace}, obj)).To(Succeed()) } + // expectServingButNotReady asserts that a MAIN still serves while Ready is + // False with the reason of the work left unfinished: Ready summarizes every + // other condition, so a serving MAIN alone does not make it True. + expectServingButNotReady := func(name, reason string) { + GinkgoHelper() + cluster := &memgraphcomv1alpha1.MemgraphCluster{} + get(name, cluster) + Expect(cluster.Status.Main).NotTo(BeEmpty(), "a MAIN keeps serving") + ready := apimeta.FindStatusCondition(cluster.Status.Conditions, memgraphcomv1alpha1.ConditionReady) + Expect(ready).NotTo(BeNil()) + Expect(ready.Status).To(Equal(metav1.ConditionFalse)) + Expect(ready.Reason).To(Equal(reason)) + } + // deleteOwned removes the workload objects a reconcile created for the // given cluster: envtest runs no garbage collector, so owner-reference // cascade deletion never fires and each spec must clean up explicitly. @@ -1114,8 +1143,7 @@ var _ = Describe("MemgraphCluster Controller", func() { Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonNoCaughtUpSurvivor)) // The retiring MAIN is still MAIN, and still serving: a paused scale-down // costs availability nothing, which is what makes waiting the better trade. - Expect(apimeta.IsStatusConditionTrue(status().Conditions, - memgraphcomv1alpha1.ConditionReady)).To(BeTrue()) + expectServingButNotReady(resourceName, memgraphcomv1alpha1.ReasonNoCaughtUpSurvivor) Expect(status().Main).To(Equal("instance_1")) By("holding there for as long as the survivor stays behind") @@ -1204,8 +1232,7 @@ var _ = Describe("MemgraphCluster Controller", func() { s := status() Expect(s.Main).To(Equal("instance_1")) - Expect(apimeta.IsStatusConditionTrue(s.Conditions, memgraphcomv1alpha1.ConditionReady)).To(BeTrue(), - "the cluster serves from the restored MAIN") + expectServingButNotReady(resourceName, memgraphcomv1alpha1.ReasonRegistrationFailed) converged := convergedCondition() Expect(converged.Status).To(Equal(metav1.ConditionFalse)) Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonRegistrationFailed)) @@ -1469,9 +1496,7 @@ var _ = Describe("MemgraphCluster Controller", func() { s := status() Expect(s.Main).To(Equal("instance_0"), "the last observed MAIN survives a rejected command") - ready := condition(memgraphcomv1alpha1.ConditionReady) - Expect(ready.Status).To(Equal(metav1.ConditionTrue), - "a cluster with a MAIN keeps serving while a registration is refused") + expectServingButNotReady(resourceName, memgraphcomv1alpha1.ReasonRegistrationFailed) converged := condition(memgraphcomv1alpha1.ConditionConverged) Expect(converged.Status).To(Equal(metav1.ConditionFalse)) Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonRegistrationFailed)) @@ -1494,13 +1519,13 @@ var _ = Describe("MemgraphCluster Controller", func() { Expect(s.DataInstances).To(Equal(int32(2))) ready := condition(memgraphcomv1alpha1.ConditionReady) Expect(ready.Status).To(Equal(metav1.ConditionTrue)) - Expect(ready.Reason).To(Equal(memgraphcomv1alpha1.ReasonMainElected)) + Expect(ready.Reason).To(Equal(memgraphcomv1alpha1.ReasonReconciled)) converged := condition(memgraphcomv1alpha1.ConditionConverged) Expect(converged.Status).To(Equal(metav1.ConditionTrue)) Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonAllInstancesRegistered)) }) - It("should stay ready but drop convergence while a lost registration is restored", func() { + It("should keep serving but drop readiness and convergence while a lost registration is restored", func() { fake.setInstances(convergedCluster()) reconcileCluster(resourceName) markWorkloadsReady(resourceName) @@ -1518,9 +1543,7 @@ var _ = Describe("MemgraphCluster Controller", func() { s := status() Expect(s.Main).To(Equal("instance_0"), "the serving MAIN is unchanged") - ready := condition(memgraphcomv1alpha1.ConditionReady) - Expect(ready.Status).To(Equal(metav1.ConditionTrue), - "a cluster with a MAIN still serves while a replica is re-registered") + expectServingButNotReady(resourceName, memgraphcomv1alpha1.ReasonRegistrationInProgress) converged := condition(memgraphcomv1alpha1.ConditionConverged) Expect(converged.Status).To(Equal(metav1.ConditionFalse)) Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonRegistrationInProgress)) @@ -2556,8 +2579,7 @@ var _ = Describe("MemgraphCluster Controller", func() { Expect(converged.Message).To(ContainSubstring(`SET DATABASE SETTING "log.to_stderr" TO "1"`)) Expect(converged.Message).To(ContainSubstring("Boolean value supports only")) Expect(result.RequeueAfter).To(Equal(requeueWhilePending)) - Expect(condition(memgraphcomv1alpha1.ConditionReady).Status).To(Equal(metav1.ConditionTrue), - "a refused setting does not make the cluster unready") + expectServingButNotReady(resourceName, memgraphcomv1alpha1.ReasonSettingsRejected) By("clearing once the flag is fixed") setFlags(memgraphcomv1alpha1.FlagsSpec{Data: map[string]memgraphcomv1alpha1.FlagValue{ @@ -3549,7 +3571,7 @@ var _ = Describe("MemgraphCluster Controller", func() { Expect(converged.Message).To(ContainSubstring("coordinators: " + futureSetting)) Expect(coordinatorSets()).To(BeEmpty(), "nothing is written for a key Memgraph does not have") Expect(result.RequeueAfter).To(Equal(requeueWhilePending)) - Expect(condition(memgraphcomv1alpha1.ConditionReady).Status).To(Equal(metav1.ConditionTrue)) + expectServingButNotReady(resourceName, memgraphcomv1alpha1.ReasonUnknownFlags) setSettings(map[string]memgraphcomv1alpha1.FlagValue{globalReadOnly: settingOn}) reconcileCluster(resourceName) @@ -3728,8 +3750,7 @@ var _ = Describe("MemgraphCluster Controller", func() { Expect(announced(secondInstance)).To(Equal(podAddress(dataSuffix, 1))) s := status() - Expect(apimeta.IsStatusConditionTrue(s.Conditions, memgraphcomv1alpha1.ConditionReady)).To(BeTrue(), - "in-cluster clients are served while the LoadBalancers are provisioned") + expectServingButNotReady(resourceName, memgraphcomv1alpha1.ReasonExternalAddressPending) converged := convergedCondition() Expect(converged).NotTo(BeNil()) Expect(converged.Status).To(Equal(metav1.ConditionFalse)) @@ -4192,8 +4213,7 @@ var _ = Describe("MemgraphCluster Controller", func() { Expect(externalNames(&gatewayv1.GatewayList{})).To(BeEmpty(), "nothing is built for an API the cluster lacks") Expect(serviceType(dataService(0))).To(Equal(corev1.ServiceTypeClusterIP)) Expect(announced(firstInstance)).To(Equal(podAddress(dataSuffix))) - Expect(apimeta.IsStatusConditionTrue(status().Conditions, memgraphcomv1alpha1.ConditionReady)).To(BeTrue(), - "the cluster still serves in-cluster") + expectServingButNotReady(resourceName, memgraphcomv1alpha1.ReasonApplyFailed) converged := convergedCondition() Expect(converged.Status).To(Equal(metav1.ConditionFalse)) Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonApplyFailed)) @@ -4328,8 +4348,7 @@ var _ = Describe("MemgraphCluster Controller", func() { bootstrapped() Expect(serviceMonitors()).To(BeEmpty(), "nothing is built for a kind the cluster lacks") - Expect(apimeta.IsStatusConditionTrue(status().Conditions, memgraphcomv1alpha1.ConditionReady)).To(BeTrue(), - "the cluster still serves") + expectServingButNotReady(resourceName, memgraphcomv1alpha1.ReasonApplyFailed) converged := convergedCondition() Expect(converged.Status).To(Equal(metav1.ConditionFalse)) Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonApplyFailed)) @@ -4779,3 +4798,100 @@ var _ = Describe("Pod watch", func() { Expect(memgraphPodChanged.Delete(event.DeleteEvent{Object: pod("a", corev1.ConditionTrue)})).To(BeTrue()) }) }) + +// Ready is the summary tools that know nothing about Memgraph read, so its +// precedence is the contract: not serving first, then LicenseApplied, +// Converged and Updated, and True only when every one of them is True for the +// current generation. +var _ = Describe("Ready", func() { + const generation int64 = 4 + + cond := func(condType string, status metav1.ConditionStatus, reason string) metav1.Condition { + return metav1.Condition{ + Type: condType, Status: status, Reason: reason, Message: reason + " message", + ObservedGeneration: generation, + } + } + allTrue := func() []metav1.Condition { + return []metav1.Condition{ + cond(memgraphcomv1alpha1.ConditionLicenseApplied, metav1.ConditionTrue, + memgraphcomv1alpha1.ReasonLicenseMatchesSecret), + cond(memgraphcomv1alpha1.ConditionConverged, metav1.ConditionTrue, + memgraphcomv1alpha1.ReasonAllInstancesRegistered), + cond(memgraphcomv1alpha1.ConditionUpdated, metav1.ConditionTrue, memgraphcomv1alpha1.ReasonAllPodsUpdated), + } + } + with := func(conditions []metav1.Condition, replacements ...metav1.Condition) []metav1.Condition { + for _, replacement := range replacements { + apimeta.SetStatusCondition(&conditions, replacement) + } + return conditions + } + serving := servingOrNot("instance_0") + + It("should be True only when the cluster serves and every other condition is True", func() { + ready := readyCondition(&serving, allTrue(), "instance_0", generation) + Expect(ready.Status).To(Equal(metav1.ConditionTrue)) + Expect(ready.Reason).To(Equal(memgraphcomv1alpha1.ReasonReconciled)) + Expect(ready.Message).To(ContainSubstring("instance_0")) + }) + + It("should put a cluster that does not serve ahead of everything else", func() { + notServing := servingOrNot("") + conditions := with(allTrue(), cond(memgraphcomv1alpha1.ConditionLicenseApplied, metav1.ConditionFalse, + memgraphcomv1alpha1.ReasonLicenseRejected)) + + ready := readyCondition(¬Serving, conditions, "", generation) + Expect(ready.Status).To(Equal(metav1.ConditionFalse)) + Expect(ready.Reason).To(Equal(memgraphcomv1alpha1.ReasonNoMainElected)) + }) + + It("should borrow the first unfinished condition's reason, license before membership before the roll", func() { + conditions := with(allTrue(), + cond(memgraphcomv1alpha1.ConditionConverged, metav1.ConditionFalse, + memgraphcomv1alpha1.ReasonRetirementInProgress), + cond(memgraphcomv1alpha1.ConditionUpdated, metav1.ConditionFalse, + memgraphcomv1alpha1.ReasonRollingRestartInProgress)) + + ready := readyCondition(&serving, conditions, "instance_0", generation) + Expect(ready.Status).To(Equal(metav1.ConditionFalse)) + Expect(ready.Reason).To(Equal(memgraphcomv1alpha1.ReasonRetirementInProgress)) + Expect(ready.Message).To(Equal("Converged: RetirementInProgress message"), + "the message names the condition the reason came from") + + conditions = with(conditions, cond(memgraphcomv1alpha1.ConditionLicenseApplied, metav1.ConditionFalse, + memgraphcomv1alpha1.ReasonLicensePending)) + Expect(readyCondition(&serving, conditions, "instance_0", generation).Reason).To( + Equal(memgraphcomv1alpha1.ReasonLicensePending)) + }) + + It("should not take a True decided for an earlier spec as True for this one", func() { + stale := cond(memgraphcomv1alpha1.ConditionUpdated, metav1.ConditionTrue, memgraphcomv1alpha1.ReasonAllPodsUpdated) + stale.ObservedGeneration = generation - 1 + + ready := readyCondition(&serving, with(allTrue(), stale), "instance_0", generation) + Expect(ready.Status).To(Equal(metav1.ConditionUnknown)) + Expect(ready.Reason).To(Equal(memgraphcomv1alpha1.ReasonReconciling)) + Expect(ready.Message).To(ContainSubstring(memgraphcomv1alpha1.ConditionUpdated)) + }) + + // A pass that ends before it observes the cluster, waiting on its own + // StatefulSet shrink for one, has no serving verdict. + It("should keep a not-serving Ready through a pass that did not observe the cluster", func() { + previous := cond(memgraphcomv1alpha1.ConditionReady, metav1.ConditionFalse, + memgraphcomv1alpha1.ReasonCoordinatorUnreachable) + + ready := readyCondition(nil, with(allTrue(), previous), "instance_0", generation) + Expect(ready.Status).To(Equal(metav1.ConditionFalse)) + Expect(ready.Reason).To(Equal(memgraphcomv1alpha1.ReasonCoordinatorUnreachable)) + }) + + It("should follow the other conditions through such a pass when the cluster served", func() { + previous := cond(memgraphcomv1alpha1.ConditionReady, metav1.ConditionFalse, + memgraphcomv1alpha1.ReasonRollingRestartInProgress) + + ready := readyCondition(nil, with(allTrue(), previous), "instance_0", generation) + Expect(ready.Status).To(Equal(metav1.ConditionTrue)) + Expect(ready.Reason).To(Equal(memgraphcomv1alpha1.ReasonReconciled)) + }) +})