diff --git a/DESIGN.md b/DESIGN.md index 6b33265..8f6ad15 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -153,7 +153,7 @@ spec: image: repository: git.ryuvia.com/niklas/terdut-server tag: v0.9.3 - replicas: 1 # terdut-server is not horizontally-scale-tested; keep the field, default 1 + replicas: 2 # default since terdut-server v0.36.0's advisory locks; see TerdutServerSpec.Replicas networking: hostname: terdut.example.com servicePort: 8080 @@ -244,10 +244,10 @@ no custom wrapper buys anything for any of these, matching how CloudNativePG and the Zalando postgres-operator both expose the same knobs. `affinity` is pure user-supplied passthrough, not a toggle-plus-generated-default the way a multi-replica-aware operator's -pod anti-affinity typically is: this operator never auto-generates -affinity of its own, since `replicas` above 1 isn't a supported topology -(the sweeper/notifier singleton constraint, §4.1's own illustrative YAML -comment). `spec.pod.disruptionBudget` is the one field here that isn't a +pod anti-affinity typically is: even though `replicas` now defaults to 2 +(terdut-server v0.36.0's advisory locks made that safe, §4.1's own +illustrative YAML comment), this operator still never auto-generates +affinity of its own. `spec.pod.disruptionBudget` is the one field here that isn't a straight PodTemplateSpec knob — when set, the controller reconciles a `PodDisruptionBudget` selecting this `TerdutServer`'s pods; clearing it deletes any it previously created (§7). `minAvailable`/`maxUnavailable` @@ -832,10 +832,12 @@ what it was, a separate install, until someone deletes it. - Automatic Deployment restart on upstream Postgres credential rotation. - `spec.pod.priorityClassName`, pod-label passthrough beyond `spec.pod.annotations`, and a HorizontalPodAutoscaler for `TerdutServer` - — all considered alongside §4.1's `spec.pod` and explicitly left out of - that round: an HPA in particular would actively contradict - `spec.replicas`'s own stance that this operator doesn't support more - than one replica (the sweeper/notifier singleton constraint). + — all considered alongside §4.1's `spec.pod` and left out of that round. + An HPA no longer contradicts anything now that `spec.replicas` defaults + to 2 (terdut-server v0.36.0's advisory locks), but it is still a + separate, not-yet-made decision: a fixed replica count has no scaling + metric, min/max bounds, or cooldown behaviour to get right, and nobody + has asked for it yet. - Admission webhooks / CEL-only validation limits (e.g. verifying a `teamRef` exists at admission time rather than surfacing it as a status condition after the fact). diff --git a/api/v1alpha1/terdutserver_types.go b/api/v1alpha1/terdutserver_types.go index ed29f6b..4e6543f 100644 --- a/api/v1alpha1/terdutserver_types.go +++ b/api/v1alpha1/terdutserver_types.go @@ -229,11 +229,11 @@ type PodSpec struct { Tolerations []corev1.Toleration `json:"tolerations,omitempty"` // affinity covers node affinity, pod affinity and pod anti-affinity in - // one field -- unlike a multi-replica-aware operator, this one never - // generates a default anti-affinity itself (replicas above 1 isn't a - // supported topology, see TerdutServerSpec.Replicas's own doc comment), - // so this is pure user-supplied passthrough, not a toggle-plus-generated- - // default. + // one field -- even though replicas now defaults to 2 (see + // TerdutServerSpec.Replicas's own doc comment), this operator still + // never generates a default anti-affinity of its own the way a + // multi-replica-aware operator typically would, so this stays pure + // user-supplied passthrough, not a toggle-plus-generated-default. // +optional Affinity *corev1.Affinity `json:"affinity,omitempty"` @@ -301,10 +301,14 @@ type TerdutServerSpec struct { // +required Image ImageSpec `json:"image"` - // replicas. terdut-server is not horizontally-scale-tested; keep this - // at its default of 1 unless you've verified otherwise -- the sweeper - // and the notifier are unsynchronised singletons. - // +kubebuilder:default=1 + // replicas. Defaults to 2: terdut-server v0.36.0 put the sweeper, the + // notifier and the migration runner each behind a Postgres advisory + // lock, and gave incident creation its own conflict resolution, so + // more than one replica no longer double-pages, races a migration, or + // drops a webhook payload. image.tag must be v0.36.0 or newer for + // that to hold -- an older terdut-server has none of these guards, + // and this field does not check the tag for you. + // +kubebuilder:default=2 // +optional Replicas int32 `json:"replicas,omitempty"` diff --git a/charts/terdut-operator/templates/crd/terdutservers.terdut.ryuvia.com.yaml b/charts/terdut-operator/templates/crd/terdutservers.terdut.ryuvia.com.yaml index e0c4c33..8d9c37b 100644 --- a/charts/terdut-operator/templates/crd/terdutservers.terdut.ryuvia.com.yaml +++ b/charts/terdut-operator/templates/crd/terdutservers.terdut.ryuvia.com.yaml @@ -327,11 +327,11 @@ spec: affinity: description: |- affinity covers node affinity, pod affinity and pod anti-affinity in - one field -- unlike a multi-replica-aware operator, this one never - generates a default anti-affinity itself (replicas above 1 isn't a - supported topology, see TerdutServerSpec.Replicas's own doc comment), - so this is pure user-supplied passthrough, not a toggle-plus-generated- - default. + one field -- even though replicas now defaults to 2 (see + TerdutServerSpec.Replicas's own doc comment), this operator still + never generates a default anti-affinity of its own the way a + multi-replica-aware operator typically would, so this stays pure + user-supplied passthrough, not a toggle-plus-generated-default. properties: nodeAffinity: description: Describes node affinity scheduling rules for @@ -4304,11 +4304,15 @@ spec: type: array type: object replicas: - default: 1 + default: 2 description: |- - replicas. terdut-server is not horizontally-scale-tested; keep this - at its default of 1 unless you've verified otherwise -- the sweeper - and the notifier are unsynchronised singletons. + replicas. Defaults to 2: terdut-server v0.36.0 put the sweeper, the + notifier and the migration runner each behind a Postgres advisory + lock, and gave incident creation its own conflict resolution, so + more than one replica no longer double-pages, races a migration, or + drops a webhook payload. image.tag must be v0.36.0 or newer for + that to hold -- an older terdut-server has none of these guards, + and this field does not check the tag for you. format: int32 type: integer sweeper: diff --git a/charts/terdut-operator/values.yaml b/charts/terdut-operator/values.yaml index 0f8669a..1b9951c 100644 --- a/charts/terdut-operator/values.yaml +++ b/charts/terdut-operator/values.yaml @@ -233,7 +233,11 @@ terdutServer: ## Required when terdutServer.enabled. # tag: "" - replicas: 1 + ## Safe above 1 since terdut-server v0.36.0 (image.tag above must be that or + ## newer): the sweeper, notifier and migration runner are each behind a + ## Postgres advisory lock, and incident creation resolves its own insert + ## conflict, matching this CRD's own spec.replicas default. + replicas: 2 networking: ## Required when terdutServer.enabled -- terdut-server's own public diff --git a/config/crd/bases/terdut.ryuvia.com_terdutservers.yaml b/config/crd/bases/terdut.ryuvia.com_terdutservers.yaml index 1d5c5fa..5f76fd2 100644 --- a/config/crd/bases/terdut.ryuvia.com_terdutservers.yaml +++ b/config/crd/bases/terdut.ryuvia.com_terdutservers.yaml @@ -324,11 +324,11 @@ spec: affinity: description: |- affinity covers node affinity, pod affinity and pod anti-affinity in - one field -- unlike a multi-replica-aware operator, this one never - generates a default anti-affinity itself (replicas above 1 isn't a - supported topology, see TerdutServerSpec.Replicas's own doc comment), - so this is pure user-supplied passthrough, not a toggle-plus-generated- - default. + one field -- even though replicas now defaults to 2 (see + TerdutServerSpec.Replicas's own doc comment), this operator still + never generates a default anti-affinity of its own the way a + multi-replica-aware operator typically would, so this stays pure + user-supplied passthrough, not a toggle-plus-generated-default. properties: nodeAffinity: description: Describes node affinity scheduling rules for @@ -4301,11 +4301,15 @@ spec: type: array type: object replicas: - default: 1 + default: 2 description: |- - replicas. terdut-server is not horizontally-scale-tested; keep this - at its default of 1 unless you've verified otherwise -- the sweeper - and the notifier are unsynchronised singletons. + replicas. Defaults to 2: terdut-server v0.36.0 put the sweeper, the + notifier and the migration runner each behind a Postgres advisory + lock, and gave incident creation its own conflict resolution, so + more than one replica no longer double-pages, races a migration, or + drops a webhook payload. image.tag must be v0.36.0 or newer for + that to hold -- an older terdut-server has none of these guards, + and this field does not check the tag for you. format: int32 type: integer sweeper: diff --git a/internal/controller/terdutserver_deployment.go b/internal/controller/terdutserver_deployment.go index f6175eb..ad3bd5a 100644 --- a/internal/controller/terdutserver_deployment.go +++ b/internal/controller/terdutserver_deployment.go @@ -37,17 +37,27 @@ func (r *TerdutServerReconciler) reconcileDeployment( _, err := controllerutil.CreateOrUpdate(ctx, r.Client, deploy, func() error { replicas := srv.Spec.Replicas if replicas == 0 { - replicas = 1 + // Only reachable for a TerdutServer stored before the + // +kubebuilder:default=2 marker existed -- the API server's own + // CRD defaulting fills this in for anything created or updated + // through it, so a fresh zero value here means a pre-existing + // object that predates the default, not a deliberate "none" + // (there is no way to request zero replicas). + replicas = 2 } labels := labelsFor(srv) deploy.Spec.Replicas = &replicas deploy.Spec.Selector = &metav1.LabelSelector{MatchLabels: labels} - // Recreate, not RollingUpdate: the sweeper and the notifier are - // unsynchronised singletons inside terdut-server, and two replicas - // overlapping during a rollout would both page for the same - // incident (matches the chart's own deployment.yaml comment). - deploy.Spec.Strategy = appsv1.DeploymentStrategy{Type: appsv1.RecreateDeploymentStrategyType} + // RollingUpdate, not Recreate: terdut-server v0.36.0 put the sweeper, + // the notifier and the migration runner each behind a Postgres + // advisory lock, and gave incident creation its own conflict + // resolution, so two replicas overlapping during a rollout no longer + // double-page, race a migration, or drop a webhook payload (matches + // the chart's own deployment.yaml comment). No explicit + // maxUnavailable/maxSurge: left at the 25%/25% default, which rounds + // to 0/1 at the default replicas: 2 -- already zero-downtime. + deploy.Spec.Strategy = appsv1.DeploymentStrategy{Type: appsv1.RollingUpdateDeploymentStrategyType} pod := srv.Spec.Pod deploy.Spec.Template = corev1.PodTemplateSpec{ // pod.Annotations is assigned directly, not merged -- nothing