diff --git a/docs/api-reference/operator-api.md b/docs/api-reference/operator-api.md index db9a88754..357e31f9d 100644 --- a/docs/api-reference/operator-api.md +++ b/docs/api-reference/operator-api.md @@ -491,7 +491,8 @@ _Appears in:_ | `availableReplicas` _integer_ | AvailableReplicas is the number of PodCliqueSet replicas that are available.
A PodCliqueSet replica is considered available when all standalone PodCliques within that replica
have MinAvailableBreached condition = False AND all PodCliqueScalingGroups (PCSG) within that replica
have MinAvailableBreached condition = False. | 0 | | | `hpaPodSelector` _string_ | Selector is the label selector that determines which pods are part of the PodGang.
PodGang is a unit of scale and this selector is used by HPA to scale the PodGang based on metrics captured for
the pods that match this selector. | | | | `podGangStatuses` _[PodGangStatus](#podgangstatus) array_ | PodGangStatuses captures the status for all the PodGang's that are part of the PodCliqueSet. | | | -| `currentGenerationHash` _string_ | CurrentGenerationHash is a hash value generated out of a collection of fields in a PodCliqueSet.
Since only a subset of fields is taken into account when generating the hash, not every change in the PodCliqueSetSpec will
be accounted for when generating this hash value. A field in PodCliqueSetSpec is included if a change to it triggers
a rolling recreate of PodCliques and/or PodCliqueScalingGroups.
Only if this value is not nil and the newly computed hash value is different from the persisted CurrentGenerationHash value
then an update needs to be triggered. | | | +| `currentGenerationHash` _string_ | CurrentGenerationHash is the opaque identity selected for the current rollout-relevant revision. | | | +| `currentRevision` _string_ | CurrentRevision names the ControllerRevision containing the selected rollout revision. | | | | `updateProgress` _[PodCliqueSetUpdateProgress](#podcliquesetupdateprogress)_ | UpdateProgress represents the progress of an update. | | | diff --git a/docs/installation.md b/docs/installation.md index a7bea21d8..0343c757e 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -118,6 +118,26 @@ Follow the instructions in the [quickstart guide](quickstart.md) to deploy a Pod ## Upgrade Notes +### Rollout revision adoption + +Before upgrading to a release that stores PodCliqueSet revisions in +`apps/v1` `ControllerRevision` objects: + +1. Apply the release CRDs before starting the new operator. The + `PodCliqueSet` CRD must preserve `status.currentRevision`. +2. Allow every existing PodCliqueSet update to finish. A legacy object is + adopted only when `status.observedGeneration` equals `metadata.generation` + and no automatic update is active. +3. Start the new operator. It records the already-observed desired workload as + a baseline ControllerRevision while preserving the existing rollout hashes; + it does not update or replace workload children during adoption. + +The next rollout-relevant spec change creates a new ControllerRevision and +uses the normal update strategy. An active or otherwise unobserved legacy +revision is left unchanged and reports a reconciliation error until the +migration precondition is resolved. Downgrading afterward to an operator that +does not understand ControllerRevision-backed selection is unsupported. + ### ClusterTopology renamed to ClusterTopologyBinding Grove does not provide automatic migration for existing `ClusterTopology` diff --git a/operator/api/core/v1alpha1/crds/grove.io_podcliquesets.yaml b/operator/api/core/v1alpha1/crds/grove.io_podcliquesets.yaml index 0784191fd..1612d5b1c 100644 --- a/operator/api/core/v1alpha1/crds/grove.io_podcliquesets.yaml +++ b/operator/api/core/v1alpha1/crds/grove.io_podcliquesets.yaml @@ -11062,13 +11062,12 @@ spec: type: object type: array currentGenerationHash: - description: |- - CurrentGenerationHash is a hash value generated out of a collection of fields in a PodCliqueSet. - Since only a subset of fields is taken into account when generating the hash, not every change in the PodCliqueSetSpec will - be accounted for when generating this hash value. A field in PodCliqueSetSpec is included if a change to it triggers - a rolling recreate of PodCliques and/or PodCliqueScalingGroups. - Only if this value is not nil and the newly computed hash value is different from the persisted CurrentGenerationHash value - then an update needs to be triggered. + description: CurrentGenerationHash is the opaque identity selected + for the current rollout-relevant revision. + type: string + currentRevision: + description: CurrentRevision names the ControllerRevision containing + the selected rollout revision. type: string hpaPodSelector: description: |- diff --git a/operator/api/core/v1alpha1/podcliqueset.go b/operator/api/core/v1alpha1/podcliqueset.go index 5c61f4e1b..1a82c1427 100644 --- a/operator/api/core/v1alpha1/podcliqueset.go +++ b/operator/api/core/v1alpha1/podcliqueset.go @@ -100,13 +100,10 @@ type PodCliqueSetStatus struct { Selector *string `json:"hpaPodSelector,omitempty"` // PodGangStatuses captures the status for all the PodGang's that are part of the PodCliqueSet. PodGangStatutes []PodGangStatus `json:"podGangStatuses,omitempty"` - // CurrentGenerationHash is a hash value generated out of a collection of fields in a PodCliqueSet. - // Since only a subset of fields is taken into account when generating the hash, not every change in the PodCliqueSetSpec will - // be accounted for when generating this hash value. A field in PodCliqueSetSpec is included if a change to it triggers - // a rolling recreate of PodCliques and/or PodCliqueScalingGroups. - // Only if this value is not nil and the newly computed hash value is different from the persisted CurrentGenerationHash value - // then an update needs to be triggered. + // CurrentGenerationHash is the opaque identity selected for the current rollout-relevant revision. CurrentGenerationHash *string `json:"currentGenerationHash,omitempty"` + // CurrentRevision names the ControllerRevision containing the selected rollout revision. + CurrentRevision *string `json:"currentRevision,omitempty"` // UpdateProgress represents the progress of an update. UpdateProgress *PodCliqueSetUpdateProgress `json:"updateProgress,omitempty"` } diff --git a/operator/api/core/v1alpha1/zz_generated.deepcopy.go b/operator/api/core/v1alpha1/zz_generated.deepcopy.go index 6302ec362..d9b90dba6 100644 --- a/operator/api/core/v1alpha1/zz_generated.deepcopy.go +++ b/operator/api/core/v1alpha1/zz_generated.deepcopy.go @@ -754,6 +754,11 @@ func (in *PodCliqueSetStatus) DeepCopyInto(out *PodCliqueSetStatus) { *out = new(string) **out = **in } + if in.CurrentRevision != nil { + in, out := &in.CurrentRevision, &out.CurrentRevision + *out = new(string) + **out = **in + } if in.UpdateProgress != nil { in, out := &in.UpdateProgress, &out.UpdateProgress *out = new(PodCliqueSetUpdateProgress) diff --git a/operator/charts/templates/clusterrole.yaml b/operator/charts/templates/clusterrole.yaml index 4d9b3c278..fa44df211 100644 --- a/operator/charts/templates/clusterrole.yaml +++ b/operator/charts/templates/clusterrole.yaml @@ -5,6 +5,16 @@ metadata: labels: {{- include "operator.clusterrole.labels" . | nindent 4 }} rules: +- apiGroups: + - apps + resources: + - controllerrevisions + verbs: + - create + - delete + - get + - list + - watch - apiGroups: - scheduler.grove.io resources: diff --git a/operator/go.mod b/operator/go.mod index d89b9a0be..7396b1bee 100644 --- a/operator/go.mod +++ b/operator/go.mod @@ -92,7 +92,7 @@ require ( github.com/lib/pq v1.10.9 // indirect github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de // indirect github.com/mailru/easyjson v0.9.0 // indirect - github.com/mattn/go-colorable v0.1.13 // indirect + github.com/mattn/go-colorable v0.1.14 // indirect github.com/mattn/go-isatty v0.0.20 // indirect github.com/mattn/go-runewidth v0.0.9 // indirect github.com/mitchellh/copystructure v1.2.0 // indirect @@ -136,6 +136,7 @@ require ( go.yaml.in/yaml/v2 v2.4.3 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/crypto v0.48.0 // indirect + golang.org/x/mod v0.33.0 // indirect golang.org/x/net v0.51.0 // indirect golang.org/x/oauth2 v0.34.0 // indirect golang.org/x/sys v0.42.0 // indirect diff --git a/operator/go.sum b/operator/go.sum index 3595b39a5..d32d56a53 100644 --- a/operator/go.sum +++ b/operator/go.sum @@ -195,9 +195,8 @@ github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de h1:9TO3cAIGXtEhn github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de/go.mod h1:zAbeS9B/r2mtpb6U+EI2rYA5OAXxsYw6wTamcNW+zcE= github.com/mailru/easyjson v0.9.0 h1:PrnmzHw7262yW8sTBwxi1PdJA3Iw/EKBa8psRf7d9a4= github.com/mailru/easyjson v0.9.0/go.mod h1:1+xMtQp2MRNVL/V1bOzuP3aP8VNwRW55fQUto+XFtTU= -github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= -github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg= -github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= +github.com/mattn/go-colorable v0.1.14 h1:9A9LHSqF/7dyVVX6g0U9cwm9pG3kP9gSzcuIPHPsaIE= +github.com/mattn/go-colorable v0.1.14/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8= github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/mattn/go-runewidth v0.0.9 h1:Lm995f3rfxdpd6TSmuVCHVb/QhupuXlYr8sCI/QdE+0= @@ -376,8 +375,8 @@ golang.org/x/crypto v0.48.0 h1:/VRzVqiRSggnhY7gNRxPauEQ5Drw9haKdM0jqfcCFts= golang.org/x/crypto v0.48.0/go.mod h1:r0kV5h3qnFPlQnBSrULhlsRfryS2pmewsg+XfMgkVos= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= -golang.org/x/mod v0.32.0 h1:9F4d3PHLljb6x//jOyokMv3eX+YDeepZSEo3mFJy93c= -golang.org/x/mod v0.32.0/go.mod h1:SgipZ/3h2Ci89DlEtEXWUk/HteuRin+HHhN+WbNhguU= +golang.org/x/mod v0.33.0 h1:tHFzIWbBifEmbwtGz65eaWyGiGZatSrT9prnU8DbVL8= +golang.org/x/mod v0.33.0/go.mod h1:swjeQEj+6r7fODbD2cqrnje9PnziFuw4bmLbBZFrQ5w= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= @@ -395,7 +394,6 @@ golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5h golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210616094352-59db8d763f22/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.42.0 h1:omrd2nAlyT5ESRdCLYdm3+fMfNFE/+Rf4bDIQImRJeo= golang.org/x/sys v0.42.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= diff --git a/operator/internal/controller/common/component/utils/podclique.go b/operator/internal/controller/common/component/utils/podclique.go index 67e613f3f..e03aa9046 100644 --- a/operator/internal/controller/common/component/utils/podclique.go +++ b/operator/internal/controller/common/component/utils/podclique.go @@ -18,18 +18,16 @@ package utils import ( "context" - "fmt" "slices" "time" apicommon "github.com/ai-dynamo/grove/operator/api/common" "github.com/ai-dynamo/grove/operator/api/common/constants" grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" "github.com/ai-dynamo/grove/operator/internal/utils" - k8sutils "github.com/ai-dynamo/grove/operator/internal/utils/kubernetes" "github.com/samber/lo" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -198,19 +196,6 @@ func groupPCLQsByLabel(pclqs []grovecorev1alpha1.PodClique, labelKey string) map return grouped } -// ComputePCLQPodTemplateHash computes the pod template hash for the PCLQ pod spec. -func ComputePCLQPodTemplateHash(pclqTemplateSpec *grovecorev1alpha1.PodCliqueTemplateSpec, priorityClassName string) string { - podTemplateSpec := corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: pclqTemplateSpec.Labels, - Annotations: pclqTemplateSpec.Annotations, - }, - Spec: pclqTemplateSpec.Spec.PodSpec, - } - podTemplateSpec.Spec.PriorityClassName = priorityClassName - return k8sutils.ComputeHash(&podTemplateSpec) -} - // IsPCLQAutoUpdateInProgress checks if PodClique is under an auto-orchestrated update. func IsPCLQAutoUpdateInProgress(pclq *grovecorev1alpha1.PodClique) bool { return pclq.Status.UpdateProgress != nil && pclq.Status.UpdateProgress.UpdateEndedAt == nil @@ -223,17 +208,13 @@ func IsLastPCLQUpdateCompleted(pclq *grovecorev1alpha1.PodClique) bool { return pclq.Status.UpdateProgress != nil && pclq.Status.UpdateProgress.UpdateEndedAt != nil } -// GetExpectedPCLQPodTemplateHash finds the matching PodCliqueTemplateSpec from the PodCliqueSet and computes the pod template hash for the PCLQ pod spec. -func GetExpectedPCLQPodTemplateHash(pcs *grovecorev1alpha1.PodCliqueSet, pclqObjectMeta metav1.ObjectMeta) (string, error) { +// GetExpectedPCLQPodTemplateHash returns the selected pod template identity for a PodClique. +func GetExpectedPCLQPodTemplateHash(revision *commonrevision.SelectedRevision, pclqObjectMeta metav1.ObjectMeta) (string, error) { cliqueName, err := utils.GetPodCliqueNameFromPodCliqueFQN(pclqObjectMeta) if err != nil { return "", err } - matchingPCLQTemplateSpec := FindPodCliqueTemplateSpecByName(pcs, cliqueName) - if matchingPCLQTemplateSpec == nil { - return "", fmt.Errorf("pod clique template not found for cliqueName: %s", cliqueName) - } - return ComputePCLQPodTemplateHash(matchingPCLQTemplateSpec, pcs.Spec.Template.PriorityClassName), nil + return revision.CliqueHash(cliqueName) } // FindPodCliqueTemplateSpecByName retrieves the PodCliqueTemplateSpec from the PodCliqueSet by its name. diff --git a/operator/internal/controller/common/component/utils/podcliquescalinggroup.go b/operator/internal/controller/common/component/utils/podcliquescalinggroup.go index 588e46971..430f209d2 100644 --- a/operator/internal/controller/common/component/utils/podcliquescalinggroup.go +++ b/operator/internal/controller/common/component/utils/podcliquescalinggroup.go @@ -22,6 +22,7 @@ import ( apicommon "github.com/ai-dynamo/grove/operator/api/common" grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" "github.com/samber/lo" "sigs.k8s.io/controller-runtime/pkg/client" @@ -122,8 +123,8 @@ func GetPCSGsByPCSReplicaIndex(ctx context.Context, cl client.Client, pcsObjKey return pcsgsByPCSReplicaIndex, nil } -// GetPCLQTemplateHashes generates the Pod template hash for all PCLQs in a PCSG. Returns a map of [PCLQ Name : PodTemplateHas] -func GetPCLQTemplateHashes(pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup) map[string]string { +// GetPCLQTemplateHashes returns the selected Pod template identities for all PCLQs in a PCSG. +func GetPCLQTemplateHashes(revision *commonrevision.SelectedRevision, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup) map[string]string { pclqTemplateSpecs := make([]*grovecorev1alpha1.PodCliqueTemplateSpec, 0, len(pcsg.Spec.CliqueNames)) for _, cliqueName := range pcsg.Spec.CliqueNames { pclqTemplateSpec := FindPodCliqueTemplateSpecByName(pcs, cliqueName) @@ -136,7 +137,11 @@ func GetPCLQTemplateHashes(pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev for pcsgReplicaIndex := range int(pcsg.Spec.Replicas) { for _, pclqTemplateSpec := range pclqTemplateSpecs { pclqFQN := apicommon.GeneratePodCliqueName(apicommon.ResourceNameReplica{Name: pcsg.Name, Replica: pcsgReplicaIndex}, pclqTemplateSpec.Name) - cliqueTemplateSpecHashes[pclqFQN] = ComputePCLQPodTemplateHash(pclqTemplateSpec, pcs.Spec.Template.PriorityClassName) + hash, err := revision.CliqueHash(pclqTemplateSpec.Name) + if err != nil { + continue + } + cliqueTemplateSpecHashes[pclqFQN] = hash } } return cliqueTemplateSpecHashes @@ -145,9 +150,9 @@ func GetPCLQTemplateHashes(pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev // GetPCLQsInPCSGPendingUpdate collects the PodClique FQNs that are pending updates. // It identifies PCLQ pending update by comparing the current PodTemplateHash label on an existing PCLQ with that of // a computed PodTemplateHash from the latest PodCliqueSet resource. -func GetPCLQsInPCSGPendingUpdate(pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, existingPCLQs []grovecorev1alpha1.PodClique) []string { +func GetPCLQsInPCSGPendingUpdate(revision *commonrevision.SelectedRevision, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, existingPCLQs []grovecorev1alpha1.PodClique) []string { pclqFQNsPendingUpdate := make([]string, 0, len(existingPCLQs)) - expectedPCLQPodTemplateHashes := GetPCLQTemplateHashes(pcs, pcsg) + expectedPCLQPodTemplateHashes := GetPCLQTemplateHashes(revision, pcs, pcsg) for _, existingPCLQ := range existingPCLQs { existingPodTemplateHash := existingPCLQ.Labels[apicommon.LabelPodTemplateHash] expectedPodTemplateHash := expectedPCLQPodTemplateHashes[existingPCLQ.Name] diff --git a/operator/internal/controller/common/component/utils/revision.go b/operator/internal/controller/common/component/utils/revision.go new file mode 100644 index 000000000..75eabc5ee --- /dev/null +++ b/operator/internal/controller/common/component/utils/revision.go @@ -0,0 +1,51 @@ +// /* +// Copyright 2026 The Grove Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// */ + +package utils + +import ( + "context" + "fmt" + + grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" + + appsv1 "k8s.io/api/apps/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// GetSelectedPodCliqueSetRevision loads and validates the revision selected by the PodCliqueSet. +func GetSelectedPodCliqueSetRevision(ctx context.Context, cl client.Client, pcs *grovecorev1alpha1.PodCliqueSet) (*commonrevision.SelectedRevision, error) { + if pcs.Status.CurrentRevision == nil || *pcs.Status.CurrentRevision == "" { + return nil, fmt.Errorf("PodCliqueSet %v has no selected ControllerRevision", client.ObjectKeyFromObject(pcs)) + } + controllerRevision := &appsv1.ControllerRevision{} + if err := cl.Get(ctx, client.ObjectKey{Namespace: pcs.Namespace, Name: *pcs.Status.CurrentRevision}, controllerRevision); err != nil { + return nil, err + } + if !metav1.IsControlledBy(controllerRevision, pcs) { + return nil, fmt.Errorf("ControllerRevision %v is not controlled by PodCliqueSet %v", client.ObjectKeyFromObject(controllerRevision), client.ObjectKeyFromObject(pcs)) + } + selectedRevision, err := commonrevision.DecodeSelectedRevision(controllerRevision.Data.Raw) + if err != nil { + return nil, fmt.Errorf("ControllerRevision %v has invalid revision data: %w", client.ObjectKeyFromObject(controllerRevision), err) + } + if pcs.Status.CurrentGenerationHash == nil || *pcs.Status.CurrentGenerationHash != selectedRevision.GenerationHash() { + return nil, fmt.Errorf("PodCliqueSet %v generation identity does not match ControllerRevision %v", client.ObjectKeyFromObject(pcs), client.ObjectKeyFromObject(controllerRevision)) + } + return selectedRevision, nil +} diff --git a/operator/internal/controller/common/revision/data.go b/operator/internal/controller/common/revision/data.go new file mode 100644 index 000000000..4df505bf6 --- /dev/null +++ b/operator/internal/controller/common/revision/data.go @@ -0,0 +1,105 @@ +// /* +// Copyright 2026 The Grove Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// */ + +// Package revision defines the immutable data stored in a PodCliqueSet ControllerRevision. +package revision + +import ( + "encoding/json" + "fmt" + "slices" +) + +// DataVersion is the current ControllerRevision data schema version. +const DataVersion = 1 + +// Data is the persisted ControllerRevision.Data schema. Revision construction code populates it; +// reconciliation code should consume a validated SelectedRevision instead. +type Data struct { + Version int `json:"version"` + Desired json.RawMessage `json:"desired"` + Cliques map[string]json.RawMessage `json:"cliques"` + GenerationHash string `json:"generationHash"` + CliqueHashes map[string]string `json:"cliqueHashes"` +} + +// SelectedRevision is a validated, read-oriented view of persisted revision data. +// It deliberately exposes no maps or owned byte slices so reconciliation code cannot mutate +// the selected rollout identity after loading it. +type SelectedRevision struct { + data Data +} + +// DecodeSelectedRevision decodes and validates persisted ControllerRevision data. +func DecodeSelectedRevision(raw []byte) (*SelectedRevision, error) { + data := Data{} + if err := json.Unmarshal(raw, &data); err != nil { + return nil, fmt.Errorf("could not decode selected revision: %w", err) + } + if err := validateData(data); err != nil { + return nil, err + } + return &SelectedRevision{data: data}, nil +} + +// Desired returns a copy of the complete desired rollout content. +func (r *SelectedRevision) Desired() json.RawMessage { + return slices.Clone(r.data.Desired) +} + +// Clique returns a copy of the desired rollout content for one clique. +func (r *SelectedRevision) Clique(name string) (json.RawMessage, bool) { + clique, ok := r.data.Cliques[name] + return slices.Clone(clique), ok +} + +// GenerationHash returns the persisted PodCliqueSet generation identity. +func (r *SelectedRevision) GenerationHash() string { + return r.data.GenerationHash +} + +// CliqueHash returns the persisted template identity selected for one clique. +func (r *SelectedRevision) CliqueHash(name string) (string, error) { + hash, ok := r.data.CliqueHashes[name] + if !ok { + return "", fmt.Errorf("no selected pod template hash for cliqueName: %s", name) + } + return hash, nil +} + +func validateData(data Data) error { + if data.Version != DataVersion { + return fmt.Errorf("unsupported selected revision version: %d", data.Version) + } + if data.GenerationHash == "" { + return fmt.Errorf("selected revision has no generation hash") + } + if !json.Valid(data.Desired) { + return fmt.Errorf("selected revision has invalid desired content") + } + if len(data.Cliques) != len(data.CliqueHashes) { + return fmt.Errorf("selected revision has different clique content and hash counts") + } + for name, clique := range data.Cliques { + if !json.Valid(clique) { + return fmt.Errorf("selected revision has invalid content for clique %s", name) + } + if data.CliqueHashes[name] == "" { + return fmt.Errorf("selected revision has no hash for clique %s", name) + } + } + return nil +} diff --git a/operator/internal/controller/common/revision/data_test.go b/operator/internal/controller/common/revision/data_test.go new file mode 100644 index 000000000..ebe4e8d13 --- /dev/null +++ b/operator/internal/controller/common/revision/data_test.go @@ -0,0 +1,124 @@ +// /* +// Copyright 2026 The Grove Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// */ + +package revision + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDecodeSelectedRevision(t *testing.T) { + raw := mustMarshalData(t, Data{ + Version: DataVersion, + Desired: json.RawMessage(`[{"spec":{"containers":[]}}]`), + Cliques: map[string]json.RawMessage{"worker": json.RawMessage(`{"spec":{"containers":[]}}`)}, + GenerationHash: "generation", + CliqueHashes: map[string]string{"worker": "template"}, + }) + + selected, err := DecodeSelectedRevision(raw) + require.NoError(t, err) + assert.Equal(t, "generation", selected.GenerationHash()) + hash, err := selected.CliqueHash("worker") + require.NoError(t, err) + assert.Equal(t, "template", hash) + _, err = selected.CliqueHash("missing") + require.Error(t, err) + + desired := selected.Desired() + clique, ok := selected.Clique("worker") + require.True(t, ok) + desired[0] = 'x' + clique[0] = 'x' + assert.JSONEq(t, `[{"spec":{"containers":[]}}]`, string(selected.Desired())) + clique, ok = selected.Clique("worker") + require.True(t, ok) + assert.JSONEq(t, `{"spec":{"containers":[]}}`, string(clique)) +} + +func TestDecodeSelectedRevisionRejectsInvalidData(t *testing.T) { + tests := []struct { + name string + raw func(*testing.T) []byte + }{ + { + name: "malformed JSON", + raw: func(*testing.T) []byte { return []byte(`{`) }, + }, + { + name: "unsupported version", + raw: func(t *testing.T) []byte { + return mustMarshalData(t, validData(func(data *Data) { data.Version++ })) + }, + }, + { + name: "empty generation hash", + raw: func(t *testing.T) []byte { + return mustMarshalData(t, validData(func(data *Data) { data.GenerationHash = "" })) + }, + }, + { + name: "missing desired content", + raw: func(*testing.T) []byte { + return []byte(`{"version":1,"cliques":{"worker":{}},"generationHash":"generation","cliqueHashes":{"worker":"template"}}`) + }, + }, + { + name: "clique and hash sets differ", + raw: func(t *testing.T) []byte { + return mustMarshalData(t, validData(func(data *Data) { + data.CliqueHashes = map[string]string{"other": "template"} + })) + }, + }, + { + name: "empty clique hash", + raw: func(t *testing.T) []byte { + return mustMarshalData(t, validData(func(data *Data) { data.CliqueHashes["worker"] = "" })) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := DecodeSelectedRevision(tt.raw(t)) + require.Error(t, err) + }) + } +} + +func validData(mutate func(*Data)) Data { + data := Data{ + Version: DataVersion, + Desired: json.RawMessage(`[]`), + Cliques: map[string]json.RawMessage{"worker": json.RawMessage(`{}`)}, + GenerationHash: "generation", + CliqueHashes: map[string]string{"worker": "template"}, + } + mutate(&data) + return data +} + +func mustMarshalData(t *testing.T, data Data) []byte { + t.Helper() + raw, err := json.Marshal(data) + require.NoError(t, err) + return raw +} diff --git a/operator/internal/controller/manager.go b/operator/internal/controller/manager.go index 7768e2519..546ed45a7 100644 --- a/operator/internal/controller/manager.go +++ b/operator/internal/controller/manager.go @@ -32,6 +32,7 @@ import ( "github.com/ai-dynamo/grove/operator/internal/webhook" "github.com/go-logr/logr" + appsv1 "k8s.io/api/apps/v1" autoscalingv2 "k8s.io/api/autoscaling/v2" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -126,7 +127,7 @@ func createManagerOptions(operatorCfg *configv1alpha1.OperatorConfiguration) ctr } // cacheOptions returns cache configuration that restricts informers for shared -// core types to only grove-managed resources via label selectors. +// Kubernetes types to only grove-managed resources via label selectors. // Grove CRDs are not filtered because all instances are grove-managed by definition. func cacheOptions() cache.Options { managedByGrove := cache.ByObject{ @@ -136,6 +137,7 @@ func cacheOptions() cache.Options { } return cache.Options{ ByObject: map[client.Object]cache.ByObject{ + &appsv1.ControllerRevision{}: managedByGrove, &corev1.Pod{}: managedByGrove, &corev1.ServiceAccount{}: managedByGrove, &corev1.Service{}: managedByGrove, diff --git a/operator/internal/controller/manager_test.go b/operator/internal/controller/manager_test.go index f85028518..efb85a189 100644 --- a/operator/internal/controller/manager_test.go +++ b/operator/internal/controller/manager_test.go @@ -35,6 +35,7 @@ import ( "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" autoscalingv2 "k8s.io/api/autoscaling/v2" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -361,6 +362,7 @@ func TestCreateManagerOptions(t *testing.T) { // Secret is intentionally excluded — the webhook TLS secret may be created // by Helm without the managed-by label. expectedTypes := []client.Object{ + &appsv1.ControllerRevision{}, &corev1.Pod{}, &corev1.ServiceAccount{}, &corev1.Service{}, diff --git a/operator/internal/controller/podclique/components/pod/syncflow.go b/operator/internal/controller/podclique/components/pod/syncflow.go index 1e3d63f6d..d98c323b9 100644 --- a/operator/internal/controller/podclique/components/pod/syncflow.go +++ b/operator/internal/controller/podclique/components/pod/syncflow.go @@ -60,7 +60,15 @@ func (r _resource) prepareSyncFlow(ctx context.Context, logger logr.Logger, pclq ) } - sc.expectedPodTemplateHash, err = componentutils.GetExpectedPCLQPodTemplateHash(sc.pcs, pclq.ObjectMeta) + selectedRevision, err := componentutils.GetSelectedPodCliqueSetRevision(ctx, r.client, sc.pcs) + if err != nil { + return nil, groveerr.WrapError(err, + errCodeGetPodCliqueTemplate, + component.OperationSync, + fmt.Sprintf("failed to get PodCliqueSet revision for PodClique: %v", client.ObjectKeyFromObject(pclq)), + ) + } + sc.expectedPodTemplateHash, err = componentutils.GetExpectedPCLQPodTemplateHash(selectedRevision, pclq.ObjectMeta) if err != nil { return nil, groveerr.WrapError(err, errCodeGetPodCliqueTemplate, diff --git a/operator/internal/controller/podclique/reconcilespec.go b/operator/internal/controller/podclique/reconcilespec.go index a6aa4e5b3..8c957c954 100644 --- a/operator/internal/controller/podclique/reconcilespec.go +++ b/operator/internal/controller/podclique/reconcilespec.go @@ -75,20 +75,25 @@ func (r *Reconciler) processUpdate(ctx context.Context, logger logr.Logger, pclq if err != nil { return ctrlcommon.ReconcileWithErrors(fmt.Sprintf("could not get owner PodCliqueSet for PodClique: %v", pclqObjectKey), err) } + selectedRevision, err := componentutils.GetSelectedPodCliqueSetRevision(ctx, r.client, pcs) + if err != nil { + return ctrlcommon.ReconcileWithErrors("PodCliqueSet revision is not initialized", err) + } + podTemplateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(selectedRevision, pclq.ObjectMeta) + if err != nil { + return ctrlcommon.ReconcileWithErrors("PodCliqueSet revision does not contain PodClique", err) + } // Handle OnDelete strategy first if !componentutils.IsAutoUpdateStrategy(pcs) { - if shouldResetOrTriggerUpdate(pcs, pclq) { - if err = r.initOrResetUpdate(ctx, pcs, pclq); err != nil { + if shouldResetOrTriggerUpdate(selectedRevision.GenerationHash(), pclq) { + if err = r.initOrResetUpdate(ctx, pcs, pclq, selectedRevision.GenerationHash(), podTemplateHash); err != nil { return ctrlcommon.ReconcileWithErrors("could not initialize update for OnDelete", err) } } return ctrlcommon.ContinueReconcile() } - if pcs.Status.CurrentGenerationHash == nil { - return ctrlcommon.ContinueReconcile() - } shouldEvaluatePCLQForUpdates, err := shouldCheckPendingUpdatesForPCLQ(logger, pcs, pclq) if err != nil { return ctrlcommon.ReconcileWithErrors("error checking if PodClique should be evaluated for pending updates", err) @@ -97,9 +102,9 @@ func (r *Reconciler) processUpdate(ctx context.Context, logger logr.Logger, pclq return ctrlcommon.ContinueReconcile() } - if shouldResetOrTriggerUpdate(pcs, pclq) { - logger.Info("PodCliqueSet has a new generation hash. Initializing or resetting update for PodClique", "PodCliqueSetGenerationHash", *pcs.Status.CurrentGenerationHash, "CurrentPodCliqueSetGenerationHash", pclq.Status.CurrentPodCliqueSetGenerationHash, "isPCLQAutoUpdateInProgress", componentutils.IsPCLQAutoUpdateInProgress(pclq), "isLastPCLQUpdateCompleted", componentutils.IsLastPCLQUpdateCompleted(pclq)) - if err = r.initOrResetUpdate(ctx, pcs, pclq); err != nil { + if shouldResetOrTriggerUpdate(selectedRevision.GenerationHash(), pclq) { + logger.Info("PodCliqueSet has a new generation hash. Initializing or resetting update for PodClique", "PodCliqueSetGenerationHash", selectedRevision.GenerationHash(), "CurrentPodCliqueSetGenerationHash", pclq.Status.CurrentPodCliqueSetGenerationHash, "isPCLQAutoUpdateInProgress", componentutils.IsPCLQAutoUpdateInProgress(pclq), "isLastPCLQUpdateCompleted", componentutils.IsLastPCLQUpdateCompleted(pclq)) + if err = r.initOrResetUpdate(ctx, pcs, pclq, selectedRevision.GenerationHash(), podTemplateHash); err != nil { return ctrlcommon.ReconcileWithErrors("could not initialize RollingRecreate update", err) } } @@ -142,14 +147,9 @@ func shouldCheckPendingUpdatesForPCLQ(logger logr.Logger, pcs *grovecorev1alpha1 } // shouldResetOrTriggerUpdate determines if an update should be started or reset based on generation hash comparison -func shouldResetOrTriggerUpdate(pcs *grovecorev1alpha1.PodCliqueSet, pclq *grovecorev1alpha1.PodClique) bool { - // Wait for the first reconciliation of the PodCliqueSet - if pcs.Status.CurrentGenerationHash == nil { - return false - } - +func shouldResetOrTriggerUpdate(generationHash string, pclq *grovecorev1alpha1.PodClique) bool { // PCLQ has never been updated yet and PCS has a new generation hash. - firstEverUpdateRequired := pclq.Status.UpdateProgress == nil && pclq.Status.CurrentPodCliqueSetGenerationHash != nil && *pcs.Status.CurrentGenerationHash != *pclq.Status.CurrentPodCliqueSetGenerationHash + firstEverUpdateRequired := pclq.Status.UpdateProgress == nil && pclq.Status.CurrentPodCliqueSetGenerationHash != nil && generationHash != *pclq.Status.CurrentPodCliqueSetGenerationHash if firstEverUpdateRequired { return true } @@ -157,9 +157,9 @@ func shouldResetOrTriggerUpdate(pcs *grovecorev1alpha1.PodCliqueSet, pclq *grove // PCLQ is undergoing an update for a different PCS generation hash // Irrespective of whether the pod template hash has changed or not, the in-progress update is stale and needs to be // reset in order to set the correct updateProgress.PodCliqueSetGenerationHash - inProgressPCLQUpdateNotStale := componentutils.IsPCLQAutoUpdateInProgress(pclq) && pclq.Status.UpdateProgress.PodCliqueSetGenerationHash == *pcs.Status.CurrentGenerationHash + inProgressPCLQUpdateNotStale := componentutils.IsPCLQAutoUpdateInProgress(pclq) && pclq.Status.UpdateProgress.PodCliqueSetGenerationHash == generationHash // PCLQ had an update in the past but that was for an older PCS generation hash. - lastCompletedUpdateIsNotStale := componentutils.IsLastPCLQUpdateCompleted(pclq) && pclq.Status.UpdateProgress.PodCliqueSetGenerationHash == *pcs.Status.CurrentGenerationHash + lastCompletedUpdateIsNotStale := componentutils.IsLastPCLQUpdateCompleted(pclq) && pclq.Status.UpdateProgress.PodCliqueSetGenerationHash == generationHash if inProgressPCLQUpdateNotStale || lastCompletedUpdateIsNotStale { return false } @@ -168,16 +168,12 @@ func shouldResetOrTriggerUpdate(pcs *grovecorev1alpha1.PodCliqueSet, pclq *grove } // initOrResetUpdate initializes or resets the update progress status for the PodClique -func (r *Reconciler) initOrResetUpdate(ctx context.Context, pcs *grovecorev1alpha1.PodCliqueSet, pclq *grovecorev1alpha1.PodClique) error { - podTemplateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(pcs, pclq.ObjectMeta) - if err != nil { - return fmt.Errorf("could not update PodClique %s status with update progress: %w", client.ObjectKeyFromObject(pclq), err) - } +func (r *Reconciler) initOrResetUpdate(ctx context.Context, pcs *grovecorev1alpha1.PodCliqueSet, pclq *grovecorev1alpha1.PodClique, generationHash, podTemplateHash string) error { // reset and start the update patch := client.MergeFrom(pclq.DeepCopy()) pclq.Status.UpdateProgress = &grovecorev1alpha1.PodCliqueUpdateProgress{ UpdateStartedAt: metav1.Now(), - PodCliqueSetGenerationHash: *pcs.Status.CurrentGenerationHash, + PodCliqueSetGenerationHash: generationHash, PodTemplateHash: podTemplateHash, } // OnDelete strategy sets UpdateEndedAt too, since we do not know when all the pods will manually be deleted, and gang termination is disabled when an update is in progress @@ -186,7 +182,7 @@ func (r *Reconciler) initOrResetUpdate(ctx context.Context, pcs *grovecorev1alph } // reset the updated replicas count to 0 so that the update can start afresh. pclq.Status.UpdatedReplicas = 0 - if err = r.client.Status().Patch(ctx, pclq, patch); err != nil { + if err := r.client.Status().Patch(ctx, pclq, patch); err != nil { return fmt.Errorf("failed to update PodClique %s status with update progress: %w", client.ObjectKeyFromObject(pclq), err) } return nil diff --git a/operator/internal/controller/podclique/reconcilespec_test.go b/operator/internal/controller/podclique/reconcilespec_test.go index 0e2befc04..b5869ed6f 100644 --- a/operator/internal/controller/podclique/reconcilespec_test.go +++ b/operator/internal/controller/podclique/reconcilespec_test.go @@ -126,33 +126,14 @@ func TestGetOrderedKindsForSync(t *testing.T) { // TestShouldResetOrTriggerUpdate tests the shouldResetOrTriggerUpdate function for PodClique func TestShouldResetOrTriggerUpdate(t *testing.T) { tests := []struct { - name string - pcs *grovecorev1alpha1.PodCliqueSet - pclq *grovecorev1alpha1.PodClique - expected bool + name string + generationHash string + pclq *grovecorev1alpha1.PodClique + expected bool }{ { - name: "waits_for_pcs_current_generation_hash_before_first_ever_update", - pcs: &grovecorev1alpha1.PodCliqueSet{ - Status: grovecorev1alpha1.PodCliqueSetStatus{ - CurrentGenerationHash: nil, - }, - }, - pclq: &grovecorev1alpha1.PodClique{ - Status: grovecorev1alpha1.PodCliqueStatus{ - UpdateProgress: nil, - CurrentPodCliqueSetGenerationHash: ptr.To("old-hash"), - }, - }, - expected: false, - }, - { - name: "first_ever_update_required", - pcs: &grovecorev1alpha1.PodCliqueSet{ - Status: grovecorev1alpha1.PodCliqueSetStatus{ - CurrentGenerationHash: ptr.To("new-hash"), - }, - }, + name: "first_ever_update_required", + generationHash: "new-hash", pclq: &grovecorev1alpha1.PodClique{ Status: grovecorev1alpha1.PodCliqueStatus{ UpdateProgress: nil, @@ -162,12 +143,8 @@ func TestShouldResetOrTriggerUpdate(t *testing.T) { expected: true, }, { - name: "in_progress_update_not_stale_same_hash", - pcs: &grovecorev1alpha1.PodCliqueSet{ - Status: grovecorev1alpha1.PodCliqueSetStatus{ - CurrentGenerationHash: ptr.To("current-hash"), - }, - }, + name: "in_progress_update_not_stale_same_hash", + generationHash: "current-hash", pclq: &grovecorev1alpha1.PodClique{ Status: grovecorev1alpha1.PodCliqueStatus{ UpdateProgress: &grovecorev1alpha1.PodCliqueUpdateProgress{ @@ -180,12 +157,8 @@ func TestShouldResetOrTriggerUpdate(t *testing.T) { expected: false, }, { - name: "in_progress_update_stale_different_hash", - pcs: &grovecorev1alpha1.PodCliqueSet{ - Status: grovecorev1alpha1.PodCliqueSetStatus{ - CurrentGenerationHash: ptr.To("new-hash"), - }, - }, + name: "in_progress_update_stale_different_hash", + generationHash: "new-hash", pclq: &grovecorev1alpha1.PodClique{ Status: grovecorev1alpha1.PodCliqueStatus{ UpdateProgress: &grovecorev1alpha1.PodCliqueUpdateProgress{ @@ -198,12 +171,8 @@ func TestShouldResetOrTriggerUpdate(t *testing.T) { expected: true, }, { - name: "last_completed_update_not_stale_same_hash", - pcs: &grovecorev1alpha1.PodCliqueSet{ - Status: grovecorev1alpha1.PodCliqueSetStatus{ - CurrentGenerationHash: ptr.To("current-hash"), - }, - }, + name: "last_completed_update_not_stale_same_hash", + generationHash: "current-hash", pclq: &grovecorev1alpha1.PodClique{ Status: grovecorev1alpha1.PodCliqueStatus{ UpdateProgress: &grovecorev1alpha1.PodCliqueUpdateProgress{ @@ -216,12 +185,8 @@ func TestShouldResetOrTriggerUpdate(t *testing.T) { expected: false, }, { - name: "last_completed_update_stale_different_hash", - pcs: &grovecorev1alpha1.PodCliqueSet{ - Status: grovecorev1alpha1.PodCliqueSetStatus{ - CurrentGenerationHash: ptr.To("new-hash"), - }, - }, + name: "last_completed_update_stale_different_hash", + generationHash: "new-hash", pclq: &grovecorev1alpha1.PodClique{ Status: grovecorev1alpha1.PodCliqueStatus{ UpdateProgress: &grovecorev1alpha1.PodCliqueUpdateProgress{ @@ -237,7 +202,7 @@ func TestShouldResetOrTriggerUpdate(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - result := shouldResetOrTriggerUpdate(tc.pcs, tc.pclq) + result := shouldResetOrTriggerUpdate(tc.generationHash, tc.pclq) assert.Equal(t, tc.expected, result) }) } @@ -265,7 +230,8 @@ func TestProcessUpdateInitializesProgressWithoutActivePCSUpdate(t *testing.T) { UpdatedReplicas: 1, } - fakeClient := testutils.SetupFakeClient(pcs, pclq) + revision := testutils.NewPodCliqueSetControllerRevision(pcs, testutils.ComputePodCliqueTemplateHashes(pcs)) + fakeClient := testutils.SetupFakeClient(pcs, pclq, revision) reconciler := &Reconciler{client: fakeClient} result := reconciler.processUpdate(context.Background(), logr.Discard(), pclq) @@ -410,7 +376,7 @@ func TestInitOrResetUpdate(t *testing.T) { fakeClient := testutils.SetupFakeClient(pcs, pclq) reconciler := &Reconciler{client: fakeClient} - err := reconciler.initOrResetUpdate(context.Background(), pcs, pclq) + err := reconciler.initOrResetUpdate(context.Background(), pcs, pclq, *pcs.Status.CurrentGenerationHash, "new-pod-template-hash") require.NoError(t, err, "initOrResetUpdate should not return errors") // Fetch the updated PCLQ from the fake client diff --git a/operator/internal/controller/podclique/reconcilestatus.go b/operator/internal/controller/podclique/reconcilestatus.go index 2df9b7612..bbafef2ac 100644 --- a/operator/internal/controller/podclique/reconcilestatus.go +++ b/operator/internal/controller/podclique/reconcilestatus.go @@ -26,6 +26,7 @@ import ( internalconstants "github.com/ai-dynamo/grove/operator/internal/constants" ctrlcommon "github.com/ai-dynamo/grove/operator/internal/controller/common" componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" k8sutils "github.com/ai-dynamo/grove/operator/internal/utils/kubernetes" "github.com/go-logr/logr" @@ -53,6 +54,10 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, logger logr.Logger, pc logger.Error(err, "could not get PodCliqueSet for PodClique", "pclqObjectKey", pclqObjectKey) return ctrlcommon.ReconcileWithErrors("could not get PodCliqueSet for PodClique", err) } + selectedRevision, err := componentutils.GetSelectedPodCliqueSetRevision(ctx, r.client, pcs) + if err != nil { + return ctrlcommon.ReconcileWithErrors("could not get PodCliqueSet revision", err) + } existingPods, err := componentutils.GetPCLQPods(ctx, r.client, pcsName, pclq) if err != nil { @@ -66,7 +71,7 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, logger logr.Logger, pc mutateReplicas(pclq, podCategories, len(existingPods)) mutateUpdatedReplica(pclq, existingPods) // mutate PodClique.Status.CurrentPodTemplateHash and PodClique.Status.CurrentPodCliqueSetGenerationHash - if err = mutateCurrentHashes(logger, pcs, pclq); err != nil { + if err = mutateCurrentHashes(logger, selectedRevision, pclq); err != nil { logger.Error(err, "failed to compute PodClique current hashes") return ctrlcommon.ReconcileWithErrors("failed to compute PodClique current hashes", err) } @@ -107,19 +112,19 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, logger logr.Logger, pc } // mutateCurrentHashes updates the PodClique's current template and generation hashes when updates are complete -func mutateCurrentHashes(logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, pclq *grovecorev1alpha1.PodClique) error { +func mutateCurrentHashes(logger logr.Logger, revision *commonrevision.SelectedRevision, pclq *grovecorev1alpha1.PodClique) error { if componentutils.IsPCLQAutoUpdateInProgress(pclq) || pclq.Status.UpdatedReplicas != pclq.Status.Replicas { logger.Info("PodClique is currently updating, cannot set PodCliqueSet CurrentGenerationHash yet") return nil } if pclq.Status.UpdateProgress == nil { - expectedPodTemplateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(pcs, pclq.ObjectMeta) + expectedPodTemplateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(revision, pclq.ObjectMeta) if err != nil { return err } if isPodCliqueTemplateHashCurrent(pclq, expectedPodTemplateHash) { pclq.Status.CurrentPodTemplateHash = ptr.To(expectedPodTemplateHash) - pclq.Status.CurrentPodCliqueSetGenerationHash = pcs.Status.CurrentGenerationHash + pclq.Status.CurrentPodCliqueSetGenerationHash = ptr.To(revision.GenerationHash()) } } else if componentutils.IsLastPCLQUpdateCompleted(pclq) { logger.Info("PodClique update has completed, setting CurrentPodCliqueSetGenerationHash") diff --git a/operator/internal/controller/podclique/reconcilestatus_test.go b/operator/internal/controller/podclique/reconcilestatus_test.go index 804217268..266479a16 100644 --- a/operator/internal/controller/podclique/reconcilestatus_test.go +++ b/operator/internal/controller/podclique/reconcilestatus_test.go @@ -25,7 +25,7 @@ import ( "github.com/ai-dynamo/grove/operator/api/common/constants" grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" internalconstants "github.com/ai-dynamo/grove/operator/internal/constants" - componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + "github.com/ai-dynamo/grove/operator/internal/utils/podtemplatehash" testutils "github.com/ai-dynamo/grove/operator/test/utils" "github.com/go-logr/logr" @@ -234,7 +234,8 @@ func TestReconcileStatusConvergesWhenReadyPodMatchesDesiredHash(t *testing.T) { } pod := createReadyOwnedPodWithHash("ready-current-pod", pclq, templateHash) - cl := testutils.SetupFakeClient(pcs, pclq, pod) + revision := testutils.NewPodCliqueSetControllerRevision(pcs, map[string]string{"worker": templateHash}) + cl := testutils.SetupFakeClient(pcs, pclq, pod, revision) r := &Reconciler{ client: cl, eventRecorder: record.NewFakeRecorder(1), @@ -257,14 +258,15 @@ func TestReconcileStatusConvergesWhenReadyPodMatchesDesiredHash(t *testing.T) { // actually converged on the current PodCliqueSet template, even though the // replica counts (Replicas == UpdatedReplicas) would superficially suggest it has. func TestMutateCurrentHashesDoesNotAdvanceWhenTemplateHashIsStale(t *testing.T) { - pcs, pclq, _ := newPodCliqueHashConvergenceFixture(t) + pcs, pclq, templateHash := newPodCliqueHashConvergenceFixture(t) pclq.Labels[apicommon.LabelPodTemplateHash] = "stale-template-hash" pclq.Status.CurrentPodTemplateHash = ptr.To("") pclq.Status.CurrentPodCliqueSetGenerationHash = ptr.To("old-generation-hash") pclq.Status.Replicas = 2 pclq.Status.UpdatedReplicas = 2 - err := mutateCurrentHashes(logr.Discard(), pcs, pclq) + selectedRevision := testutils.NewSelectedRevision(*pcs.Status.CurrentGenerationHash, map[string]string{"worker": templateHash}) + err := mutateCurrentHashes(logr.Discard(), selectedRevision, pclq) require.NoError(t, err) assert.Equal(t, "", *pclq.Status.CurrentPodTemplateHash) @@ -282,6 +284,7 @@ func newPodCliqueHashConvergenceFixture(t *testing.T) (*grovecorev1alpha1.PodCli }, } generationHash := "current-generation-hash" + templateHash := podtemplatehash.ComputePodClique(template, "") pcs := &grovecorev1alpha1.PodCliqueSet{ ObjectMeta: metav1.ObjectMeta{Name: "pcs", Namespace: "default"}, Spec: grovecorev1alpha1.PodCliqueSetSpec{ @@ -303,8 +306,6 @@ func newPodCliqueHashConvergenceFixture(t *testing.T) (*grovecorev1alpha1.PodCli }, }, } - templateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(pcs, pclq.ObjectMeta) - require.NoError(t, err) pclq.Labels[apicommon.LabelPodTemplateHash] = templateHash return pcs, pclq, templateHash } diff --git a/operator/internal/controller/podcliquescalinggroup/components/podclique/podclique.go b/operator/internal/controller/podcliquescalinggroup/components/podclique/podclique.go index 0b3eec388..726a5e72d 100644 --- a/operator/internal/controller/podcliquescalinggroup/components/podclique/podclique.go +++ b/operator/internal/controller/podcliquescalinggroup/components/podclique/podclique.go @@ -31,6 +31,7 @@ import ( "github.com/ai-dynamo/grove/operator/internal/constants" "github.com/ai-dynamo/grove/operator/internal/controller/common/component" componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" groveerr "github.com/ai-dynamo/grove/operator/internal/errors" "github.com/ai-dynamo/grove/operator/internal/mnnvl" "github.com/ai-dynamo/grove/operator/internal/utils" @@ -233,11 +234,11 @@ func (r _resource) getPCSGTemplateNumPods(pcs *grovecorev1alpha1.PodCliqueSet, p } // doCreate creates or updates a PodClique resource with proper configuration from PCS and PCSG templates -func (r _resource) doCreate(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pcsgReplicaIndex int, pclqObjectKey client.ObjectKey) error { +func (r _resource) doCreate(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, revision *commonrevision.SelectedRevision, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pcsgReplicaIndex int, pclqObjectKey client.ObjectKey) error { logger.Info("Running CreateOrUpdate PodClique", "pclqObjectKey", pclqObjectKey) pclq := emptyPodClique(pclqObjectKey) pcsgObjKey := client.ObjectKeyFromObject(pclq) - if err := r.buildResource(logger, pcs, pcsg, pcsgReplicaIndex, pclq, false); err != nil { + if err := r.buildResource(logger, pcs, revision, pcsg, pcsgReplicaIndex, pclq, false); err != nil { return err } if err := r.client.Create(ctx, pclq); err != nil { @@ -259,13 +260,13 @@ func (r _resource) doCreate(ctx context.Context, logger logr.Logger, pcs *grovec // doCreateOrUpdate creates or updates a PodClique resource using CreateOrPatch. // This preserves the existing replicas value to avoid overwriting HPA-managed scaling. -func (r _resource) doCreateOrUpdate(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pcsgReplicaIndex int, pclqObjectKey client.ObjectKey, pclqExists bool) error { +func (r _resource) doCreateOrUpdate(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, revision *commonrevision.SelectedRevision, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pcsgReplicaIndex int, pclqObjectKey client.ObjectKey, pclqExists bool) error { logger.Info("Running CreateOrUpdate PodClique", "pclqObjectKey", pclqObjectKey) pclq := emptyPodClique(pclqObjectKey) pcsgObjKey := client.ObjectKeyFromObject(pcsg) opResult, err := controllerutil.CreateOrPatch(ctx, r.client, pclq, func() error { - return r.buildResource(logger, pcs, pcsg, pcsgReplicaIndex, pclq, pclqExists) + return r.buildResource(logger, pcs, revision, pcsg, pcsgReplicaIndex, pclq, pclqExists) }) if err != nil { r.eventRecorder.Eventf(pcsg, corev1.EventTypeWarning, constants.ReasonPodCliqueCreateOrUpdateFailed, "PodClique %v creation or update failed: %v", pclqObjectKey, err) @@ -283,7 +284,7 @@ func (r _resource) doCreateOrUpdate(ctx context.Context, logger logr.Logger, pcs // buildResource constructs a PodClique resource from templates, setting up metadata, labels, dependencies and environment variables. // When pclqExists is true, the current replicas value is preserved to avoid overwriting HPA-managed scaling. -func (r _resource) buildResource(logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pcsgReplicaIndex int, pclq *grovecorev1alpha1.PodClique, pclqExists bool) error { +func (r _resource) buildResource(logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, revision *commonrevision.SelectedRevision, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pcsgReplicaIndex int, pclq *grovecorev1alpha1.PodClique, pclqExists bool) error { var err error pclqObjectKey, pcsObjectKey := client.ObjectKeyFromObject(pclq), client.ObjectKeyFromObject(pcs) pclqTemplateSpec, foundAtIndex, ok := lo.FindIndexOf(pcs.Spec.Template.Cliques, func(pclqTemplateSpec *grovecorev1alpha1.PodCliqueTemplateSpec) bool { @@ -315,7 +316,11 @@ func (r _resource) buildResource(logger logr.Logger, pcs *grovecorev1alpha1.PodC podGangName := apicommon.GeneratePodGangNameForPodCliqueOwnedByPCSG(pcs, pcsReplicaIndex, pcsg, pcsgReplicaIndex) - pclq.Labels = getLabels(pcs, pcsReplicaIndex, pcsg, pcsgReplicaIndex, pclqObjectKey, pclqTemplateSpec, podGangName) + podTemplateHash, err := revision.CliqueHash(pclqTemplateSpec.Name) + if err != nil { + return err + } + pclq.Labels = getLabels(pcs, pcsReplicaIndex, pcsg, pcsgReplicaIndex, pclqObjectKey, pclqTemplateSpec, podGangName, podTemplateHash) pclq.Annotations = maps.Clone(pclqTemplateSpec.Annotations) // PodGang owns topology selection; do not propagate a template topology annotation to PodClique pods. delete(pclq.Annotations, apiconstants.AnnotationTopologyName) @@ -469,7 +474,7 @@ func getPodCliqueSelectorLabels(pcsgObjectMeta metav1.ObjectMeta) map[string]str } // getLabels constructs the complete set of labels for a PodClique including Grove-specific, component, and template labels -func getLabels(pcs *grovecorev1alpha1.PodCliqueSet, pcsReplicaIndex int, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pcsgReplicaIndex int, pclqObjectKey client.ObjectKey, pclqTemplateSpec *grovecorev1alpha1.PodCliqueTemplateSpec, podGangName string) map[string]string { +func getLabels(pcs *grovecorev1alpha1.PodCliqueSet, pcsReplicaIndex int, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pcsgReplicaIndex int, pclqObjectKey client.ObjectKey, pclqTemplateSpec *grovecorev1alpha1.PodCliqueTemplateSpec, podGangName, podTemplateHash string) map[string]string { pclqComponentLabels := map[string]string{ apicommon.LabelAppNameKey: pclqObjectKey.Name, apicommon.LabelComponentKey: apicommon.LabelComponentNamePodCliqueScalingGroupPodClique, @@ -477,7 +482,7 @@ func getLabels(pcs *grovecorev1alpha1.PodCliqueSet, pcsReplicaIndex int, pcsg *g apicommon.LabelPodGang: podGangName, apicommon.LabelPodCliqueSetReplicaIndex: strconv.Itoa(pcsReplicaIndex), apicommon.LabelPodCliqueScalingGroupReplicaIndex: strconv.Itoa(pcsgReplicaIndex), - apicommon.LabelPodTemplateHash: componentutils.ComputePCLQPodTemplateHash(pclqTemplateSpec, pcs.Spec.Template.PriorityClassName), + apicommon.LabelPodTemplateHash: podTemplateHash, } // Add base-podgang label for scaled PodGang pods (beyond minAvailable) diff --git a/operator/internal/controller/podcliquescalinggroup/components/podclique/podclique_test.go b/operator/internal/controller/podcliquescalinggroup/components/podclique/podclique_test.go index 2a01a738b..f4117e71d 100644 --- a/operator/internal/controller/podcliquescalinggroup/components/podclique/podclique_test.go +++ b/operator/internal/controller/podcliquescalinggroup/components/podclique/podclique_test.go @@ -1028,6 +1028,7 @@ func TestBuildResource_MNNVLInjection(t *testing.T) { }, }, } + selectedRevision := testutils.NewSelectedRevision("generation", map[string]string{pclqTemplateName: "selected-worker"}) // Create PCSG with MNNVL annotations and required label for pcsReplicaIndex pcsg := &grovecorev1alpha1.PodCliqueScalingGroup{ @@ -1064,8 +1065,9 @@ func TestBuildResource_MNNVLInjection(t *testing.T) { eventRecorder: &record.FakeRecorder{}, } - err := operator.buildResource(logr.Discard(), pcs, pcsg, pcsgReplicaIndex, pclq, false) + err := operator.buildResource(logr.Discard(), pcs, selectedRevision, pcsg, pcsgReplicaIndex, pclq, false) require.NoError(t, err) + assert.Equal(t, "selected-worker", pclq.Labels[apicommon.LabelPodTemplateHash]) // Verify pod-level claims if tc.expectPodLevelClaim { @@ -1144,7 +1146,8 @@ func TestBuildResource_StripsTopologyAnnotation(t *testing.T) { } operator := &_resource{scheme: groveclientscheme.Scheme} - err := operator.buildResource(logr.Discard(), pcs, pcsg, 0, pclq, false) + selectedRevision := testutils.NewSelectedRevision("generation", map[string]string{"worker": "selected-worker"}) + err := operator.buildResource(logr.Discard(), pcs, selectedRevision, pcsg, 0, pclq, false) require.NoError(t, err) require.NotNil(t, pclq.Annotations) assert.Equal(t, "yes", pclq.Annotations["example.com/keep"]) diff --git a/operator/internal/controller/podcliquescalinggroup/components/podclique/sync.go b/operator/internal/controller/podcliquescalinggroup/components/podclique/sync.go index 78d7d2d51..38aab28de 100644 --- a/operator/internal/controller/podcliquescalinggroup/components/podclique/sync.go +++ b/operator/internal/controller/podcliquescalinggroup/components/podclique/sync.go @@ -29,6 +29,7 @@ import ( grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" "github.com/ai-dynamo/grove/operator/internal/controller/common/component" componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" groveerr "github.com/ai-dynamo/grove/operator/internal/errors" "github.com/ai-dynamo/grove/operator/internal/resourceclaim" "github.com/ai-dynamo/grove/operator/internal/utils" @@ -44,6 +45,7 @@ type syncContext struct { pcs *grovecorev1alpha1.PodCliqueSet pcsg *grovecorev1alpha1.PodCliqueScalingGroup pcsgConfig *grovecorev1alpha1.PodCliqueScalingGroupConfig + revision *commonrevision.SelectedRevision pcsReplicaIndex int existingPCLQs []grovecorev1alpha1.PodClique existingPCLQNameSet componentutils.Set[string] @@ -72,6 +74,11 @@ func (r _resource) prepareSyncContext(ctx context.Context, logger logr.Logger, p fmt.Sprintf("failed to get owner PodCliqueSet for PodCliqueScalingGroup %s", client.ObjectKeyFromObject(pcsg)), ) } + selectedRevision, err := componentutils.GetSelectedPodCliqueSetRevision(ctx, r.client, syncCtx.pcs) + if err != nil { + return nil, err + } + syncCtx.revision = selectedRevision // Resolve PCS replica index and matching PCSG config for resource sharing syncCtx.pcsReplicaIndex, err = getPCSReplicaFromPCSG(pcsg) @@ -94,7 +101,10 @@ func (r _resource) prepareSyncContext(ctx context.Context, logger logr.Logger, p syncCtx.pcsgIndicesToTerminate, syncCtx.pcsgIndicesToRequeue = getMinAvailableBreachedPCSGIndices(logger, syncCtx.existingPCLQs, syncCtx.pcs.Spec.Template.TerminationDelay.Duration) // pre-compute expected PodTemplateHash for each PCLQ - syncCtx.expectedPCLQPodTemplateHashMap = getExpectedPCLQPodTemplateHashMap(syncCtx.pcs, pcsg) + syncCtx.expectedPCLQPodTemplateHashMap, err = getExpectedPCLQPodTemplateHashMap(syncCtx.revision, syncCtx.pcs, pcsg) + if err != nil { + return nil, err + } return syncCtx, nil } @@ -213,7 +223,7 @@ func (r _resource) createExpectedPCLQs(logger logr.Logger, sc *syncContext) erro createTask := utils.Task{ Name: fmt.Sprintf("CreatePodClique-%s", pclqObjectKey), Fn: func(ctx context.Context) error { - return r.doCreate(ctx, logger, sc.pcs, sc.pcsg, pcsgReplicaIndex, pclqObjectKey) + return r.doCreate(ctx, logger, sc.pcs, sc.revision, sc.pcsg, pcsgReplicaIndex, pclqObjectKey) }, } tasks = append(tasks, createTask) @@ -243,7 +253,7 @@ func (r _resource) createOrUpdatePCLQs(logger logr.Logger, sc *syncContext) erro createOrUpdateTask := utils.Task{ Name: fmt.Sprintf("CreateOrUpdatePodClique-%s", pclqObjectKey), Fn: func(ctx context.Context) error { - return r.doCreateOrUpdate(ctx, logger, sc.pcs, sc.pcsg, pcsgReplicaIndex, pclqObjectKey, pclqExists) + return r.doCreateOrUpdate(ctx, logger, sc.pcs, sc.revision, sc.pcsg, pcsgReplicaIndex, pclqObjectKey, pclqExists) }, } tasks = append(tasks, createOrUpdateTask) @@ -336,7 +346,7 @@ func (r _resource) getExistingPCLQs(ctx context.Context, pcsg *grovecorev1alpha1 } // getExpectedPCLQPodTemplateHashMap computes the expected pod template hash for each PodClique in the PCSG -func getExpectedPCLQPodTemplateHashMap(pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup) map[string]string { +func getExpectedPCLQPodTemplateHashMap(revision *commonrevision.SelectedRevision, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup) (map[string]string, error) { pclqFQNToHash := make(map[string]string) pcsgPCLQNames := pcsg.Spec.CliqueNames for _, pcsgCliqueName := range pcsgPCLQNames { @@ -344,7 +354,10 @@ func getExpectedPCLQPodTemplateHashMap(pcs *grovecorev1alpha1.PodCliqueSet, pcsg if pclqTemplateSpec == nil { continue } - podTemplateHash := componentutils.ComputePCLQPodTemplateHash(pclqTemplateSpec, pcs.Spec.Template.PriorityClassName) + podTemplateHash, err := revision.CliqueHash(pcsgCliqueName) + if err != nil { + return nil, err + } for pcsgReplicaIndex := range int(pcsg.Spec.Replicas) { cliqueFQN := apicommon.GeneratePodCliqueName(apicommon.ResourceNameReplica{ Name: pcsg.Name, @@ -353,7 +366,7 @@ func getExpectedPCLQPodTemplateHashMap(pcs *grovecorev1alpha1.PodCliqueSet, pcsg pclqFQNToHash[cliqueFQN] = podTemplateHash } } - return pclqFQNToHash + return pclqFQNToHash, nil } // refreshExistingPCLQs removes all the excess PCLQs that belong to any PCSG replica > expectedPCSGReplicas. diff --git a/operator/internal/controller/podcliquescalinggroup/reconcilestatus.go b/operator/internal/controller/podcliquescalinggroup/reconcilestatus.go index 5c10e40de..d4612dee4 100644 --- a/operator/internal/controller/podcliquescalinggroup/reconcilestatus.go +++ b/operator/internal/controller/podcliquescalinggroup/reconcilestatus.go @@ -28,6 +28,7 @@ import ( internalconstants "github.com/ai-dynamo/grove/operator/internal/constants" ctrlcommon "github.com/ai-dynamo/grove/operator/internal/controller/common" componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" ctrlutils "github.com/ai-dynamo/grove/operator/internal/controller/utils" k8sutils "github.com/ai-dynamo/grove/operator/internal/utils/kubernetes" @@ -61,6 +62,10 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, logger logr.Logger, pc logger.Error(err, "failed to get owner PodCliqueSet") return ctrlcommon.ReconcileWithErrors("failed to get owner PodCliqueSet", err) } + selectedRevision, err := componentutils.GetSelectedPodCliqueSetRevision(ctx, r.client, pcs) + if err != nil { + return ctrlcommon.ReconcileWithErrors("failed to get PodCliqueSet revision", err) + } pclqsPerPCSGReplica, err := r.getPodCliquesPerPCSGReplica(ctx, pcs.Name, client.ObjectKeyFromObject(pcsg)) if err != nil { @@ -74,7 +79,7 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, logger logr.Logger, pc // Replica-index strays (idx >= Spec.Replicas) are also dropped for hygiene, though // mutateReplicas already ignores them via its [0, Spec.Replicas) loop bounds. pclqsPerPCSGReplica = pruneStrayPCSGPCLQs(pcsg, pclqsPerPCSGReplica) - mutateReplicas(logger, pcs, pcsg, pclqsPerPCSGReplica) + mutateReplicas(logger, selectedRevision, pcs, pcsg, pclqsPerPCSGReplica) mutateMinAvailableBreachedCondition(logger, pcsg, pclqsPerPCSGReplica) r.emitAllScheduledReplicasLostIfNeeded(pcsg, originalStatus.ScheduledReplicas) @@ -83,7 +88,7 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, logger logr.Logger, pc return ctrlcommon.ReconcileWithErrors("failed to update selector for PodCliqueScalingGroup", err) } - mutateCurrentPodCliqueSetGenerationHash(logger, pcs, pcsg, lo.Flatten(lo.Values(pclqsPerPCSGReplica))) + mutateCurrentPodCliqueSetGenerationHash(logger, selectedRevision, pcs, pcsg, lo.Flatten(lo.Values(pclqsPerPCSGReplica))) // Skip the status patch when every mutate* above left status byte-identical to what the // previous reconcile already persisted. The mutators are the only code writing @@ -108,16 +113,16 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, logger logr.Logger, pc // It also derives child-PCLQ update progress counts when an update is in flight. The iteration is bounded to // expected replica indexes [0, Spec.Replicas) — the caller has already pruned stray children — so counters stay // consistent with the spec-derived totals during scale-down. -func mutateReplicas(logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pclqsPerPCSGReplica map[string][]grovecorev1alpha1.PodClique) { +func mutateReplicas(logger logr.Logger, revision *commonrevision.SelectedRevision, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pclqsPerPCSGReplica map[string][]grovecorev1alpha1.PodClique) { pcsg.Status.Replicas = pcsg.Spec.Replicas var scheduledReplicas, availableReplicas, updatedReplicas, updatedPCLQs, totalPCLQs int32 cliqueNamesPerReplica := int32(len(pcsg.Spec.CliqueNames)) - currentPCSGenerationHash := pcs.Status.CurrentGenerationHash - expectedPCLQPodTemplateHashes := componentutils.GetPCLQTemplateHashes(pcs, pcsg) + generationHash := revision.GenerationHash() + expectedPCLQPodTemplateHashes := componentutils.GetPCLQTemplateHashes(revision, pcs, pcsg) for replicaIndex := 0; replicaIndex < int(pcsg.Spec.Replicas); replicaIndex++ { pcsgReplicaIndex := strconv.Itoa(replicaIndex) pclqs := pclqsPerPCSGReplica[pcsgReplicaIndex] - isScheduled, isAvailable, isUpdated := computeReplicaStatus(logger, currentPCSGenerationHash, expectedPCLQPodTemplateHashes, pcsgReplicaIndex, len(pcsg.Spec.CliqueNames), pclqs) + isScheduled, isAvailable, isUpdated := computeReplicaStatus(logger, generationHash, expectedPCLQPodTemplateHashes, pcsgReplicaIndex, len(pcsg.Spec.CliqueNames), pclqs) if isScheduled { scheduledReplicas++ } @@ -127,7 +132,7 @@ func mutateReplicas(logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, pcs if isUpdated { updatedReplicas++ } - updatedPCLQs += countPCSGReplicaUpdatedPCLQs(currentPCSGenerationHash, expectedPCLQPodTemplateHashes, pclqs) + updatedPCLQs += countPCSGReplicaUpdatedPCLQs(generationHash, expectedPCLQPodTemplateHashes, pclqs) } totalPCLQs = pcsg.Spec.Replicas * cliqueNamesPerReplica logger.Info("Mutating PodCliqueScalingGroup replicas", @@ -145,17 +150,14 @@ func mutateReplicas(logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, pcs // countPCSGReplicaUpdatedPCLQs counts non-terminating PCLQs in a PCSG replica whose generation // hash matches the parent PCS hash. -func countPCSGReplicaUpdatedPCLQs(pcsGenerationHash *string, expectedPCLQPodTemplateHashes map[string]string, pclqs []grovecorev1alpha1.PodClique) int32 { - if pcsGenerationHash == nil { - return 0 - } +func countPCSGReplicaUpdatedPCLQs(generationHash string, expectedPCLQPodTemplateHashes map[string]string, pclqs []grovecorev1alpha1.PodClique) int32 { var n int32 for i := range pclqs { pclq := &pclqs[i] if k8sutils.IsResourceTerminating(pclq.ObjectMeta) { continue } - if isPCSGChildPCLQUpdated(pclq, expectedPCLQPodTemplateHashes, pcsGenerationHash) { + if isPCSGChildPCLQUpdated(pclq, expectedPCLQPodTemplateHashes, generationHash) { n++ } } @@ -163,7 +165,7 @@ func countPCSGReplicaUpdatedPCLQs(pcsGenerationHash *string, expectedPCLQPodTemp } // computeReplicaStatus processes a single PodCliqueScalingGroup replica and returns whether it is scheduled and available. -func computeReplicaStatus(logger logr.Logger, currentPCSGenerationHash *string, expectedPCLQPodTemplateHashes map[string]string, pcsgReplicaIndex string, numPCSGCliqueNames int, pclqs []grovecorev1alpha1.PodClique) (isScheduled, isAvailable, isUpdated bool) { +func computeReplicaStatus(logger logr.Logger, generationHash string, expectedPCLQPodTemplateHashes map[string]string, pcsgReplicaIndex string, numPCSGCliqueNames int, pclqs []grovecorev1alpha1.PodClique) (isScheduled, isAvailable, isUpdated bool) { nonTerminatedPCSGPodCliques := lo.Filter(pclqs, func(pclq grovecorev1alpha1.PodClique, _ int) bool { return !k8sutils.IsResourceTerminating(pclq.ObjectMeta) }) @@ -184,16 +186,15 @@ func computeReplicaStatus(logger logr.Logger, currentPCSGenerationHash *string, }) isAvailable = isAvailable && len(nonTerminatedPCSGPodCliques) == numPCSGCliqueNames isUpdated = isAvailable && - currentPCSGenerationHash != nil && lo.EveryBy(nonTerminatedPCSGPodCliques, func(pclq grovecorev1alpha1.PodClique) bool { - return isPCSGChildPCLQUpdated(&pclq, expectedPCLQPodTemplateHashes, currentPCSGenerationHash) + return isPCSGChildPCLQUpdated(&pclq, expectedPCLQPodTemplateHashes, generationHash) }) } return } -func isPCSGChildPCLQUpdated(pclq *grovecorev1alpha1.PodClique, expectedPCLQPodTemplateHashes map[string]string, pcsGenerationHash *string) bool { - if pcsGenerationHash == nil || pclq.Spec.MinAvailable == nil { +func isPCSGChildPCLQUpdated(pclq *grovecorev1alpha1.PodClique, expectedPCLQPodTemplateHashes map[string]string, generationHash string) bool { + if pclq.Spec.MinAvailable == nil { return false } expectedPodTemplateHash, ok := expectedPCLQPodTemplateHashes[pclq.Name] @@ -204,7 +205,7 @@ func isPCSGChildPCLQUpdated(pclq *grovecorev1alpha1.PodClique, expectedPCLQPodTe pclq.Status.CurrentPodTemplateHash != nil && *pclq.Status.CurrentPodTemplateHash == expectedPodTemplateHash && pclq.Status.CurrentPodCliqueSetGenerationHash != nil && - *pclq.Status.CurrentPodCliqueSetGenerationHash == *pcsGenerationHash && + *pclq.Status.CurrentPodCliqueSetGenerationHash == generationHash && pclq.Status.ReadyReplicas >= *pclq.Spec.MinAvailable && pclq.Status.UpdatedReplicas >= *pclq.Spec.MinAvailable } @@ -384,8 +385,8 @@ func mutateSelector(pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1 } // mutateCurrentPodCliqueSetGenerationHash updates the current generation hash when all PodCliques are updated and no rolling update is in progress -func mutateCurrentPodCliqueSetGenerationHash(logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, existingPCLQs []grovecorev1alpha1.PodClique) { - pclqFQNsPendingUpdate := componentutils.GetPCLQsInPCSGPendingUpdate(pcs, pcsg, existingPCLQs) +func mutateCurrentPodCliqueSetGenerationHash(logger logr.Logger, revision *commonrevision.SelectedRevision, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, existingPCLQs []grovecorev1alpha1.PodClique) { + pclqFQNsPendingUpdate := componentutils.GetPCLQsInPCSGPendingUpdate(revision, pcs, pcsg, existingPCLQs) if len(pclqFQNsPendingUpdate) > 0 { logger.Info("Found PodCliques associated to PodCliqueScalingGroup pending update", "pclqFQNsPendingUpdate", pclqFQNsPendingUpdate) return @@ -394,23 +395,17 @@ func mutateCurrentPodCliqueSetGenerationHash(logger logr.Logger, pcs *grovecorev logger.Info("PodCliqueScalingGroup is currently updating, cannot set PodCliqueSet CurrentGenerationHash yet") return } - if pcs.Status.CurrentGenerationHash == nil { + if !havePCSGPodCliquesConverged(revision, pcs, pcsg, existingPCLQs) { return } - if !havePCSGPodCliquesConverged(pcs, pcsg, existingPCLQs) { - return - } - pcsg.Status.CurrentPodCliqueSetGenerationHash = pcs.Status.CurrentGenerationHash + pcsg.Status.CurrentPodCliqueSetGenerationHash = ptr.To(revision.GenerationHash()) } // havePCSGPodCliquesConverged reports whether every expected PodClique in the // PodCliqueScalingGroup has reconciled its template and generation hashes to the // current PodCliqueSet spec. -func havePCSGPodCliquesConverged(pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, existingPCLQs []grovecorev1alpha1.PodClique) bool { - if pcs.Status.CurrentGenerationHash == nil { - return false - } - expectedPCLQPodTemplateHashes := componentutils.GetPCLQTemplateHashes(pcs, pcsg) +func havePCSGPodCliquesConverged(revision *commonrevision.SelectedRevision, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, existingPCLQs []grovecorev1alpha1.PodClique) bool { + expectedPCLQPodTemplateHashes := componentutils.GetPCLQTemplateHashes(revision, pcs, pcsg) if len(expectedPCLQPodTemplateHashes) != int(pcsg.Spec.Replicas)*len(pcsg.Spec.CliqueNames) { return false } @@ -428,7 +423,7 @@ func havePCSGPodCliquesConverged(pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grov if pclq.Status.CurrentPodTemplateHash == nil || *pclq.Status.CurrentPodTemplateHash != expectedPodTemplateHash { return false } - if pclq.Status.CurrentPodCliqueSetGenerationHash == nil || *pclq.Status.CurrentPodCliqueSetGenerationHash != *pcs.Status.CurrentGenerationHash { + if pclq.Status.CurrentPodCliqueSetGenerationHash == nil || *pclq.Status.CurrentPodCliqueSetGenerationHash != revision.GenerationHash() { return false } } diff --git a/operator/internal/controller/podcliquescalinggroup/reconcilestatus_test.go b/operator/internal/controller/podcliquescalinggroup/reconcilestatus_test.go index 9bdc8bb56..b0d295380 100644 --- a/operator/internal/controller/podcliquescalinggroup/reconcilestatus_test.go +++ b/operator/internal/controller/podcliquescalinggroup/reconcilestatus_test.go @@ -41,14 +41,13 @@ import ( func TestComputeReplicaStatus(t *testing.T) { tests := []struct { - name string - expectedSize int - cliques []grovecorev1alpha1.PodClique - minAvailable int32 - pcsGenerationHash *string - wantScheduled bool - wantAvailable bool - wantUpdated bool + name string + expectedSize int + cliques []grovecorev1alpha1.PodClique + minAvailable int32 + wantScheduled bool + wantAvailable bool + wantUpdated bool }{ { name: "healthy vs failed states", @@ -98,7 +97,7 @@ func TestComputeReplicaStatus(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - scheduled, available, updated := computeReplicaStatus(logr.Discard(), tt.pcsGenerationHash, nil, "0", tt.expectedSize, tt.cliques) + scheduled, available, updated := computeReplicaStatus(logr.Discard(), "generation", nil, "0", tt.expectedSize, tt.cliques) assert.Equal(t, tt.wantScheduled, scheduled, "scheduled mismatch") assert.Equal(t, tt.wantAvailable, available, "available mismatch") @@ -544,7 +543,8 @@ func TestReconcileStatus(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { pcsg, pcs, cliques := tt.setup() - allObjects := append([]client.Object{pcsg, pcs}, cliques...) + revision := testutils.NewPodCliqueSetControllerRevision(pcs, testutils.ComputePodCliqueTemplateHashes(pcs)) + allObjects := append([]client.Object{pcsg, pcs, revision}, cliques...) fakeClient := testutils.SetupFakeClient(allObjects...) reconciler := &Reconciler{client: fakeClient} @@ -587,7 +587,8 @@ func TestReconcileStatus_EdgeCases(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { pcs := testutils.NewPodCliqueSetBuilder("test-pcs", "test-ns", uuid.NewUUID()).Build() - fakeClient := testutils.SetupFakeClient(tt.pcsg, pcs) + revision := testutils.NewPodCliqueSetControllerRevision(pcs, testutils.ComputePodCliqueTemplateHashes(pcs)) + fakeClient := testutils.SetupFakeClient(tt.pcsg, pcs, revision) reconciler := &Reconciler{client: fakeClient} result := reconciler.reconcileStatus(ctx, logr.Discard(), client.ObjectKeyFromObject(tt.pcsg)) @@ -726,9 +727,10 @@ func TestPCSGMutateReplicasWritesUpdateProgressCounts(t *testing.T) { }, } + selectedRevision := testutils.NewSelectedRevision(*pcs.Status.CurrentGenerationHash, testutils.ComputePodCliqueTemplateHashes(pcs)) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mutateReplicas(logr.Discard(), pcs, tt.pcsg, tt.pclqsPerReplica(t, tt.pcsg)) + mutateReplicas(logr.Discard(), selectedRevision, pcs, tt.pcsg, tt.pclqsPerReplica(t, tt.pcsg)) if !tt.wantWritten { require.Nil(t, tt.pcsg.Status.UpdateProgress, "UpdateProgress must remain nil") @@ -751,7 +753,8 @@ func TestCountPCSGReplicaUpdatedPCLQs(t *testing.T) { pcsg := testutils.NewPodCliqueScalingGroupBuilder("test-pcsg", "test-ns", "test-pcs", 0). WithReplicas(1). WithCliqueNames([]string{"frontend"}).Build() - expectedHashes := componentutils.GetPCLQTemplateHashes(pcs, pcsg) + selectedRevision := testutils.NewSelectedRevision(hash, testutils.ComputePodCliqueTemplateHashes(pcs)) + expectedHashes := componentutils.GetPCLQTemplateHashes(selectedRevision, pcs, pcsg) mk := func(currHash *string, terminating bool) grovecorev1alpha1.PodClique { p := testutils.NewPCSGPodCliqueBuilder("test-pcsg-0-frontend", "test-ns", "test-pcs", "test-pcsg", 0, 0).Build() if currHash != nil { @@ -767,15 +770,14 @@ func TestCountPCSGReplicaUpdatedPCLQs(t *testing.T) { tests := []struct { name string - hash *string + hash string in []grovecorev1alpha1.PodClique want int32 }{ - {"nil parent hash → 0", nil, []grovecorev1alpha1.PodClique{mk(&hash, false)}, 0}, - {"empty input → 0", &hash, nil, 0}, - {"all matching", &hash, []grovecorev1alpha1.PodClique{mk(&hash, false), mk(&hash, false)}, 2}, - {"mixed", &hash, []grovecorev1alpha1.PodClique{mk(&hash, false), mk(&otherHash, false), mk(nil, false)}, 1}, - {"terminating matching excluded", &hash, []grovecorev1alpha1.PodClique{mk(&hash, false), mk(&hash, true)}, 1}, + {"empty input → 0", hash, nil, 0}, + {"all matching", hash, []grovecorev1alpha1.PodClique{mk(&hash, false), mk(&hash, false)}, 2}, + {"mixed", hash, []grovecorev1alpha1.PodClique{mk(&hash, false), mk(&otherHash, false), mk(nil, false)}, 1}, + {"terminating matching excluded", hash, []grovecorev1alpha1.PodClique{mk(&hash, false), mk(&hash, true)}, 1}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -793,6 +795,7 @@ func TestHavePCSGPodCliquesConverged(t *testing.T) { pcsg := testutils.NewPodCliqueScalingGroupBuilder("test-pcsg", "test-ns", "test-pcs", 0). WithReplicas(1). WithCliqueNames([]string{"frontend"}).Build() + selectedRevision := testutils.NewSelectedRevision(hash, testutils.ComputePodCliqueTemplateHashes(pcs)) base := markPCSGPCLQConverged(t, pcs, pcsg, testutils.NewPCSGPodCliqueBuilder("test-pcsg-0-frontend", "test-ns", "test-pcs", "test-pcsg", 0, 0).Build(), hash) @@ -855,7 +858,7 @@ func TestHavePCSGPodCliquesConverged(t *testing.T) { if tt.mutate != nil && len(pclqs) > 0 { tt.mutate(&pclqs[0]) } - assert.Equal(t, tt.want, havePCSGPodCliquesConverged(pcs, pcsg, pclqs)) + assert.Equal(t, tt.want, havePCSGPodCliquesConverged(selectedRevision, pcs, pcsg, pclqs)) }) } } @@ -934,7 +937,7 @@ func TestReconcileStatusBoundedDuringScaleDown(t *testing.T) { } objs := []client.Object{ - pcsg, pcs, + pcsg, pcs, testutils.NewPodCliqueSetControllerRevision(pcs, testutils.ComputePodCliqueTemplateHashes(pcs)), mkChild("test-pcsg-0-frontend", 0), mkChild("test-pcsg-0-backend", 0), mkChild("test-pcsg-1-frontend", 1), @@ -970,7 +973,8 @@ func pcsgChildName(pcsgName string, replicaIndex int, cliqueName string) string func markPCSGPCLQConverged(t testing.TB, pcs *grovecorev1alpha1.PodCliqueSet, pcsg *grovecorev1alpha1.PodCliqueScalingGroup, pclq *grovecorev1alpha1.PodClique, generationHash string) *grovecorev1alpha1.PodClique { t.Helper() - expectedHashes := componentutils.GetPCLQTemplateHashes(pcs, pcsg) + selectedRevision := testutils.NewSelectedRevision(generationHash, testutils.ComputePodCliqueTemplateHashes(pcs)) + expectedHashes := componentutils.GetPCLQTemplateHashes(selectedRevision, pcs, pcsg) expectedTemplateHash, ok := expectedHashes[pclq.Name] require.True(t, ok, "expected template hash for %s", pclq.Name) if pclq.Labels == nil { diff --git a/operator/internal/controller/podcliqueset/components/podclique/podclique.go b/operator/internal/controller/podcliqueset/components/podclique/podclique.go index a7348958d..e572b278c 100644 --- a/operator/internal/controller/podcliqueset/components/podclique/podclique.go +++ b/operator/internal/controller/podcliqueset/components/podclique/podclique.go @@ -29,6 +29,7 @@ import ( "github.com/ai-dynamo/grove/operator/internal/constants" "github.com/ai-dynamo/grove/operator/internal/controller/common/component" componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" groveerr "github.com/ai-dynamo/grove/operator/internal/errors" "github.com/ai-dynamo/grove/operator/internal/mnnvl" "github.com/ai-dynamo/grove/operator/internal/utils" @@ -87,6 +88,10 @@ func (r _resource) GetExistingResourceNames(ctx context.Context, logger logr.Log // Sync synchronizes all resources that the PodClique Operator manages. func (r _resource) Sync(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet) error { + selectedRevision, err := componentutils.GetSelectedPodCliqueSetRevision(ctx, r.client, pcs) + if err != nil { + return err + } existingPCLQFQNs, err := r.GetExistingResourceNames(ctx, logger, pcs.ObjectMeta) if err != nil { return groveerr.WrapError(err, @@ -99,7 +104,7 @@ func (r _resource) Sync(ctx context.Context, logger logr.Logger, pcs *grovecorev if err := r.triggerDeletionOfExcessPCLQs(ctx, logger, pcs, existingPCLQFQNs); err != nil { return err } - if err := r.createOrUpdatePCLQs(ctx, logger, pcs, existingPCLQFQNs); err != nil { + if err := r.createOrUpdatePCLQs(ctx, logger, pcs, selectedRevision, existingPCLQFQNs); err != nil { return err } @@ -126,7 +131,7 @@ func (r _resource) triggerDeletionOfExcessPCLQs(ctx context.Context, logger logr } // createOrUpdatePCLQs creates or updates all expected PodCliques for the PodCliqueSet. -func (r _resource) createOrUpdatePCLQs(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, existingPCLQFQNs []string) error { +func (r _resource) createOrUpdatePCLQs(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, revision *commonrevision.SelectedRevision, existingPCLQFQNs []string) error { expectedPCLQNames, _ := componentutils.GetExpectedPCLQNamesGroupByOwner(pcs) tasks := make([]utils.Task, 0, len(expectedPCLQNames)) existingPCLQNameSet := componentutils.NewSet(existingPCLQFQNs) @@ -141,7 +146,7 @@ func (r _resource) createOrUpdatePCLQs(ctx context.Context, logger logr.Logger, createOrUpdateTask := utils.Task{ Name: fmt.Sprintf("CreateOrUpdatePodClique-%s", pclqObjectKey), Fn: func(ctx context.Context) error { - return r.doCreateOrUpdate(ctx, logger, pcs, pcsReplica, pclqObjectKey, pclqExists) + return r.doCreateOrUpdate(ctx, logger, pcs, revision, pcsReplica, pclqObjectKey, pclqExists) }, } tasks = append(tasks, createOrUpdateTask) @@ -260,13 +265,13 @@ func (r _resource) Delete(ctx context.Context, logger logr.Logger, pcsObjectMeta } // doCreateOrUpdate creates or updates a single PodClique resource. -func (r _resource) doCreateOrUpdate(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, pcsReplica int32, pclqObjectKey client.ObjectKey, pclqExists bool) error { +func (r _resource) doCreateOrUpdate(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, revision *commonrevision.SelectedRevision, pcsReplica int32, pclqObjectKey client.ObjectKey, pclqExists bool) error { logger.Info("Running CreateOrUpdate PodClique", "pclqObjectKey", pclqObjectKey) pclq := emptyPodClique(pclqObjectKey) pcsObjKey := client.ObjectKeyFromObject(pcs) opResult, err := controllerutil.CreateOrPatch(ctx, r.client, pclq, func() error { - return r.buildResource(logger, pclq, pcs, int(pcsReplica), pclqExists) + return r.buildResource(logger, pclq, pcs, revision, int(pcsReplica), pclqExists) }) if err != nil { r.eventRecorder.Eventf(pcs, corev1.EventTypeWarning, constants.ReasonPodCliqueCreateOrUpdateFailed, "PodClique %v creation or updation failed: %v", pclqObjectKey, err) @@ -283,7 +288,7 @@ func (r _resource) doCreateOrUpdate(ctx context.Context, logger logr.Logger, pcs } // buildResource configures a PodClique with the desired state from the template. -func (r _resource) buildResource(logger logr.Logger, pclq *grovecorev1alpha1.PodClique, pcs *grovecorev1alpha1.PodCliqueSet, pcsReplica int, pclqExists bool) error { +func (r _resource) buildResource(logger logr.Logger, pclq *grovecorev1alpha1.PodClique, pcs *grovecorev1alpha1.PodCliqueSet, revision *commonrevision.SelectedRevision, pcsReplica int, pclqExists bool) error { var err error pclqObjectKey, pcsObjectKey := client.ObjectKeyFromObject(pclq), client.ObjectKeyFromObject(pcs) pclqTemplateSpec, foundAtIndex, ok := lo.FindIndexOf(pcs.Spec.Template.Cliques, func(pclqTemplateSpec *grovecorev1alpha1.PodCliqueTemplateSpec) bool { @@ -307,7 +312,11 @@ func (r _resource) buildResource(logger logr.Logger, pclq *grovecorev1alpha1.Pod } // Add finalizer at creation so PCLQ controller does not need a separate PATCH on first reconcile. controllerutil.AddFinalizer(pclq, apiconstants.FinalizerPodClique) - pclq.Labels = getLabels(pcs, pcsReplica, pclqObjectKey, pclqTemplateSpec, apicommon.GeneratePodGangNameForPodCliqueOwnedByPodCliqueSet(pcs, pcsReplica)) + podTemplateHash, err := revision.CliqueHash(pclqTemplateSpec.Name) + if err != nil { + return err + } + pclq.Labels = getLabels(pcs, pcsReplica, pclqObjectKey, pclqTemplateSpec, apicommon.GeneratePodGangNameForPodCliqueOwnedByPodCliqueSet(pcs, pcsReplica), podTemplateHash) pclq.Annotations = maps.Clone(pclqTemplateSpec.Annotations) // PodGang owns topology selection; do not propagate a template topology annotation to PodClique pods. delete(pclq.Annotations, apiconstants.AnnotationTopologyName) @@ -386,13 +395,13 @@ func getPodCliqueSelectorLabels(pcsObjectMeta metav1.ObjectMeta) map[string]stri } // getLabels constructs labels for a PodClique resource including pod template hash. -func getLabels(pcs *grovecorev1alpha1.PodCliqueSet, pcsReplica int, pclqObjectKey client.ObjectKey, pclqTemplateSpec *grovecorev1alpha1.PodCliqueTemplateSpec, podGangName string) map[string]string { +func getLabels(pcs *grovecorev1alpha1.PodCliqueSet, pcsReplica int, pclqObjectKey client.ObjectKey, pclqTemplateSpec *grovecorev1alpha1.PodCliqueTemplateSpec, podGangName, podTemplateHash string) map[string]string { pclqComponentLabels := map[string]string{ apicommon.LabelAppNameKey: pclqObjectKey.Name, apicommon.LabelComponentKey: apicommon.LabelComponentNamePodCliqueSetPodClique, apicommon.LabelPodCliqueSetReplicaIndex: strconv.Itoa(pcsReplica), apicommon.LabelPodGang: podGangName, - apicommon.LabelPodTemplateHash: componentutils.ComputePCLQPodTemplateHash(pclqTemplateSpec, pcs.Spec.Template.PriorityClassName), + apicommon.LabelPodTemplateHash: podTemplateHash, } return lo.Assign( pclqTemplateSpec.Labels, diff --git a/operator/internal/controller/podcliqueset/components/podclique/podclique_test.go b/operator/internal/controller/podcliqueset/components/podclique/podclique_test.go index a706cf5d7..f1e89a74f 100644 --- a/operator/internal/controller/podcliqueset/components/podclique/podclique_test.go +++ b/operator/internal/controller/podcliqueset/components/podclique/podclique_test.go @@ -21,6 +21,7 @@ import ( "fmt" "testing" + apicommon "github.com/ai-dynamo/grove/operator/api/common" apiconstants "github.com/ai-dynamo/grove/operator/api/common/constants" grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" groveclientscheme "github.com/ai-dynamo/grove/operator/internal/client" @@ -441,6 +442,7 @@ func TestBuildResource_MNNVLInjection(t *testing.T) { pcsBuilder.WithPodCliqueTemplateSpec(pclqTemplateSpec) pcs := pcsBuilder.Build() + selectedRevision := testutils.NewSelectedRevision("generation", map[string]string{pclqTemplateName: "selected-worker"}) // Create empty PodClique with matching name suffix pclqName := fmt.Sprintf("%s-%d-%s", testPCSName, pcsReplica, pclqTemplateName) @@ -458,8 +460,9 @@ func TestBuildResource_MNNVLInjection(t *testing.T) { eventRecorder: record.NewFakeRecorder(10), } - err := operator.buildResource(logr.Discard(), pclq, pcs, pcsReplica, false) + err := operator.buildResource(logr.Discard(), pclq, pcs, selectedRevision, pcsReplica, false) require.NoError(t, err) + assert.Equal(t, "selected-worker", pclq.Labels[apicommon.LabelPodTemplateHash]) // Verify pod-level claims if tc.expectPodLevelClaim { @@ -512,7 +515,8 @@ func TestBuildResource_StripsTopologyAnnotation(t *testing.T) { } operator := &_resource{scheme: groveclientscheme.Scheme} - err := operator.buildResource(logr.Discard(), pclq, pcs, 0, false) + selectedRevision := testutils.NewSelectedRevision("generation", testutils.ComputePodCliqueTemplateHashes(pcs)) + err := operator.buildResource(logr.Discard(), pclq, pcs, selectedRevision, 0, false) require.NoError(t, err) require.NotNil(t, pclq.Annotations) assert.Equal(t, "yes", pclq.Annotations["example.com/keep"]) diff --git a/operator/internal/controller/podcliqueset/components/podcliquesetreplica/rollingupdate.go b/operator/internal/controller/podcliqueset/components/podcliquesetreplica/rollingupdate.go index 573365065..fd1b03494 100644 --- a/operator/internal/controller/podcliqueset/components/podcliquesetreplica/rollingupdate.go +++ b/operator/internal/controller/podcliqueset/components/podcliquesetreplica/rollingupdate.go @@ -27,6 +27,7 @@ import ( grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" "github.com/ai-dynamo/grove/operator/internal/controller/common/component" componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" groveerr "github.com/ai-dynamo/grove/operator/internal/errors" "github.com/go-logr/logr" @@ -57,22 +58,25 @@ func (r _resource) orchestrateRollingUpdate(ctx context.Context, logger logr.Log // pick the next replica index to update. nextReplicaToUpdate := updateWork.getNextReplicaToUpdate(pcs, minAvailableBreachedPCSReplicaIndices) - if err = r.updatePCSWithNextSelectedReplica(ctx, logger, pcs, nextReplicaToUpdate); err != nil { - return err + if nextReplicaToUpdate == nil { + return nil } - - if nextReplicaToUpdate != nil { - return groveerr.New( - groveerr.ErrCodeContinueReconcileAndRequeue, - component.OperationSync, - fmt.Sprintf("commencing rolling update of PodCliqueSet replica index %d", *nextReplicaToUpdate), - ) + if err = r.startPCSReplicaUpdate(ctx, logger, pcs, *nextReplicaToUpdate); err != nil { + return err } - return nil + return groveerr.New( + groveerr.ErrCodeContinueReconcileAndRequeue, + component.OperationSync, + fmt.Sprintf("commencing rolling update of PodCliqueSet replica index %d", *nextReplicaToUpdate), + ) } // computePendingUpdateWork identifies replicas that need updating and tracks current update progress. func (r _resource) computePendingUpdateWork(ctx context.Context, pcs *grovecorev1alpha1.PodCliqueSet, pcsIndicesToTerminate []int) (*pendingUpdateWork, error) { + selectedRevision, err := componentutils.GetSelectedPodCliqueSetRevision(ctx, r.client, pcs) + if err != nil { + return nil, err + } replicaInfos, err := r.getPCSReplicaInfos(ctx, pcs, pcsIndicesToTerminate) if err != nil { return nil, err @@ -80,7 +84,7 @@ func (r _resource) computePendingUpdateWork(ctx context.Context, pcs *grovecorev // iterate through each replica pendingWork := &pendingUpdateWork{} for _, replicaInfo := range replicaInfos { - replicaInfo.computeUpdateProgress(pcs) + replicaInfo.computeUpdateProgress(selectedRevision, pcs) if len(pcs.Status.UpdateProgress.CurrentlyUpdating) > 0 && pcs.Status.UpdateProgress.CurrentlyUpdating[0].ReplicaIndex == int32(replicaInfo.replicaIndex) { @@ -146,22 +150,16 @@ func (r _resource) updatePCSWithReplicaUpdateProgress(ctx context.Context, logge return nil } -// updatePCSWithNextSelectedReplica initiates an update for the next replica or marks completion. -func (r _resource) updatePCSWithNextSelectedReplica(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, nextPCSReplicaToUpdate *int) error { +// startPCSReplicaUpdate records the next replica selected for rolling update. +// Aggregate rollout completion is derived and persisted by the parent status reconciler. +func (r _resource) startPCSReplicaUpdate(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet, nextPCSReplicaToUpdate int) error { original := pcs.DeepCopy() - - if nextPCSReplicaToUpdate == nil { - logger.Info("Rolling update has completed") - pcs.Status.UpdateProgress.UpdateEndedAt = ptr.To(metav1.Now()) - pcs.Status.UpdateProgress.CurrentlyUpdating = nil - } else { - logger.Info("Initiating rolling update for next replica index", "nextReplicaIndex", *nextPCSReplicaToUpdate) - pcs.Status.UpdateProgress.CurrentlyUpdating = []grovecorev1alpha1.PodCliqueSetReplicaUpdateProgress{ - { - ReplicaIndex: int32(*nextPCSReplicaToUpdate), - UpdateStartedAt: metav1.Now(), - }, - } + logger.Info("Initiating rolling update for next replica index", "nextReplicaIndex", nextPCSReplicaToUpdate) + pcs.Status.UpdateProgress.CurrentlyUpdating = []grovecorev1alpha1.PodCliqueSetReplicaUpdateProgress{ + { + ReplicaIndex: int32(nextPCSReplicaToUpdate), + UpdateStartedAt: metav1.Now(), + }, } return r.patchUpdateProgressStatus(ctx, logger, pcs, original) } @@ -236,20 +234,17 @@ func (w *pendingUpdateWork) getNextReplicaToUpdate(pcs *grovecorev1alpha1.PodCli } // computeUpdateProgress calculates update completion for a PCS replica. -func (pri *pcsReplicaInfo) computeUpdateProgress(pcs *grovecorev1alpha1.PodCliqueSet) { +func (pri *pcsReplicaInfo) computeUpdateProgress(revision *commonrevision.SelectedRevision, pcs *grovecorev1alpha1.PodCliqueSet) { updatedPCLQs := 0 for _, pclq := range pri.pclqs { - if isPCLQUpdateComplete(pcs, &pclq) { + if isPCLQUpdateComplete(revision, &pclq) { updatedPCLQs++ } } updatedPCSGs := 0 - if pcs.Status.CurrentGenerationHash != nil { - currentHash := *pcs.Status.CurrentGenerationHash - for _, pcsg := range pri.pcsgs { - if componentutils.IsPCSGUpdateComplete(&pcsg, currentHash) { - updatedPCSGs++ - } + for _, pcsg := range pri.pcsgs { + if componentutils.IsPCSGUpdateComplete(&pcsg, revision.GenerationHash()) { + updatedPCSGs++ } } pri.updateProgress = replicaUpdateProgress{ @@ -275,11 +270,11 @@ func (pri *pcsReplicaInfo) getNumScheduledPods(pcs *grovecorev1alpha1.PodCliqueS } // isPCLQUpdateComplete checks if a PodClique has completed its update to the target generation and template. -func isPCLQUpdateComplete(pcs *grovecorev1alpha1.PodCliqueSet, pclq *grovecorev1alpha1.PodClique) bool { - if pcs.Status.CurrentGenerationHash == nil || pclq.Spec.MinAvailable == nil { +func isPCLQUpdateComplete(revision *commonrevision.SelectedRevision, pclq *grovecorev1alpha1.PodClique) bool { + if pclq.Spec.MinAvailable == nil { return false } - expectedPodTemplateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(pcs, pclq.ObjectMeta) + expectedPodTemplateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(revision, pclq.ObjectMeta) if err != nil || expectedPodTemplateHash == "" { return false } @@ -287,7 +282,7 @@ func isPCLQUpdateComplete(pcs *grovecorev1alpha1.PodCliqueSet, pclq *grovecorev1 pclq.Status.CurrentPodTemplateHash != nil && *pclq.Status.CurrentPodTemplateHash == expectedPodTemplateHash && pclq.Status.CurrentPodCliqueSetGenerationHash != nil && - *pclq.Status.CurrentPodCliqueSetGenerationHash == *pcs.Status.CurrentGenerationHash && + *pclq.Status.CurrentPodCliqueSetGenerationHash == revision.GenerationHash() && pclq.Status.UpdatedReplicas >= *pclq.Spec.MinAvailable && pclq.Status.ReadyReplicas >= *pclq.Spec.MinAvailable } diff --git a/operator/internal/controller/podcliqueset/components/podcliquesetreplica/rollingupdate_test.go b/operator/internal/controller/podcliqueset/components/podcliquesetreplica/rollingupdate_test.go new file mode 100644 index 000000000..d97061061 --- /dev/null +++ b/operator/internal/controller/podcliqueset/components/podcliquesetreplica/rollingupdate_test.go @@ -0,0 +1,46 @@ +// /* +// Copyright 2026 The Grove Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// */ + +package podcliquesetreplica + +import ( + "context" + "testing" + + grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" + testutils "github.com/ai-dynamo/grove/operator/test/utils" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestStartPCSReplicaUpdate(t *testing.T) { + pcs := &grovecorev1alpha1.PodCliqueSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, + Status: grovecorev1alpha1.PodCliqueSetStatus{ + UpdateProgress: &grovecorev1alpha1.PodCliqueSetUpdateProgress{UpdateStartedAt: metav1.Now()}, + }, + } + fakeClient := testutils.SetupFakeClient(pcs) + r := _resource{client: fakeClient} + + require.NoError(t, r.startPCSReplicaUpdate(context.Background(), logr.Discard(), pcs, 2)) + require.Len(t, pcs.Status.UpdateProgress.CurrentlyUpdating, 1) + assert.Equal(t, int32(2), pcs.Status.UpdateProgress.CurrentlyUpdating[0].ReplicaIndex) + assert.Nil(t, pcs.Status.UpdateProgress.UpdateEndedAt) +} diff --git a/operator/internal/controller/podcliqueset/reconciler.go b/operator/internal/controller/podcliqueset/reconciler.go index 39bac1ef3..20e8c182d 100644 --- a/operator/internal/controller/podcliqueset/reconciler.go +++ b/operator/internal/controller/podcliqueset/reconciler.go @@ -30,6 +30,7 @@ import ( "github.com/ai-dynamo/grove/operator/internal/scheduler" "github.com/go-logr/logr" + "k8s.io/client-go/tools/cache" ctrl "sigs.k8s.io/controller-runtime" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -38,12 +39,12 @@ import ( // Reconciler reconciles PodCliqueSet resources. type Reconciler struct { - config configv1alpha1.PodCliqueSetControllerConfiguration - tasConfig configv1alpha1.TopologyAwareSchedulingConfiguration - client ctrlclient.Client - reconcileStatusRecorder ctrlcommon.ReconcileErrorRecorder - operatorRegistry component.OperatorRegistry[grovecorev1alpha1.PodCliqueSet] - pcsGenerationHashExpectations sync.Map + config configv1alpha1.PodCliqueSetControllerConfiguration + tasConfig configv1alpha1.TopologyAwareSchedulingConfiguration + client ctrlclient.Client + reconcileStatusRecorder ctrlcommon.ReconcileErrorRecorder + operatorRegistry component.OperatorRegistry[grovecorev1alpha1.PodCliqueSet] + pcsRevisionExpectations sync.Map } // NewReconciler creates a new reconciler for PodCliqueSet. @@ -51,12 +52,12 @@ func NewReconciler(mgr ctrl.Manager, controllerCfg configv1alpha1.PodCliqueSetCo eventRecorder := mgr.GetEventRecorderFor(controllerName) client := mgr.GetClient() return &Reconciler{ - config: controllerCfg, - tasConfig: topologyAwareSchedulingConfig, - client: client, - reconcileStatusRecorder: ctrlcommon.NewReconcileErrorRecorder(client), - operatorRegistry: pcscomponent.CreateOperatorRegistry(mgr, eventRecorder, topologyAwareSchedulingConfig, networkConfig, schedRegistry), - pcsGenerationHashExpectations: sync.Map{}, + config: controllerCfg, + tasConfig: topologyAwareSchedulingConfig, + client: client, + reconcileStatusRecorder: ctrlcommon.NewReconcileErrorRecorder(client), + operatorRegistry: pcscomponent.CreateOperatorRegistry(mgr, eventRecorder, topologyAwareSchedulingConfig, networkConfig, schedRegistry), + pcsRevisionExpectations: sync.Map{}, } } @@ -66,6 +67,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu pcs := &grovecorev1alpha1.PodCliqueSet{} if result := ctrlutils.GetPodCliqueSet(ctx, r.client, logger, req.NamespacedName, pcs); ctrlcommon.ShortCircuitReconcileFlow(result) { + if !result.HasErrors() { + pcsObjectName := cache.NamespacedNameAsObjectName(req.NamespacedName).String() + r.pcsRevisionExpectations.Delete(pcsObjectName) + } return result.Result() } @@ -74,16 +79,26 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } reconcileSpecFlowResult := r.reconcileSpec(ctx, logger, pcs) + if ctrlcommon.ShortCircuitReconcileFlow(reconcileSpecFlowResult) { + return reconcileSpecFlowResult.Result() + } + if statusReconcileResult := r.reconcileStatus(ctx, logger, pcs); ctrlcommon.ShortCircuitReconcileFlow(statusReconcileResult) { return statusReconcileResult.Result() } + if historyReconcileResult := r.truncateRevisionHistory(ctx, logger, pcs); ctrlcommon.ShortCircuitReconcileFlow(historyReconcileResult) { + return historyReconcileResult.Result() + } + return reconcileSpecFlowResult.Result() } // reconcileDelete handles PodCliqueSet deletion when a deletion timestamp is set. func (r *Reconciler) reconcileDelete(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet) ctrlcommon.ReconcileStepResult { if !pcs.DeletionTimestamp.IsZero() { + pcsObjectName := cache.NamespacedNameAsObjectName(ctrlclient.ObjectKeyFromObject(pcs)).String() + r.pcsRevisionExpectations.Delete(pcsObjectName) if !controllerutil.ContainsFinalizer(pcs, constants.FinalizerPodCliqueSet) { return ctrlcommon.DoNotRequeue() } diff --git a/operator/internal/controller/podcliqueset/reconcilespec.go b/operator/internal/controller/podcliqueset/reconcilespec.go index a0de15b29..3d099d33a 100644 --- a/operator/internal/controller/podcliqueset/reconcilespec.go +++ b/operator/internal/controller/podcliqueset/reconcilespec.go @@ -34,8 +34,6 @@ import ( "github.com/samber/lo" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/tools/cache" - "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) @@ -45,7 +43,7 @@ func (r *Reconciler) reconcileSpec(ctx context.Context, logger logr.Logger, pcs rLog := logger.WithValues("operation", "spec-reconcile") reconcileStepFns := []ctrlcommon.ReconcileStepFn[grovecorev1alpha1.PodCliqueSet]{ r.ensureFinalizer, - r.processGenerationHashChange, + r.processRevision, r.syncPodCliqueSetResources, r.updateObservedGeneration, } @@ -70,47 +68,13 @@ func (r *Reconciler) ensureFinalizer(ctx context.Context, logger logr.Logger, pc return ctrlcommon.ContinueReconcile() } -// processGenerationHashChange computes the generation hash given a PodCliqueSet resource and if the generation has -// changed from the previously persisted pcs.status.generationHash then it resets the pcs.status.updateProgress -func (r *Reconciler) processGenerationHashChange(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet) ctrlcommon.ReconcileStepResult { - pcsObjectKey := client.ObjectKeyFromObject(pcs) - pcsObjectName := cache.NamespacedNameAsObjectName(pcsObjectKey).String() - - // if the generationHash is not reflected correctly yet, requeue. Allow the informer cache to catch-up. - if !r.isGenerationHashExpectationSatisfied(pcsObjectName, pcs.Status.CurrentGenerationHash) { - return ctrlcommon.ReconcileAfter(constants.ComponentSyncRetryInterval, fmt.Sprintf("CurrentGenerationHash is not up-to-date for PodCliqueSet: %v", pcsObjectKey)) - } - r.pcsGenerationHashExpectations.Delete(pcsObjectName) - - newGenerationHash := computeGenerationHash(pcs) - if pcs.Status.CurrentGenerationHash == nil { - // update the generation hash and continue reconciliation. No rolling update is required. - if err := r.setGenerationHashAndUpdateStatus(ctx, pcs, pcsObjectName, newGenerationHash); err != nil { - logger.Error(err, "failed to set generation hash on PCS", "newGenerationHash", newGenerationHash) - return ctrlcommon.ReconcileWithErrors("error updating generation hash", err) - } - return ctrlcommon.ContinueReconcile() - } - - if newGenerationHash != *pcs.Status.CurrentGenerationHash { - // trigger rolling update by setting or overriding pcs.Status.UpdateProgress. - if err := r.initUpdateProgress(ctx, pcs, pcsObjectName, newGenerationHash); err != nil { - return ctrlcommon.ReconcileWithErrors(fmt.Sprintf("could not triggering rolling update for PCS: %v", pcsObjectKey), err) - } - } - - return ctrlcommon.ContinueReconcile() -} - -// isGenerationHashExpectationSatisfied checks if the current generation hash matches expectations. -func (r *Reconciler) isGenerationHashExpectationSatisfied(pcsObjectName string, pcsGenerationHash *string) bool { - expectedGenerationHash, ok := r.pcsGenerationHashExpectations.Load(pcsObjectName) - return !ok || (pcsGenerationHash != nil && expectedGenerationHash.(string) == *pcsGenerationHash) -} - // computeGenerationHash calculates a hash of the PodCliqueSet pod template specifications. func computeGenerationHash(pcs *grovecorev1alpha1.PodCliqueSet) string { - podTemplateSpecs := lo.Map(pcs.Spec.Template.Cliques, func(pclqTemplateSpec *grovecorev1alpha1.PodCliqueTemplateSpec, _ int) *corev1.PodTemplateSpec { + return k8sutils.ComputeHash(rolloutPodTemplateSpecs(pcs)...) +} + +func rolloutPodTemplateSpecs(pcs *grovecorev1alpha1.PodCliqueSet) []*corev1.PodTemplateSpec { + return lo.Map(pcs.Spec.Template.Cliques, func(pclqTemplateSpec *grovecorev1alpha1.PodCliqueTemplateSpec, _ int) *corev1.PodTemplateSpec { podTemplateSpec := &corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: pclqTemplateSpec.Labels, @@ -121,34 +85,6 @@ func computeGenerationHash(pcs *grovecorev1alpha1.PodCliqueSet) string { podTemplateSpec.Spec.PriorityClassName = pcs.Spec.Template.PriorityClassName return podTemplateSpec }) - return k8sutils.ComputeHash(podTemplateSpecs...) -} - -// setGenerationHashAndUpdateStatus updates the PodCliqueSet status with the new generation hash, stores the expectation, and updates the status subresource. -func (r *Reconciler) setGenerationHashAndUpdateStatus(ctx context.Context, pcs *grovecorev1alpha1.PodCliqueSet, pcsObjectName, generationHash string) error { - pcs.Status.CurrentGenerationHash = &generationHash - if err := r.client.Status().Update(ctx, pcs); err != nil { - return fmt.Errorf("could not update CurrentGenerationHash for PodCliqueSet: %v: %w", client.ObjectKeyFromObject(pcs), err) - } - r.pcsGenerationHashExpectations.Store(pcsObjectName, generationHash) - return nil -} - -// initUpdateProgress initializes a new rolling update by resetting progress tracking. -func (r *Reconciler) initUpdateProgress(ctx context.Context, pcs *grovecorev1alpha1.PodCliqueSet, pcsObjectName, newGenerationHash string) error { - pcs.Status.UpdateProgress = &grovecorev1alpha1.PodCliqueSetUpdateProgress{ - UpdateStartedAt: metav1.Now(), - } - // OnDelete strategy sets UpdateEndedAt too, since we do not know when all the pods will manually be deleted, and gang termination is disabled when an update is in progress - if pcs.Spec.UpdateStrategy != nil && pcs.Spec.UpdateStrategy.Type == grovecorev1alpha1.OnDeleteStrategy { - pcs.Status.UpdateProgress.UpdateEndedAt = ptr.To(metav1.Now()) - } - pcs.Status.UpdatedReplicas = 0 - pcs.Status.CurrentGenerationHash = &newGenerationHash - if err := r.setGenerationHashAndUpdateStatus(ctx, pcs, pcsObjectName, newGenerationHash); err != nil { - return fmt.Errorf("could not set UpdateProgress for PodCliqueSet: %v: %w", client.ObjectKeyFromObject(pcs), err) - } - return nil } // syncPodCliqueSetResources synchronizes all managed child resources. Components are diff --git a/operator/internal/controller/podcliqueset/reconcilespec_test.go b/operator/internal/controller/podcliqueset/reconcilespec_test.go index 3501a1ad6..d2e21388d 100644 --- a/operator/internal/controller/podcliqueset/reconcilespec_test.go +++ b/operator/internal/controller/podcliqueset/reconcilespec_test.go @@ -149,8 +149,8 @@ func TestGetKindSyncGroups(t *testing.T) { } } -// TestInitUpdateProgress tests the initUpdateProgress function for both OnDelete and RollingRecreate strategies -func TestInitUpdateProgress(t *testing.T) { +// TestSetRevisionStatus tests update initialization for both OnDelete and RollingRecreate strategies. +func TestSetRevisionStatus(t *testing.T) { tests := []struct { name string setupPCS func() *grovecorev1alpha1.PodCliqueSet @@ -267,15 +267,18 @@ func TestInitUpdateProgress(t *testing.T) { fakeClient := testutils.SetupFakeClient(pcs) reconciler := &Reconciler{ - client: fakeClient, - pcsGenerationHashExpectations: sync.Map{}, + client: fakeClient, + pcsRevisionExpectations: sync.Map{}, } newGenerationHash := "new-hash" pcsObjectName := pcs.Namespace + "/" + pcs.Name - err := reconciler.initUpdateProgress(context.Background(), pcs, pcsObjectName, newGenerationHash) - require.NoError(t, err, "initUpdateProgress should not return errors") + err := reconciler.setRevisionStatus(context.Background(), pcs, pcsObjectName, revisionSelection{ + name: "new-revision", + generationHash: newGenerationHash, + }, true) + require.NoError(t, err, "setRevisionStatus should not return errors") // Fetch the updated PCS from the fake client updatedPCS := &grovecorev1alpha1.PodCliqueSet{} @@ -295,11 +298,13 @@ func TestInitUpdateProgress(t *testing.T) { assert.Equal(t, int32(0), updatedPCS.Status.UpdateProgress.UpdatedPodCliqueScalingGroupsCount, "UpdatedPodCliqueScalingGroupsCount should be reset to 0") assert.Equal(t, int32(0), updatedPCS.Status.UpdateProgress.UpdatedPodCliquesCount, "UpdatedPodCliquesCount should be reset to 0") - // Currently updating is not set by the initUpdateProgress function + // Currently updating is not set by revision initialization. assert.Nil(t, updatedPCS.Status.UpdateProgress.CurrentlyUpdating, "CurrentlyUpdating should be nil") assert.Equal(t, int32(0), updatedPCS.Status.UpdatedReplicas, "UpdatedReplicas should be reset to 0") require.NotNil(t, updatedPCS.Status.CurrentGenerationHash) assert.Equal(t, newGenerationHash, *updatedPCS.Status.CurrentGenerationHash) + require.NotNil(t, updatedPCS.Status.CurrentRevision) + assert.Equal(t, "new-revision", *updatedPCS.Status.CurrentRevision) }) } } diff --git a/operator/internal/controller/podcliqueset/reconcilestatus.go b/operator/internal/controller/podcliqueset/reconcilestatus.go index 8f904c3a1..c0475c20d 100644 --- a/operator/internal/controller/podcliqueset/reconcilestatus.go +++ b/operator/internal/controller/podcliqueset/reconcilestatus.go @@ -29,6 +29,7 @@ import ( "github.com/ai-dynamo/grove/operator/internal/clustertopology" ctrlcommon "github.com/ai-dynamo/grove/operator/internal/controller/common" componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" k8sutils "github.com/ai-dynamo/grove/operator/internal/utils/kubernetes" "github.com/go-logr/logr" @@ -45,9 +46,13 @@ import ( func (r *Reconciler) reconcileStatus(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet) ctrlcommon.ReconcileStepResult { // Snapshot status before mutations so we can skip the Update call when nothing changes. originalStatus := pcs.Status.DeepCopy() + selectedRevision, err := componentutils.GetSelectedPodCliqueSetRevision(ctx, r.client, pcs) + if err != nil { + return ctrlcommon.ReconcileWithErrors("failed to get PodCliqueSet revision", err) + } // Calculate available replicas using PCSG-inspired approach - err := r.mutateReplicas(ctx, logger, pcs) + err = r.mutateReplicas(ctx, logger, selectedRevision, pcs) if err != nil { return ctrlcommon.ReconcileWithErrors("failed to mutate replicas status", err) } @@ -79,10 +84,10 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, logger logr.Logger, pc } // mutateReplicas updates the PodCliqueSet status replica counts and update-progress counts. -func (r *Reconciler) mutateReplicas(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet) error { +func (r *Reconciler) mutateReplicas(ctx context.Context, logger logr.Logger, revision *commonrevision.SelectedRevision, pcs *grovecorev1alpha1.PodCliqueSet) error { // Set basic replica count pcs.Status.Replicas = pcs.Spec.Replicas - stats, err := r.computeAvailableAndUpdatedReplicas(ctx, logger, pcs) + stats, err := r.computeAvailableAndUpdatedReplicas(ctx, logger, revision, pcs) if err != nil { return fmt.Errorf("could not compute available replicas: %w", err) } @@ -94,6 +99,12 @@ func (r *Reconciler) mutateReplicas(ctx context.Context, logger logr.Logger, pcs pcs.Status.UpdateProgress.UpdatedPodCliqueScalingGroupsCount = stats.updatedPCSGs pcs.Status.UpdateProgress.TotalPodCliqueScalingGroupsCount = stats.totalPCSGs } + if shouldCompleteAutoUpdate(pcs, stats) { + logger.Info("Rolling update has completed from aggregate child status") + completedAt := metav1.Now() + pcs.Status.UpdateProgress.UpdateEndedAt = &completedAt + pcs.Status.UpdateProgress.CurrentlyUpdating = nil + } return nil } @@ -112,6 +123,18 @@ func mutateSelector(pcs *grovecorev1alpha1.PodCliqueSet) error { return nil } +// shouldCompleteAutoUpdate makes aggregate status the sole owner of automatic rollout +// completion. Every predicate is derived from the same child observation as the counts that +// will be persisted with UpdateEndedAt. +func shouldCompleteAutoUpdate(pcs *grovecorev1alpha1.PodCliqueSet, stats pcsReplicaStats) bool { + progress := pcs.Status.UpdateProgress + return componentutils.IsAutoUpdateStrategy(pcs) && + progress != nil && progress.UpdateEndedAt == nil && + stats.updatedReplicas == pcs.Spec.Replicas && + stats.updatedPCLQs == stats.totalPCLQs && + stats.updatedPCSGs == stats.totalPCSGs +} + // pcsReplicaStats aggregates replica- and child-level update progress derived from a single // pass over the informer cache. type pcsReplicaStats struct { @@ -128,12 +151,11 @@ type pcsReplicaStats struct { // computeAvailableAndUpdatedReplicas walks the PCS's standalone PCLQs and PCSGs once and // returns aggregate availability and update counts. Replaces the prior O(N²) accumulator that // stored fully-qualified child names in status. -func (r *Reconciler) computeAvailableAndUpdatedReplicas(ctx context.Context, logger logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet) (pcsReplicaStats, error) { +func (r *Reconciler) computeAvailableAndUpdatedReplicas(ctx context.Context, logger logr.Logger, revision *commonrevision.SelectedRevision, pcs *grovecorev1alpha1.PodCliqueSet) (pcsReplicaStats, error) { var ( stats pcsReplicaStats pcsObjectKey = client.ObjectKeyFromObject(pcs) ) - expectedPCSGFQNsPerPCSReplica := componentutils.GetExpectedPCSGFQNsPerPCSReplica(pcs) expectedStandAlonePCLQFQNsPerPCSReplica := componentutils.GetExpectedStandAlonePCLQFQNsPerPCSReplica(pcs) @@ -179,10 +201,10 @@ func (r *Reconciler) computeAvailableAndUpdatedReplicas(ctx context.Context, log stats.totalPCLQs += int32(expectedPCLQCount) stats.totalPCSGs += int32(expectedPCSGCount) - stats.updatedPCLQs += countUpdatedPCLQs(pcs, replicaStandalonePCLQs) - stats.updatedPCSGs += countUpdatedPCSGs(pcs.Status.CurrentGenerationHash, replicaPCSGs) + stats.updatedPCLQs += countUpdatedPCLQs(revision, replicaStandalonePCLQs) + stats.updatedPCSGs += countUpdatedPCSGs(revision.GenerationHash(), replicaPCSGs) - isReplicaAvailable, isReplicaUpdated := r.computeReplicaStatus(pcs, replicaPCSGs, + isReplicaAvailable, isReplicaUpdated := r.computeReplicaStatus(revision, replicaPCSGs, replicaStandalonePCLQs, expectedPCSGCount, expectedPCLQCount) if isReplicaAvailable { stats.availableReplicas++ @@ -200,17 +222,14 @@ func (r *Reconciler) computeAvailableAndUpdatedReplicas(ctx context.Context, log } // countUpdatedPCLQs counts non-terminating standalone PCLQs that have fully converged to the PCS hash. -func countUpdatedPCLQs(pcs *grovecorev1alpha1.PodCliqueSet, pclqs []grovecorev1alpha1.PodClique) int32 { - if pcs.Status.CurrentGenerationHash == nil { - return 0 - } +func countUpdatedPCLQs(revision *commonrevision.SelectedRevision, pclqs []grovecorev1alpha1.PodClique) int32 { var n int32 for i := range pclqs { pclq := &pclqs[i] if k8sutils.IsResourceTerminating(pclq.ObjectMeta) { continue } - if isStandalonePCLQUpdated(pcs, pclq) { + if isStandalonePCLQUpdated(revision, pclq) { n++ } } @@ -218,17 +237,14 @@ func countUpdatedPCLQs(pcs *grovecorev1alpha1.PodCliqueSet, pclqs []grovecorev1a } // countUpdatedPCSGs counts non-terminating PCSGs whose update completed at the PCS hash. -func countUpdatedPCSGs(pcsGenerationHash *string, pcsgs []grovecorev1alpha1.PodCliqueScalingGroup) int32 { - if pcsGenerationHash == nil { - return 0 - } +func countUpdatedPCSGs(generationHash string, pcsgs []grovecorev1alpha1.PodCliqueScalingGroup) int32 { var n int32 for i := range pcsgs { pcsg := &pcsgs[i] if k8sutils.IsResourceTerminating(pcsg.ObjectMeta) { continue } - if componentutils.IsPCSGUpdateComplete(pcsg, *pcsGenerationHash) { + if componentutils.IsPCSGUpdateComplete(pcsg, generationHash) { n++ } } @@ -236,14 +252,14 @@ func countUpdatedPCSGs(pcsGenerationHash *string, pcsgs []grovecorev1alpha1.PodC } // computeReplicaStatus determines if a replica is available and updated based on its components. -func (r *Reconciler) computeReplicaStatus(pcs *grovecorev1alpha1.PodCliqueSet, replicaPCSGs []grovecorev1alpha1.PodCliqueScalingGroup, standalonePCLQs []grovecorev1alpha1.PodClique, expectedPCSGs int, expectedStandalonePCLQs int) (bool, bool) { - pclqsAvailable, pclqsUpdated := r.computePCLQsStatus(pcs, expectedStandalonePCLQs, standalonePCLQs) - pcsgsAvailable, pcsgsUpdated := r.computePCSGsStatus(pcs.Status.CurrentGenerationHash, expectedPCSGs, replicaPCSGs) +func (r *Reconciler) computeReplicaStatus(revision *commonrevision.SelectedRevision, replicaPCSGs []grovecorev1alpha1.PodCliqueScalingGroup, standalonePCLQs []grovecorev1alpha1.PodClique, expectedPCSGs int, expectedStandalonePCLQs int) (bool, bool) { + pclqsAvailable, pclqsUpdated := r.computePCLQsStatus(revision, expectedStandalonePCLQs, standalonePCLQs) + pcsgsAvailable, pcsgsUpdated := r.computePCSGsStatus(revision.GenerationHash(), expectedPCSGs, replicaPCSGs) return pclqsAvailable && pcsgsAvailable, pclqsUpdated && pcsgsUpdated } // computePCLQsStatus checks if standalone PodCliques are available and updated. -func (r *Reconciler) computePCLQsStatus(pcs *grovecorev1alpha1.PodCliqueSet, expectedStandalonePCLQs int, existingPCLQs []grovecorev1alpha1.PodClique) (isAvailable, isUpdated bool) { +func (r *Reconciler) computePCLQsStatus(revision *commonrevision.SelectedRevision, expectedStandalonePCLQs int, existingPCLQs []grovecorev1alpha1.PodClique) (isAvailable, isUpdated bool) { nonTerminatedPCLQs := lo.Filter(existingPCLQs, func(pclq grovecorev1alpha1.PodClique, _ int) bool { return !k8sutils.IsResourceTerminating(pclq.ObjectMeta) }) @@ -254,18 +270,18 @@ func (r *Reconciler) computePCLQsStatus(pcs *grovecorev1alpha1.PodCliqueSet, exp }) isUpdated = isAvailable && lo.EveryBy(nonTerminatedPCLQs, func(pclq grovecorev1alpha1.PodClique) bool { - return isStandalonePCLQUpdated(pcs, &pclq) + return isStandalonePCLQUpdated(revision, &pclq) }) return } // isStandalonePCLQUpdated checks if a standalone PodClique is fully updated to the expected pod template and PodCliqueSet generation hashes. -func isStandalonePCLQUpdated(pcs *grovecorev1alpha1.PodCliqueSet, pclq *grovecorev1alpha1.PodClique) bool { - if pcs.Status.CurrentGenerationHash == nil || pclq.Spec.MinAvailable == nil { +func isStandalonePCLQUpdated(revision *commonrevision.SelectedRevision, pclq *grovecorev1alpha1.PodClique) bool { + if pclq.Spec.MinAvailable == nil { return false } - expectedPodTemplateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(pcs, pclq.ObjectMeta) + expectedPodTemplateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(revision, pclq.ObjectMeta) if err != nil || expectedPodTemplateHash == "" { return false } @@ -273,13 +289,13 @@ func isStandalonePCLQUpdated(pcs *grovecorev1alpha1.PodCliqueSet, pclq *grovecor pclq.Status.CurrentPodTemplateHash != nil && *pclq.Status.CurrentPodTemplateHash == expectedPodTemplateHash && pclq.Status.CurrentPodCliqueSetGenerationHash != nil && - *pclq.Status.CurrentPodCliqueSetGenerationHash == *pcs.Status.CurrentGenerationHash && + *pclq.Status.CurrentPodCliqueSetGenerationHash == revision.GenerationHash() && pclq.Status.ReadyReplicas >= *pclq.Spec.MinAvailable && pclq.Status.UpdatedReplicas >= *pclq.Spec.MinAvailable } // computePCSGsStatus checks if PodCliqueScalingGroups are available and updated. -func (r *Reconciler) computePCSGsStatus(pcsGenerationHash *string, expectedPCSGs int, pcsgs []grovecorev1alpha1.PodCliqueScalingGroup) (isAvailable, isUpdated bool) { +func (r *Reconciler) computePCSGsStatus(generationHash string, expectedPCSGs int, pcsgs []grovecorev1alpha1.PodCliqueScalingGroup) (isAvailable, isUpdated bool) { nonTerminatedPCSGs := lo.Filter(pcsgs, func(pcsg grovecorev1alpha1.PodCliqueScalingGroup, _ int) bool { return !k8sutils.IsResourceTerminating(pcsg.ObjectMeta) }) @@ -290,7 +306,7 @@ func (r *Reconciler) computePCSGsStatus(pcsGenerationHash *string, expectedPCSGs }) isUpdated = isAvailable && lo.EveryBy(nonTerminatedPCSGs, func(pcsg grovecorev1alpha1.PodCliqueScalingGroup) bool { - return pcsGenerationHash != nil && componentutils.IsPCSGUpdateComplete(&pcsg, *pcsGenerationHash) + return componentutils.IsPCSGUpdateComplete(&pcsg, generationHash) }) return diff --git a/operator/internal/controller/podcliqueset/reconcilestatus_test.go b/operator/internal/controller/podcliqueset/reconcilestatus_test.go index 0e6a17065..fcb20d9c7 100644 --- a/operator/internal/controller/podcliqueset/reconcilestatus_test.go +++ b/operator/internal/controller/podcliqueset/reconcilestatus_test.go @@ -26,6 +26,7 @@ import ( configv1alpha1 "github.com/ai-dynamo/grove/operator/api/config/v1alpha1" grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" testutils "github.com/ai-dynamo/grove/operator/test/utils" "github.com/go-logr/logr" @@ -275,7 +276,7 @@ func TestComputePCSAvailableReplicas(t *testing.T) { cl := testutils.CreateDefaultFakeClient(existingObjects) reconciler := &Reconciler{client: cl} // Compute available replicas - stats, err := reconciler.computeAvailableAndUpdatedReplicas(context.Background(), logr.Discard(), pcs) + stats, err := reconciler.computeAvailableAndUpdatedReplicas(context.Background(), logr.Discard(), selectedRevisionForPCS(pcs), pcs) assert.NoError(t, err) assert.Equal(t, tt.expectedAvailable, stats.availableReplicas, "Available replicas mismatch") }) @@ -783,7 +784,7 @@ func TestComputePCSUpdateProgressCounts(t *testing.T) { cl := testutils.CreateDefaultFakeClient(objects) r := &Reconciler{client: cl} - stats, err := r.computeAvailableAndUpdatedReplicas(context.Background(), logr.Discard(), pcs) + stats, err := r.computeAvailableAndUpdatedReplicas(context.Background(), logr.Discard(), selectedRevisionForPCS(pcs), pcs) require.NoError(t, err) assert.Equal(t, tt.wantUpdatedPCLQs, stats.updatedPCLQs, "updatedPCLQs") assert.Equal(t, tt.wantTotalPCLQs, stats.totalPCLQs, "totalPCLQs") @@ -827,14 +828,14 @@ func TestPCSMutateReplicasWritesUpdateProgressCounts(t *testing.T) { pcs, children := build(false) cl := testutils.CreateDefaultFakeClient(append([]client.Object{pcs}, children...)) r := &Reconciler{client: cl} - require.NoError(t, r.mutateReplicas(context.Background(), logr.Discard(), pcs)) + require.NoError(t, r.mutateReplicas(context.Background(), logr.Discard(), selectedRevisionForPCS(pcs), pcs)) assert.Nil(t, pcs.Status.UpdateProgress, "UpdateProgress must remain nil when not initialized") }) t.Run("UpdateProgress non-nil — counts populated from informer cache", func(t *testing.T) { pcs, children := build(true) cl := testutils.CreateDefaultFakeClient(append([]client.Object{pcs}, children...)) r := &Reconciler{client: cl} - require.NoError(t, r.mutateReplicas(context.Background(), logr.Discard(), pcs)) + require.NoError(t, r.mutateReplicas(context.Background(), logr.Discard(), selectedRevisionForPCS(pcs), pcs)) require.NotNil(t, pcs.Status.UpdateProgress) assert.Equal(t, int32(2), pcs.Status.UpdateProgress.UpdatedPodCliquesCount) assert.Equal(t, int32(2), pcs.Status.UpdateProgress.TotalPodCliquesCount) @@ -843,6 +844,95 @@ func TestPCSMutateReplicasWritesUpdateProgressCounts(t *testing.T) { }) } +func TestMutateReplicasCompletesUpdateFromAggregateSnapshot(t *testing.T) { + const ( + currentHash = "gen-hash-current" + oldHash = "gen-hash-old" + ) + pcsUID := uuid.NewUUID() + tests := []struct { + name string + childHashes []string + completedBefore bool + onDelete bool + wantUpdatedReplicas int32 + wantUpdateCompleted bool + wantCurrentlyUpdating bool + }{ + { + name: "active auto update completes with its fully converged counts", + childHashes: []string{currentHash, currentHash}, + wantUpdatedReplicas: 2, + wantUpdateCompleted: true, + }, + { + name: "active auto update stays open with a partial snapshot", + childHashes: []string{currentHash, oldHash}, + wantUpdatedReplicas: 1, + wantCurrentlyUpdating: true, + }, + { + name: "completed update reports the current snapshot without reopening", + childHashes: []string{currentHash, oldHash}, + completedBefore: true, + wantUpdatedReplicas: 1, + wantUpdateCompleted: true, + }, + { + name: "OnDelete completion remains owned by revision selection", + childHashes: []string{currentHash, currentHash}, + onDelete: true, + wantUpdatedReplicas: 2, + wantCurrentlyUpdating: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + progress := &grovecorev1alpha1.PodCliqueSetUpdateProgress{ + UpdateStartedAt: metav1.Now(), + UpdatedPodCliquesCount: 2, + TotalPodCliquesCount: 2, + CurrentlyUpdating: []grovecorev1alpha1.PodCliqueSetReplicaUpdateProgress{{ + ReplicaIndex: 1, + UpdateStartedAt: metav1.Now(), + }}, + } + if tt.completedBefore { + progress.UpdateEndedAt = ptr.To(metav1.Now()) + progress.CurrentlyUpdating = nil + } + pcsBuilder := testutils.NewPodCliqueSetBuilder(testPCSName, testNamespace, pcsUID). + WithReplicas(2). + WithStandaloneClique("worker"). + WithPodCliqueSetGenerationHash(ptr.To(currentHash)). + WithUpdateProgress(progress) + if tt.onDelete { + pcsBuilder = pcsBuilder.WithUpdateStrategy(&grovecorev1alpha1.PodCliqueSetUpdateStrategy{ + Type: grovecorev1alpha1.OnDeleteStrategy, + }) + } + pcs := pcsBuilder.Build() + pcs.Status.UpdatedReplicas = 2 + + children := make([]client.Object, 0, len(tt.childHashes)) + for replicaIndex, hash := range tt.childHashes { + pclq := testutils.NewPodCliqueBuilder(testPCSName, pcsUID, "worker", testNamespace, int32(replicaIndex)).Build() + children = append(children, markStandalonePCLQConverged(t, pcs, pclq, hash)) + } + cl := testutils.CreateDefaultFakeClient(append([]client.Object{pcs}, children...)) + r := &Reconciler{client: cl} + + require.NoError(t, r.mutateReplicas(context.Background(), logr.Discard(), selectedRevisionForPCS(pcs), pcs)) + assert.Equal(t, tt.wantUpdatedReplicas, pcs.Status.UpdatedReplicas) + assert.Equal(t, tt.wantUpdatedReplicas, pcs.Status.UpdateProgress.UpdatedPodCliquesCount) + assert.Equal(t, int32(2), pcs.Status.UpdateProgress.TotalPodCliquesCount) + assert.Equal(t, tt.wantUpdateCompleted, pcs.Status.UpdateProgress.UpdateEndedAt != nil) + assert.Equal(t, tt.wantCurrentlyUpdating, len(pcs.Status.UpdateProgress.CurrentlyUpdating) > 0) + }) + } +} + func TestCountUpdatedPCLQs(t *testing.T) { hash := "h" otherHash := "old" @@ -876,7 +966,6 @@ func TestCountUpdatedPCLQs(t *testing.T) { in []grovecorev1alpha1.PodClique want int32 }{ - {"nil hash -> 0 (early return guards against uninitialized PCS)", testutils.NewPodCliqueSetBuilder(testPCSName, testNamespace, pcsUID).WithStandaloneClique("worker").Build(), []grovecorev1alpha1.PodClique{matching}, 0}, {"empty input -> 0", pcs, nil, 0}, {"all matching", pcs, []grovecorev1alpha1.PodClique{matching, matching}, 2}, {"none matching", pcs, []grovecorev1alpha1.PodClique{nonMatching, noHash}, 0}, @@ -889,14 +978,14 @@ func TestCountUpdatedPCLQs(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, countUpdatedPCLQs(tt.pcs, tt.in)) + assert.Equal(t, tt.want, countUpdatedPCLQs(selectedRevisionForPCS(tt.pcs), tt.in)) }) } } func markStandalonePCLQConverged(t testing.TB, pcs *grovecorev1alpha1.PodCliqueSet, pclq *grovecorev1alpha1.PodClique, generationHash string) *grovecorev1alpha1.PodClique { t.Helper() - expectedTemplateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(pcs, pclq.ObjectMeta) + expectedTemplateHash, err := componentutils.GetExpectedPCLQPodTemplateHash(selectedRevisionForPCS(pcs), pclq.ObjectMeta) require.NoError(t, err) if pclq.Labels == nil { pclq.Labels = map[string]string{} @@ -930,14 +1019,13 @@ func TestCountUpdatedPCSGs(t *testing.T) { tests := []struct { name string - hash *string + hash string in []grovecorev1alpha1.PodCliqueScalingGroup want int32 }{ - {"nil hash → 0", nil, []grovecorev1alpha1.PodCliqueScalingGroup{matching}, 0}, - {"all matching", &hash, []grovecorev1alpha1.PodCliqueScalingGroup{matching, matching}, 2}, - {"mixed", &hash, []grovecorev1alpha1.PodCliqueScalingGroup{matching, nonMatching}, 1}, - {"terminating excluded", &hash, []grovecorev1alpha1.PodCliqueScalingGroup{matching, terminatingMatching}, 1}, + {"all matching", hash, []grovecorev1alpha1.PodCliqueScalingGroup{matching, matching}, 2}, + {"mixed", hash, []grovecorev1alpha1.PodCliqueScalingGroup{matching, nonMatching}, 1}, + {"terminating excluded", hash, []grovecorev1alpha1.PodCliqueScalingGroup{matching, terminatingMatching}, 1}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -946,6 +1034,14 @@ func TestCountUpdatedPCSGs(t *testing.T) { } } +func selectedRevisionForPCS(pcs *grovecorev1alpha1.PodCliqueSet) *commonrevision.SelectedRevision { + generationHash := "test-generation" + if pcs.Status.CurrentGenerationHash != nil { + generationHash = *pcs.Status.CurrentGenerationHash + } + return testutils.NewSelectedRevision(generationHash, candidateCliqueHashes(pcs)) +} + func TestFlattenNamesToSet(t *testing.T) { tests := []struct { name string diff --git a/operator/internal/controller/podcliqueset/register.go b/operator/internal/controller/podcliqueset/register.go index b9cadcd2e..d3939fe88 100644 --- a/operator/internal/controller/podcliqueset/register.go +++ b/operator/internal/controller/podcliqueset/register.go @@ -26,6 +26,7 @@ import ( grovectrlutils "github.com/ai-dynamo/grove/operator/internal/controller/utils" "github.com/ai-dynamo/grove/operator/internal/utils" + appsv1 "k8s.io/api/apps/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -51,6 +52,7 @@ func (r *Reconciler) RegisterWithManager(mgr manager.Manager) error { MaxConcurrentReconciles: *r.config.ConcurrentSyncs, }). For(&grovecorev1alpha1.PodCliqueSet{}, builder.WithPredicates(podCliqueSetPredicate())). + Owns(&appsv1.ControllerRevision{}). Watches( &grovecorev1alpha1.ClusterTopologyBinding{}, handler.EnqueueRequestsFromMapFunc(mapClusterTopologyToPodCliqueSets(r.client)), diff --git a/operator/internal/controller/podcliqueset/revision.go b/operator/internal/controller/podcliqueset/revision.go new file mode 100644 index 000000000..59099b77a --- /dev/null +++ b/operator/internal/controller/podcliqueset/revision.go @@ -0,0 +1,375 @@ +// /* +// Copyright 2026 The Grove Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// */ + +package podcliqueset + +import ( + "bytes" + "context" + "crypto/sha256" + "encoding/json" + "fmt" + "maps" + "strings" + + apicommon "github.com/ai-dynamo/grove/operator/api/common" + grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" + "github.com/ai-dynamo/grove/operator/internal/constants" + ctrlcommon "github.com/ai-dynamo/grove/operator/internal/controller/common" + componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" + groveutils "github.com/ai-dynamo/grove/operator/internal/utils" + "github.com/ai-dynamo/grove/operator/internal/utils/podtemplatehash" + + "github.com/go-logr/logr" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/cache" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type revisionSelection struct { + name string + generationHash string +} + +type revisionExpectation struct { + uid types.UID + selection revisionSelection +} + +func (r *Reconciler) processRevision(ctx context.Context, _ logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet) ctrlcommon.ReconcileStepResult { + pcsObjectKey := client.ObjectKeyFromObject(pcs) + pcsObjectName := cache.NamespacedNameAsObjectName(pcsObjectKey).String() + currentRevision, expectationResult := r.observeRevisionExpectation(ctx, pcsObjectName, pcs) + if ctrlcommon.ShortCircuitReconcileFlow(expectationResult) { + return expectationResult + } + + if pcs.Status.CurrentRevision == nil { + return r.initializeControllerRevision(ctx, pcs, pcsObjectName) + } + + if currentRevision == nil { + var err error + currentRevision, err = componentutils.GetSelectedPodCliqueSetRevision(ctx, r.client, pcs) + if err != nil { + return ctrlcommon.ReconcileWithErrors("error loading current ControllerRevision", err) + } + } + + desired, desiredCliques, err := marshalDesiredRevision(pcs) + if err != nil { + return ctrlcommon.ReconcileWithErrors("error serializing desired revision", err) + } + equal, err := semanticallyEqualJSON[[]corev1.PodTemplateSpec](currentRevision.Desired(), desired) + if err != nil { + return ctrlcommon.ReconcileWithErrors("error comparing desired revision", err) + } + if equal { + return ctrlcommon.ContinueReconcile() + } + + cliqueHashes := make(map[string]string, len(pcs.Spec.Template.Cliques)) + for _, clique := range pcs.Spec.Template.Cliques { + if old, ok := currentRevision.Clique(clique.Name); ok { + cliqueEqual, err := semanticallyEqualJSON[corev1.PodTemplateSpec](old, desiredCliques[clique.Name]) + if err != nil { + return ctrlcommon.ReconcileWithErrors(fmt.Sprintf("error comparing revision for clique %s", clique.Name), err) + } + if cliqueEqual { + cliqueHashes[clique.Name], err = currentRevision.CliqueHash(clique.Name) + if err != nil { + return ctrlcommon.ReconcileWithErrors(fmt.Sprintf("error loading selected identity for clique %s", clique.Name), err) + } + continue + } + } + cliqueHashes[clique.Name] = podtemplatehash.ComputePodClique(clique, pcs.Spec.Template.PriorityClassName) + } + + data := commonrevision.Data{ + Version: commonrevision.DataVersion, + Desired: desired, + Cliques: desiredCliques, + GenerationHash: computeGenerationHash(pcs), + CliqueHashes: cliqueHashes, + } + if err = r.createAndSelectControllerRevision(ctx, pcs, pcsObjectName, data, true); err != nil { + return ctrlcommon.ReconcileWithErrors("error creating and selecting ControllerRevision", err) + } + return ctrlcommon.ReconcileAfter(constants.ComponentSyncRetryInterval, fmt.Sprintf("waiting for selected ControllerRevision %q to be observed for PodCliqueSet: %v", *pcs.Status.CurrentRevision, pcsObjectKey)) +} + +func (r *Reconciler) initializeControllerRevision(ctx context.Context, pcs *grovecorev1alpha1.PodCliqueSet, pcsObjectName string) ctrlcommon.ReconcileStepResult { + legacy := pcs.Status.ObservedGeneration != nil + if legacy { + if *pcs.Status.ObservedGeneration != pcs.Generation { + return ctrlcommon.ReconcileWithErrors("legacy revision migration blocked", fmt.Errorf("PodCliqueSet %v changed after its last observed generation", client.ObjectKeyFromObject(pcs))) + } + if pcs.Status.UpdateProgress != nil && pcs.Status.UpdateProgress.UpdateEndedAt == nil { + return ctrlcommon.ReconcileWithErrors("legacy revision migration blocked", fmt.Errorf("PodCliqueSet %v has an active update", client.ObjectKeyFromObject(pcs))) + } + } else if pcs.Status.CurrentGenerationHash != nil { + return ctrlcommon.ReconcileWithErrors("revision initialization blocked", fmt.Errorf("PodCliqueSet %v has a generation hash but no observed generation", client.ObjectKeyFromObject(pcs))) + } + + generationHash := computeGenerationHash(pcs) + cliqueHashes := candidateCliqueHashes(pcs) + if legacy { + var err error + cliqueHashes, err = r.legacyCliqueHashes(ctx, pcs, cliqueHashes) + if err != nil { + return ctrlcommon.ReconcileWithErrors("legacy revision migration blocked", err) + } + if pcs.Status.CurrentGenerationHash != nil { + generationHash = *pcs.Status.CurrentGenerationHash + } + } + + desired, desiredCliques, err := marshalDesiredRevision(pcs) + if err != nil { + return ctrlcommon.ReconcileWithErrors("error serializing desired revision", err) + } + data := commonrevision.Data{ + Version: commonrevision.DataVersion, + Desired: desired, + Cliques: desiredCliques, + GenerationHash: generationHash, + CliqueHashes: cliqueHashes, + } + if err = r.createAndSelectControllerRevision(ctx, pcs, pcsObjectName, data, false); err != nil { + return ctrlcommon.ReconcileWithErrors("error creating and selecting ControllerRevision", err) + } + if legacy { + return ctrlcommon.ReconcileAfter(constants.ComponentSyncRetryInterval, "adopted legacy PodCliqueSet revision without mutating children") + } + return ctrlcommon.ReconcileAfter(constants.ComponentSyncRetryInterval, fmt.Sprintf("waiting for selected ControllerRevision %q to be observed for PodCliqueSet: %v", *pcs.Status.CurrentRevision, client.ObjectKeyFromObject(pcs))) +} + +func marshalDesiredRevision(pcs *grovecorev1alpha1.PodCliqueSet) (json.RawMessage, map[string]json.RawMessage, error) { + podTemplates := rolloutPodTemplateSpecs(pcs) + desired, err := json.Marshal(podTemplates) + if err != nil { + return nil, nil, fmt.Errorf("could not serialize PodCliqueSet %v: %w", client.ObjectKeyFromObject(pcs), err) + } + cliques := make(map[string]json.RawMessage, len(pcs.Spec.Template.Cliques)) + for i, clique := range pcs.Spec.Template.Cliques { + cliques[clique.Name], err = json.Marshal(podTemplates[i]) + if err != nil { + return nil, nil, fmt.Errorf("could not serialize clique %s: %w", clique.Name, err) + } + } + return desired, cliques, nil +} + +func candidateCliqueHashes(pcs *grovecorev1alpha1.PodCliqueSet) map[string]string { + hashes := make(map[string]string, len(pcs.Spec.Template.Cliques)) + for _, clique := range pcs.Spec.Template.Cliques { + hashes[clique.Name] = podtemplatehash.ComputePodClique(clique, pcs.Spec.Template.PriorityClassName) + } + return hashes +} + +func semanticallyEqualJSON[T any](left, right json.RawMessage) (bool, error) { + var leftValue, rightValue T + if err := json.Unmarshal(left, &leftValue); err != nil { + return false, fmt.Errorf("could not decode stored revision content: %w", err) + } + if err := json.Unmarshal(right, &rightValue); err != nil { + return false, fmt.Errorf("could not decode proposed revision content: %w", err) + } + return equality.Semantic.DeepEqual(leftValue, rightValue), nil +} + +func (r *Reconciler) legacyCliqueHashes(ctx context.Context, pcs *grovecorev1alpha1.PodCliqueSet, hashes map[string]string) (map[string]string, error) { + labels := apicommon.GetDefaultLabelsForPodCliqueSetManagedResources(pcs.Name) + pcsgList := &grovecorev1alpha1.PodCliqueScalingGroupList{} + if err := r.client.List(ctx, pcsgList, client.InNamespace(pcs.Namespace), client.MatchingLabels(labels)); err != nil { + return nil, fmt.Errorf("could not list PodCliqueScalingGroups: %w", err) + } + pcsgUIDs := make(map[types.UID]struct{}, len(pcsgList.Items)) + for i := range pcsgList.Items { + if metav1.IsControlledBy(&pcsgList.Items[i], pcs) { + pcsgUIDs[pcsgList.Items[i].UID] = struct{}{} + } + } + + pclqList := &grovecorev1alpha1.PodCliqueList{} + if err := r.client.List(ctx, pclqList, client.InNamespace(pcs.Namespace), client.MatchingLabels(labels)); err != nil { + return nil, fmt.Errorf("could not list PodCliques: %w", err) + } + found := make(map[string]string, len(hashes)) + for i := range pclqList.Items { + pclq := &pclqList.Items[i] + if !pclq.DeletionTimestamp.IsZero() { + continue + } + owner := metav1.GetControllerOfNoCopy(pclq) + pcsgOwned := false + if owner != nil { + _, pcsgOwned = pcsgUIDs[owner.UID] + } + if !metav1.IsControlledBy(pclq, pcs) && !pcsgOwned { + continue + } + cliqueName, err := groveutils.GetPodCliqueNameFromPodCliqueFQN(pclq.ObjectMeta) + if err != nil { + return nil, err + } + if _, exists := hashes[cliqueName]; !exists { + continue + } + hash := pclq.Labels[apicommon.LabelPodTemplateHash] + if hash == "" { + return nil, fmt.Errorf("PodClique %v has no pod template hash", client.ObjectKeyFromObject(pclq)) + } + if previous, exists := found[cliqueName]; exists && previous != hash { + return nil, fmt.Errorf("clique %s has conflicting pod template hashes", cliqueName) + } + found[cliqueName] = hash + } + maps.Copy(hashes, found) + return hashes, nil +} + +func (r *Reconciler) ensureControllerRevision(ctx context.Context, pcs *grovecorev1alpha1.PodCliqueSet, data commonrevision.Data) (*appsv1.ControllerRevision, error) { + raw, err := json.Marshal(data) + if err != nil { + return nil, fmt.Errorf("could not serialize ControllerRevision data: %w", err) + } + hash := fmt.Sprintf("%x", sha256.Sum256(raw))[:16] + prefix := pcs.Name + if len(prefix) > 236 { + prefix = strings.TrimRight(prefix[:236], "-.") + } + revision := &appsv1.ControllerRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: prefix + "-" + hash, + Namespace: pcs.Namespace, + Labels: apicommon.GetDefaultLabelsForPodCliqueSetManagedResources(pcs.Name), + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(pcs, grovecorev1alpha1.SchemeGroupVersion.WithKind("PodCliqueSet"))}, + }, + Data: runtime.RawExtension{Raw: raw}, + Revision: pcs.Generation, + } + if err = r.client.Create(ctx, revision); err == nil { + return revision, nil + } else if !apierrors.IsAlreadyExists(err) { + return nil, err + } + + existing := &appsv1.ControllerRevision{} + if err = r.client.Get(ctx, client.ObjectKeyFromObject(revision), existing); err != nil { + return nil, err + } + if !metav1.IsControlledBy(existing, pcs) || !bytes.Equal(existing.Data.Raw, raw) { + return nil, fmt.Errorf("ControllerRevision %v already exists with different ownership or data", client.ObjectKeyFromObject(existing)) + } + return existing, nil +} + +func (r *Reconciler) createAndSelectControllerRevision(ctx context.Context, pcs *grovecorev1alpha1.PodCliqueSet, pcsObjectName string, data commonrevision.Data, startUpdate bool) error { + revision, err := r.ensureControllerRevision(ctx, pcs, data) + if err != nil { + return fmt.Errorf("could not create ControllerRevision: %w", err) + } + selection := revisionSelection{name: revision.Name, generationHash: data.GenerationHash} + if err = r.setRevisionStatus(ctx, pcs, pcsObjectName, selection, startUpdate); err != nil { + return fmt.Errorf("could not select ControllerRevision: %w", err) + } + return nil +} + +func (r *Reconciler) truncateRevisionHistory(ctx context.Context, _ logr.Logger, pcs *grovecorev1alpha1.PodCliqueSet) ctrlcommon.ReconcileStepResult { + if !shouldTruncateRevisionHistory(pcs) { + return ctrlcommon.ContinueReconcile() + } + + revisions := &appsv1.ControllerRevisionList{} + labels := apicommon.GetDefaultLabelsForPodCliqueSetManagedResources(pcs.Name) + if err := r.client.List(ctx, revisions, client.InNamespace(pcs.Namespace), client.MatchingLabels(labels)); err != nil { + return ctrlcommon.ReconcileWithErrors("error listing ControllerRevision history", err) + } + for i := range revisions.Items { + revision := &revisions.Items[i] + if revision.Name == *pcs.Status.CurrentRevision || !metav1.IsControlledBy(revision, pcs) { + continue + } + if err := client.IgnoreNotFound(r.client.Delete(ctx, revision)); err != nil { + return ctrlcommon.ReconcileWithErrors("error truncating ControllerRevision history", + fmt.Errorf("could not delete ControllerRevision %v: %w", client.ObjectKeyFromObject(revision), err)) + } + } + return ctrlcommon.ContinueReconcile() +} + +func shouldTruncateRevisionHistory(pcs *grovecorev1alpha1.PodCliqueSet) bool { + // OnDelete marks orchestration complete immediately, before users replace the old Pods. + // Retain its history because UpdateEndedAt is not proof that historical revisions are unused. + return pcs.Status.CurrentRevision != nil && *pcs.Status.CurrentRevision != "" && + componentutils.IsAutoUpdateStrategy(pcs) && + pcs.Status.UpdateProgress != nil && pcs.Status.UpdateProgress.UpdateEndedAt != nil +} + +func (r *Reconciler) setRevisionStatus(ctx context.Context, pcs *grovecorev1alpha1.PodCliqueSet, pcsObjectName string, selection revisionSelection, startUpdate bool) error { + pcs.Status.CurrentRevision = ptr.To(selection.name) + pcs.Status.CurrentGenerationHash = ptr.To(selection.generationHash) + if startUpdate { + pcs.Status.UpdateProgress = &grovecorev1alpha1.PodCliqueSetUpdateProgress{UpdateStartedAt: metav1.Now()} + if pcs.Spec.UpdateStrategy != nil && pcs.Spec.UpdateStrategy.Type == grovecorev1alpha1.OnDeleteStrategy { + pcs.Status.UpdateProgress.UpdateEndedAt = ptr.To(metav1.Now()) + } + pcs.Status.UpdatedReplicas = 0 + } + if err := r.client.Status().Update(ctx, pcs); err != nil { + return fmt.Errorf("could not update revision status for PodCliqueSet %v: %w", client.ObjectKeyFromObject(pcs), err) + } + r.pcsRevisionExpectations.Store(pcsObjectName, revisionExpectation{uid: pcs.UID, selection: selection}) + return nil +} + +func (r *Reconciler) observeRevisionExpectation(ctx context.Context, pcsObjectName string, pcs *grovecorev1alpha1.PodCliqueSet) (*commonrevision.SelectedRevision, ctrlcommon.ReconcileStepResult) { + value, ok := r.pcsRevisionExpectations.Load(pcsObjectName) + if !ok { + return nil, ctrlcommon.ContinueReconcile() + } + expected := value.(revisionExpectation) + if expected.uid != pcs.UID { + r.pcsRevisionExpectations.CompareAndDelete(pcsObjectName, value) + return nil, ctrlcommon.ContinueReconcile() + } + pcsObjectKey := client.ObjectKeyFromObject(pcs) + if pcs.Status.CurrentRevision == nil || pcs.Status.CurrentGenerationHash == nil || + *pcs.Status.CurrentRevision != expected.selection.name || *pcs.Status.CurrentGenerationHash != expected.selection.generationHash { + return nil, ctrlcommon.ReconcileAfter(constants.ComponentSyncRetryInterval, fmt.Sprintf("current revision is not up-to-date for PodCliqueSet: %v", pcsObjectKey)) + } + selectedRevision, err := componentutils.GetSelectedPodCliqueSetRevision(ctx, r.client, pcs) + if apierrors.IsNotFound(err) { + return nil, ctrlcommon.ReconcileAfter(constants.ComponentSyncRetryInterval, fmt.Sprintf("selected ControllerRevision %q is not yet visible for PodCliqueSet: %v", expected.selection.name, pcsObjectKey)) + } + if err != nil { + return nil, ctrlcommon.ReconcileWithErrors("error loading current ControllerRevision", err) + } + r.pcsRevisionExpectations.CompareAndDelete(pcsObjectName, value) + return selectedRevision, ctrlcommon.ContinueReconcile() +} diff --git a/operator/internal/controller/podcliqueset/revision_test.go b/operator/internal/controller/podcliqueset/revision_test.go new file mode 100644 index 000000000..d6277cec6 --- /dev/null +++ b/operator/internal/controller/podcliqueset/revision_test.go @@ -0,0 +1,624 @@ +// /* +// Copyright 2026 The Grove Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// */ + +package podcliqueset + +import ( + "context" + "encoding/json" + "sync" + "testing" + + apicommon "github.com/ai-dynamo/grove/operator/api/common" + apicommonconstants "github.com/ai-dynamo/grove/operator/api/common/constants" + grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" + ctrlcommon "github.com/ai-dynamo/grove/operator/internal/controller/common" + "github.com/ai-dynamo/grove/operator/internal/controller/common/component" + componentutils "github.com/ai-dynamo/grove/operator/internal/controller/common/component/utils" + commonrevision "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" + testutils "github.com/ai-dynamo/grove/operator/test/utils" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/uuid" + "k8s.io/client-go/tools/cache" + "k8s.io/utils/ptr" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" +) + +func TestControllerRevisionInitializesNewPodCliqueSet(t *testing.T) { + ctx := context.Background() + pcs := testutils.NewPodCliqueSetBuilder("new", "default", uuid.NewUUID()). + WithPodCliqueParameters("worker", 1, nil). + Build() + fakeClient := testutils.SetupFakeClient(pcs) + r := &Reconciler{client: fakeClient, pcsRevisionExpectations: sync.Map{}} + expectedGenerationHash := computeGenerationHash(pcs) + expectedCliqueHashes := candidateCliqueHashes(pcs) + + result := r.processRevision(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + assert.True(t, reconcileRequeues(result)) + require.NotNil(t, pcs.Status.CurrentGenerationHash) + assert.Equal(t, expectedGenerationHash, *pcs.Status.CurrentGenerationHash) + assert.Equal(t, expectedCliqueHashes, selectedCliqueHashes(ctx, t, fakeClient, pcs)) + assert.Equal(t, 1, controllerRevisionCount(ctx, t, fakeClient)) + assert.Nil(t, pcs.Status.UpdateProgress) +} + +func TestControllerRevisionExpectationDoesNotBlockRecreatedPodCliqueSet(t *testing.T) { + ctx := context.Background() + pcs := testutils.NewPodCliqueSetBuilder("recreated", "default", uuid.NewUUID()). + WithPodCliqueParameters("worker", 1, nil). + Build() + fakeClient := testutils.SetupFakeClient(pcs) + r := &Reconciler{client: fakeClient, pcsRevisionExpectations: sync.Map{}} + pcsObjectName := cache.NamespacedNameAsObjectName(client.ObjectKeyFromObject(pcs)).String() + r.pcsRevisionExpectations.Store(pcsObjectName, revisionExpectation{ + uid: uuid.NewUUID(), + selection: revisionSelection{name: "deleted-revision", generationHash: "deleted-generation"}, + }) + + result := r.processRevision(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + assert.True(t, reconcileRequeues(result)) + require.NotNil(t, pcs.Status.CurrentRevision) + assert.NotEqual(t, "deleted-revision", *pcs.Status.CurrentRevision) + + value, ok := r.pcsRevisionExpectations.Load(pcsObjectName) + require.True(t, ok) + assert.Equal(t, pcs.UID, value.(revisionExpectation).uid) +} + +func TestControllerRevisionExpectationWaitsForSelectedRevisionCacheVisibility(t *testing.T) { + ctx := context.Background() + pcs := testutils.NewPodCliqueSetBuilder("cache-lag", "default", uuid.NewUUID()).Build() + desired, cliques, err := marshalDesiredRevision(pcs) + require.NoError(t, err) + generationHash := computeGenerationHash(pcs) + raw, err := json.Marshal(commonrevision.Data{ + Version: commonrevision.DataVersion, + Desired: desired, + Cliques: cliques, + GenerationHash: generationHash, + CliqueHashes: candidateCliqueHashes(pcs), + }) + require.NoError(t, err) + revision := ownedControllerRevision(pcs, "cache-lag-revision", raw) + pcs.Status.CurrentRevision = ptr.To(revision.Name) + pcs.Status.CurrentGenerationHash = ptr.To(generationHash) + + baseClient := testutils.SetupFakeClient(pcs, revision) + revisionVisible := false + laggingClient := interceptor.NewClient(baseClient, interceptor.Funcs{ + Get: func(ctx context.Context, interceptedClient client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*appsv1.ControllerRevision); ok && key.Name == revision.Name && !revisionVisible { + return apierrors.NewNotFound(appsv1.Resource("controllerrevisions"), key.Name) + } + return interceptedClient.Get(ctx, key, obj, opts...) + }, + }) + r := &Reconciler{client: laggingClient, pcsRevisionExpectations: sync.Map{}} + pcsObjectName := cache.NamespacedNameAsObjectName(client.ObjectKeyFromObject(pcs)).String() + r.pcsRevisionExpectations.Store(pcsObjectName, revisionExpectation{ + uid: pcs.UID, + selection: revisionSelection{name: revision.Name, generationHash: generationHash}, + }) + + result := r.processRevision(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + assert.True(t, reconcileRequeues(result)) + assert.Contains(t, result.GetDescription(), "is not yet visible") + _, ok := r.pcsRevisionExpectations.Load(pcsObjectName) + assert.True(t, ok) + + revisionVisible = true + result = r.processRevision(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + assert.False(t, reconcileRequeues(result)) + _, ok = r.pcsRevisionExpectations.Load(pcsObjectName) + assert.False(t, ok) +} + +func TestControllerRevisionExpectationClearedOnDeletion(t *testing.T) { + pcs := testutils.NewPodCliqueSetBuilder("deleting", "default", uuid.NewUUID()).Build() + now := metav1.Now() + pcs.DeletionTimestamp = &now + pcsObjectName := cache.NamespacedNameAsObjectName(client.ObjectKeyFromObject(pcs)).String() + r := &Reconciler{pcsRevisionExpectations: sync.Map{}} + r.pcsRevisionExpectations.Store(pcsObjectName, revisionExpectation{ + uid: pcs.UID, + selection: revisionSelection{name: "selected", generationHash: "generation"}, + }) + + result := r.reconcileDelete(context.Background(), logr.Discard(), pcs) + require.False(t, result.HasErrors()) + _, ok := r.pcsRevisionExpectations.Load(pcsObjectName) + assert.False(t, ok) +} + +func TestControllerRevisionExpectationClearedWhenPodCliqueSetIsGone(t *testing.T) { + req := ctrl.Request{NamespacedName: client.ObjectKey{Namespace: "default", Name: "deleted"}} + pcsObjectName := cache.NamespacedNameAsObjectName(req.NamespacedName).String() + r := &Reconciler{client: testutils.SetupFakeClient(), pcsRevisionExpectations: sync.Map{}} + r.pcsRevisionExpectations.Store(pcsObjectName, revisionExpectation{ + uid: uuid.NewUUID(), + selection: revisionSelection{name: "selected", generationHash: "generation"}, + }) + + result, err := r.Reconcile(context.Background(), req) + require.NoError(t, err) + assert.False(t, result.Requeue) + _, ok := r.pcsRevisionExpectations.Load(pcsObjectName) + assert.False(t, ok) +} + +func TestControllerRevisionFailsClosedWhenSelectedRevisionIsMissing(t *testing.T) { + tests := []struct { + name string + currentGenerationHash func(*grovecorev1alpha1.PodCliqueSet) string + }{ + { + name: "desired state is unchanged", + currentGenerationHash: func(pcs *grovecorev1alpha1.PodCliqueSet) string { + return computeGenerationHash(pcs) + }, + }, + { + name: "desired state has advanced", + currentGenerationHash: func(*grovecorev1alpha1.PodCliqueSet) string { + return "selected-generation" + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + pcs := testutils.NewPodCliqueSetBuilder("missing", "default", uuid.NewUUID()). + WithPodCliqueParameters("worker", 1, nil). + Build() + pcs.Status.CurrentRevision = ptr.To("missing-revision") + pcs.Status.CurrentGenerationHash = ptr.To(tt.currentGenerationHash(pcs)) + originalStatus := pcs.Status.DeepCopy() + fakeClient := testutils.SetupFakeClient(pcs) + r := &Reconciler{client: fakeClient, pcsRevisionExpectations: sync.Map{}} + + result := r.processRevision(ctx, logr.Discard(), pcs) + require.True(t, result.HasErrors()) + assert.True(t, result.NeedsRequeue()) + assert.Equal(t, "error loading current ControllerRevision", result.GetDescription()) + require.Len(t, result.GetErrors(), 1) + assert.True(t, apierrors.IsNotFound(result.GetErrors()[0])) + assert.True(t, equality.Semantic.DeepEqual(*originalStatus, pcs.Status)) + persistedPCS := getPCS(ctx, t, fakeClient, pcs) + assert.True(t, equality.Semantic.DeepEqual(*originalStatus, persistedPCS.Status)) + assert.Equal(t, 0, controllerRevisionCount(ctx, t, fakeClient)) + }) + } +} + +func TestControllerRevisionUpgrade(t *testing.T) { + ctx := context.Background() + pcs := testutils.NewPodCliqueSetBuilder("legacy", "default", uuid.NewUUID()). + WithReplicas(1). + WithPodCliqueParameters("worker", 1, nil). + WithPodCliqueParameters("sidecar", 1, nil). + Build() + pcs.Generation = 7 + pcs.Status.ObservedGeneration = ptr.To(int64(7)) + pcs.Status.CurrentGenerationHash = ptr.To("alpha8-generation") + pcs.Spec.Template.Cliques[0].Spec.PodSpec.Containers = []corev1.Container{{Name: "worker", Image: "worker:v1"}} + pcs.Spec.Template.Cliques[1].Spec.PodSpec.Containers = []corev1.Container{{Name: "sidecar", Image: "sidecar:v1"}} + + pcsg := &grovecorev1alpha1.PodCliqueScalingGroup{ObjectMeta: metav1.ObjectMeta{ + Name: "legacy-0-group", + Namespace: pcs.Namespace, + UID: uuid.NewUUID(), + Labels: apicommon.GetDefaultLabelsForPodCliqueSetManagedResources(pcs.Name), + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(pcs, grovecorev1alpha1.SchemeGroupVersion.WithKind("PodCliqueSet"))}, + }} + worker := legacyPCLQ(pcs, "legacy-0-group-0-worker", "alpha8-worker", pcsg) + sidecar := legacyPCLQ(pcs, "legacy-0-sidecar", "alpha8-sidecar", pcs) + fakeClient := testutils.SetupFakeClient(pcs, pcsg, worker, sidecar) + originalWorker := getPCLQ(ctx, t, fakeClient, worker) + originalSidecar := getPCLQ(ctx, t, fakeClient, sidecar) + r := &Reconciler{client: fakeClient, pcsRevisionExpectations: sync.Map{}} + + result := r.processRevision(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + assert.True(t, reconcileRequeues(result)) + require.NotNil(t, pcs.Status.CurrentGenerationHash) + assert.Equal(t, "alpha8-generation", *pcs.Status.CurrentGenerationHash) + assert.Equal(t, map[string]string{ + "sidecar": "alpha8-sidecar", + "worker": "alpha8-worker", + }, selectedCliqueHashes(ctx, t, fakeClient, pcs)) + assert.Equal(t, 1, controllerRevisionCount(ctx, t, fakeClient)) + assert.Nil(t, pcs.Status.UpdateProgress) + assert.True(t, childrenEqual(ctx, t, fakeClient, originalWorker, originalSidecar)) + + pcs = getPCS(ctx, t, fakeClient, pcs) + result = r.processRevision(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + assert.False(t, reconcileRequeues(result)) + assert.Equal(t, "alpha8-generation", *pcs.Status.CurrentGenerationHash) + assert.Equal(t, map[string]string{ + "sidecar": "alpha8-sidecar", + "worker": "alpha8-worker", + }, selectedCliqueHashes(ctx, t, fakeClient, pcs)) + assert.Equal(t, 1, controllerRevisionCount(ctx, t, fakeClient)) + assert.Nil(t, pcs.Status.UpdateProgress) + + pcs.Generation++ + pcs.Spec.Replicas = 2 + result = r.processRevision(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + assert.False(t, reconcileRequeues(result)) + assert.Equal(t, "alpha8-generation", *pcs.Status.CurrentGenerationHash) + assert.Equal(t, map[string]string{ + "sidecar": "alpha8-sidecar", + "worker": "alpha8-worker", + }, selectedCliqueHashes(ctx, t, fakeClient, pcs)) + assert.Equal(t, 1, controllerRevisionCount(ctx, t, fakeClient)) + assert.Nil(t, pcs.Status.UpdateProgress) + + pcs.Generation++ + pcs.Spec.Template.Cliques[0].Spec.PodSpec.Containers[0].Image = "worker:v2" + expectedGenerationHash := computeGenerationHash(pcs) + expectedWorkerHash := candidateCliqueHashes(pcs)["worker"] + result = r.processRevision(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + assert.True(t, reconcileRequeues(result)) + assert.Equal(t, expectedGenerationHash, *pcs.Status.CurrentGenerationHash) + assert.Equal(t, map[string]string{ + "sidecar": "alpha8-sidecar", + "worker": expectedWorkerHash, + }, selectedCliqueHashes(ctx, t, fakeClient, pcs)) + assert.Equal(t, 2, controllerRevisionCount(ctx, t, fakeClient)) + assert.NotNil(t, pcs.Status.UpdateProgress) +} + +func TestControllerRevisionUpgradeRejectsActiveLegacyUpdate(t *testing.T) { + pcs := testutils.NewPodCliqueSetBuilder("legacy", "default", uuid.NewUUID()). + WithPodCliqueParameters("worker", 1, nil). + Build() + pcs.Generation = 3 + pcs.Status.ObservedGeneration = ptr.To(int64(3)) + pcs.Status.CurrentGenerationHash = ptr.To("alpha8-generation") + pcs.Status.UpdateProgress = &grovecorev1alpha1.PodCliqueSetUpdateProgress{UpdateStartedAt: metav1.Now()} + fakeClient := testutils.SetupFakeClient(pcs) + r := &Reconciler{client: fakeClient, pcsRevisionExpectations: sync.Map{}} + + result := r.processRevision(context.Background(), logr.Discard(), pcs) + require.True(t, result.HasErrors()) + assert.False(t, reconcileRequeues(result)) + assert.Equal(t, "alpha8-generation", *pcs.Status.CurrentGenerationHash) + assert.Equal(t, 0, controllerRevisionCount(context.Background(), t, fakeClient)) + assert.NotNil(t, pcs.Status.UpdateProgress) +} + +func TestSemanticallyEqualJSON(t *testing.T) { + tests := []struct { + name string + left json.RawMessage + right json.RawMessage + wantEqual bool + wantError bool + }{ + { + name: "normalizes representation differences", + left: json.RawMessage(`[{"metadata":{"labels":{"app":"worker"},"creationTimestamp":null},"spec":{"containers":[{"name":"worker","image":"v1","resources":{}}]}}]`), + right: json.RawMessage(`[{"spec":{"containers":[{"resources":{},"image":"v1","name":"worker"}]},"metadata":{"labels":{"app":"worker"}}}]`), + wantEqual: true, + }, + { + name: "detects semantic changes", + left: json.RawMessage(`[{"spec":{"containers":[{"name":"worker","image":"v1"}]}}]`), + right: json.RawMessage(`[{"spec":{"containers":[{"name":"worker","image":"v2"}]}}]`), + wantEqual: false, + }, + { + name: "fails closed for malformed stored content", + left: json.RawMessage(`[{`), + right: json.RawMessage(`[]`), + wantError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + equal, err := semanticallyEqualJSON[[]corev1.PodTemplateSpec](tt.left, tt.right) + if tt.wantError { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantEqual, equal) + }) + } +} + +func TestControllerRevisionPreservesSemanticallyEqualCliqueIdentity(t *testing.T) { + ctx := context.Background() + pcs := testutils.NewPodCliqueSetBuilder("semantic", "default", uuid.NewUUID()). + WithPodCliqueParameters("worker", 1, nil). + WithPodCliqueParameters("sidecar", 1, nil). + Build() + pcs.Spec.Template.Cliques[0].Spec.PodSpec.Containers = []corev1.Container{{Name: "worker", Image: "worker:v1"}} + pcs.Spec.Template.Cliques[1].Spec.PodSpec.Containers = []corev1.Container{{Name: "sidecar", Image: "sidecar:v1"}} + desired, cliques, err := marshalDesiredRevision(pcs) + require.NoError(t, err) + oldHashes := map[string]string{"worker": "worker-old", "sidecar": "sidecar-old"} + data := commonrevision.Data{ + Version: commonrevision.DataVersion, + Desired: desired, + Cliques: cliques, + GenerationHash: "generation-old", + CliqueHashes: oldHashes, + } + var indentedSidecar json.RawMessage + indentedSidecar, err = json.MarshalIndent(data.Cliques["sidecar"], "", " ") + require.NoError(t, err) + data.Cliques["sidecar"] = indentedSidecar + raw, err := json.Marshal(data) + require.NoError(t, err) + revision := ownedControllerRevision(pcs, "semantic-old", raw) + pcs.Status.CurrentRevision = ptr.To(revision.Name) + pcs.Status.CurrentGenerationHash = ptr.To(data.GenerationHash) + + pcs.Spec.Template.Cliques[0].Spec.PodSpec.Containers[0].Image = "worker:v2" + fakeClient := testutils.SetupFakeClient(pcs, revision) + r := &Reconciler{client: fakeClient, pcsRevisionExpectations: sync.Map{}} + result := r.processRevision(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + + selected := selectedCliqueHashes(ctx, t, fakeClient, pcs) + assert.Equal(t, "sidecar-old", selected["sidecar"]) + assert.NotEqual(t, "worker-old", selected["worker"]) +} + +func TestControllerRevisionIgnoresSemanticallyEqualDesiredRepresentation(t *testing.T) { + ctx := context.Background() + pcs := testutils.NewPodCliqueSetBuilder("semantic-noop", "default", uuid.NewUUID()). + WithPodCliqueParameters("worker", 1, nil). + Build() + pcs.Spec.Template.Cliques[0].Spec.PodSpec.Containers = []corev1.Container{{Name: "worker", Image: "worker:v1"}} + desired, cliques, err := marshalDesiredRevision(pcs) + require.NoError(t, err) + indentedDesired, err := json.MarshalIndent(desired, "", " ") + require.NoError(t, err) + require.NotEqual(t, string(desired), string(indentedDesired)) + data := commonrevision.Data{ + Version: commonrevision.DataVersion, + Desired: indentedDesired, + Cliques: cliques, + GenerationHash: "generation-old", + CliqueHashes: map[string]string{"worker": "worker-old"}, + } + raw, err := json.Marshal(data) + require.NoError(t, err) + revision := ownedControllerRevision(pcs, "semantic-noop-old", raw) + pcs.Status.CurrentRevision = ptr.To(revision.Name) + pcs.Status.CurrentGenerationHash = ptr.To(data.GenerationHash) + fakeClient := testutils.SetupFakeClient(pcs, revision) + r := &Reconciler{client: fakeClient, pcsRevisionExpectations: sync.Map{}} + + result := r.processRevision(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + assert.Equal(t, 1, controllerRevisionCount(ctx, t, fakeClient)) + assert.Nil(t, pcs.Status.UpdateProgress) + assert.Equal(t, revision.Name, *pcs.Status.CurrentRevision) +} + +func TestTruncateRevisionHistory(t *testing.T) { + completedAt := metav1.Now() + tests := []struct { + name string + configure func(*grovecorev1alpha1.PodCliqueSet) + wantOld bool + wantForeign bool + }{ + { + name: "completed automatic update truncates owned history", + configure: func(pcs *grovecorev1alpha1.PodCliqueSet) { + pcs.Status.UpdateProgress = &grovecorev1alpha1.PodCliqueSetUpdateProgress{UpdateEndedAt: &completedAt} + }, + wantOld: false, + wantForeign: true, + }, + { + name: "active automatic update retains history", + configure: func(pcs *grovecorev1alpha1.PodCliqueSet) { + pcs.Status.UpdateProgress = &grovecorev1alpha1.PodCliqueSetUpdateProgress{UpdateStartedAt: metav1.Now()} + }, + wantOld: true, + wantForeign: true, + }, + { + name: "on-delete update retains history", + configure: func(pcs *grovecorev1alpha1.PodCliqueSet) { + pcs.Spec.UpdateStrategy = &grovecorev1alpha1.PodCliqueSetUpdateStrategy{Type: grovecorev1alpha1.OnDeleteStrategy} + pcs.Status.UpdateProgress = &grovecorev1alpha1.PodCliqueSetUpdateProgress{UpdateEndedAt: &completedAt} + }, + wantOld: true, + wantForeign: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + pcs := testutils.NewPodCliqueSetBuilder("history", "default", uuid.NewUUID()).Build() + pcs.Status.CurrentRevision = ptr.To("history-current") + tt.configure(pcs) + current := ownedControllerRevision(pcs, "history-current", json.RawMessage(`{}`)) + old := ownedControllerRevision(pcs, "history-old", json.RawMessage(`{}`)) + foreign := ownedControllerRevision(pcs, "history-foreign", json.RawMessage(`{}`)) + foreign.OwnerReferences[0].UID = uuid.NewUUID() + fakeClient := testutils.SetupFakeClient(pcs, current, old, foreign) + r := &Reconciler{client: fakeClient, pcsRevisionExpectations: sync.Map{}} + + result := r.truncateRevisionHistory(ctx, logr.Discard(), pcs) + require.False(t, result.HasErrors()) + assertRevisionExists(ctx, t, fakeClient, current.Name, true) + assertRevisionExists(ctx, t, fakeClient, old.Name, tt.wantOld) + assertRevisionExists(ctx, t, fakeClient, foreign.Name, tt.wantForeign) + }) + } +} + +func TestReconcileTruncatesRevisionHistoryAfterAggregateCompletion(t *testing.T) { + ctx := context.Background() + pcs := testutils.NewPodCliqueSetBuilder("history", "default", uuid.NewUUID()). + WithReplicas(0). + WithUpdateProgress(&grovecorev1alpha1.PodCliqueSetUpdateProgress{UpdateStartedAt: metav1.Now()}). + Build() + pcs.Generation = 1 + pcs.Status.ObservedGeneration = ptr.To(pcs.Generation) + pcs.Finalizers = []string{apicommonconstants.FinalizerPodCliqueSet} + current := testutils.NewPodCliqueSetControllerRevision(pcs, map[string]string{}) + old := ownedControllerRevision(pcs, "history-old", json.RawMessage(`{}`)) + fakeClient := testutils.SetupFakeClient(pcs, current, old) + r := &Reconciler{ + client: fakeClient, + operatorRegistry: component.NewOperatorRegistry[grovecorev1alpha1.PodCliqueSet](), + pcsRevisionExpectations: sync.Map{}, + } + + result, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(pcs)}) + require.NoError(t, err) + assert.False(t, result.Requeue) + + updatedPCS := &grovecorev1alpha1.PodCliqueSet{} + require.NoError(t, fakeClient.Get(ctx, client.ObjectKeyFromObject(pcs), updatedPCS)) + require.NotNil(t, updatedPCS.Status.UpdateProgress) + assert.NotNil(t, updatedPCS.Status.UpdateProgress.UpdateEndedAt) + assertRevisionExists(ctx, t, fakeClient, current.Name, true) + assertRevisionExists(ctx, t, fakeClient, old.Name, false) +} + +func ownedControllerRevision(pcs *grovecorev1alpha1.PodCliqueSet, name string, raw json.RawMessage) *appsv1.ControllerRevision { + return &appsv1.ControllerRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: pcs.Namespace, + Labels: apicommon.GetDefaultLabelsForPodCliqueSetManagedResources(pcs.Name), + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(pcs, grovecorev1alpha1.SchemeGroupVersion.WithKind("PodCliqueSet"))}, + }, + Data: runtime.RawExtension{Raw: raw}, + } +} + +func assertRevisionExists(ctx context.Context, t *testing.T, cl client.Client, name string, want bool) { + t.Helper() + err := cl.Get(ctx, client.ObjectKey{Namespace: "default", Name: name}, &appsv1.ControllerRevision{}) + if want { + require.NoError(t, err) + } else { + require.True(t, apierrors.IsNotFound(err), "expected ControllerRevision %s to be deleted, got %v", name, err) + } +} + +func legacyPCLQ(pcs *grovecorev1alpha1.PodCliqueSet, name, hash string, owner client.Object) *grovecorev1alpha1.PodClique { + labels := apicommon.GetDefaultLabelsForPodCliqueSetManagedResources(pcs.Name) + labels[apicommon.LabelPodTemplateHash] = hash + labels[apicommon.LabelPodCliqueSetReplicaIndex] = "0" + if pcsg, ok := owner.(*grovecorev1alpha1.PodCliqueScalingGroup); ok { + labels[apicommon.LabelPodCliqueScalingGroup] = pcsg.Name + labels[apicommon.LabelPodCliqueScalingGroupReplicaIndex] = "0" + } + gvk := grovecorev1alpha1.SchemeGroupVersion.WithKind("PodCliqueSet") + if _, ok := owner.(*grovecorev1alpha1.PodCliqueScalingGroup); ok { + gvk = grovecorev1alpha1.SchemeGroupVersion.WithKind("PodCliqueScalingGroup") + } + return &grovecorev1alpha1.PodClique{ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: pcs.Namespace, + UID: uuid.NewUUID(), + Labels: labels, + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(owner, gvk)}, + }} +} + +func reconcileRequeues(result ctrlcommon.ReconcileStepResult) bool { + ctrlResult, _ := result.Result() + return ctrlResult.RequeueAfter > 0 +} + +func controllerRevisionCount(ctx context.Context, t *testing.T, cl client.Client) int { + t.Helper() + list := &appsv1.ControllerRevisionList{} + if err := cl.List(ctx, list); err != nil { + t.Fatal(err) + } + return len(list.Items) +} + +func selectedCliqueHashes(ctx context.Context, t *testing.T, cl client.Client, pcs *grovecorev1alpha1.PodCliqueSet) map[string]string { + t.Helper() + selectedRevision, err := componentutils.GetSelectedPodCliqueSetRevision(ctx, cl, pcs) + if err != nil { + t.Fatal(err) + } + hashes := make(map[string]string, len(pcs.Spec.Template.Cliques)) + for _, clique := range pcs.Spec.Template.Cliques { + hash, err := selectedRevision.CliqueHash(clique.Name) + if err != nil { + t.Fatal(err) + } + hashes[clique.Name] = hash + } + return hashes +} + +func childrenEqual(ctx context.Context, t *testing.T, cl client.Client, expected ...*grovecorev1alpha1.PodClique) bool { + t.Helper() + for _, want := range expected { + got := &grovecorev1alpha1.PodClique{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(want), got); err != nil || !equality.Semantic.DeepEqual(want, got) { + return false + } + } + return true +} + +func getPCS(ctx context.Context, t *testing.T, cl client.Client, pcs *grovecorev1alpha1.PodCliqueSet) *grovecorev1alpha1.PodCliqueSet { + t.Helper() + updated := &grovecorev1alpha1.PodCliqueSet{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(pcs), updated); err != nil { + t.Fatal(err) + } + return updated +} + +func getPCLQ(ctx context.Context, t *testing.T, cl client.Client, pclq *grovecorev1alpha1.PodClique) *grovecorev1alpha1.PodClique { + t.Helper() + updated := &grovecorev1alpha1.PodClique{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(pclq), updated); err != nil { + t.Fatal(err) + } + return updated +} diff --git a/operator/internal/utils/kubernetes/pod.go b/operator/internal/utils/kubernetes/pod.go index 34fb5cdfe..91d6e49c4 100644 --- a/operator/internal/utils/kubernetes/pod.go +++ b/operator/internal/utils/kubernetes/pod.go @@ -17,14 +17,11 @@ package kubernetes import ( - "fmt" - "hash/fnv" + "github.com/ai-dynamo/grove/operator/internal/utils/podtemplatehash" "github.com/go-logr/logr" "github.com/samber/lo" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/util/dump" - "k8s.io/apimachinery/pkg/util/rand" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -136,12 +133,7 @@ func HasAnyContainerNotStarted(pod *corev1.Pod) bool { // ComputeHash computes a hash given one or more corev1.PodTemplateSpec. func ComputeHash(podTemplateSpecs ...*corev1.PodTemplateSpec) string { - podTemplateSpecHasher := fnv.New64a() - podTemplateSpecHasher.Reset() - for _, podTemplateSpec := range podTemplateSpecs { - _, _ = fmt.Fprintf(podTemplateSpecHasher, "%v", dump.ForHash(podTemplateSpec)) - } - return rand.SafeEncodeString(fmt.Sprint(podTemplateSpecHasher.Sum64())) + return podtemplatehash.Compute(podTemplateSpecs...) } // GetContainerStatusIfTerminatedErroneously gets the first occurrence of corev1.ContainerStatus (across init, sidecar and main containers) diff --git a/operator/internal/utils/podtemplatehash/hash.go b/operator/internal/utils/podtemplatehash/hash.go new file mode 100644 index 000000000..28f8d2a46 --- /dev/null +++ b/operator/internal/utils/podtemplatehash/hash.go @@ -0,0 +1,52 @@ +// /* +// Copyright 2026 The Grove Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// */ + +// Package podtemplatehash owns the canonical hashing of Grove pod templates. +package podtemplatehash + +import ( + "fmt" + "hash/fnv" + + grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/dump" + "k8s.io/apimachinery/pkg/util/rand" +) + +// Compute returns the canonical hash for one or more PodTemplateSpecs. +func Compute(podTemplateSpecs ...*corev1.PodTemplateSpec) string { + hasher := fnv.New64a() + for _, podTemplateSpec := range podTemplateSpecs { + _, _ = fmt.Fprintf(hasher, "%v", dump.ForHash(podTemplateSpec)) + } + return rand.SafeEncodeString(fmt.Sprint(hasher.Sum64())) +} + +// ComputePodClique returns the canonical pod template hash for a PodClique template. +func ComputePodClique(pclqTemplateSpec *grovecorev1alpha1.PodCliqueTemplateSpec, priorityClassName string) string { + podTemplateSpec := corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: pclqTemplateSpec.Labels, + Annotations: pclqTemplateSpec.Annotations, + }, + Spec: pclqTemplateSpec.Spec.PodSpec, + } + podTemplateSpec.Spec.PriorityClassName = priorityClassName + return Compute(&podTemplateSpec) +} diff --git a/operator/test/utils/pcs.go b/operator/test/utils/pcs.go index 7afa11858..383e9178a 100644 --- a/operator/test/utils/pcs.go +++ b/operator/test/utils/pcs.go @@ -17,12 +17,19 @@ package utils import ( + "encoding/json" "time" + apicommon "github.com/ai-dynamo/grove/operator/api/common" grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" + revisiondata "github.com/ai-dynamo/grove/operator/internal/controller/common/revision" + "github.com/ai-dynamo/grove/operator/internal/utils/podtemplatehash" + appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" ) // PodCliqueSetBuilder is a builder for PodCliqueSet objects. @@ -171,6 +178,68 @@ func (b *PodCliqueSetBuilder) Build() *grovecorev1alpha1.PodCliqueSet { return b.pcs } +// ComputePodCliqueTemplateHashes returns candidate clique hashes for controller tests. +func ComputePodCliqueTemplateHashes(pcs *grovecorev1alpha1.PodCliqueSet) map[string]string { + hashes := make(map[string]string, len(pcs.Spec.Template.Cliques)) + for _, clique := range pcs.Spec.Template.Cliques { + hashes[clique.Name] = podtemplatehash.ComputePodClique(clique, pcs.Spec.Template.PriorityClassName) + } + return hashes +} + +// NewSelectedRevision creates a validated selected revision fixture. +func NewSelectedRevision(generationHash string, cliqueHashes map[string]string) *revisiondata.SelectedRevision { + cliques := make(map[string]json.RawMessage, len(cliqueHashes)) + for name := range cliqueHashes { + cliques[name] = json.RawMessage(`{}`) + } + raw, err := json.Marshal(revisiondata.Data{ + Version: revisiondata.DataVersion, + Desired: json.RawMessage(`[]`), + Cliques: cliques, + GenerationHash: generationHash, + CliqueHashes: cliqueHashes, + }) + if err != nil { + panic(err) + } + selectedRevision, err := revisiondata.DecodeSelectedRevision(raw) + if err != nil { + panic(err) + } + return selectedRevision +} + +// NewPodCliqueSetControllerRevision creates a valid selected revision fixture and updates the PCS status reference. +func NewPodCliqueSetControllerRevision(pcs *grovecorev1alpha1.PodCliqueSet, cliqueHashes map[string]string) *appsv1.ControllerRevision { + if pcs.Status.CurrentGenerationHash == nil { + pcs.Status.CurrentGenerationHash = ptr.To("test-generation") + } + cliques := make(map[string]json.RawMessage, len(cliqueHashes)) + for name := range cliqueHashes { + cliques[name] = json.RawMessage(`{}`) + } + data, _ := json.Marshal(revisiondata.Data{ + Version: revisiondata.DataVersion, + Desired: json.RawMessage(`[]`), + Cliques: cliques, + GenerationHash: *pcs.Status.CurrentGenerationHash, + CliqueHashes: cliqueHashes, + }) + name := pcs.Name + "-test-revision" + pcs.Status.CurrentRevision = ptr.To(name) + return &appsv1.ControllerRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: pcs.Namespace, + Labels: apicommon.GetDefaultLabelsForPodCliqueSetManagedResources(pcs.Name), + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(pcs, grovecorev1alpha1.SchemeGroupVersion.WithKind("PodCliqueSet"))}, + }, + Data: runtime.RawExtension{Raw: data}, + Revision: pcs.Generation, + } +} + func createEmptyPodCliqueSet(name, namespace string, uid types.UID) *grovecorev1alpha1.PodCliqueSet { return &grovecorev1alpha1.PodCliqueSet{ ObjectMeta: metav1.ObjectMeta{ diff --git a/operator/test/utils/setup.go b/operator/test/utils/setup.go index 553c1ca8c..188559ad2 100644 --- a/operator/test/utils/setup.go +++ b/operator/test/utils/setup.go @@ -19,6 +19,7 @@ package utils import ( grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" + appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -35,6 +36,7 @@ import ( func SetupFakeClient(objects ...client.Object) client.WithWatch { scheme := runtime.NewScheme() utilruntime.Must(grovecorev1alpha1.AddToScheme(scheme)) + utilruntime.Must(appsv1.AddToScheme(scheme)) utilruntime.Must(v1.AddToScheme(scheme)) return fake.NewClientBuilder().