diff --git a/internal/controller/workloads/discovery_config_mode_test.go b/internal/controller/workloads/discovery_config_mode_test.go index f7099f383..911f82e80 100644 --- a/internal/controller/workloads/discovery_config_mode_test.go +++ b/internal/controller/workloads/discovery_config_mode_test.go @@ -37,17 +37,16 @@ import ( func TestEnsureDiscoveryConfigMode(t *testing.T) { type testCase struct { - name string - mutateRBG func(*workloadsv1alpha2.RoleBasedGroup) - buildExtraObjects func(*workloadsv1alpha2.RoleBasedGroup) []runtime.Object - wantRequeue bool - wantMode constants.DiscoveryConfigMode - wantModeAnnotation bool + name string + mutateRBG func(*workloadsv1alpha2.RoleBasedGroup) + buildExtraObjects func(*workloadsv1alpha2.RoleBasedGroup) []runtime.Object + wantRequeue bool + wantMode constants.DiscoveryConfigMode } tests := []testCase{ { - name: "missing annotation with legacy role configmap should set legacy mode without requeue", + name: "missing annotation with legacy role configmap sets legacy mode", buildExtraObjects: func(rbg *workloadsv1alpha2.RoleBasedGroup) []runtime.Object { return []runtime.Object{ &corev1.ConfigMap{ @@ -58,33 +57,37 @@ func TestEnsureDiscoveryConfigMode(t *testing.T) { }, } }, - wantRequeue: false, - wantMode: constants.LegacyDiscoveryConfigMode, - wantModeAnnotation: true, + wantRequeue: false, + wantMode: constants.LegacyDiscoveryConfigMode, }, { - name: "missing annotation without legacy signal should set refine mode without requeue", - wantRequeue: false, - wantMode: constants.RefineDiscoveryConfigMode, - wantModeAnnotation: true, + name: "missing annotation with observed generation sets legacy mode", + mutateRBG: func(rbg *workloadsv1alpha2.RoleBasedGroup) { + rbg.Status.ObservedGeneration = 1 + }, + wantRequeue: false, + wantMode: constants.LegacyDiscoveryConfigMode, + }, + { + name: "missing annotation without legacy signal sets refine mode", + wantRequeue: false, + wantMode: constants.RefineDiscoveryConfigMode, }, { - name: "existing legacy annotation should not requeue", + name: "existing legacy annotation is a no-op", mutateRBG: func(rbg *workloadsv1alpha2.RoleBasedGroup) { rbg.SetDiscoveryConfigMode(constants.LegacyDiscoveryConfigMode) }, - wantRequeue: false, - wantMode: constants.LegacyDiscoveryConfigMode, - wantModeAnnotation: true, + wantRequeue: false, + wantMode: constants.LegacyDiscoveryConfigMode, }, { - name: "existing refine annotation should not requeue", + name: "existing refine annotation is a no-op", mutateRBG: func(rbg *workloadsv1alpha2.RoleBasedGroup) { rbg.SetDiscoveryConfigMode(constants.RefineDiscoveryConfigMode) }, - wantRequeue: false, - wantMode: constants.RefineDiscoveryConfigMode, - wantModeAnnotation: true, + wantRequeue: false, + wantMode: constants.RefineDiscoveryConfigMode, }, } @@ -111,7 +114,7 @@ func TestEnsureDiscoveryConfigMode(t *testing.T) { current := &workloadsv1alpha2.RoleBasedGroup{} key := types.NamespacedName{Name: rbg.Name, Namespace: rbg.Namespace} if err := client.Get(context.Background(), key, current); err != nil { - t.Fatalf("get rbg error: %v", err) + t.Fatalf("get rbg: %v", err) } requeue, err := reconciler.ensureDiscoveryConfigMode(context.Background(), current) @@ -122,16 +125,16 @@ func TestEnsureDiscoveryConfigMode(t *testing.T) { t.Fatalf("requeue = %v, want %v", requeue, tt.wantRequeue) } + if got := current.GetDiscoveryConfigMode(); got != tt.wantMode { + t.Fatalf("in-memory mode = %s, want %s", got, tt.wantMode) + } + persisted := &workloadsv1alpha2.RoleBasedGroup{} if err := client.Get(context.Background(), key, persisted); err != nil { - t.Fatalf("get persisted rbg error: %v", err) + t.Fatalf("get persisted rbg: %v", err) } if got := persisted.GetDiscoveryConfigMode(); got != tt.wantMode { - t.Fatalf("mode = %s, want %s", got, tt.wantMode) - } - _, hasModeAnnotation := persisted.Annotations[constants.DiscoveryConfigModeAnnotationKey] - if hasModeAnnotation != tt.wantModeAnnotation { - t.Fatalf("has discovery-config-mode annotation = %v, want %v", hasModeAnnotation, tt.wantModeAnnotation) + t.Fatalf("persisted mode = %s, want %s", got, tt.wantMode) } }) } diff --git a/internal/controller/workloads/rolebasedgroup_controller.go b/internal/controller/workloads/rolebasedgroup_controller.go index 8f8e75a57..d25b9ca37 100644 --- a/internal/controller/workloads/rolebasedgroup_controller.go +++ b/internal/controller/workloads/rolebasedgroup_controller.go @@ -55,6 +55,7 @@ import ( lwsv1 "sigs.k8s.io/lws/api/leaderworkerset/v1" "sigs.k8s.io/rbgs/api/workloads/constants" workloadsv1alpha2 "sigs.k8s.io/rbgs/api/workloads/v1alpha2" + applyconfiguration "sigs.k8s.io/rbgs/client-go/applyconfiguration/workloads/v1alpha2" "sigs.k8s.io/rbgs/pkg/coordination/coordinationscaling" "sigs.k8s.io/rbgs/pkg/dependency" "sigs.k8s.io/rbgs/pkg/discovery" @@ -339,11 +340,19 @@ func (r *RoleBasedGroupReconciler) ensureDiscoveryConfigMode( mode = constants.RefineDiscoveryConfigMode } - old := rbg.DeepCopy() - rbg.SetDiscoveryConfigMode(mode) - if err := r.client.Patch(ctx, rbg, client.MergeFrom(old)); err != nil { + gvk := utils.GetRbgGVK() + applyCfg := applyconfiguration.RoleBasedGroup(rbg.Name, rbg.Namespace). + WithKind(gvk.Kind). + WithAPIVersion(gvk.GroupVersion().String()). + WithAnnotations(map[string]string{ + constants.DiscoveryConfigModeAnnotationKey: string(mode), + }) + if err := utils.PatchObjectApplyConfigurationWithFieldManager( + ctx, r.client, applyCfg, utils.PatchSpec, utils.RBGDiscoveryFieldManager, + ); err != nil { return true, err } + rbg.SetDiscoveryConfigMode(mode) log.FromContext(ctx).Info("Initialized discovery config mode", "mode", mode) // Don't requeue here - continue to reconcile ConfigMap and workloads in the same loop diff --git a/internal/controller/workloads/rolebasedgroupscalingadapter_controller.go b/internal/controller/workloads/rolebasedgroupscalingadapter_controller.go index b5f7a3f5a..68866db59 100644 --- a/internal/controller/workloads/rolebasedgroupscalingadapter_controller.go +++ b/internal/controller/workloads/rolebasedgroupscalingadapter_controller.go @@ -24,7 +24,6 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -32,7 +31,6 @@ import ( "k8s.io/apimachinery/pkg/types" metaapplyv1 "k8s.io/client-go/applyconfigurations/meta/v1" "k8s.io/client-go/tools/record" - "k8s.io/client-go/util/retry" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -498,28 +496,25 @@ func (r *RoleBasedGroupScalingAdapterReconciler) GetTargetRbgFromAdapter( func (r *RoleBasedGroupScalingAdapterReconciler) updateRoleReplicas( ctx context.Context, rbg *workloadsv1alpha2.RoleBasedGroup, targetRoleName string, newReplicas *int32, ) error { - return retry.RetryOnConflict( - retry.DefaultBackoff, func() error { - for index, role := range rbg.Spec.Roles { - if role.Name == targetRoleName { - role.Replicas = newReplicas - rbg.Spec.Roles[index] = role - break - } - } - if err := r.client.Update(ctx, rbg); err != nil { - if apierrors.IsConflict(err) { - if err := r.client.Get( - ctx, types.NamespacedName{Name: rbg.Name, Namespace: rbg.Namespace}, rbg, - ); err != nil { - return err - } - } - return err - } - return nil - }, - ) + gvk := utils.GetRbgGVK() + applyCfg := applyconfiguration.RoleBasedGroup(rbg.Name, rbg.Namespace). + WithKind(gvk.Kind). + WithAPIVersion(gvk.GroupVersion().String()). + WithSpec( + applyconfiguration.RoleBasedGroupSpec(). + WithRoles( + applyconfiguration.RoleSpec(). + WithName(targetRoleName). + WithReplicas(*newReplicas), + ), + ) + + if err := utils.PatchObjectApplyConfigurationWithFieldManager( + ctx, r.client, applyCfg, utils.PatchSpec, utils.RBGReplicaFieldManager, + ); err != nil { + return fmt.Errorf("apply replica update for role %q: %w", targetRoleName, err) + } + return nil } // extractLabelSelectorDefault extracts a LabelSelector string from the given role's scale subresource. diff --git a/internal/controller/workloads/rolebasedgroupscalingadapter_controller_test.go b/internal/controller/workloads/rolebasedgroupscalingadapter_controller_test.go index 9f0d3077b..8febeafed 100644 --- a/internal/controller/workloads/rolebasedgroupscalingadapter_controller_test.go +++ b/internal/controller/workloads/rolebasedgroupscalingadapter_controller_test.go @@ -1110,3 +1110,42 @@ func TestRBGRoleStatusPredicate(t *testing.T) { })) }) } + +// TestUpdateRoleReplicas only covers behaviors the controller-runtime fake +// client can faithfully model: that the apply config is accepted and the +// targeted role's replicas reflect the desired value. SSA list-merge by +// name and sibling-field preservation are real API-server behaviors and +// must be verified via envtest or kind. +func TestUpdateRoleReplicas(t *testing.T) { + scheme := runtime.NewScheme() + _ = workloadsv1alpha2.AddToScheme(scheme) + _ = corev1.AddToScheme(scheme) + + rbg := wrappersv2.BuildBasicRoleBasedGroup("test-rbg", "default"). + WithRoles([]workloadsv1alpha2.RoleSpec{ + wrappersv2.BuildStandaloneRole("worker").WithReplicas(1).Obj(), + }).Obj() + + cl := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(rbg).Build() + reconciler := &RoleBasedGroupScalingAdapterReconciler{ + client: cl, + recorder: record.NewFakeRecorder(10), + } + ctx := ctrl.LoggerInto(context.Background(), zap.New().WithValues("env", "unit-test")) + + if err := reconciler.updateRoleReplicas(ctx, rbg, "worker", ptr.To(int32(5))); err != nil { + t.Fatalf("updateRoleReplicas: %v", err) + } + + updated := &workloadsv1alpha2.RoleBasedGroup{} + if err := cl.Get(ctx, types.NamespacedName{Name: rbg.Name, Namespace: rbg.Namespace}, updated); err != nil { + t.Fatalf("get updated RBG: %v", err) + } + role, err := updated.GetRole("worker") + if err != nil { + t.Fatalf("worker role missing after update: %v", err) + } + if role.Replicas == nil || *role.Replicas != 5 { + t.Errorf("worker replicas = %v, want 5", role.Replicas) + } +} diff --git a/internal/controller/workloads/rolebasedgroupset_controller.go b/internal/controller/workloads/rolebasedgroupset_controller.go index db12a9114..cf39df95d 100644 --- a/internal/controller/workloads/rolebasedgroupset_controller.go +++ b/internal/controller/workloads/rolebasedgroupset_controller.go @@ -18,6 +18,7 @@ package workloads import ( "context" + "encoding/json" "fmt" "reflect" "sort" @@ -39,6 +40,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/rbgs/api/workloads/constants" workloadsv1alpha2 "sigs.k8s.io/rbgs/api/workloads/v1alpha2" + applyconfiguration "sigs.k8s.io/rbgs/client-go/applyconfiguration/workloads/v1alpha2" "sigs.k8s.io/rbgs/pkg/utils" ) @@ -431,107 +433,102 @@ func (r *RoleBasedGroupSetReconciler) needsTemplateAnnotationUpdate( return false } -// updateExistingRBGs updates existing RoleBasedGroup instances to match the current template. +// updateExistingRBGs updates existing RoleBasedGroup instances to match the current template +// using Server-Side Apply. This claims ownership of metadata.labels, metadata.annotations, +// and spec.roles under the "rbg" field manager instead of the default "manager". func (r *RoleBasedGroupSetReconciler) updateExistingRBGs( ctx context.Context, rbgset *workloadsv1alpha2.RoleBasedGroupSet, rbgsToUpdate []*workloadsv1alpha2.RoleBasedGroup, ) error { logger := log.FromContext(ctx) allErrs := make([]error, 0, len(rbgsToUpdate)) - for _, rbg := range rbgsToUpdate { + roleACs, err := toRoleSpecApplyConfigurations(rbgset.Spec.GroupTemplate.Spec.Roles) + if err != nil { + return fmt.Errorf("converting roles to apply configurations: %w", err) + } + gvk := utils.GetRbgGVK() - // Use retry mechanism to handle potential conflicts - err := retry.RetryOnConflict( - retry.DefaultRetry, func() error { - // Get the latest version of the RBG - latestRBG := &workloadsv1alpha2.RoleBasedGroup{} - if err := r.client.Get( - ctx, types.NamespacedName{ - Name: rbg.Name, - Namespace: rbg.Namespace, - }, latestRBG, - ); err != nil { - return err - } - - // Update the spec from template - latestRBG.Spec.Roles = rbgset.Spec.GroupTemplate.Spec.Roles - - // Sync labels and annotations from the template - r.syncRBGMetadata(rbgset, latestRBG) - - // Perform the update - return r.client.Update(ctx, latestRBG) - }, - ) + for _, rbg := range rbgsToUpdate { + rbgIndex := rbg.Labels[constants.GroupSetIndexLabelKey] + syncedLabels, syncedAnnotations := buildSyncedMetadata(rbgset, rbgIndex) + + applyCfg := applyconfiguration.RoleBasedGroup(rbg.Name, rbg.Namespace). + WithKind(gvk.Kind). + WithAPIVersion(gvk.GroupVersion().String()). + WithLabels(syncedLabels). + WithSpec(applyconfiguration.RoleBasedGroupSpec().WithRoles(roleACs...)) + + // Omitting annotations when the template has none releases this manager's + // claim under SSA without clearing annotations owned by other field managers. + if len(syncedAnnotations) > 0 { + applyCfg = applyCfg.WithAnnotations(syncedAnnotations) + } - if err != nil { + if err := utils.PatchObjectApplyConfigurationWithFieldManager( + ctx, r.client, applyCfg, utils.PatchSpec, utils.RBGSetSyncFieldManager, + ); err != nil { allErrs = append(allErrs, fmt.Errorf("failed to update RoleBasedGroup %s: %w", rbg.Name, err)) } else { logger.Info("Successfully updated RoleBasedGroup", "name", rbg.Name) } - } - // Aggregate all concurrent errors return utilerrors.NewAggregate(allErrs) } -// syncRBGMetadata syncs the labels and annotations from Template to the child RBG. -// System-managed labels (GroupSetNameLabelKey, GroupSetIndexLabelKey) are preserved. -func (r *RoleBasedGroupSetReconciler) syncRBGMetadata( - rbgset *workloadsv1alpha2.RoleBasedGroupSet, rbg *workloadsv1alpha2.RoleBasedGroup, -) { - // Sync labels: merge template labels first, then overwrite with system-managed labels - // to ensure system labels cannot be overridden by template labels. - newLabels := make(map[string]string, len(rbgset.Spec.GroupTemplate.Labels)+2) +// buildSyncedMetadata computes the desired labels and annotations for a child RBG +// from the parent RoleBasedGroupSet's template. System-managed labels +// (GroupSetNameLabelKey, GroupSetIndexLabelKey) always win over template labels. +func buildSyncedMetadata( + rbgset *workloadsv1alpha2.RoleBasedGroupSet, rbgIndex string, +) (labels, annotations map[string]string) { + labels = make(map[string]string, len(rbgset.Spec.GroupTemplate.Labels)+2) for k, v := range rbgset.Spec.GroupTemplate.Labels { - newLabels[k] = v + labels[k] = v } - newLabels[constants.GroupSetNameLabelKey] = rbgset.Name - newLabels[constants.GroupSetIndexLabelKey] = rbg.Labels[constants.GroupSetIndexLabelKey] - rbg.Labels = newLabels + labels[constants.GroupSetNameLabelKey] = rbgset.Name + labels[constants.GroupSetIndexLabelKey] = rbgIndex - // Sync annotations: replace with exactly what the template specifies. - if len(rbgset.Spec.GroupTemplate.Annotations) == 0 { - rbg.Annotations = nil - } else { - newAnnotations := make(map[string]string, len(rbgset.Spec.GroupTemplate.Annotations)) + if len(rbgset.Spec.GroupTemplate.Annotations) > 0 { + annotations = make(map[string]string, len(rbgset.Spec.GroupTemplate.Annotations)) for k, v := range rbgset.Spec.GroupTemplate.Annotations { - newAnnotations[k] = v + annotations[k] = v } - rbg.Annotations = newAnnotations } + + return labels, annotations } -// newRBGForSet creates a new RoleBasedGroup object based on the set's template. -func newRBGForSet(rbgset *workloadsv1alpha2.RoleBasedGroupSet, index int) *workloadsv1alpha2.RoleBasedGroup { - // Merge template labels first, then overwrite with system-managed labels to ensure - // system labels cannot be overridden by template labels. - rbgLabels := make(map[string]string, len(rbgset.Spec.GroupTemplate.Labels)+2) - for k, v := range rbgset.Spec.GroupTemplate.Labels { - rbgLabels[k] = v +// toRoleSpecApplyConfigurations converts a slice of RoleSpec to their apply-configuration +// equivalents via JSON round-trip. This is safe because both types share identical JSON tags. +// A field-by-field conversion would be fragile and incomplete given the depth of nested types +// (PodTemplateSpec, containers, volumes, etc.). +func toRoleSpecApplyConfigurations(roles []workloadsv1alpha2.RoleSpec) ([]*applyconfiguration.RoleSpecApplyConfiguration, error) { + if len(roles) == 0 { + return nil, nil } - rbgLabels[constants.GroupSetNameLabelKey] = rbgset.Name - rbgLabels[constants.GroupSetIndexLabelKey] = fmt.Sprintf("%d", index) - - // Copy annotations from the template. - var rbgAnnotations map[string]string - if len(rbgset.Spec.GroupTemplate.Annotations) > 0 { - rbgAnnotations = make(map[string]string, len(rbgset.Spec.GroupTemplate.Annotations)) - for k, v := range rbgset.Spec.GroupTemplate.Annotations { - rbgAnnotations[k] = v - } + b, err := json.Marshal(roles) + if err != nil { + return nil, fmt.Errorf("marshaling roles: %w", err) + } + var roleACs []*applyconfiguration.RoleSpecApplyConfiguration + if err := json.Unmarshal(b, &roleACs); err != nil { + return nil, fmt.Errorf("unmarshaling roles to apply configurations: %w", err) } + return roleACs, nil +} +// newRBGForSet creates a new RoleBasedGroup object based on the set's template. +// The OwnerReference is set by the caller (scaleUp). +func newRBGForSet(rbgset *workloadsv1alpha2.RoleBasedGroupSet, index int) *workloadsv1alpha2.RoleBasedGroup { + labels, annotations := buildSyncedMetadata(rbgset, fmt.Sprintf("%d", index)) return &workloadsv1alpha2.RoleBasedGroup{ ObjectMeta: metav1.ObjectMeta{ Namespace: rbgset.Namespace, Name: fmt.Sprintf("%s-%d", rbgset.Name, index), - Labels: rbgLabels, - Annotations: rbgAnnotations, - // The OwnerReference will be set in the scaleUp function. + Labels: labels, + Annotations: annotations, }, Spec: workloadsv1alpha2.RoleBasedGroupSpec{ Roles: rbgset.Spec.GroupTemplate.Spec.Roles, diff --git a/internal/controller/workloads/rolebasedgroupset_controller_test.go b/internal/controller/workloads/rolebasedgroupset_controller_test.go index ba5af3f1a..8236b25af 100644 --- a/internal/controller/workloads/rolebasedgroupset_controller_test.go +++ b/internal/controller/workloads/rolebasedgroupset_controller_test.go @@ -22,16 +22,17 @@ import ( "testing" "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/rbgs/api/workloads/constants" - workloadsv1alpha2 "sigs.k8s.io/rbgs/api/workloads/v1alpha2" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/rbgs/api/workloads/constants" + workloadsv1alpha2 "sigs.k8s.io/rbgs/api/workloads/v1alpha2" + applyconfiguration "sigs.k8s.io/rbgs/client-go/applyconfiguration/workloads/v1alpha2" ) // TestRoleBasedGroupSetReconciler_scaleUp tests the scaleUp function. @@ -735,17 +736,17 @@ func TestNewRBGForSet_MetadataPropagation(t *testing.T) { } } -// TestSyncRBGMetadata tests the syncRBGMetadata method. -func TestSyncRBGMetadata(t *testing.T) { +// TestBuildSyncedMetadata tests the buildSyncedMetadata method. +func TestBuildSyncedMetadata(t *testing.T) { tests := []struct { name string rbgset *workloadsv1alpha2.RoleBasedGroupSet - rbg *workloadsv1alpha2.RoleBasedGroup + rbgIndex string expectedLabels map[string]string expectedAnnotations map[string]string }{ { - name: "Syncs template labels and annotations, preserves system labels", + name: "Builds template labels and annotations, includes system labels", rbgset: &workloadsv1alpha2.RoleBasedGroupSet{ ObjectMeta: metav1.ObjectMeta{Name: "test-rbgset"}, Spec: workloadsv1alpha2.RoleBasedGroupSetSpec{ @@ -755,16 +756,7 @@ func TestSyncRBGMetadata(t *testing.T) { }, }, }, - rbg: &workloadsv1alpha2.RoleBasedGroup{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - constants.GroupSetNameLabelKey: "test-rbgset", - constants.GroupSetIndexLabelKey: "1", - "old-label": "old-value", - }, - Annotations: map[string]string{"old-annotation": "old-value"}, - }, - }, + rbgIndex: "1", expectedLabels: map[string]string{ constants.GroupSetNameLabelKey: "test-rbgset", constants.GroupSetIndexLabelKey: "1", @@ -773,22 +765,14 @@ func TestSyncRBGMetadata(t *testing.T) { expectedAnnotations: map[string]string{"app.io/env": "prod"}, }, { - name: "Clears annotations when template has none", + name: "Returns nil annotations when template has none", rbgset: &workloadsv1alpha2.RoleBasedGroupSet{ ObjectMeta: metav1.ObjectMeta{Name: "test-rbgset"}, Spec: workloadsv1alpha2.RoleBasedGroupSetSpec{ GroupTemplate: workloadsv1alpha2.RoleBasedGroupTemplateSpec{}, }, }, - rbg: &workloadsv1alpha2.RoleBasedGroup{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - constants.GroupSetNameLabelKey: "test-rbgset", - constants.GroupSetIndexLabelKey: "0", - }, - Annotations: map[string]string{"stale-annotation": "value"}, - }, - }, + rbgIndex: "0", expectedLabels: map[string]string{ constants.GroupSetNameLabelKey: "test-rbgset", constants.GroupSetIndexLabelKey: "0", @@ -796,7 +780,7 @@ func TestSyncRBGMetadata(t *testing.T) { expectedAnnotations: nil, }, { - name: "Removes extra non-system labels not in template", + name: "Only system labels when template has no labels", rbgset: &workloadsv1alpha2.RoleBasedGroupSet{ ObjectMeta: metav1.ObjectMeta{Name: "test-rbgset"}, Spec: workloadsv1alpha2.RoleBasedGroupSetSpec{ @@ -805,19 +789,32 @@ func TestSyncRBGMetadata(t *testing.T) { }, }, }, - rbg: &workloadsv1alpha2.RoleBasedGroup{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - constants.GroupSetNameLabelKey: "test-rbgset", - constants.GroupSetIndexLabelKey: "0", - "stale-label": "old", + rbgIndex: "0", + expectedLabels: map[string]string{ + constants.GroupSetNameLabelKey: "test-rbgset", + constants.GroupSetIndexLabelKey: "0", + "env": "prod", + }, + expectedAnnotations: nil, + }, + { + name: "System labels cannot be overridden by template labels", + rbgset: &workloadsv1alpha2.RoleBasedGroupSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-rbgset"}, + Spec: workloadsv1alpha2.RoleBasedGroupSetSpec{ + GroupTemplate: workloadsv1alpha2.RoleBasedGroupTemplateSpec{ + Labels: map[string]string{ + constants.GroupSetIndexLabelKey: "99", + "tier": "backend", + }, }, }, }, + rbgIndex: "1", expectedLabels: map[string]string{ constants.GroupSetNameLabelKey: "test-rbgset", - constants.GroupSetIndexLabelKey: "0", - "env": "prod", + constants.GroupSetIndexLabelKey: "1", // system label wins + "tier": "backend", }, expectedAnnotations: nil, }, @@ -826,10 +823,9 @@ func TestSyncRBGMetadata(t *testing.T) { for _, tt := range tests { t.Run( tt.name, func(t *testing.T) { - r := &RoleBasedGroupSetReconciler{} - r.syncRBGMetadata(tt.rbgset, tt.rbg) - assert.Equal(t, tt.expectedLabels, tt.rbg.Labels) - assert.Equal(t, tt.expectedAnnotations, tt.rbg.Annotations) + labels, annotations := buildSyncedMetadata(tt.rbgset, tt.rbgIndex) + assert.Equal(t, tt.expectedLabels, labels) + assert.Equal(t, tt.expectedAnnotations, annotations) }, ) } @@ -1147,3 +1143,292 @@ func TestRoleBasedGroupSetReconciler_Reconcile_StatusUpdate(t *testing.T) { ) } } + +// TestToRoleSpecApplyConfigurations tests JSON round-trip conversion of RoleSpec to apply configurations. +func TestToRoleSpecApplyConfigurations(t *testing.T) { + tests := []struct { + name string + roles []workloadsv1alpha2.RoleSpec + wantErr bool + verify func(t *testing.T, result []*applyconfiguration.RoleSpecApplyConfiguration) + }{ + { + name: "nil roles returns nil", + roles: nil, + verify: func(t *testing.T, result []*applyconfiguration.RoleSpecApplyConfiguration) { + assert.Nil(t, result) + }, + }, + { + name: "empty roles returns nil", + roles: []workloadsv1alpha2.RoleSpec{}, + verify: func(t *testing.T, result []*applyconfiguration.RoleSpecApplyConfiguration) { + assert.Nil(t, result) + }, + }, + { + name: "basic role with name and replicas", + roles: []workloadsv1alpha2.RoleSpec{ + { + Name: "worker", + Replicas: ptr.To(int32(3)), + }, + }, + verify: func(t *testing.T, result []*applyconfiguration.RoleSpecApplyConfiguration) { + assert.Len(t, result, 1) + assert.NotNil(t, result[0].Name) + assert.Equal(t, "worker", *result[0].Name) + assert.NotNil(t, result[0].Replicas) + assert.Equal(t, int32(3), *result[0].Replicas) + }, + }, + { + name: "role with labels annotations and dependencies", + roles: []workloadsv1alpha2.RoleSpec{ + { + Name: "trainer", + Replicas: ptr.To(int32(2)), + Labels: map[string]string{"role": "trainer"}, + Annotations: map[string]string{"note": "important"}, + Dependencies: []string{"coordinator"}, + }, + }, + verify: func(t *testing.T, result []*applyconfiguration.RoleSpecApplyConfiguration) { + assert.Len(t, result, 1) + assert.Equal(t, map[string]string{"role": "trainer"}, result[0].Labels) + assert.Equal(t, map[string]string{"note": "important"}, result[0].Annotations) + assert.Equal(t, []string{"coordinator"}, result[0].Dependencies) + }, + }, + { + name: "role with scaling adapter", + roles: []workloadsv1alpha2.RoleSpec{ + { + Name: "scaler", + Replicas: ptr.To(int32(1)), + ScalingAdapter: &workloadsv1alpha2.ScalingAdapter{ + Enable: true, + }, + }, + }, + verify: func(t *testing.T, result []*applyconfiguration.RoleSpecApplyConfiguration) { + assert.Len(t, result, 1) + assert.NotNil(t, result[0].ScalingAdapter) + assert.NotNil(t, result[0].ScalingAdapter.Enable) + assert.True(t, *result[0].ScalingAdapter.Enable) + }, + }, + { + name: "role with standalone pattern and env vars", + roles: []workloadsv1alpha2.RoleSpec{ + { + Name: "worker", + Replicas: ptr.To(int32(2)), + Pattern: workloadsv1alpha2.Pattern{ + StandalonePattern: &workloadsv1alpha2.StandalonePattern{ + TemplateSource: workloadsv1alpha2.TemplateSource{ + Template: &corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "main", + Image: "busybox:latest", + Env: []corev1.EnvVar{ + {Name: "FOO", Value: "bar"}, + {Name: "BAZ", Value: "qux"}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + verify: func(t *testing.T, result []*applyconfiguration.RoleSpecApplyConfiguration) { + assert.Len(t, result, 1) + assert.NotNil(t, result[0].StandalonePattern) + assert.NotNil(t, result[0].StandalonePattern.Template) + containers := result[0].StandalonePattern.Template.Spec.Containers + assert.Len(t, containers, 1) + assert.Equal(t, "main", *containers[0].Name) + assert.Equal(t, "busybox:latest", *containers[0].Image) + assert.Len(t, containers[0].Env, 2) + assert.Equal(t, "FOO", *containers[0].Env[0].Name) + assert.Equal(t, "bar", *containers[0].Env[0].Value) + assert.Equal(t, "BAZ", *containers[0].Env[1].Name) + assert.Equal(t, "qux", *containers[0].Env[1].Value) + }, + }, + { + name: "multiple roles preserves order", + roles: []workloadsv1alpha2.RoleSpec{ + {Name: "coordinator", Replicas: ptr.To(int32(1))}, + {Name: "worker", Replicas: ptr.To(int32(4))}, + {Name: "evaluator", Replicas: ptr.To(int32(1))}, + }, + verify: func(t *testing.T, result []*applyconfiguration.RoleSpecApplyConfiguration) { + assert.Len(t, result, 3) + assert.Equal(t, "coordinator", *result[0].Name) + assert.Equal(t, "worker", *result[1].Name) + assert.Equal(t, "evaluator", *result[2].Name) + assert.Equal(t, int32(4), *result[1].Replicas) + }, + }, + } + + for _, tt := range tests { + t.Run( + tt.name, func(t *testing.T) { + result, err := toRoleSpecApplyConfigurations(tt.roles) + if tt.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + tt.verify(t, result) + }, + ) + } +} + +// TestRoleBasedGroupSetReconciler_updateExistingRBGs_SSA tests that updateExistingRBGs +// uses SSA Apply to sync labels, annotations, and spec.roles to child RBGs. +func TestRoleBasedGroupSetReconciler_updateExistingRBGs_SSA(t *testing.T) { + scheme := runtime.NewScheme() + _ = workloadsv1alpha2.AddToScheme(scheme) + + tests := []struct { + name string + rbgset *workloadsv1alpha2.RoleBasedGroupSet + existingRBGs []*workloadsv1alpha2.RoleBasedGroup + expectedLabels map[string]string + expectedAnnotations map[string]string + expectedRoleName string + expectErr bool + }{ + { + name: "syncs labels and annotations from template", + rbgset: &workloadsv1alpha2.RoleBasedGroupSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-rbgset", Namespace: "default"}, + Spec: workloadsv1alpha2.RoleBasedGroupSetSpec{ + Replicas: ptr.To(int32(1)), + GroupTemplate: workloadsv1alpha2.RoleBasedGroupTemplateSpec{ + Labels: map[string]string{"tier": "backend"}, + Annotations: map[string]string{"app.io/env": "prod"}, + Spec: workloadsv1alpha2.RoleBasedGroupSpec{ + Roles: []workloadsv1alpha2.RoleSpec{{Name: "new-role", Replicas: ptr.To(int32(1))}}, + }, + }, + }, + }, + existingRBGs: []*workloadsv1alpha2.RoleBasedGroup{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rbgset-0", + Namespace: "default", + Labels: map[string]string{ + constants.GroupSetNameLabelKey: "test-rbgset", + constants.GroupSetIndexLabelKey: "0", + }, + }, + Spec: workloadsv1alpha2.RoleBasedGroupSpec{ + Roles: []workloadsv1alpha2.RoleSpec{{Name: "old-role", Replicas: ptr.To(int32(1))}}, + }, + }, + }, + expectedLabels: map[string]string{ + constants.GroupSetNameLabelKey: "test-rbgset", + constants.GroupSetIndexLabelKey: "0", + "tier": "backend", + }, + expectedAnnotations: map[string]string{"app.io/env": "prod"}, + expectedRoleName: "new-role", + }, + { + name: "system labels are preserved", + rbgset: &workloadsv1alpha2.RoleBasedGroupSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-rbgset", Namespace: "default"}, + Spec: workloadsv1alpha2.RoleBasedGroupSetSpec{ + Replicas: ptr.To(int32(1)), + GroupTemplate: workloadsv1alpha2.RoleBasedGroupTemplateSpec{ + Spec: workloadsv1alpha2.RoleBasedGroupSpec{ + Roles: []workloadsv1alpha2.RoleSpec{{Name: "role-a", Replicas: ptr.To(int32(1))}}, + }, + }, + }, + }, + existingRBGs: []*workloadsv1alpha2.RoleBasedGroup{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rbgset-2", + Namespace: "default", + Labels: map[string]string{ + constants.GroupSetNameLabelKey: "test-rbgset", + constants.GroupSetIndexLabelKey: "2", + }, + }, + Spec: workloadsv1alpha2.RoleBasedGroupSpec{ + Roles: []workloadsv1alpha2.RoleSpec{{Name: "old-role", Replicas: ptr.To(int32(1))}}, + }, + }, + }, + expectedLabels: map[string]string{ + constants.GroupSetNameLabelKey: "test-rbgset", + constants.GroupSetIndexLabelKey: "2", + }, + expectedAnnotations: nil, + expectedRoleName: "role-a", + }, + } + + for _, tt := range tests { + t.Run( + tt.name, func(t *testing.T) { + objs := make([]client.Object, 0, len(tt.existingRBGs)) + for _, rbg := range tt.existingRBGs { + objs = append(objs, rbg.DeepCopy()) + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() + r := &RoleBasedGroupSetReconciler{ + client: fakeClient, + scheme: scheme, + } + + err := r.updateExistingRBGs(context.Background(), tt.rbgset, tt.existingRBGs) + if tt.expectErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + + // Verify the last RBG in the list (the one we always expect to succeed) + lastRBG := tt.existingRBGs[len(tt.existingRBGs)-1] + updatedRBG := &workloadsv1alpha2.RoleBasedGroup{} + err = fakeClient.Get(context.Background(), types.NamespacedName{ + Name: lastRBG.Name, + Namespace: lastRBG.Namespace, + }, updatedRBG) + assert.NoError(t, err) + + // Verify labels + for k, v := range tt.expectedLabels { + assert.Equal(t, v, updatedRBG.Labels[k], "label %s mismatch", k) + } + + // Verify annotations + if tt.expectedAnnotations != nil { + for k, v := range tt.expectedAnnotations { + assert.Equal(t, v, updatedRBG.Annotations[k], "annotation %s mismatch", k) + } + } + + // Verify roles + assert.NotEmpty(t, updatedRBG.Spec.Roles) + assert.Equal(t, tt.expectedRoleName, updatedRBG.Spec.Roles[0].Name) + }, + ) + } +} diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 9ad8a5d99..e42360ae6 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -40,6 +40,27 @@ const ( // patches (Force=true, FieldManager="rbg") cannot overwrite conditions owned here. PodControllerFieldManager = "rbg-pod-controller" + // RBGReplicaFieldManager is used when the RBGSA controller writes + // spec.roles[].replicas back to its parent RBG. + // + // SSA-claim release semantics: when the same field manager re-applies with a + // narrower claim, fields it previously owned but no longer claims are released + // and (if no other manager owns them) removed by the API server. The discovery + // annotation Apply, the RBGSet→RBG sync Apply, and this replica Apply each + // claim disjoint subsets of the RBG, so they MUST run under distinct field + // managers — otherwise they ping-pong and silently strip each other's writes. + RBGReplicaFieldManager = "rbg-replicas" + + // RBGDiscoveryFieldManager is used when the RBG controller writes the + // discovery-config-mode annotation. See RBGReplicaFieldManager for the + // rationale behind a distinct field manager. + RBGDiscoveryFieldManager = "rbg-discovery" + + // RBGSetSyncFieldManager is used when the RBGSet controller syncs metadata + // and spec.roles from a parent RBGSet template down to its child RBGs. See + // RBGReplicaFieldManager for the rationale behind a distinct field manager. + RBGSetSyncFieldManager = "rbg-set-sync" + PatchAll PatchType = "all" PatchSpec PatchType = "spec" PatchStatus PatchType = "status" @@ -47,11 +68,22 @@ const ( type PatchType string -// nolint:staticcheck -// TODO: SA1019: Use client.Client.Apply() instead func PatchObjectApplyConfiguration( ctx context.Context, k8sClient client.Client, objApplyConfig interface{}, patchType PatchType, +) error { + return PatchObjectApplyConfigurationWithFieldManager(ctx, k8sClient, objApplyConfig, patchType, FieldManager) +} + +// PatchObjectApplyConfigurationWithFieldManager is like PatchObjectApplyConfiguration but uses +// the provided field manager. Use a distinct field manager for each logical Apply path that +// targets disjoint fields on the same object, otherwise SSA-claim-release semantics will cause +// the Apply calls to strip each other's writes. +// +//nolint:staticcheck // SA1019: Use client.Client.Apply() instead +func PatchObjectApplyConfigurationWithFieldManager( + ctx context.Context, k8sClient client.Client, + objApplyConfig interface{}, patchType PatchType, fieldManager string, ) error { logger := log.FromContext(ctx) obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(objApplyConfig) @@ -66,14 +98,10 @@ func PatchObjectApplyConfiguration( logger.V(1).Info("patch content", "patchObject", patch.Object) - // Use server side apply and add fieldmanager to the rbg owned fields - // If there are conflicts in the fields owned by the rbg controller, rbg will obtain the ownership and force override - // these fields to the ones desired by the rbg controller - // TODO b/316776287 add E2E test for SSA if patchType == PatchSpec || patchType == PatchAll { err = k8sClient.Patch( ctx, patch, client.Apply, &client.PatchOptions{ - FieldManager: FieldManager, + FieldManager: fieldManager, Force: ptr.To[bool](true), }, ) @@ -88,7 +116,7 @@ func PatchObjectApplyConfiguration( ctx, patch, client.Apply, &client.SubResourcePatchOptions{ PatchOptions: client.PatchOptions{ - FieldManager: FieldManager, + FieldManager: fieldManager, Force: ptr.To[bool](true), }, },