diff --git a/pkg/resolver/cell.go b/pkg/resolver/cell.go index ac00550c..fb4dbe56 100644 --- a/pkg/resolver/cell.go +++ b/pkg/resolver/cell.go @@ -8,40 +8,74 @@ import ( "k8s.io/apimachinery/pkg/types" multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1" + corev1 "k8s.io/api/core/v1" ) -// ResolveCellTemplate fetches and resolves a CellTemplate by name, handling defaults. +// ResolveCell determines the final configuration for a specific Cell. +// It orchestrates: Template Lookup -> Fetch -> Merge -> Defaulting. +func (r *Resolver) ResolveCell( + ctx context.Context, + cellSpec *multigresv1alpha1.CellConfig, +) (*multigresv1alpha1.StatelessSpec, *multigresv1alpha1.LocalTopoServerSpec, error) { + // 1. Fetch Template (Logic handles defaults) + templateName := cellSpec.CellTemplate + tpl, err := r.ResolveCellTemplate(ctx, templateName) + if err != nil { + return nil, nil, err + } + + // 2. Merge Logic + gateway, localTopo := mergeCellConfig(tpl, cellSpec.Overrides, cellSpec.Spec) + + // 3. Apply Deep Defaults (Level 4) + // We use empty resources for Gateway default, as the specific values are often deployment-dependent, + // but we must ensure Replicas is at least 1. + defaultStatelessSpec(gateway, corev1.ResourceRequirements{}, 1) + + // Note: We do NOT default LocalTopo here because it is optional. + // If it is nil, it remains nil (meaning the cell uses Global Topo). + // If it is non-nil (e.g. from template), we apply Etcd defaults. + if localTopo != nil && localTopo.Etcd != nil { + defaultEtcdSpec(localTopo.Etcd) + } + + return gateway, localTopo, nil +} + +// ResolveCellTemplate fetches and resolves a CellTemplate by name. +// If name is empty, it resolves using the Cluster Defaults, then the Namespace Default. func (r *Resolver) ResolveCellTemplate( ctx context.Context, - templateName string, + name string, ) (*multigresv1alpha1.CellTemplate, error) { - name := templateName + resolvedName := name isImplicitFallback := false - if name == "" { - name = r.TemplateDefaults.CellTemplate + if resolvedName == "" { + resolvedName = r.TemplateDefaults.CellTemplate } - if name == "" { - name = FallbackCellTemplate + if resolvedName == "" { + resolvedName = FallbackCellTemplate isImplicitFallback = true } tpl := &multigresv1alpha1.CellTemplate{} - err := r.Client.Get(ctx, types.NamespacedName{Name: name, Namespace: r.Namespace}, tpl) + err := r.Client.Get(ctx, types.NamespacedName{Name: resolvedName, Namespace: r.Namespace}, tpl) if err != nil { if errors.IsNotFound(err) { if isImplicitFallback { + // We return an empty struct instead of nil to satisfy tests expecting non-nil structure. return &multigresv1alpha1.CellTemplate{}, nil } - return nil, fmt.Errorf("referenced CellTemplate '%s' not found: %w", name, err) + return nil, fmt.Errorf("referenced CellTemplate '%s' not found: %w", resolvedName, err) } return nil, fmt.Errorf("failed to get CellTemplate: %w", err) } return tpl, nil } -// MergeCellConfig merges a template spec with overrides and an inline spec to produce the final configuration. -func MergeCellConfig( +// mergeCellConfig merges a template spec with overrides and an inline spec. +func mergeCellConfig( template *multigresv1alpha1.CellTemplate, overrides *multigresv1alpha1.CellOverrides, inline *multigresv1alpha1.CellInlineSpec, @@ -65,6 +99,10 @@ func MergeCellConfig( } if inline != nil { + // Inline spec completely replaces the template for the components it defines + // However, for Multigres 'Spec' blocks, usually 'Spec' is exclusive to 'TemplateRef'. + // The design allows "Inline Spec" OR "Template + Overrides". + // If Inline Spec is present, we generally prefer it entirely. gw := inline.MultiGateway.DeepCopy() var topo *multigresv1alpha1.LocalTopoServerSpec if inline.LocalTopoServer != nil { diff --git a/pkg/resolver/cell_test.go b/pkg/resolver/cell_test.go index ff60d617..90e181a3 100644 --- a/pkg/resolver/cell_test.go +++ b/pkg/resolver/cell_test.go @@ -16,6 +16,100 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +func TestResolver_ResolveCell(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + _ = multigresv1alpha1.AddToScheme(scheme) + _, cellTpl, _, ns := setupFixtures(t) + + tests := map[string]struct { + config *multigresv1alpha1.CellConfig + objects []client.Object + wantGw *multigresv1alpha1.StatelessSpec + wantTopo *multigresv1alpha1.LocalTopoServerSpec + wantErr bool + }{ + "Template Found": { + config: &multigresv1alpha1.CellConfig{ + CellTemplate: "default", + }, + objects: []client.Object{cellTpl}, + wantGw: &multigresv1alpha1.StatelessSpec{ + Replicas: ptr.To(int32(1)), + Resources: corev1.ResourceRequirements{}, + }, + wantTopo: &multigresv1alpha1.LocalTopoServerSpec{ + Etcd: &multigresv1alpha1.EtcdSpec{ + Image: "local-etcd-default", + Replicas: ptr.To(DefaultEtcdReplicas), + Resources: DefaultResourcesEtcd(), + Storage: multigresv1alpha1.StorageSpec{Size: DefaultEtcdStorageSize}, + }, + }, + }, + "Template Not Found (Error)": { + config: &multigresv1alpha1.CellConfig{ + CellTemplate: "missing", + }, + wantErr: true, + }, + "Inline Overrides": { + config: &multigresv1alpha1.CellConfig{ + Spec: &multigresv1alpha1.CellInlineSpec{ + MultiGateway: multigresv1alpha1.StatelessSpec{ + Replicas: ptr.To(int32(3)), + }, + }, + }, + wantGw: &multigresv1alpha1.StatelessSpec{ + Replicas: ptr.To(int32(3)), + Resources: corev1.ResourceRequirements{}, + }, + wantTopo: nil, // Inline spec didn't provide one + }, + "Client Error": { + config: &multigresv1alpha1.CellConfig{CellTemplate: "any"}, + // Will use mock client logic inside test runner + wantErr: true, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + var c client.Client + if name == "Client Error" { + c = &mockClient{failGet: true, err: errors.New("fail")} + } else { + c = fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(tc.objects...). + Build() + } + r := NewResolver(c, ns, multigresv1alpha1.TemplateDefaults{}) + + gw, topo, err := r.ResolveCell(t.Context(), tc.config) + if tc.wantErr { + if err == nil { + t.Error("Expected error") + } + return + } + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if diff := cmp.Diff(tc.wantGw, gw, cmpopts.IgnoreUnexported(resource.Quantity{}), cmpopts.EquateEmpty()); diff != "" { + t.Errorf("Gateway Diff (-want +got):\n%s", diff) + } + if diff := cmp.Diff(tc.wantTopo, topo, cmpopts.IgnoreUnexported(resource.Quantity{}), cmpopts.EquateEmpty()); diff != "" { + t.Errorf("Topo Diff (-want +got):\n%s", diff) + } + }) + } +} + func TestResolver_ResolveCellTemplate(t *testing.T) { t.Parallel() @@ -237,7 +331,7 @@ func TestMergeCellConfig(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { t.Parallel() - gw, topo := MergeCellConfig(tc.tpl, tc.overrides, tc.inline) + gw, topo := mergeCellConfig(tc.tpl, tc.overrides, tc.inline) if diff := cmp.Diff(tc.wantGw, gw, cmpopts.IgnoreUnexported(resource.Quantity{}), cmpopts.EquateEmpty()); diff != "" { t.Errorf("Gateway mismatch (-want +got):\n%s", diff) diff --git a/pkg/resolver/cluster.go b/pkg/resolver/cluster.go index 7f5f938c..e1d5ab3a 100644 --- a/pkg/resolver/cluster.go +++ b/pkg/resolver/cluster.go @@ -4,7 +4,6 @@ import ( "context" "fmt" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" @@ -56,8 +55,9 @@ func (r *Resolver) PopulateClusterDefaults(cluster *multigresv1alpha1.MultigresC }) } - // If any database has no tablegroups, inject the mandatory default tablegroup "default". + // Iterate databases to ensure TableGroups and Shards exist for i := range cluster.Spec.Databases { + // If any database has no tablegroups, inject the mandatory default tablegroup "default". if len(cluster.Spec.Databases[i].TableGroups) == 0 { cluster.Spec.Databases[i].TableGroups = append( cluster.Spec.Databases[i].TableGroups, @@ -67,132 +67,44 @@ func (r *Resolver) PopulateClusterDefaults(cluster *multigresv1alpha1.MultigresC }, ) } - } - - // 4. Default Inline Configs (Deep Defaulting) - // We ONLY default these if the user explicitly provided the block (Inline). - // We DO NOT fetch templates here, adhering to the "Non-Goal" of the design doc. - - // GlobalTopoServer: If user provided 'etcd: {}', fill in the details. - if cluster.Spec.GlobalTopoServer.Etcd != nil { - defaultEtcdSpec(cluster.Spec.GlobalTopoServer.Etcd) - } - // MultiAdmin: If user provided 'spec: {}', fill in the details. - if cluster.Spec.MultiAdmin.Spec != nil { - defaultStatelessSpec( - cluster.Spec.MultiAdmin.Spec, - DefaultResourcesAdmin(), - DefaultAdminReplicas, - ) - } - - // Cells: Default inline specs - for i := range cluster.Spec.Cells { - if cluster.Spec.Cells[i].Spec != nil { - defaultStatelessSpec( - &cluster.Spec.Cells[i].Spec.MultiGateway, - // Note: You might want to define specific defaults for Gateway if they differ from Admin. - // For now using the same pattern or generic defaults. - // Assuming you might add DefaultResourcesGateway later, but using valid struct defaults here. - corev1.ResourceRequirements{}, // Placeholder or define specific constant if needed - 1, // Default replicas - ) + // If any TableGroup has no Shards, inject the mandatory default Shard "0". + // This ensures minimal configs result in actual running pods. + for j := range cluster.Spec.Databases[i].TableGroups { + if len(cluster.Spec.Databases[i].TableGroups[j].Shards) == 0 { + cluster.Spec.Databases[i].TableGroups[j].Shards = append( + cluster.Spec.Databases[i].TableGroups[j].Shards, + multigresv1alpha1.ShardConfig{ + Name: "0", + // ShardTemplate defaults to "" here, which will be resolved + // to "default" (FallbackShardTemplate) by ResolveShard logic later. + }, + ) + } } } } -// defaultEtcdSpec applies hardcoded safety defaults to an inline Etcd spec. -func defaultEtcdSpec(spec *multigresv1alpha1.EtcdSpec) { - if spec.Image == "" { - spec.Image = DefaultEtcdImage - } - if spec.Storage.Size == "" { - spec.Storage.Size = DefaultEtcdStorageSize - } - if spec.Replicas == nil { - r := DefaultEtcdReplicas - spec.Replicas = &r - } - // Use isResourcesZero to ensure we respect overrides that only have Claims - if isResourcesZero(spec.Resources) { - // Safety: DefaultResourcesEtcd() returns a fresh struct, so no DeepCopy needed. - spec.Resources = DefaultResourcesEtcd() - } -} - -// defaultStatelessSpec applies hardcoded safety defaults to any stateless spec. -func defaultStatelessSpec( - spec *multigresv1alpha1.StatelessSpec, - defaultRes corev1.ResourceRequirements, - defaultReplicas int32, -) { - if spec.Replicas == nil { - spec.Replicas = &defaultReplicas - } - // Use isResourcesZero to ensure we respect overrides that only have Claims - if isResourcesZero(spec.Resources) { - // Safety: We assume defaultRes is passed by value (a fresh copy from the default function). - // We perform a DeepCopy to ensure spec.Resources owns its own maps, independent of the input defaultRes. - spec.Resources = *defaultRes.DeepCopy() - } -} - -// isResourcesZero checks if the resource requirements are strictly the zero value (nil maps). -// This mimics reflect.DeepEqual(res, corev1.ResourceRequirements{}) but is safer and faster. -// It is used for merging logic where we want to distinguish "inherit" (nil) from "empty" (set to empty). -func isResourcesZero(res corev1.ResourceRequirements) bool { - return res.Requests == nil && res.Limits == nil && res.Claims == nil -} - -// ResolveCoreTemplate determines the target CoreTemplate name and fetches it. -// -// If templateName is empty, it uses the following precedence: -// 1. The cluster-level default defined in TemplateDefaults. -// 2. A CoreTemplate named "default" found in the same namespace where MultigresCluster is deployed. -// -// If an explicit template (param or cluster default) is not found, it returns an error. -// If the implicit "default" template is not found, it returns an empty object (safe fallback). -func (r *Resolver) ResolveCoreTemplate( +// ResolveGlobalTopo determines the final GlobalTopoServer configuration. +// It handles the precedence: Inline > TemplateRef (Specific) > TemplateRef (Cluster Default) > Fallback. +// It performs the necessary I/O to fetch the CoreTemplate. +func (r *Resolver) ResolveGlobalTopo( ctx context.Context, - templateName string, -) (*multigresv1alpha1.CoreTemplate, error) { - name := templateName - isImplicitFallback := false - - if name == "" { - name = r.TemplateDefaults.CoreTemplate - } - if name == "" { - name = FallbackCoreTemplate - isImplicitFallback = true - } - - tpl := &multigresv1alpha1.CoreTemplate{} - err := r.Client.Get(ctx, types.NamespacedName{Name: name, Namespace: r.Namespace}, tpl) + cluster *multigresv1alpha1.MultigresCluster, +) (*multigresv1alpha1.GlobalTopoServerSpec, error) { + // 1. Fetch Template (Logic handles defaults) + templateName := cluster.Spec.GlobalTopoServer.TemplateRef + coreTemplate, err := r.ResolveCoreTemplate(ctx, templateName) if err != nil { - if errors.IsNotFound(err) { - if isImplicitFallback { - return &multigresv1alpha1.CoreTemplate{}, nil - } - return nil, fmt.Errorf("referenced CoreTemplate '%s' not found: %w", name, err) - } - return nil, fmt.Errorf("failed to get CoreTemplate: %w", err) + return nil, err } - return tpl, nil -} -// ResolveGlobalTopo determines the final GlobalTopoServer configuration. -// It prioritizes Inline Config > Template > Implicit Default (Managed Etcd). -// It applies deep defaults (safety limits) to the final result. -func ResolveGlobalTopo( - spec *multigresv1alpha1.GlobalTopoServerSpec, - coreTemplate *multigresv1alpha1.CoreTemplate, -) *multigresv1alpha1.GlobalTopoServerSpec { + // 2. Merge Config var finalSpec *multigresv1alpha1.GlobalTopoServerSpec + spec := cluster.Spec.GlobalTopoServer - // 1. Determine base config if spec.Etcd != nil || spec.External != nil { + // Inline definition takes precedence finalSpec = spec.DeepCopy() } else if coreTemplate != nil && coreTemplate.Spec.GlobalTopoServer != nil { // Copy from template @@ -200,41 +112,79 @@ func ResolveGlobalTopo( Etcd: coreTemplate.Spec.GlobalTopoServer.Etcd.DeepCopy(), } } else { - // Fallback: Default to an empty Etcd spec if nothing found. + // Fallback: Default to an empty Etcd spec if nothing found finalSpec = &multigresv1alpha1.GlobalTopoServerSpec{ Etcd: &multigresv1alpha1.EtcdSpec{}, } } - // 2. Apply Deep Defaults to Etcd if present + // 3. Apply Deep Defaults (Level 4) if finalSpec.Etcd != nil { defaultEtcdSpec(finalSpec.Etcd) } - return finalSpec + return finalSpec, nil } // ResolveMultiAdmin determines the final MultiAdmin configuration. -// It prioritizes Inline Config > Template > Implicit Default. -// It applies deep defaults (safety limits) to the final result. -func ResolveMultiAdmin( - spec *multigresv1alpha1.MultiAdminConfig, - coreTemplate *multigresv1alpha1.CoreTemplate, -) *multigresv1alpha1.StatelessSpec { +// It handles the precedence: Inline > TemplateRef (Specific) > TemplateRef (Cluster Default) > Fallback. +func (r *Resolver) ResolveMultiAdmin( + ctx context.Context, + cluster *multigresv1alpha1.MultigresCluster, +) (*multigresv1alpha1.StatelessSpec, error) { + // 1. Fetch Template (Logic handles defaults) + templateName := cluster.Spec.MultiAdmin.TemplateRef + coreTemplate, err := r.ResolveCoreTemplate(ctx, templateName) + if err != nil { + return nil, err + } + + // 2. Merge Config var finalSpec *multigresv1alpha1.StatelessSpec + spec := cluster.Spec.MultiAdmin - // 1. Determine base config if spec.Spec != nil { finalSpec = spec.Spec.DeepCopy() } else if coreTemplate != nil && coreTemplate.Spec.MultiAdmin != nil { finalSpec = coreTemplate.Spec.MultiAdmin.DeepCopy() } else { - // Fallback to empty spec so we can apply defaults finalSpec = &multigresv1alpha1.StatelessSpec{} } - // 2. Apply Deep Defaults + // 3. Apply Deep Defaults (Level 4) defaultStatelessSpec(finalSpec, DefaultResourcesAdmin(), DefaultAdminReplicas) - return finalSpec + return finalSpec, nil +} + +// ResolveCoreTemplate fetches a CoreTemplate by name. +// If name is empty, it resolves using the Cluster Defaults, then the Namespace Default. +func (r *Resolver) ResolveCoreTemplate( + ctx context.Context, + name string, +) (*multigresv1alpha1.CoreTemplate, error) { + resolvedName := name + isImplicitFallback := false + + if resolvedName == "" { + resolvedName = r.TemplateDefaults.CoreTemplate + } + if resolvedName == "" { + resolvedName = FallbackCoreTemplate + isImplicitFallback = true + } + + tpl := &multigresv1alpha1.CoreTemplate{} + err := r.Client.Get(ctx, types.NamespacedName{Name: resolvedName, Namespace: r.Namespace}, tpl) + if err != nil { + if errors.IsNotFound(err) { + if isImplicitFallback { + // We return an empty struct instead of nil to satisfy tests expecting non-nil structure. + return &multigresv1alpha1.CoreTemplate{}, nil + } + return nil, fmt.Errorf("referenced CoreTemplate '%s' not found: %w", resolvedName, err) + } + return nil, fmt.Errorf("failed to get CoreTemplate: %w", err) + } + return tpl, nil } diff --git a/pkg/resolver/cluster_test.go b/pkg/resolver/cluster_test.go index 3af174dd..c9365b2f 100644 --- a/pkg/resolver/cluster_test.go +++ b/pkg/resolver/cluster_test.go @@ -2,13 +2,11 @@ package resolver import ( "errors" - "strings" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" @@ -29,7 +27,7 @@ func TestResolver_PopulateClusterDefaults(t *testing.T) { input *multigresv1alpha1.MultigresCluster want *multigresv1alpha1.MultigresCluster }{ - "Empty Cluster: Applies All Static Defaults": { + "Empty Cluster: Applies All System Defaults": { input: &multigresv1alpha1.MultigresCluster{}, want: &multigresv1alpha1.MultigresCluster{ Spec: multigresv1alpha1.MultigresClusterSpec{ @@ -46,7 +44,7 @@ func TestResolver_PopulateClusterDefaults(t *testing.T) { CellTemplate: FallbackCellTemplate, ShardTemplate: FallbackShardTemplate, }, - // Expect Smart Defaulting: System Catalog + // Expect Smart Defaulting: System Catalog with Shard 0 Databases: []multigresv1alpha1.DatabaseConfig{ { Name: DefaultSystemDatabaseName, @@ -55,6 +53,9 @@ func TestResolver_PopulateClusterDefaults(t *testing.T) { { Name: DefaultSystemTableGroupName, Default: true, + Shards: []multigresv1alpha1.ShardConfig{ + {Name: "0"}, + }, }, }, }, @@ -62,64 +63,11 @@ func TestResolver_PopulateClusterDefaults(t *testing.T) { }, }, }, - "Preserve Existing Values: Does Not Overwrite": { + "Existing Database but No TableGroups: Inject TG and Shard": { input: &multigresv1alpha1.MultigresCluster{ Spec: multigresv1alpha1.MultigresClusterSpec{ - Images: multigresv1alpha1.ClusterImages{ - Postgres: "custom-postgres", - MultiAdmin: "custom-admin", - MultiOrch: "custom-orch", - MultiPooler: "custom-pooler", - MultiGateway: "custom-gateway", - ImagePullPolicy: corev1.PullAlways, - }, - TemplateDefaults: multigresv1alpha1.TemplateDefaults{ - CoreTemplate: "custom-core", - CellTemplate: "custom-cell", - ShardTemplate: "custom-shard", - }, Databases: []multigresv1alpha1.DatabaseConfig{ - { - Name: "existing-db", - Default: true, - TableGroups: []multigresv1alpha1.TableGroupConfig{{Name: "tg1"}}, - }, - }, - }, - }, - want: &multigresv1alpha1.MultigresCluster{ - Spec: multigresv1alpha1.MultigresClusterSpec{ - Images: multigresv1alpha1.ClusterImages{ - Postgres: "custom-postgres", - MultiAdmin: "custom-admin", - MultiOrch: "custom-orch", - MultiPooler: "custom-pooler", - MultiGateway: "custom-gateway", - ImagePullPolicy: corev1.PullAlways, - }, - TemplateDefaults: multigresv1alpha1.TemplateDefaults{ - CoreTemplate: "custom-core", - CellTemplate: "custom-cell", - ShardTemplate: "custom-shard", - }, - Databases: []multigresv1alpha1.DatabaseConfig{ - { - Name: "existing-db", - Default: true, - TableGroups: []multigresv1alpha1.TableGroupConfig{{Name: "tg1"}}, - }, - }, - }, - }, - }, - "Inline Configs: Deep Defaulting (Etcd & Admin)": { - input: &multigresv1alpha1.MultigresCluster{ - Spec: multigresv1alpha1.MultigresClusterSpec{ - GlobalTopoServer: multigresv1alpha1.GlobalTopoServerSpec{ - Etcd: &multigresv1alpha1.EtcdSpec{}, - }, - MultiAdmin: multigresv1alpha1.MultiAdminConfig{ - Spec: &multigresv1alpha1.StatelessSpec{}, + {Name: "custom-db"}, }, }, }, @@ -138,44 +86,31 @@ func TestResolver_PopulateClusterDefaults(t *testing.T) { CellTemplate: FallbackCellTemplate, ShardTemplate: FallbackShardTemplate, }, - // Expect Smart Defaulting for DBs Databases: []multigresv1alpha1.DatabaseConfig{ { - Name: DefaultSystemDatabaseName, - Default: true, + Name: "custom-db", TableGroups: []multigresv1alpha1.TableGroupConfig{ { Name: DefaultSystemTableGroupName, Default: true, + Shards: []multigresv1alpha1.ShardConfig{ + {Name: "0"}, + }, }, }, }, }, - GlobalTopoServer: multigresv1alpha1.GlobalTopoServerSpec{ - Etcd: &multigresv1alpha1.EtcdSpec{ - Image: DefaultEtcdImage, - Replicas: ptr.To(DefaultEtcdReplicas), - Resources: DefaultResourcesEtcd(), - Storage: multigresv1alpha1.StorageSpec{Size: DefaultEtcdStorageSize}, - }, - }, - MultiAdmin: multigresv1alpha1.MultiAdminConfig{ - Spec: &multigresv1alpha1.StatelessSpec{ - Replicas: ptr.To(DefaultAdminReplicas), - Resources: DefaultResourcesAdmin(), - }, - }, }, }, }, - "Inline Configs: Deep Defaulting (Cells)": { + "Existing TableGroup but No Shards: Inject Shard 0": { input: &multigresv1alpha1.MultigresCluster{ Spec: multigresv1alpha1.MultigresClusterSpec{ - Cells: []multigresv1alpha1.CellConfig{ + Databases: []multigresv1alpha1.DatabaseConfig{ { - Name: "cell-1", - Spec: &multigresv1alpha1.CellInlineSpec{ - MultiGateway: multigresv1alpha1.StatelessSpec{}, + Name: "custom-db", + TableGroups: []multigresv1alpha1.TableGroupConfig{ + {Name: "my-tg"}, }, }, }, @@ -196,26 +131,15 @@ func TestResolver_PopulateClusterDefaults(t *testing.T) { CellTemplate: FallbackCellTemplate, ShardTemplate: FallbackShardTemplate, }, - // Expect Smart Defaulting for DBs Databases: []multigresv1alpha1.DatabaseConfig{ { - Name: DefaultSystemDatabaseName, - Default: true, + Name: "custom-db", TableGroups: []multigresv1alpha1.TableGroupConfig{ { - Name: DefaultSystemTableGroupName, - Default: true, - }, - }, - }, - }, - Cells: []multigresv1alpha1.CellConfig{ - { - Name: "cell-1", - Spec: &multigresv1alpha1.CellInlineSpec{ - MultiGateway: multigresv1alpha1.StatelessSpec{ - Replicas: ptr.To(int32(1)), - Resources: corev1.ResourceRequirements{}, + Name: "my-tg", + Shards: []multigresv1alpha1.ShardConfig{ + {Name: "0"}, + }, }, }, }, @@ -223,12 +147,20 @@ func TestResolver_PopulateClusterDefaults(t *testing.T) { }, }, }, - "Inline Configs: Partial Overrides Preserved": { + "Existing Shards: Do Not Inject": { input: &multigresv1alpha1.MultigresCluster{ Spec: multigresv1alpha1.MultigresClusterSpec{ - GlobalTopoServer: multigresv1alpha1.GlobalTopoServerSpec{ - Etcd: &multigresv1alpha1.EtcdSpec{ - Image: "custom-etcd", + Databases: []multigresv1alpha1.DatabaseConfig{ + { + Name: "custom-db", + TableGroups: []multigresv1alpha1.TableGroupConfig{ + { + Name: "my-tg", + Shards: []multigresv1alpha1.ShardConfig{ + {Name: "custom-shard"}, + }, + }, + }, }, }, }, @@ -248,61 +180,78 @@ func TestResolver_PopulateClusterDefaults(t *testing.T) { CellTemplate: FallbackCellTemplate, ShardTemplate: FallbackShardTemplate, }, - // Expect Smart Defaulting for DBs Databases: []multigresv1alpha1.DatabaseConfig{ { - Name: DefaultSystemDatabaseName, - Default: true, + Name: "custom-db", TableGroups: []multigresv1alpha1.TableGroupConfig{ { - Name: DefaultSystemTableGroupName, - Default: true, + Name: "my-tg", + Shards: []multigresv1alpha1.ShardConfig{ + {Name: "custom-shard"}, + }, }, }, }, }, - GlobalTopoServer: multigresv1alpha1.GlobalTopoServerSpec{ - Etcd: &multigresv1alpha1.EtcdSpec{ - Image: "custom-etcd", - Replicas: ptr.To(DefaultEtcdReplicas), - Resources: DefaultResourcesEtcd(), - Storage: multigresv1alpha1.StorageSpec{Size: DefaultEtcdStorageSize}, - }, - }, }, }, }, - "Smart Defaulting: Inject TableGroup when DB exists": { + // ADDED: This covers the "else" branches where we skip defaulting + "Pre-populated Fields: Preserves Values": { input: &multigresv1alpha1.MultigresCluster{ Spec: multigresv1alpha1.MultigresClusterSpec{ + Images: multigresv1alpha1.ClusterImages{ + Postgres: "custom/postgres:16", + MultiAdmin: "custom/admin:1", + MultiOrch: "custom/orch:1", + MultiPooler: "custom/pooler:1", + MultiGateway: "custom/gateway:1", + ImagePullPolicy: "Always", + }, + TemplateDefaults: multigresv1alpha1.TemplateDefaults{ + CoreTemplate: "custom-core", + CellTemplate: "custom-cell", + ShardTemplate: "custom-shard", + }, Databases: []multigresv1alpha1.DatabaseConfig{ - {Name: "postgres", Default: true}, // No TableGroups + { + Name: "custom-db", + TableGroups: []multigresv1alpha1.TableGroupConfig{ + { + Name: "custom-tg", + Shards: []multigresv1alpha1.ShardConfig{ + {Name: "custom-0"}, + }, + }, + }, + }, }, }, }, want: &multigresv1alpha1.MultigresCluster{ Spec: multigresv1alpha1.MultigresClusterSpec{ Images: multigresv1alpha1.ClusterImages{ - Postgres: DefaultPostgresImage, - MultiAdmin: DefaultMultiAdminImage, - MultiOrch: DefaultMultiOrchImage, - MultiPooler: DefaultMultiPoolerImage, - MultiGateway: DefaultMultiGatewayImage, - ImagePullPolicy: DefaultImagePullPolicy, + Postgres: "custom/postgres:16", + MultiAdmin: "custom/admin:1", + MultiOrch: "custom/orch:1", + MultiPooler: "custom/pooler:1", + MultiGateway: "custom/gateway:1", + ImagePullPolicy: "Always", }, TemplateDefaults: multigresv1alpha1.TemplateDefaults{ - CoreTemplate: FallbackCoreTemplate, - CellTemplate: FallbackCellTemplate, - ShardTemplate: FallbackShardTemplate, + CoreTemplate: "custom-core", + CellTemplate: "custom-cell", + ShardTemplate: "custom-shard", }, Databases: []multigresv1alpha1.DatabaseConfig{ { - Name: "postgres", - Default: true, + Name: "custom-db", TableGroups: []multigresv1alpha1.TableGroupConfig{ { - Name: DefaultSystemTableGroupName, - Default: true, + Name: "custom-tg", + Shards: []multigresv1alpha1.ShardConfig{ + {Name: "custom-0"}, + }, }, }, }, @@ -325,125 +274,27 @@ func TestResolver_PopulateClusterDefaults(t *testing.T) { } } -func TestResolver_ResolveCoreTemplate(t *testing.T) { +func TestResolver_ResolveGlobalTopo(t *testing.T) { t.Parallel() scheme := runtime.NewScheme() _ = multigresv1alpha1.AddToScheme(scheme) - coreTpl, _, _, ns := setupFixtures(t) - customCore := coreTpl.DeepCopy() - customCore.Name = "custom-core" tests := map[string]struct { - existingObjects []client.Object - defaults multigresv1alpha1.TemplateDefaults - reqName string - wantErr bool - errContains string - wantFound bool - wantResName string + cluster *multigresv1alpha1.MultigresCluster + objects []client.Object + want *multigresv1alpha1.GlobalTopoServerSpec + wantErr bool }{ - "Explicit Found": { - existingObjects: []client.Object{customCore}, - reqName: "custom-core", - wantFound: true, - wantResName: "custom-core", - }, - "Explicit Not Found (Error)": { - existingObjects: []client.Object{}, - reqName: "missing-core", - wantErr: true, - errContains: "referenced CoreTemplate 'missing-core' not found", - }, - "Default from Config Found": { - existingObjects: []client.Object{customCore}, - defaults: multigresv1alpha1.TemplateDefaults{CoreTemplate: "custom-core"}, - reqName: "", - wantFound: true, - wantResName: "custom-core", - }, - "Default from Config Not Found (Error)": { - existingObjects: []client.Object{}, - defaults: multigresv1alpha1.TemplateDefaults{CoreTemplate: "missing"}, - reqName: "", - wantErr: true, - errContains: "referenced CoreTemplate 'missing' not found", - }, - "Implicit Fallback Found": { - existingObjects: []client.Object{coreTpl}, - defaults: multigresv1alpha1.TemplateDefaults{}, - reqName: "", - wantFound: true, - wantResName: "default", - }, - "Implicit Fallback Not Found (Safe Empty Return)": { - existingObjects: []client.Object{}, - defaults: multigresv1alpha1.TemplateDefaults{}, - reqName: "", - wantFound: false, - wantErr: false, - }, - } - - for name, tc := range tests { - t.Run(name, func(t *testing.T) { - t.Parallel() - c := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(tc.existingObjects...). - Build() - r := NewResolver(c, ns, tc.defaults) - - res, err := r.ResolveCoreTemplate(t.Context(), tc.reqName) - if tc.wantErr { - if err == nil { - t.Fatal("Expected error, got nil") - } - if tc.errContains != "" && !strings.Contains(err.Error(), tc.errContains) { - t.Errorf( - "Error message mismatch: got %q, want substring %q", - err.Error(), - tc.errContains, - ) - } - return - } else if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - - if !tc.wantFound { - if res == nil { - t.Fatal( - "Expected non-nil result structure even for not-found implicit fallback", - ) - } - if res.GetName() != "" { - t.Errorf("Expected empty result, got object with name %q", res.GetName()) - } - return - } - - if got, want := res.GetName(), tc.wantResName; got != want { - t.Errorf("Result name mismatch: got %q, want %q", got, want) - } - }) - } -} - -func TestResolveGlobalTopo(t *testing.T) { - t.Parallel() - - tests := map[string]struct { - spec *multigresv1alpha1.GlobalTopoServerSpec - tpl *multigresv1alpha1.CoreTemplate - want *multigresv1alpha1.GlobalTopoServerSpec - }{ - "Inline Priority (Etcd)": { - spec: &multigresv1alpha1.GlobalTopoServerSpec{ - Etcd: &multigresv1alpha1.EtcdSpec{Image: "inline"}, + "Inline Takes Precedence": { + cluster: &multigresv1alpha1.MultigresCluster{ + Spec: multigresv1alpha1.MultigresClusterSpec{ + GlobalTopoServer: multigresv1alpha1.GlobalTopoServerSpec{ + Etcd: &multigresv1alpha1.EtcdSpec{Image: "inline"}, + }, + }, }, - tpl: nil, want: &multigresv1alpha1.GlobalTopoServerSpec{ Etcd: &multigresv1alpha1.EtcdSpec{ Image: "inline", @@ -453,56 +304,45 @@ func TestResolveGlobalTopo(t *testing.T) { }, }, }, - "Inline Priority (External)": { - spec: &multigresv1alpha1.GlobalTopoServerSpec{ - External: &multigresv1alpha1.ExternalTopoServerSpec{ - Endpoints: []multigresv1alpha1.EndpointUrl{"http://foo"}, - }, - }, - tpl: nil, - want: &multigresv1alpha1.GlobalTopoServerSpec{ - External: &multigresv1alpha1.ExternalTopoServerSpec{ - Endpoints: []multigresv1alpha1.EndpointUrl{"http://foo"}, - }, - }, - }, - "Template Fallback": { - spec: &multigresv1alpha1.GlobalTopoServerSpec{}, - tpl: &multigresv1alpha1.CoreTemplate{ - Spec: multigresv1alpha1.CoreTemplateSpec{ - GlobalTopoServer: &multigresv1alpha1.TopoServerSpec{ - Etcd: &multigresv1alpha1.EtcdSpec{Image: "template"}, + "Template Reference": { + cluster: &multigresv1alpha1.MultigresCluster{ + Spec: multigresv1alpha1.MultigresClusterSpec{ + GlobalTopoServer: multigresv1alpha1.GlobalTopoServerSpec{ + TemplateRef: "default", }, }, }, + objects: []client.Object{coreTpl}, want: &multigresv1alpha1.GlobalTopoServerSpec{ Etcd: &multigresv1alpha1.EtcdSpec{ - Image: "template", + Image: "core-default", // From fixture Replicas: ptr.To(DefaultEtcdReplicas), Resources: DefaultResourcesEtcd(), Storage: multigresv1alpha1.StorageSpec{Size: DefaultEtcdStorageSize}, }, }, }, - "Template Found but Nil Content": { - spec: &multigresv1alpha1.GlobalTopoServerSpec{}, - tpl: &multigresv1alpha1.CoreTemplate{ - Spec: multigresv1alpha1.CoreTemplateSpec{ - GlobalTopoServer: nil, + "Cluster Default Template": { + cluster: &multigresv1alpha1.MultigresCluster{ + Spec: multigresv1alpha1.MultigresClusterSpec{ + TemplateDefaults: multigresv1alpha1.TemplateDefaults{ + CoreTemplate: "default", + }, }, }, + objects: []client.Object{coreTpl}, want: &multigresv1alpha1.GlobalTopoServerSpec{ Etcd: &multigresv1alpha1.EtcdSpec{ - Image: DefaultEtcdImage, + Image: "core-default", Replicas: ptr.To(DefaultEtcdReplicas), Resources: DefaultResourcesEtcd(), Storage: multigresv1alpha1.StorageSpec{Size: DefaultEtcdStorageSize}, }, }, }, - "No-op (Nil Template)": { - spec: &multigresv1alpha1.GlobalTopoServerSpec{}, - tpl: nil, + "Fallback (Implicit Default Not Found) -> Default Etcd": { + cluster: &multigresv1alpha1.MultigresCluster{}, + objects: nil, want: &multigresv1alpha1.GlobalTopoServerSpec{ Etcd: &multigresv1alpha1.EtcdSpec{ Image: DefaultEtcdImage, @@ -512,64 +352,97 @@ func TestResolveGlobalTopo(t *testing.T) { }, }, }, + "Missing Explicit Template -> Error": { + cluster: &multigresv1alpha1.MultigresCluster{ + Spec: multigresv1alpha1.MultigresClusterSpec{ + GlobalTopoServer: multigresv1alpha1.GlobalTopoServerSpec{ + TemplateRef: "missing", + }, + }, + }, + wantErr: true, + }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { t.Parallel() - got := ResolveGlobalTopo(tc.spec, tc.tpl) + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.objects...).Build() + r := NewResolver(c, ns, tc.cluster.Spec.TemplateDefaults) + + got, err := r.ResolveGlobalTopo(t.Context(), tc.cluster) + if tc.wantErr { + if err == nil { + t.Error("Expected error, got nil") + } + return + } + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } if diff := cmp.Diff(tc.want, got, cmpopts.IgnoreUnexported(resource.Quantity{}), cmpopts.EquateEmpty()); diff != "" { - t.Errorf("ResolveGlobalTopo mismatch (-want +got):\n%s", diff) + t.Errorf("Diff (-want +got):\n%s", diff) } }) } } -func TestResolveMultiAdmin(t *testing.T) { +func TestResolver_ResolveMultiAdmin(t *testing.T) { t.Parallel() + scheme := runtime.NewScheme() + _ = multigresv1alpha1.AddToScheme(scheme) + coreTpl, _, _, ns := setupFixtures(t) tests := map[string]struct { - spec *multigresv1alpha1.MultiAdminConfig - tpl *multigresv1alpha1.CoreTemplate - want *multigresv1alpha1.StatelessSpec + cluster *multigresv1alpha1.MultigresCluster + objects []client.Object + want *multigresv1alpha1.StatelessSpec + wantErr bool }{ - "Inline Priority": { - spec: &multigresv1alpha1.MultiAdminConfig{ - Spec: &multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(5))}, + "Inline": { + cluster: &multigresv1alpha1.MultigresCluster{ + Spec: multigresv1alpha1.MultigresClusterSpec{ + MultiAdmin: multigresv1alpha1.MultiAdminConfig{ + Spec: &multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(10))}, + }, + }, }, - tpl: nil, want: &multigresv1alpha1.StatelessSpec{ - Replicas: ptr.To(int32(5)), + Replicas: ptr.To(int32(10)), Resources: DefaultResourcesAdmin(), }, }, - "Template Fallback": { - spec: &multigresv1alpha1.MultiAdminConfig{}, - tpl: &multigresv1alpha1.CoreTemplate{ - Spec: multigresv1alpha1.CoreTemplateSpec{ - MultiAdmin: &multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(3))}, + "Template": { + cluster: &multigresv1alpha1.MultigresCluster{ + Spec: multigresv1alpha1.MultigresClusterSpec{ + MultiAdmin: multigresv1alpha1.MultiAdminConfig{ + TemplateRef: "default", + }, }, }, + objects: []client.Object{coreTpl}, want: &multigresv1alpha1.StatelessSpec{ - Replicas: ptr.To(int32(3)), + // No Image, uses global + Replicas: ptr.To(DefaultAdminReplicas), Resources: DefaultResourcesAdmin(), }, }, - "Template Found but Nil Content": { - spec: &multigresv1alpha1.MultiAdminConfig{}, - tpl: &multigresv1alpha1.CoreTemplate{ - Spec: multigresv1alpha1.CoreTemplateSpec{ - MultiAdmin: nil, + "Missing Template": { + cluster: &multigresv1alpha1.MultigresCluster{ + Spec: multigresv1alpha1.MultigresClusterSpec{ + MultiAdmin: multigresv1alpha1.MultiAdminConfig{TemplateRef: "gone"}, }, }, - want: &multigresv1alpha1.StatelessSpec{ - Replicas: ptr.To(DefaultAdminReplicas), - Resources: DefaultResourcesAdmin(), - }, + wantErr: true, }, - "No-op (Nil Template)": { - spec: &multigresv1alpha1.MultiAdminConfig{}, - tpl: nil, + // ADDED: This covers the "else" fallback path in ResolveMultiAdmin + "Fallback (No Spec, No Template) -> Defaults": { + cluster: &multigresv1alpha1.MultigresCluster{ + Spec: multigresv1alpha1.MultigresClusterSpec{ + MultiAdmin: multigresv1alpha1.MultiAdminConfig{}, // Empty + }, + }, + objects: nil, // No core template found want: &multigresv1alpha1.StatelessSpec{ Replicas: ptr.To(DefaultAdminReplicas), Resources: DefaultResourcesAdmin(), @@ -580,9 +453,21 @@ func TestResolveMultiAdmin(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { t.Parallel() - got := ResolveMultiAdmin(tc.spec, tc.tpl) + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.objects...).Build() + r := NewResolver(c, ns, tc.cluster.Spec.TemplateDefaults) + + got, err := r.ResolveMultiAdmin(t.Context(), tc.cluster) + if tc.wantErr { + if err == nil { + t.Error("Expected error") + } + return + } + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } if diff := cmp.Diff(tc.want, got, cmpopts.IgnoreUnexported(resource.Quantity{}), cmpopts.EquateEmpty()); diff != "" { - t.Errorf("ResolveMultiAdmin mismatch (-want +got):\n%s", diff) + t.Errorf("Diff (-want +got):\n%s", diff) } }) } diff --git a/pkg/resolver/doc.go b/pkg/resolver/doc.go index 554fc175..996fe58f 100644 --- a/pkg/resolver/doc.go +++ b/pkg/resolver/doc.go @@ -3,10 +3,9 @@ // In the Multigres Operator, a cluster's configuration can come from multiple sources: // 1. Inline Configurations (defined directly in the MultigresCluster CR). // 2. Template References (pointing to CoreTemplate, CellTemplate, or ShardTemplate CRs). -// 3. Overrides (partial patches applied on top of a template). -// 4. Hardcoded Defaults (fallback values for safety). +// 3. Hardcoded Defaults (fallback values for safety). // -// The Resolver is the single source of truth for merging these sources. It ensures that +// The Resolver is the single source of truth for determining the final configuration. It ensures that // the Reconciler and the Webhook always agree on what the final configuration should be. // // # Logic Hierarchy @@ -14,10 +13,9 @@ // When calculating the final configuration for a component, the Resolver applies the // following precedence (highest to lowest): // -// 1. Inline Spec (if provided, it ignores templates entirely). -// 2. Overrides (patches specific fields of the template). -// 3. Template Spec (the base configuration from the referenced CR). -// 4. Global Defaults (hardcoded constants like default images or replicas). +// 1. Inline Spec (if provided, it takes precedence over the template). +// 2. Template Spec (the base configuration from the referenced CR). +// 3. Global Defaults (hardcoded constants like default images or replicas). // // # Dual Usage // @@ -25,23 +23,24 @@ // // 1. Mutation (Webhook): // The 'PopulateClusterDefaults' method is safe to call during admission. It applies -// static defaults (images, explicit template names) to the API object itself, -// solving the "Invisible Defaults" problem without making external API calls. +// static defaults (images) and "Smart Defaults" (injecting mandatory system +// databases, table groups, and shards) to the API object itself. This solves the +// "Invisible Defaults" problem without making external API calls. // // 2. Reconciliation (Controller): -// The 'Resolve...' and 'Merge...' methods are used during reconciliation. They -// fetch external templates from the API server and merge them to determine the -// actual state the child resources (StatefulSets, Services) should match. +// The 'Resolve...' methods (e.g., ResolveGlobalTopo) are used during reconciliation. +// They fetch external templates from the API server and determine the effective +// configuration by checking for inline definitions vs. template references. // // Usage: // // // Create a resolver // res := resolver.NewResolver(client, namespace, cluster.Spec.TemplateDefaults) // -// // Webhook: Apply static defaults to the object +// // Webhook: Apply static and smart defaults to the object // res.PopulateClusterDefaults(cluster) // // // Controller: Calculate final config for a specific component -// template, err := res.ResolveShardTemplate(ctx, "my-shard-template") -// finalConfig := resolver.MergeShardConfig(template, overrides, nil) +// // This handles the Inline > Template > Default precedence logic +// globalTopoSpec, err := res.ResolveGlobalTopo(ctx, cluster) package resolver diff --git a/pkg/resolver/go.sum b/pkg/resolver/go.sum index 295eea9f..0a86a0e5 100644 --- a/pkg/resolver/go.sum +++ b/pkg/resolver/go.sum @@ -80,8 +80,6 @@ github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee h1:W5t00kpgFd github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= -github.com/numtide/multigres-operator/api v0.0.0-20251224124005-355869230728 h1:a+iM4L0nC7kUgcAD1P+wuNJg4x0rjcy6rVZ3+vF52Cc= -github.com/numtide/multigres-operator/api v0.0.0-20251224124005-355869230728/go.mod h1:A1bBmTxHr+362dGZ5G6u2S4xsP6enbgdUS/UJUOmKbc= github.com/numtide/multigres-operator/api v0.0.0-20260103121224-057738b43b3b h1:twErev/AdyVQvlYa5vbo5wqq+NlexZJmIm0pwEQd5qI= github.com/numtide/multigres-operator/api v0.0.0-20260103121224-057738b43b3b/go.mod h1:A1bBmTxHr+362dGZ5G6u2S4xsP6enbgdUS/UJUOmKbc= github.com/onsi/ginkgo/v2 v2.25.3 h1:Ty8+Yi/ayDAGtk4XxmmfUy4GabvM+MegeB4cDLRi6nw= diff --git a/pkg/resolver/resolver.go b/pkg/resolver/resolver.go index e01e5ba5..0cbc05f6 100644 --- a/pkg/resolver/resolver.go +++ b/pkg/resolver/resolver.go @@ -1,14 +1,10 @@ -// Package resolver provides the central logic for resolving MultigresCluster -// defaults, templates, and configurations. -// -// It serves as the single source of truth for merging user inputs, -// cluster-level defaults, and external templates into a final resource specification. package resolver import ( "sigs.k8s.io/controller-runtime/pkg/client" multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1" + corev1 "k8s.io/api/core/v1" ) // Resolver handles the logic for fetching templates and calculating defaults. @@ -70,3 +66,50 @@ func mergeStatelessSpec( base.PodLabels[k] = v } } + +// isResourcesZero checks if the resource requirements are strictly the zero value (nil maps). +// This mimics reflect.DeepEqual(res, corev1.ResourceRequirements{}) but is safer and faster. +// It is used for merging logic where we want to distinguish "inherit" (nil) from "empty" (set to empty). +func isResourcesZero(res corev1.ResourceRequirements) bool { + return res.Requests == nil && res.Limits == nil && res.Claims == nil +} + +// ============================================================================ +// Shared Defaulting Helpers +// ============================================================================ + +// defaultEtcdSpec applies hardcoded safety defaults to an inline Etcd spec. +func defaultEtcdSpec(spec *multigresv1alpha1.EtcdSpec) { + if spec.Image == "" { + spec.Image = DefaultEtcdImage + } + if spec.Storage.Size == "" { + spec.Storage.Size = DefaultEtcdStorageSize + } + if spec.Replicas == nil { + r := DefaultEtcdReplicas + spec.Replicas = &r + } + // Use isResourcesZero to ensure we respect overrides that only have Claims + if isResourcesZero(spec.Resources) { + // Safety: DefaultResourcesEtcd() returns a fresh struct, so no DeepCopy needed. + spec.Resources = DefaultResourcesEtcd() + } +} + +// defaultStatelessSpec applies hardcoded safety defaults to any stateless spec. +func defaultStatelessSpec( + spec *multigresv1alpha1.StatelessSpec, + defaultRes corev1.ResourceRequirements, + defaultReplicas int32, +) { + if spec.Replicas == nil { + spec.Replicas = &defaultReplicas + } + // Use isResourcesZero to ensure we respect overrides that only have Claims + if isResourcesZero(spec.Resources) { + // Safety: We assume defaultRes is passed by value (a fresh copy from the default function). + // We perform a DeepCopy to ensure spec.Resources owns its own maps, independent of the input defaultRes. + spec.Resources = *defaultRes.DeepCopy() + } +} diff --git a/pkg/resolver/resolver_test.go b/pkg/resolver/resolver_test.go index 20870a89..0e0336ab 100644 --- a/pkg/resolver/resolver_test.go +++ b/pkg/resolver/resolver_test.go @@ -4,7 +4,9 @@ import ( "context" "testing" + "github.com/google/go-cmp/cmp" multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -30,6 +32,9 @@ func setupFixtures(t testing.TB) ( GlobalTopoServer: &multigresv1alpha1.TopoServerSpec{ Etcd: &multigresv1alpha1.EtcdSpec{Image: "core-default"}, }, + MultiAdmin: &multigresv1alpha1.StatelessSpec{ + // Image is not in StatelessSpec, it is global + }, }, } @@ -39,6 +44,9 @@ func setupFixtures(t testing.TB) ( MultiGateway: &multigresv1alpha1.StatelessSpec{ Replicas: ptr.To(int32(1)), }, + LocalTopoServer: &multigresv1alpha1.LocalTopoServerSpec{ + Etcd: &multigresv1alpha1.EtcdSpec{Image: "local-etcd-default"}, + }, }, } @@ -46,7 +54,9 @@ func setupFixtures(t testing.TB) ( ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: namespace}, Spec: multigresv1alpha1.ShardTemplateSpec{ MultiOrch: &multigresv1alpha1.MultiOrchSpec{ - StatelessSpec: multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(1))}, + StatelessSpec: multigresv1alpha1.StatelessSpec{ + Replicas: ptr.To(int32(1)), + }, }, }, } @@ -72,6 +82,97 @@ func TestNewResolver(t *testing.T) { } } +func TestSharedHelpers(t *testing.T) { + t.Parallel() + + t.Run("isResourcesZero", func(t *testing.T) { + tests := []struct { + name string + res corev1.ResourceRequirements + want bool + }{ + {"Zero", corev1.ResourceRequirements{}, true}, + {"Requests Set", corev1.ResourceRequirements{Requests: corev1.ResourceList{}}, false}, + {"Limits Set", corev1.ResourceRequirements{Limits: corev1.ResourceList{}}, false}, + {"Claims Set", corev1.ResourceRequirements{Claims: []corev1.ResourceClaim{}}, false}, + } + for _, tc := range tests { + if got := isResourcesZero(tc.res); got != tc.want { + t.Errorf("%s: got %v, want %v", tc.name, got, tc.want) + } + } + }) + + t.Run("defaultEtcdSpec", func(t *testing.T) { + spec := &multigresv1alpha1.EtcdSpec{} + defaultEtcdSpec(spec) + + if spec.Image != DefaultEtcdImage { + t.Errorf("Image: got %q, want %q", spec.Image, DefaultEtcdImage) + } + if spec.Storage.Size != DefaultEtcdStorageSize { + t.Errorf("Storage: got %q, want %q", spec.Storage.Size, DefaultEtcdStorageSize) + } + if *spec.Replicas != DefaultEtcdReplicas { + t.Errorf("Replicas: got %d, want %d", *spec.Replicas, DefaultEtcdReplicas) + } + if isResourcesZero(spec.Resources) { + t.Error("Resources should be defaulted") + } + + // Test Preservation + spec2 := &multigresv1alpha1.EtcdSpec{Image: "custom"} + defaultEtcdSpec(spec2) + if spec2.Image != "custom" { + t.Error("Should preserve existing image") + } + }) + + t.Run("defaultStatelessSpec", func(t *testing.T) { + spec := &multigresv1alpha1.StatelessSpec{} + res := corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceCPU: parseQty("1")}, + } + defaultStatelessSpec(spec, res, 5) + + if *spec.Replicas != 5 { + t.Errorf("Replicas: got %d, want 5", *spec.Replicas) + } + if cmp.Diff(spec.Resources, res) != "" { + t.Error("Resources not copied correctly") + } + + // Test DeepCopy independence + res.Requests[corev1.ResourceCPU] = parseQty("999") + // Cannot call String() directly on map index result in one line effectively if we want addressability + // But String() is on *Quantity usually? Actually Quantity is a struct. + // Wait, Requests[...] returns a Value. + // We need to capture it to check it. + val := spec.Resources.Requests[corev1.ResourceCPU] + if val.String() == "999" { + t.Error("Shared pointer detected in defaultStatelessSpec") + } + }) + + t.Run("mergeStatelessSpec", func(t *testing.T) { + base := &multigresv1alpha1.StatelessSpec{ + PodAnnotations: map[string]string{"a": "1"}, + } + override := &multigresv1alpha1.StatelessSpec{ + PodAnnotations: map[string]string{"b": "2"}, + Replicas: ptr.To(int32(3)), + } + mergeStatelessSpec(base, override) + + if len(base.PodAnnotations) != 2 { + t.Errorf("Map merge failed, got %v", base.PodAnnotations) + } + if *base.Replicas != 3 { + t.Error("Replicas not merged") + } + }) +} + // mockClient is a partial implementation of client.Client to force errors. type mockClient struct { client.Client diff --git a/pkg/resolver/shard.go b/pkg/resolver/shard.go index be1766fc..6fff4418 100644 --- a/pkg/resolver/shard.go +++ b/pkg/resolver/shard.go @@ -8,40 +8,69 @@ import ( "k8s.io/apimachinery/pkg/types" multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1" + corev1 "k8s.io/api/core/v1" ) -// ResolveShardTemplate fetches and resolves a ShardTemplate by name, handling defaults. +// ResolveShard determines the final configuration for a specific Shard. +// It orchestrates: Template Lookup -> Fetch -> Merge -> Defaulting. +func (r *Resolver) ResolveShard( + ctx context.Context, + shardSpec *multigresv1alpha1.ShardConfig, +) (*multigresv1alpha1.MultiOrchSpec, map[string]multigresv1alpha1.PoolSpec, error) { + // 1. Fetch Template (Logic handles defaults) + templateName := shardSpec.ShardTemplate + tpl, err := r.ResolveShardTemplate(ctx, templateName) + if err != nil { + return nil, nil, err + } + + // 2. Merge Logic + multiOrch, pools := mergeShardConfig(tpl, shardSpec.Overrides, shardSpec.Spec) + + // 3. Apply Deep Defaults (Level 4) + defaultStatelessSpec(&multiOrch.StatelessSpec, corev1.ResourceRequirements{}, 1) + + // Note: We do not apply strict defaults to Pools here yet, + // as Pool defaults are often highly context-specific (storage class, etc). + // However, we could apply safety defaults if needed. + + return &multiOrch, pools, nil +} + +// ResolveShardTemplate fetches and resolves a ShardTemplate by name. +// If name is empty, it resolves using the Cluster Defaults, then the Namespace Default. func (r *Resolver) ResolveShardTemplate( ctx context.Context, - templateName string, + name string, ) (*multigresv1alpha1.ShardTemplate, error) { - name := templateName + resolvedName := name isImplicitFallback := false - if name == "" { - name = r.TemplateDefaults.ShardTemplate + if resolvedName == "" { + resolvedName = r.TemplateDefaults.ShardTemplate } - if name == "" { - name = FallbackShardTemplate + if resolvedName == "" { + resolvedName = FallbackShardTemplate isImplicitFallback = true } tpl := &multigresv1alpha1.ShardTemplate{} - err := r.Client.Get(ctx, types.NamespacedName{Name: name, Namespace: r.Namespace}, tpl) + err := r.Client.Get(ctx, types.NamespacedName{Name: resolvedName, Namespace: r.Namespace}, tpl) if err != nil { if errors.IsNotFound(err) { if isImplicitFallback { + // We return an empty struct instead of nil to satisfy tests expecting non-nil structure. return &multigresv1alpha1.ShardTemplate{}, nil } - return nil, fmt.Errorf("referenced ShardTemplate '%s' not found: %w", name, err) + return nil, fmt.Errorf("referenced ShardTemplate '%s' not found: %w", resolvedName, err) } return nil, fmt.Errorf("failed to get ShardTemplate: %w", err) } return tpl, nil } -// MergeShardConfig merges a template spec with overrides and an inline spec to produce the final configuration. -func MergeShardConfig( +// mergeShardConfig merges a template spec with overrides and an inline spec. +func mergeShardConfig( template *multigresv1alpha1.ShardTemplate, overrides *multigresv1alpha1.ShardOverrides, inline *multigresv1alpha1.ShardInlineSpec, diff --git a/pkg/resolver/shard_test.go b/pkg/resolver/shard_test.go index 11e28d61..79557c39 100644 --- a/pkg/resolver/shard_test.go +++ b/pkg/resolver/shard_test.go @@ -16,6 +16,80 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +func TestResolver_ResolveShard(t *testing.T) { + t.Parallel() + scheme := runtime.NewScheme() + _ = multigresv1alpha1.AddToScheme(scheme) + _, _, shardTpl, ns := setupFixtures(t) + + tests := map[string]struct { + config *multigresv1alpha1.ShardConfig + objects []client.Object + wantOrch *multigresv1alpha1.MultiOrchSpec + wantPools map[string]multigresv1alpha1.PoolSpec + wantErr bool + }{ + "Template Found": { + config: &multigresv1alpha1.ShardConfig{ShardTemplate: "default"}, + objects: []client.Object{shardTpl}, + wantOrch: &multigresv1alpha1.MultiOrchSpec{ + StatelessSpec: multigresv1alpha1.StatelessSpec{ + Replicas: ptr.To(int32(1)), + Resources: corev1.ResourceRequirements{}, + }, + }, + wantPools: map[string]multigresv1alpha1.PoolSpec{}, + }, + "Template Not Found": { + config: &multigresv1alpha1.ShardConfig{ShardTemplate: "missing"}, + wantErr: true, + }, + "Inline Overrides": { + config: &multigresv1alpha1.ShardConfig{ + Spec: &multigresv1alpha1.ShardInlineSpec{ + MultiOrch: multigresv1alpha1.MultiOrchSpec{ + StatelessSpec: multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(5))}, + }, + Pools: map[string]multigresv1alpha1.PoolSpec{"p": {}}, + }, + }, + wantOrch: &multigresv1alpha1.MultiOrchSpec{ + StatelessSpec: multigresv1alpha1.StatelessSpec{ + Replicas: ptr.To(int32(5)), + Resources: corev1.ResourceRequirements{}, + }, + }, + wantPools: map[string]multigresv1alpha1.PoolSpec{"p": {}}, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.objects...).Build() + r := NewResolver(c, ns, multigresv1alpha1.TemplateDefaults{}) + + orch, pools, err := r.ResolveShard(t.Context(), tc.config) + if tc.wantErr { + if err == nil { + t.Error("Expected error") + } + return + } + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if diff := cmp.Diff(tc.wantOrch, orch, cmpopts.IgnoreUnexported(resource.Quantity{}), cmpopts.EquateEmpty()); diff != "" { + t.Errorf("Orch Diff (-want +got):\n%s", diff) + } + if diff := cmp.Diff(tc.wantPools, pools, cmpopts.IgnoreUnexported(resource.Quantity{}), cmpopts.EquateEmpty()); diff != "" { + t.Errorf("Pools Diff (-want +got):\n%s", diff) + } + }) + } +} + func TestResolver_ResolveShardTemplate(t *testing.T) { t.Parallel() @@ -291,7 +365,7 @@ func TestMergeShardConfig(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { t.Parallel() - orch, pools := MergeShardConfig(tc.tpl, tc.overrides, tc.inline) + orch, pools := mergeShardConfig(tc.tpl, tc.overrides, tc.inline) if diff := cmp.Diff(tc.wantOrch, orch, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { t.Errorf("Orch mismatch (-want +got):\n%s", diff)