diff --git a/api/v1beta2/applyconfiguration/api/v1beta2/labelconfig.go b/api/v1beta2/applyconfiguration/api/v1beta2/labelconfig.go index 3fd9152d1..6be5ca1a4 100644 --- a/api/v1beta2/applyconfiguration/api/v1beta2/labelconfig.go +++ b/api/v1beta2/applyconfiguration/api/v1beta2/labelconfig.go @@ -27,6 +27,14 @@ type LabelConfigApplyConfiguration struct { // by the match labels. // Deprecated: This setting will be removed in the next major release. FilterOnOwnerReferences *bool `json:"filterOnOwnerReference,omitempty"` + // IncludePodTemplateGenerationLabel determines whether the operator stamps + // each Pod at creation time with the "foundationdb.org/pod-template-generation" + // label, a class-scoped hash of the rendered PodSpec. The label is intended + // for use with topologySpreadConstraints.matchLabelKeys so that a rolling + // update spreads the new pod generation independently of the old one. It is + // applied only when a Pod is created, so enabling it on an existing cluster + // populates the label gradually as Pods are recreated. Defaults to false. + IncludePodTemplateGenerationLabel *bool `json:"includePodTemplateGenerationLabel,omitempty"` } // LabelConfigApplyConfiguration constructs a declarative configuration of the LabelConfig type for use with @@ -90,3 +98,11 @@ func (b *LabelConfigApplyConfiguration) WithFilterOnOwnerReferences(value bool) b.FilterOnOwnerReferences = &value return b } + +// WithIncludePodTemplateGenerationLabel sets the IncludePodTemplateGenerationLabel field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the IncludePodTemplateGenerationLabel field is set to the value of the last call. +func (b *LabelConfigApplyConfiguration) WithIncludePodTemplateGenerationLabel(value bool) *LabelConfigApplyConfiguration { + b.IncludePodTemplateGenerationLabel = &value + return b +} diff --git a/api/v1beta2/foundationdb_labels.go b/api/v1beta2/foundationdb_labels.go index fbcc7f80b..e922ef960 100644 --- a/api/v1beta2/foundationdb_labels.go +++ b/api/v1beta2/foundationdb_labels.go @@ -25,6 +25,27 @@ const ( // pod spec. LastSpecKey = "foundationdb.org/last-applied-spec" + // PodTemplateGenerationLabel is the label name carrying a content hash + // that identifies a Pod's "generation" — the cohort of pods of the same + // process class that render to the same desired PodSpec. Its value is the + // first 16 hex characters of a SHA-256 over the canonical rendered + // PodSpec for the class, computed from the same source LastSpecKey uses + // to drive pod replacement, so the label rotates exactly when the + // rendered PodSpec for the class changes. + // + // Suitable for use with TopologySpreadConstraints.matchLabelKeys to scope + // spread to the cohort of pods sharing the same generation. Emission is + // opt-in via LabelConfig.IncludePodTemplateGenerationLabel. + PodTemplateGenerationLabel = "foundationdb.org/pod-template-generation" + + // PodTemplateGenerationRemovalValue is the PodTemplateGenerationLabel value + // stamped on pods whose process group is marked for removal. It buckets + // these doomed pods away from any live generation so they do not participate + // in — and therefore cannot skew — the surviving cohort's topology spread. + // It is not a 16-hex SHA-256 prefix, so it can never collide with a real + // generation value. + PodTemplateGenerationRemovalValue = "marked-for-removal" + // LastConfigMapKey provides the annotation name we use to store the hash of the // config map. LastConfigMapKey = "foundationdb.org/last-applied-config-map" diff --git a/api/v1beta2/foundationdbcluster_types.go b/api/v1beta2/foundationdbcluster_types.go index e824df1c7..79de2f3a4 100644 --- a/api/v1beta2/foundationdbcluster_types.go +++ b/api/v1beta2/foundationdbcluster_types.go @@ -2565,6 +2565,15 @@ type LabelConfig struct { // by the match labels. // Deprecated: This setting will be removed in the next major release. FilterOnOwnerReferences *bool `json:"filterOnOwnerReference,omitempty"` + + // IncludePodTemplateGenerationLabel determines whether the operator stamps + // each Pod at creation time with the "foundationdb.org/pod-template-generation" + // label, a class-scoped hash of the rendered PodSpec. The label is intended + // for use with topologySpreadConstraints.matchLabelKeys so that a rolling + // update spreads the new pod generation independently of the old one. It is + // applied only when a Pod is created, so enabling it on an existing cluster + // populates the label gradually as Pods are recreated. Defaults to false. + IncludePodTemplateGenerationLabel *bool `json:"includePodTemplateGenerationLabel,omitempty"` } // PublicIPSource models options for how a pod gets its public IP. @@ -2684,6 +2693,13 @@ func (cluster *FoundationDBCluster) ShouldFilterOnOwnerReferences() bool { return ptr.Deref(cluster.Spec.LabelConfig.FilterOnOwnerReferences, false) } +// ShouldIncludePodTemplateGenerationLabel returns whether the cluster wants +// Pods to be labeled with a content hash of their canonical rendered PodSpec +// under PodTemplateGenerationLabel. Defaults to false. +func (cluster *FoundationDBCluster) ShouldIncludePodTemplateGenerationLabel() bool { + return ptr.Deref(cluster.Spec.LabelConfig.IncludePodTemplateGenerationLabel, false) +} + // SkipProcessGroup checks if a ProcessGroupStatus should be skipped during reconciliation. func (cluster *FoundationDBCluster) SkipProcessGroup(processGroup *ProcessGroupStatus) bool { if processGroup == nil { diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index eba2fb8e4..7fe83545b 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -1849,6 +1849,11 @@ func (in *LabelConfig) DeepCopyInto(out *LabelConfig) { *out = new(bool) **out = **in } + if in.IncludePodTemplateGenerationLabel != nil { + in, out := &in.IncludePodTemplateGenerationLabel, &out.IncludePodTemplateGenerationLabel + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LabelConfig. diff --git a/config/crd/bases/apps.foundationdb.org_foundationdbclusters.yaml b/config/crd/bases/apps.foundationdb.org_foundationdbclusters.yaml index e7cc03102..568b8338b 100644 --- a/config/crd/bases/apps.foundationdb.org_foundationdbclusters.yaml +++ b/config/crd/bases/apps.foundationdb.org_foundationdbclusters.yaml @@ -420,6 +420,8 @@ spec: properties: filterOnOwnerReference: type: boolean + includePodTemplateGenerationLabel: + type: boolean matchLabels: additionalProperties: type: string diff --git a/controllers/add_pods.go b/controllers/add_pods.go index e01d27a51..61fa29b1a 100644 --- a/controllers/add_pods.go +++ b/controllers/add_pods.go @@ -143,6 +143,18 @@ func (a addPods) reconcile( pod.Annotations[fdbv1beta2.PublicIPAnnotation] = ip } + createArgs := []any{ + "processGroupID", processGroup.ProcessGroupID, + "processClass", processGroup.ProcessClass, + } + if cluster.ShouldIncludePodTemplateGenerationLabel() { + createArgs = append(createArgs, + "podTemplateGeneration", + pod.ObjectMeta.Labels[fdbv1beta2.PodTemplateGenerationLabel], + ) + } + logger.Info("Creating pod", createArgs...) + err = r.PodLifecycleManager.CreatePod(logr.NewContext(ctx, logger), r, pod) if err != nil { if errors.IsQuotaExceeded(err) { diff --git a/controllers/add_pods_test.go b/controllers/add_pods_test.go index 656609a61..7c1a18785 100644 --- a/controllers/add_pods_test.go +++ b/controllers/add_pods_test.go @@ -29,6 +29,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + "k8s.io/utils/ptr" ) var _ = Describe("add_pods", func() { @@ -159,6 +160,35 @@ var _ = Describe("add_pods", func() { }) }) }) + + When("the pod-template-generation label is enabled", func() { + BeforeEach(func() { + cluster.Spec.LabelConfig.IncludePodTemplateGenerationLabel = ptr.To(true) + }) + + It("should stamp the created pod with the generation hash", func() { + hash, hashErr := internal.GetPodGenerationHash( + cluster, + fdbv1beta2.ProcessClassStorage, + ) + Expect(hashErr).NotTo(HaveOccurred()) + expectNewPodToHaveGenerationLabel(newPods, newProcessGroupID, hash) + }) + + When("the process group is being removed", func() { + BeforeEach(func() { + processGroupWithoutPod.MarkForRemoval() + }) + + It("should stamp the created pod with the removal sentinel", func() { + expectNewPodToHaveGenerationLabel( + newPods, + newProcessGroupID, + fdbv1beta2.PodTemplateGenerationRemovalValue, + ) + }) + }) + }) }) }) @@ -191,3 +221,26 @@ func expectNewPodToHaveBeenCreated( Expect(podHaveBeenChecked).To(BeTrue()) } + +func expectNewPodToHaveGenerationLabel( + newPods *corev1.PodList, + newProcessGroup fdbv1beta2.ProcessGroupID, + expectedValue string, +) { + expectedPodName := string("operator-test-1-" + newProcessGroup) + var podHaveBeenChecked bool + for _, pod := range newPods.Items { + if pod.Name != expectedPodName { + continue + } + + Expect(pod.Labels).To(HaveKeyWithValue( + fdbv1beta2.PodTemplateGenerationLabel, + expectedValue, + )) + podHaveBeenChecked = true + break + } + + Expect(podHaveBeenChecked).To(BeTrue()) +} diff --git a/controllers/update_pods.go b/controllers/update_pods.go index 5e9e68590..d63dc123f 100644 --- a/controllers/update_pods.go +++ b/controllers/update_pods.go @@ -393,10 +393,23 @@ func getPodsToUpdate( continue } - logger.Info( - "Update Pod", + updateArgs := []any{ "processGroupID", processGroup.ProcessGroupID, + "processClass", + processGroup.ProcessClass, + "podName", + pod.Name, + "nodeName", + pod.Spec.NodeName, + } + if cluster.ShouldIncludePodTemplateGenerationLabel() { + updateArgs = append(updateArgs, + "oldPodTemplateGeneration", + pod.ObjectMeta.Labels[fdbv1beta2.PodTemplateGenerationLabel], + ) + } + updateArgs = append(updateArgs, "reason", fmt.Sprintf( "specHash has changed from %s to %s", @@ -404,6 +417,7 @@ func getPodsToUpdate( pod.ObjectMeta.Annotations[fdbv1beta2.LastSpecKey], ), ) + logger.Info("Update Pod", updateArgs...) podClient, message := reconciler.getPodClient(cluster, pod) if podClient == nil { diff --git a/docs/cluster_spec.md b/docs/cluster_spec.md index 13da89d4f..547ced086 100644 --- a/docs/cluster_spec.md +++ b/docs/cluster_spec.md @@ -320,6 +320,7 @@ LabelConfig allows customizing labels used by the operator. | processGroupIDLabels | ProcessGroupIDLabels provides the labels that we use for the process group ID field. The first label will be used by the operator when filtering resources. | []string | false | | processClassLabels | ProcessClassLabels provides the labels that we use for the process class field. The first label will be used by the operator when filtering resources. | []string | false | | filterOnOwnerReference | FilterOnOwnerReferences determines whether we should check that resources are owned by the cluster object, in addition to the constraints provided by the match labels. **Deprecated: This setting will be removed in the next major release.** | *bool | false | +| includePodTemplateGenerationLabel | IncludePodTemplateGenerationLabel determines whether the operator stamps each Pod at creation time with the \"foundationdb.org/pod-template-generation\" label, a class-scoped hash of the rendered PodSpec. The label is intended for use with topologySpreadConstraints.matchLabelKeys so that a rolling update spreads the new pod generation independently of the old one. It is applied only when a Pod is created, so enabling it on an existing cluster populates the label gradually as Pods are recreated. Defaults to false. | *bool | false | [Back to TOC](#table-of-contents) diff --git a/docs/manual/fault_domains.md b/docs/manual/fault_domains.md index 029475790..44ef44608 100644 --- a/docs/manual/fault_domains.md +++ b/docs/manual/fault_domains.md @@ -406,6 +406,93 @@ In particular (to quote the Kubernetes documentation): > - only `.spec.minAvailable` can be used, not `.spec.maxUnavailable`. > - only an integer value can be used with `.spec.minAvailable`, not a percentage. +## Spreading Pods Across Pod Template Generations + +`topologySpreadConstraints` keep pods of a process class spread across fault +domains. During a rolling update, though, a plain `labelSelector` counts the +old pods and the new pods as the same group: while the operator recreates pods +one fault domain at a time, the new pods and the surviving old pods are weighed +together, which can let the scheduler place several new pods into the same fault +domain. Because topology spread constraints are only evaluated when a pod is +scheduled — the scheduler never moves a pod that is already running — that +imbalance in the new generation is not corrected once the update finishes; it +persists until those pods happen to be recreated again. + +To avoid this you can scope a spread constraint to a single *pod template +generation* — the cohort of pods of the same process class that render to the +same `PodSpec`. When you enable it, the operator stamps every pod at creation +time with the `foundationdb.org/pod-template-generation` label, whose value is a +hash of the rendered `PodSpec` for that process class. The hash rotates exactly +when the rendered `PodSpec` for the class changes, so a spec change that triggers +a rolling update also starts a new generation. + +Enable the label in the cluster spec: + +```yaml +apiVersion: apps.foundationdb.org/v1beta2 +kind: FoundationDBCluster +metadata: + name: sample-cluster +spec: + labels: + includePodTemplateGenerationLabel: true +``` + +Then reference the label from a spread constraint with `matchLabelKeys`. The +scheduler reads the label's value from the pod being scheduled and only counts +existing pods that share the same value, so each generation is spread +independently: + +```yaml + podTemplate: + spec: + topologySpreadConstraints: + - maxSkew: 1 + topologyKey: topology.kubernetes.io/zone + whenUnsatisfiable: DoNotSchedule + # Only consider pods of this cluster and process class ... + labelSelector: + matchLabels: + foundationdb.org/fdb-cluster-name: sample-cluster + foundationdb.org/fdb-process-class: storage + # ... that also belong to the same pod template generation. + matchLabelKeys: + - foundationdb.org/pod-template-generation +``` + +`matchLabelKeys` for topology spread requires the +`MatchLabelKeysInPodTopologySpread` feature to be enabled in your cluster. Do +not add the generation label to `labelSelector.matchLabels` yourself — the +scheduler derives its value from the incoming pod automatically. + +### Rollout behavior + +The label is applied only when a pod is **created**; the operator never patches +it onto a running pod. This is deliberate: rewriting the value in place on a +surviving pod would make the scheduler treat the old and new generations as one +during the update, which is exactly what this label prevents. + +As a consequence, enabling `includePodTemplateGenerationLabel` on an existing +cluster does not label the current pods immediately. The label is populated +gradually, as pods are recreated for unrelated reasons (upgrades, replacements, +or spec changes). Until every pod is recreated there will be a mix of labeled +and unlabeled pods; Kubernetes ignores a `matchLabelKeys` entry for pods that do +not carry the key, so the constraint degrades gracefully to the broader +`labelSelector` spread rather than failing to schedule. To have every pod carry +the label from the start, enable the setting when the cluster is first created. + +### Pods marked for removal + +When a process group is marked for removal (for example during a replacement), +any pod the operator creates for it is stamped with the fixed value +`marked-for-removal` instead of a generation hash. This buckets the doomed pod +away from every live generation, so it is never counted toward — and therefore +cannot skew — the spread of the pods that will survive. The sentinel is applied +only to pods created while the group is already being removed; pods that were +created earlier keep whatever value they had (the operator never rewrites the +label in place, per the rule above). Because `marked-for-removal` is not a +16-character hash, it can never collide with a real generation value. + ## Coordinators Per default the FDB operator will try to select the best fitting processes to be coordinators. diff --git a/e2e/test_operator/operator_test.go b/e2e/test_operator/operator_test.go index e79a66427..db410a8a3 100644 --- a/e2e/test_operator/operator_test.go +++ b/e2e/test_operator/operator_test.go @@ -1722,6 +1722,86 @@ var _ = Describe("Operator", Label("e2e", "pr"), func() { }, ) + When("the pod template generation label is enabled", func() { + var initialSetting *bool + var originalStorageNames map[string]bool + var podToReplace corev1.Pod + + BeforeEach(func(ctx SpecContext) { + // Enabling the opt-in label does not relabel running pods, so we + // recreate a single storage pod below so it gets stamped. + spec := fdbCluster.GetCluster(ctx).Spec.DeepCopy() + initialSetting = spec.LabelConfig.IncludePodTemplateGenerationLabel + spec.LabelConfig.IncludePodTemplateGenerationLabel = ptr.To(true) + fdbCluster.UpdateClusterSpecWithSpec(ctx, spec) + + storagePods := fdbCluster.GetStoragePods(ctx) + Expect(storagePods.Items).NotTo(BeEmpty()) + originalStorageNames = make(map[string]bool, len(storagePods.Items)) + for _, pod := range storagePods.Items { + originalStorageNames[pod.Name] = true + } + + // Recreate a single storage pod so it is stamped with the label + // while the remaining storage pods and all log pods survive + // unlabeled, exercising the transitional mixed state. + podToReplace = storagePods.Items[0] + fdbCluster.ReplacePod(ctx, podToReplace, false) + }) + + AfterEach(func(ctx SpecContext) { + spec := fdbCluster.GetCluster(ctx).Spec.DeepCopy() + spec.LabelConfig.IncludePodTemplateGenerationLabel = initialSetting + fdbCluster.UpdateClusterSpecWithSpec(ctx, spec) + Expect(fdbCluster.ClearProcessGroupsToRemove(ctx)).NotTo(HaveOccurred()) + }) + + It("should stamp recreated pods with the generation label but leave surviving pods untouched", func(ctx SpecContext) { + expectedHash, err := internal.GetPodGenerationHash( + fdbCluster.GetCluster(ctx), + fdbv1beta2.ProcessClassStorage, + ) + Expect(err).NotTo(HaveOccurred()) + + lastForcedReconciliationTime := time.Now() + forceReconcileDuration := 4 * time.Minute + Eventually(func(g Gomega) { + // Force a reconcile periodically to speed up the replacement. + if time.Since(lastForcedReconciliationTime) >= forceReconcileDuration { + fdbCluster.ForceReconcile(ctx) + lastForcedReconciliationTime = time.Now() + } + + // The replaced pod must be gone. + g.Expect(fdbCluster.GetPodsNames(ctx)). + NotTo(ContainElement(podToReplace.Name)) + + // A newly created storage pod (not in the original set) must + // carry the class-scoped hash, while storage pods that survived + // must not have the label. + var sawRecreatedPod bool + for _, pod := range fdbCluster.GetStoragePods(ctx).Items { + if originalStorageNames[pod.Name] { + g.Expect(pod.Labels). + NotTo(HaveKey(fdbv1beta2.PodTemplateGenerationLabel)) + continue + } + sawRecreatedPod = true + g.Expect(pod.Labels). + To(HaveKeyWithValue(fdbv1beta2.PodTemplateGenerationLabel, expectedHash)) + } + g.Expect(sawRecreatedPod).To(BeTrue()) + + // Log pods were never recreated, so they must not have gained + // the label; it is never patched onto a surviving pod. + for _, pod := range fdbCluster.GetLogPods(ctx).Items { + g.Expect(pod.Labels). + NotTo(HaveKey(fdbv1beta2.PodTemplateGenerationLabel)) + } + }).WithTimeout(15 * time.Minute).WithPolling(5 * time.Second).Should(Succeed()) + }) + }) + // This test is pending, as all Pods will be restarted at the same time, which will lead to unavailability without // using DNS. PWhen("crash looping the sidecar for all Pods", func() { diff --git a/internal/pod_helper.go b/internal/pod_helper.go index 96fc7819b..f9c757ed7 100644 --- a/internal/pod_helper.go +++ b/internal/pod_helper.go @@ -107,6 +107,38 @@ func GetPodSpecHash( return GetJSONHash(spec) } +// PodGenerationHashLength is the number of hex characters returned by +// GetPodGenerationHash — fits under Kubernetes' 63-char label value limit +// while giving 64 bits of entropy. +const PodGenerationHashLength = 16 + +// GetPodGenerationHash returns a class-scoped hash of the rendered PodSpec, +// truncated to PodGenerationHashLength hex characters for direct use as a +// Kubernetes label value. The spec is rendered with a sentinel +// ProcessGroupID so every pod of the same class produces the same hash; +// any field that flows into GetPodSpec contributes automatically. The hash +// is computed from the same source LastSpecKey uses to drive pod +// replacement, so it rotates exactly when the rendered PodSpec for the +// class changes. See PodTemplateGenerationLabel for the consumer. +func GetPodGenerationHash( + cluster *fdbv1beta2.FoundationDBCluster, + processClass fdbv1beta2.ProcessClass, +) (string, error) { + canonical := &fdbv1beta2.ProcessGroupStatus{ + ProcessClass: processClass, + ProcessGroupID: fdbv1beta2.ProcessGroupID(string(processClass) + "-generation"), + } + spec, err := GetPodSpec(cluster, canonical) + if err != nil { + return "", err + } + hash, err := GetJSONHash(spec) + if err != nil { + return "", err + } + return hash[:PodGenerationHashLength], nil +} + // GetJSONHash serializes an object to JSON and takes a hash of the resulting // JSON. func GetJSONHash(object any) (string, error) { @@ -395,6 +427,24 @@ func podMetadataCorrect(desiredMetadata metav1.ObjectMeta, pod *corev1.Pod) (boo } desiredMetadata.Annotations[fdbv1beta2.IPFamilyAnnotation] = strconv.Itoa(ipFamily) + // Preserve PodTemplateGenerationLabel on surviving pods. When opted in via + // LabelConfig.IncludePodTemplateGenerationLabel, this label participates + // in scheduling decisions through TSC matchLabelKeys; rewriting it in + // place on a surviving pod would let the scheduler see all surviving pods + // as one generation during a rolling update. It rotates only when + // updatePods deletes the pod and addPods recreates it with the new spec + // — same pattern as fdbv1beta2.LastSpecKey above. + if value, ok := pod.ObjectMeta.Labels[fdbv1beta2.PodTemplateGenerationLabel]; ok { + if desiredMetadata.Labels == nil { + desiredMetadata.Labels = make(map[string]string) + } + desiredMetadata.Labels[fdbv1beta2.PodTemplateGenerationLabel] = value + } else { + // Pod predates the label; don't patch it in place. The label will be + // applied naturally on recreation. + delete(desiredMetadata.Labels, fdbv1beta2.PodTemplateGenerationLabel) + } + return MetadataCorrect(desiredMetadata, &pod.ObjectMeta), nil } diff --git a/internal/pod_helper_test.go b/internal/pod_helper_test.go index dae9d2166..62cf973d9 100644 --- a/internal/pod_helper_test.go +++ b/internal/pod_helper_test.go @@ -30,6 +30,7 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" ) var _ = Describe("pod_helper", func() { @@ -749,5 +750,201 @@ var _ = Describe("pod_helper", func() { }, }, ), + Entry("PodTemplateGenerationLabel matches between pod and desired (steady state)", + testCase{ + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + fdbv1beta2.PodTemplateGenerationLabel: "abcdef0123456789", + }, + Annotations: map[string]string{ + fdbv1beta2.LastSpecKey: "1", + fdbv1beta2.ImageTypeAnnotation: string(fdbv1beta2.ImageTypeSplit), + fdbv1beta2.IPFamilyAnnotation: strconv.Itoa( + fdbv1beta2.PodIPFamilyUnset, + ), + }, + }, + }, + metadata: metav1.ObjectMeta{ + Labels: map[string]string{ + fdbv1beta2.PodTemplateGenerationLabel: "abcdef0123456789", + }, + Annotations: map[string]string{ + fdbv1beta2.LastSpecKey: "1", + fdbv1beta2.ImageTypeAnnotation: string(fdbv1beta2.ImageTypeSplit), + fdbv1beta2.IPFamilyAnnotation: strconv.Itoa(fdbv1beta2.PodIPFamilyUnset), + }, + }, + expected: true, + expectedMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + fdbv1beta2.PodTemplateGenerationLabel: "abcdef0123456789", + }, + Annotations: map[string]string{ + fdbv1beta2.LastSpecKey: "1", + fdbv1beta2.ImageTypeAnnotation: string(fdbv1beta2.ImageTypeSplit), + fdbv1beta2.IPFamilyAnnotation: strconv.Itoa(fdbv1beta2.PodIPFamilyUnset), + }, + }, + }, + ), + Entry("PodTemplateGenerationLabel preserved when pod and desired differ (mid-roll)", + testCase{ + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + fdbv1beta2.PodTemplateGenerationLabel: "oldhash000000000", + }, + Annotations: map[string]string{ + fdbv1beta2.LastSpecKey: "1", + fdbv1beta2.ImageTypeAnnotation: string(fdbv1beta2.ImageTypeSplit), + fdbv1beta2.IPFamilyAnnotation: strconv.Itoa( + fdbv1beta2.PodIPFamilyUnset, + ), + }, + }, + }, + metadata: metav1.ObjectMeta{ + Labels: map[string]string{ + fdbv1beta2.PodTemplateGenerationLabel: "newhash000000000", + }, + Annotations: map[string]string{ + fdbv1beta2.LastSpecKey: "2", + fdbv1beta2.ImageTypeAnnotation: string(fdbv1beta2.ImageTypeSplit), + fdbv1beta2.IPFamilyAnnotation: strconv.Itoa(fdbv1beta2.PodIPFamilyUnset), + }, + }, + expected: true, + expectedMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + fdbv1beta2.PodTemplateGenerationLabel: "oldhash000000000", + }, + Annotations: map[string]string{ + fdbv1beta2.LastSpecKey: "1", + fdbv1beta2.ImageTypeAnnotation: string(fdbv1beta2.ImageTypeSplit), + fdbv1beta2.IPFamilyAnnotation: strconv.Itoa(fdbv1beta2.PodIPFamilyUnset), + }, + }, + }, + ), + Entry("PodTemplateGenerationLabel missing from pod is not patched in", + testCase{ + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + fdbv1beta2.LastSpecKey: "1", + fdbv1beta2.ImageTypeAnnotation: string(fdbv1beta2.ImageTypeSplit), + fdbv1beta2.IPFamilyAnnotation: strconv.Itoa( + fdbv1beta2.PodIPFamilyUnset, + ), + }, + }, + }, + metadata: metav1.ObjectMeta{ + Labels: map[string]string{ + fdbv1beta2.PodTemplateGenerationLabel: "newhash000000000", + }, + Annotations: map[string]string{ + fdbv1beta2.LastSpecKey: "1", + fdbv1beta2.ImageTypeAnnotation: string(fdbv1beta2.ImageTypeSplit), + fdbv1beta2.IPFamilyAnnotation: strconv.Itoa(fdbv1beta2.PodIPFamilyUnset), + }, + }, + expected: true, + expectedMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + fdbv1beta2.LastSpecKey: "1", + fdbv1beta2.ImageTypeAnnotation: string(fdbv1beta2.ImageTypeSplit), + fdbv1beta2.IPFamilyAnnotation: strconv.Itoa(fdbv1beta2.PodIPFamilyUnset), + }, + }, + }, + ), ) + + Describe("GetPodGenerationHash", func() { + var cluster *fdbv1beta2.FoundationDBCluster + + BeforeEach(func() { + cluster = CreateDefaultCluster() + Expect(NormalizeClusterSpec(cluster, DeprecationOptions{})).NotTo(HaveOccurred()) + }) + + It("is deterministic for an unchanged cluster", func() { + first, err := GetPodGenerationHash(cluster, fdbv1beta2.ProcessClassStorage) + Expect(err).NotTo(HaveOccurred()) + second, err := GetPodGenerationHash(cluster, fdbv1beta2.ProcessClassStorage) + Expect(err).NotTo(HaveOccurred()) + Expect(first).To(Equal(second)) + }) + + It("yields distinct hashes for distinct process classes", func() { + storage, err := GetPodGenerationHash(cluster, fdbv1beta2.ProcessClassStorage) + Expect(err).NotTo(HaveOccurred()) + log, err := GetPodGenerationHash(cluster, fdbv1beta2.ProcessClassLog) + Expect(err).NotTo(HaveOccurred()) + Expect(storage).NotTo(Equal(log)) + }) + + // Comprehensive coverage of "what affects the rendered PodSpec" lives + // with GetPodSpec's own tests. These smoke tests confirm + // GetPodGenerationHash actually delegates to GetPodSpec — i.e., that + // mutations that DO affect the rendered PodSpec rotate the hash, and + // mutations that DON'T affect it leave the hash stable. + DescribeTable("rotation tracks the rendered PodSpec", + func(mutate func(*fdbv1beta2.FoundationDBCluster), expectChange bool) { + baseline, err := GetPodGenerationHash( + cluster, + fdbv1beta2.ProcessClassStorage, + ) + Expect(err).NotTo(HaveOccurred()) + + mutate(cluster) + mutated, err := GetPodGenerationHash( + cluster, + fdbv1beta2.ProcessClassStorage, + ) + Expect(err).NotTo(HaveOccurred()) + + if expectChange { + Expect(mutated).NotTo(Equal(baseline)) + } else { + Expect(mutated).To(Equal(baseline)) + } + }, + Entry("ImageType change rotates (split <-> unified flips rendering)", + func(c *fdbv1beta2.FoundationDBCluster) { + t := fdbv1beta2.ImageTypeUnified + c.Spec.ImageType = &t + }, + true, + ), + Entry("podTemplate.spec.nodeSelector change rotates", + func(c *fdbv1beta2.FoundationDBCluster) { + settings := c.Spec.Processes[fdbv1beta2.ProcessClassGeneral] + if settings.PodTemplate == nil { + settings.PodTemplate = &corev1.PodTemplateSpec{} + } + settings.PodTemplate.Spec.NodeSelector = map[string]string{ + "test": "value", + } + c.Spec.Processes[fdbv1beta2.ProcessClassGeneral] = settings + }, + true, + ), + Entry("AutomationOptions change does NOT rotate", + func(c *fdbv1beta2.FoundationDBCluster) { + c.Spec.AutomationOptions.MaxConcurrentReplacements = ptr.To(99) + }, + false, + ), + Entry("DatabaseConfiguration change does NOT rotate", + func(c *fdbv1beta2.FoundationDBCluster) { + c.Spec.DatabaseConfiguration.Storage = 99 + }, + false, + ), + ) + }) }) diff --git a/internal/pod_models.go b/internal/pod_models.go index c71fbd98d..ed5b45248 100644 --- a/internal/pod_models.go +++ b/internal/pod_models.go @@ -116,6 +116,31 @@ func GetService( }, nil } +// podTemplateGenerationValue returns the value to stamp under +// fdbv1beta2.PodTemplateGenerationLabel for a process group's new pod. A +// process group being removed gets the PodTemplateGenerationRemovalValue +// sentinel so its (doomed) pod is bucketed away from any live generation and +// cannot skew the surviving cohort's topology spread; every other pod gets the +// rendered-PodSpec generation hash. +func podTemplateGenerationValue( + cluster *fdbv1beta2.FoundationDBCluster, + processGroup *fdbv1beta2.ProcessGroupStatus, +) (string, error) { + // ProcessGroupIsBeingRemoved (rather than processGroup.IsMarkedForRemoval) + // so the sentinel also applies in the window after a group is listed in + // spec.processGroupsToRemove[WithoutExclusion] but before its status + // removal timestamp is set: with DNS in the cluster file, addPods runs + // before updateStatus, so a pod recreated in that window would otherwise + // get the live generation hash and never be corrected. addPods always + // passes a cluster.Status.ProcessGroups element, so this also covers + // status-marked groups. + if cluster.ProcessGroupIsBeingRemoved(processGroup.ProcessGroupID) { + return fdbv1beta2.PodTemplateGenerationRemovalValue, nil + } + + return GetPodGenerationHash(cluster, processGroup.ProcessClass) +} + // GetPod builds a pod for a new process group func GetPod( cluster *fdbv1beta2.FoundationDBCluster, @@ -141,6 +166,21 @@ func GetPod( metadata.Name = processGroup.GetPodName(cluster) metadata.OwnerReferences = owner + // The generation label is only stamped at pod-creation time. Reconcile + // passes preserve the existing value via the carve-out in + // podMetadataCorrect, so there's no need to recompute it on every + // PodMetadataCorrect call. + if cluster.ShouldIncludePodTemplateGenerationLabel() { + generation, err := podTemplateGenerationValue(cluster, processGroup) + if err != nil { + return nil, err + } + if metadata.Labels == nil { + metadata.Labels = make(map[string]string) + } + metadata.Labels[fdbv1beta2.PodTemplateGenerationLabel] = generation + } + return &corev1.Pod{ ObjectMeta: metadata, Spec: *spec, diff --git a/internal/pod_models_test.go b/internal/pod_models_test.go index 119a1c230..0ff23abc7 100644 --- a/internal/pod_models_test.go +++ b/internal/pod_models_test.go @@ -207,6 +207,95 @@ var _ = Describe("pod_models", func() { })) }) }) + + Context("with the pod-template-generation label opted in", func() { + BeforeEach(func() { + cluster.Spec.LabelConfig.IncludePodTemplateGenerationLabel = ptr.To(true) + pod, err = GetPod( + cluster, + GetProcessGroup(cluster, fdbv1beta2.ProcessClassStorage, 1), + ) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should set the pod-template-generation label to the generation hash", func() { + hash, err := GetPodGenerationHash( + cluster, + fdbv1beta2.ProcessClassStorage, + ) + Expect(err).NotTo(HaveOccurred()) + Expect(pod.ObjectMeta.Labels).To(HaveKeyWithValue( + fdbv1beta2.PodTemplateGenerationLabel, + hash, + )) + }) + + It("should set the same label value on distinct pods of the same class", func() { + other, otherErr := GetPod( + cluster, + GetProcessGroup(cluster, fdbv1beta2.ProcessClassStorage, 2), + ) + Expect(otherErr).NotTo(HaveOccurred()) + Expect(other.ObjectMeta.Labels[fdbv1beta2.PodTemplateGenerationLabel]). + To(Equal(pod.ObjectMeta.Labels[fdbv1beta2.PodTemplateGenerationLabel])) + }) + + It("should set different label values on pods of distinct classes", func() { + logPod, logErr := GetPod( + cluster, + GetProcessGroup(cluster, fdbv1beta2.ProcessClassLog, 1), + ) + Expect(logErr).NotTo(HaveOccurred()) + Expect(logPod.ObjectMeta.Labels[fdbv1beta2.PodTemplateGenerationLabel]). + NotTo(Equal(pod.ObjectMeta.Labels[fdbv1beta2.PodTemplateGenerationLabel])) + }) + + It("should set the removal sentinel for a process group marked for removal", func() { + processGroup := GetProcessGroup(cluster, fdbv1beta2.ProcessClassStorage, 1) + processGroup.MarkForRemoval() + // Mirror how addPods feeds GetPod: the process group is an + // element of cluster.Status.ProcessGroups. + cluster.Status.ProcessGroups = []*fdbv1beta2.ProcessGroupStatus{processGroup} + + doomedPod, removalErr := GetPod(cluster, processGroup) + Expect(removalErr).NotTo(HaveOccurred()) + Expect(doomedPod.ObjectMeta.Labels).To(HaveKeyWithValue( + fdbv1beta2.PodTemplateGenerationLabel, + fdbv1beta2.PodTemplateGenerationRemovalValue, + )) + }) + + It("should set the removal sentinel for a process group listed in spec.processGroupsToRemove before status is marked", func() { + processGroup := GetProcessGroup(cluster, fdbv1beta2.ProcessClassStorage, 1) + // Spec lists it for removal, but the status removal timestamp + // is NOT yet set (the addPods-before-updateStatus window). + cluster.Spec.ProcessGroupsToRemove = []fdbv1beta2.ProcessGroupID{ + processGroup.ProcessGroupID, + } + Expect(processGroup.IsMarkedForRemoval()).To(BeFalse()) + + doomedPod, removalErr := GetPod(cluster, processGroup) + Expect(removalErr).NotTo(HaveOccurred()) + Expect(doomedPod.ObjectMeta.Labels).To(HaveKeyWithValue( + fdbv1beta2.PodTemplateGenerationLabel, + fdbv1beta2.PodTemplateGenerationRemovalValue, + )) + }) + }) + + Context("with the pod-template-generation label disabled (default)", func() { + BeforeEach(func() { + pod, err = GetPod( + cluster, + GetProcessGroup(cluster, fdbv1beta2.ProcessClassStorage, 1), + ) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should not set the pod-template-generation label", func() { + Expect(pod.ObjectMeta.Labels).NotTo(HaveKey(fdbv1beta2.PodTemplateGenerationLabel)) + }) + }) }) })