diff --git a/.golangci.json b/.golangci.json index c2b46fa50..20d188a26 100644 --- a/.golangci.json +++ b/.golangci.json @@ -38,6 +38,10 @@ "std-error-handling" ], "rules": [ + { + "path": "func/evaluator/.*", + "text": "ST1001" + }, { "linters": ["staticcheck"], "text": "SA1019.*client\\.Apply" diff --git a/api/porchconfig/v1alpha1/function_config_types.go b/api/porchconfig/v1alpha1/function_config_types.go index 03c8a2d35..d5f28da0f 100644 --- a/api/porchconfig/v1alpha1/function_config_types.go +++ b/api/porchconfig/v1alpha1/function_config_types.go @@ -15,6 +15,8 @@ package v1alpha1 import ( + "iter" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -65,6 +67,14 @@ type FunctionConfigStatus struct { Error string `json:"error,omitempty"` } +// TagIterable is a simple interface to allow code deduplication when storing the Tags field of each config +// +// +kubebuilder:object:generate=false +// TODO: should this be here, in the api, or just unexported in store.go? +type TagIterable interface { + IterTags() iter.Seq[string] +} + type PodExecutorConfig struct { // Image tags which the pod executor configuration will be applied to. // If tags is empty, the configuration will apply to all pods created for the image. TODO: this is not implemented @@ -79,6 +89,19 @@ type PodExecutorConfig struct { // TODO: add warmup section } +func (conf *PodExecutorConfig) IterTags() iter.Seq[string] { + return func(yield func(string) bool) { + if conf == nil { + return + } + for _, tag := range conf.Tags { + if !yield(tag) { + return + } + } + } +} + type TemplateOverrides struct { ServiceAccountName string `json:"serviceAccountName,omitempty"` SecurityContext *corev1.PodSecurityContext `json:"securityContext,omitempty"` @@ -101,6 +124,19 @@ type BinaryExecutorConfig struct { Path string `json:"path"` } +func (conf *BinaryExecutorConfig) IterTags() iter.Seq[string] { + return func(yield func(string) bool) { + if conf == nil { + return + } + for _, tag := range conf.Tags { + if !yield(tag) { + return + } + } + } +} + type GoExecutorConfig struct { // Image tags which can be substituted with a go function call. // +kubebuilder:validation:MinItems=1 @@ -109,3 +145,16 @@ type GoExecutorConfig struct { // If empty, `.spec.image` will be used instead. ID *string `json:"id,omitempty"` } + +func (conf *GoExecutorConfig) IterTags() iter.Seq[string] { + return func(yield func(string) bool) { + if conf == nil { + return + } + for _, tag := range conf.Tags { + if !yield(tag) { + return + } + } + } +} diff --git a/controllers/functionconfigs/reconciler.go b/controllers/functionconfigs/reconciler.go new file mode 100644 index 000000000..e2f98ed14 --- /dev/null +++ b/controllers/functionconfigs/reconciler.go @@ -0,0 +1,159 @@ +// Copyright 2026 The kpt 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 functionconfigs + +import ( + "context" + + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/klog/v2" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +const ( + BaseFinalizer = "config.porch.kpt.dev/functionconfig" + ServerFinalizer = BaseFinalizer + "-porch-server" + FunctionRunnerFinalizer = BaseFinalizer + "-function-runner" + ControllerFinalizer = BaseFinalizer + "-controller" +) + +type ReconcilerFor string + +const ( + ReconcilerForFunctionRunner ReconcilerFor = "function-runner" + ReconcilerForServer ReconcilerFor = "server" + ReconcilerForController ReconcilerFor = "controller" +) + +type FunctionConfigReconciler struct { + Client client.Client + FunctionConfigStore *FunctionConfigStore + // For indicates which component the reconciler is collecting the configs for + // TODO: remove after merging of function-runner into server + For ReconcilerFor +} + +func (r *FunctionConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, finalErr error) { + klog.Infof("FunctionConfig %q changed", req.NamespacedName) + obj := &configapi.FunctionConfig{} + err := r.Client.Get(ctx, req.NamespacedName, obj) + if apierrors.IsNotFound(err) { + r.FunctionConfigStore.DeleteByObjName(req.NamespacedName) + return ctrl.Result{}, nil + } + if err != nil { + return ctrl.Result{}, err + } + + if obj.DeletionTimestamp != nil { + if err := r.removeFinalizer(ctx, obj); err != nil { + return ctrl.Result{}, err + } + + r.FunctionConfigStore.Delete(obj.Spec.Image) + return ctrl.Result{}, nil + } + + if err := r.addFinalizer(ctx, obj); err != nil { + return ctrl.Result{}, err + } + + defer func() { + patch := client.MergeFrom(obj.DeepCopy()) + + if finalErr != nil { + obj.Status.Error = finalErr.Error() + } else { + obj.Status.Error = "" + switch r.For { + case ReconcilerForFunctionRunner: + obj.Status.FunctionRunnerObservedGeneration = obj.Generation + case ReconcilerForServer: + obj.Status.ApiServerObservedGeneration = obj.Generation + case ReconcilerForController: + obj.Status.ControllerObservedGeneration = obj.Generation + } + } + + if err := r.Client.Status().Patch(ctx, obj, patch); err != nil { + klog.Errorf("Failed to update status of FunctionConfig %q: %v", obj.Name, err) + if finalErr == nil { + finalErr = err + } + } + }() + + if err := r.FunctionConfigStore.Store(obj); err != nil { + klog.Errorf("Failed to store FunctionConfig %q: %v", obj.Name, err) + // TODO: we shouldn't have a requeue loop here if we can't insert into the cache, but if the user deletes the + // conflicting config, then this one won't be applied until a new event + return ctrl.Result{}, IgnoreConflict(err) + } + + return ctrl.Result{}, nil +} + +func (r *FunctionConfigReconciler) removeFinalizer(ctx context.Context, obj *configapi.FunctionConfig) error { + patch := client.MergeFrom(obj.DeepCopy()) + + switch r.For { + case ReconcilerForFunctionRunner: + controllerutil.RemoveFinalizer(obj, FunctionRunnerFinalizer) + case ReconcilerForServer: + controllerutil.RemoveFinalizer(obj, ServerFinalizer) + case ReconcilerForController: + controllerutil.RemoveFinalizer(obj, ControllerFinalizer) + } + + if err := r.Client.Patch(ctx, obj, patch); err != nil { + klog.Errorf("Failed to remove finalizer from FunctionConfig %q: %v", obj.Name, err) + return err + } + + return nil +} + +func (r *FunctionConfigReconciler) addFinalizer(ctx context.Context, obj *configapi.FunctionConfig) error { + patch := client.MergeFrom(obj.DeepCopy()) + + updated := false + switch r.For { + case ReconcilerForFunctionRunner: + updated = controllerutil.AddFinalizer(obj, FunctionRunnerFinalizer) + case ReconcilerForServer: + updated = controllerutil.AddFinalizer(obj, ServerFinalizer) + case ReconcilerForController: + updated = controllerutil.AddFinalizer(obj, ControllerFinalizer) + } + + if updated { + if err := r.Client.Patch(ctx, obj, patch); err != nil { + klog.Errorf("Failed to add finalizer to FunctionConfig %q: %v", obj.Name, err) + return err + } + } + + return nil +} + +func IgnoreConflict(err error) error { + if apierrors.IsConflict(err) { + return nil + } + return err +} diff --git a/controllers/functionconfigs/reconciler/functionconfigreconciler.go b/controllers/functionconfigs/reconciler/functionconfigreconciler.go deleted file mode 100644 index b7a225002..000000000 --- a/controllers/functionconfigs/reconciler/functionconfigreconciler.go +++ /dev/null @@ -1,392 +0,0 @@ -// Copyright 2026 The kpt 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 reconciler - -import ( - "context" - "maps" - "path/filepath" - "regexp" - "slices" - "strings" - "sync" - - "github.com/kptdev/krm-functions-catalog/functions/go/apply-replacements/replacements" - setNamespace "github.com/kptdev/krm-functions-catalog/functions/go/set-namespace/transformer" - "github.com/kptdev/krm-functions-catalog/functions/go/starlark/starlark" - fnsdk "github.com/kptdev/krm-functions-sdk/go/fn" - configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - "github.com/kptdev/porch/pkg/util" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" - "k8s.io/klog/v2" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" -) - -const BaseFinalizer = "config.porch.kpt.dev/functionconfig" -const ServerFinalizer = BaseFinalizer + "-porch-server" -const FunctionRunnerFinalizer = BaseFinalizer + "-function-runner" -const ControllerFinalizer = BaseFinalizer + "-controller" - -type BinaryCacheEntry struct { - PrefixRegex string - Tags map[string]string -} - -type BuiltInCacheEntry struct { - PrefixRegex string - Process fnsdk.ResourceListProcessor - Tags []string -} - -type FunctionConfigStore struct { - mu sync.RWMutex - - functionConfigurations map[string]*configapi.FunctionConfig - binaryExecutorCache map[string]BinaryCacheEntry - builtInExecutorCache map[string]BuiltInCacheEntry - - defaultImagePrefix string - defaultBinaryDir string -} - -func NewFunctionConfigStore(defaultImagePrefix, defaultBinaryDir string) *FunctionConfigStore { - return &FunctionConfigStore{ - functionConfigurations: make(map[string]*configapi.FunctionConfig), - binaryExecutorCache: make(map[string]BinaryCacheEntry), - builtInExecutorCache: make(map[string]BuiltInCacheEntry), - defaultImagePrefix: strings.TrimRight(defaultImagePrefix, "/"), - defaultBinaryDir: strings.TrimRight(defaultBinaryDir, "/"), - } -} - -func (s *FunctionConfigStore) UpsertFunctionConfig(name string, obj *configapi.FunctionConfig) { - s.mu.Lock() - defer s.mu.Unlock() - s.functionConfigurations[name] = obj -} - -func (s *FunctionConfigStore) generateRegexPattern(prefixes []string, imageName string) string { - var preparedPrefixes []string - for _, prefix := range prefixes { - if prefix == "" { - preparedPrefixes = append(preparedPrefixes, regexp.QuoteMeta(s.defaultImagePrefix)) - } else { - preparedPrefixes = append(preparedPrefixes, regexp.QuoteMeta(prefix)) - } - } - - return "^(?:" + strings.Join(preparedPrefixes, "|") + ")$" - -} - -func splitImage(image string) (name string, tag string) { - lastSlash := strings.LastIndex(image, "/") - lastColon := strings.LastIndex(image, ":") - - if lastColon > lastSlash { - return image[:lastColon], image[lastColon+1:] - } - return image, "" -} - -func (s *FunctionConfigStore) UpdateBinaryCache(_ string, obj *configapi.FunctionConfig) { - s.mu.Lock() - defer s.mu.Unlock() - - var binaryCacheEntry BinaryCacheEntry - binaryCacheEntry.Tags = make(map[string]string) - // Create a prefix Regex - binaryCacheEntry.PrefixRegex = s.generateRegexPattern(obj.Spec.Prefixes, obj.Spec.Image) - - abs := obj.Spec.BinaryExecutor.Path - if abs[0] != '/' { - var err error - abs, err = filepath.Abs(filepath.Join(s.defaultBinaryDir, obj.Spec.BinaryExecutor.Path)) - if err != nil { - klog.Warningf("Failed to cache %q: %v", obj.Spec.Image, err) - return - } - } - - for _, tag := range obj.Spec.BinaryExecutor.Tags { - binaryCacheEntry.Tags[tag] = abs - } - s.binaryExecutorCache[obj.Spec.Image] = binaryCacheEntry -} - -func (s *FunctionConfigStore) UpdateExecCache(name string, functionConfig *configapi.FunctionConfig) { - s.mu.Lock() - defer s.mu.Unlock() - - id := name - if functionConfig.Spec.GoExecutor.ID != nil { - id = *functionConfig.Spec.GoExecutor.ID - } - - applyMappings := func(id string, fn fnsdk.ResourceListProcessorFunc) { - //Clear previous entries for the actual function - for img := range s.builtInExecutorCache { - if strings.Contains(img, name) { - delete(s.builtInExecutorCache, img) - } - } - - s.builtInExecutorCache[id] = BuiltInCacheEntry{ - Process: fn, - Tags: functionConfig.Spec.GoExecutor.Tags, - PrefixRegex: s.generateRegexPattern(functionConfig.Spec.Prefixes, functionConfig.Spec.Image), - } - } - - if functionConfig.Name == "apply-replacements" { - applyMappings(id, replacements.ApplyReplacements) - } - if functionConfig.Name == "set-namespace" { - applyMappings(id, setNamespace.Run) - } - if functionConfig.Name == "starlark" { - applyMappings(id, starlark.Process) - } -} - -func (s *FunctionConfigStore) DeleteFunctionConfig(key types.NamespacedName) { - s.mu.Lock() - defer s.mu.Unlock() - delete(s.functionConfigurations, key.Name) -} - -func (s *FunctionConfigStore) GetFunctionConfig(name string) (*configapi.FunctionConfig, bool) { - s.mu.RLock() - defer s.mu.RUnlock() - config, ok := s.functionConfigurations[name] - return config, ok -} - -func (s *FunctionConfigStore) GetBinaryFromCache(image string) (string, bool) { - s.mu.RLock() - defer s.mu.RUnlock() - - image, tag := splitImage(image) - prefixToCheck := util.GetImageRepository(image) - binaryStore, exists := s.binaryExecutorCache[util.GetImageName(image)] - if exists { - regex := regexp.MustCompile(binaryStore.PrefixRegex) - if regex.MatchString(prefixToCheck) { - binaryPath, tagExists := binaryStore.Tags[tag] - if tagExists { - return binaryPath, true - } - } - } - return "", false -} - -func (s *FunctionConfigStore) GetBinaryFromCacheByConstraint(image, tag string) (string, bool) { - s.mu.RLock() - defer s.mu.RUnlock() - - baseName := util.GetImageName(image) - cacheEntry := s.binaryExecutorCache[baseName] - - cacheKeys := make([]string, 0, len(s.binaryExecutorCache)) - for k := range cacheEntry.Tags { - cacheKeys = append(cacheKeys, k) - } - - selectedKey, err := util.FindBestSemverMatch(tag, image, cacheKeys) - if err != nil { - return "", false - } - selectedBinary := cacheEntry.Tags[selectedKey] - - prefixToCheck, tag := splitImage(image) - regex := regexp.MustCompile(cacheEntry.PrefixRegex) - if regex.MatchString(prefixToCheck) { - binaryPath, tagExists := cacheEntry.Tags[tag] - if tagExists { - return binaryPath, true - } - } - - return selectedBinary, true -} - -func (s *FunctionConfigStore) GetExecCache() map[string]BuiltInCacheEntry { - s.mu.RLock() - defer s.mu.RUnlock() - return s.builtInExecutorCache -} - -// GetProcessorFromCache looks up a function processor by image, holding the read lock for the duration of the lookup. -func (s *FunctionConfigStore) GetProcessorFromCache(image string) (fnsdk.ResourceListProcessor, bool) { - s.mu.RLock() - defer s.mu.RUnlock() - baseName := util.GetImageName(image) - tag := util.GetImageTag(image) - entry, found := s.builtInExecutorCache[baseName] - prefixToCheck := util.GetImageRepository(image) - if prefixToCheck == "" { - prefixToCheck = s.defaultImagePrefix - } - if slices.Contains(entry.Tags, tag) { - regex := regexp.MustCompile(entry.PrefixRegex) - if regex.MatchString(prefixToCheck) { - return entry.Process, found - } - } - return nil, false - -} - -func (s *FunctionConfigStore) List() []*configapi.FunctionConfig { - s.mu.Lock() - defer s.mu.Unlock() - - return slices.Collect(maps.Values(s.functionConfigurations)) -} - -type ReconcilerFor string - -const ( - ReconcilerForFunctionRunner ReconcilerFor = "function-runner" - ReconcilerForServer ReconcilerFor = "server" - ReconcilerForController ReconcilerFor = "controller" -) - -type FunctionConfigReconciler struct { - Client client.Client - FunctionConfigStore *FunctionConfigStore - // For indicates which component the reconciler is collecting the configs for - // TODO: remove after merging of function-runner into server - For ReconcilerFor -} - -func (r *FunctionConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, finalErr error) { - klog.Infof("FunctionConfig %q changed", req.NamespacedName) - obj := &configapi.FunctionConfig{} - err := r.Client.Get(ctx, req.NamespacedName, obj) - if apierrors.IsNotFound(err) { - r.FunctionConfigStore.DeleteFunctionConfig(req.NamespacedName) - return ctrl.Result{}, nil - } - if err != nil { - return ctrl.Result{}, err - } - - if obj.DeletionTimestamp != nil { - if err := r.removeFinalizer(ctx, obj); err != nil { - return ctrl.Result{}, err - } - - r.FunctionConfigStore.DeleteFunctionConfig(req.NamespacedName) - return ctrl.Result{}, nil - } - - if err := r.addFinalizer(ctx, obj); err != nil { - return ctrl.Result{}, err - } - - defer func() { - patch := client.MergeFrom(obj.DeepCopy()) - - if finalErr != nil { - obj.Status.Error = finalErr.Error() - } else { - obj.Status.Error = "" - switch r.For { - case ReconcilerForFunctionRunner: - obj.Status.FunctionRunnerObservedGeneration = obj.Generation - case ReconcilerForServer: - obj.Status.ApiServerObservedGeneration = obj.Generation - case ReconcilerForController: - obj.Status.ControllerObservedGeneration = obj.Generation - } - } - - if err := r.Client.Status().Patch(ctx, obj, patch); err != nil { - klog.Errorf("Failed to update status of FunctionConfig %q: %v", obj.Name, err) - if finalErr == nil { - finalErr = err - } - } - }() - - // Check if the FunctionConfig already exists in the store with a different name to avoid duplications - image := obj.Spec.Image - fc, exists := r.FunctionConfigStore.GetFunctionConfig(image) - - if exists && fc.Name != obj.Name { - klog.Infof("FunctionConfig for %s image is already in the store with a different name", image) - return ctrl.Result{}, nil - } - - r.FunctionConfigStore.UpsertFunctionConfig(obj.Name, obj) - - if obj.Spec.BinaryExecutor != nil { - r.FunctionConfigStore.UpdateBinaryCache(obj.Name, obj) - } - - if obj.Spec.GoExecutor != nil { - r.FunctionConfigStore.UpdateExecCache(obj.Name, obj) - } - - return ctrl.Result{}, nil -} - -func (r *FunctionConfigReconciler) removeFinalizer(ctx context.Context, obj *configapi.FunctionConfig) error { - patch := client.MergeFrom(obj.DeepCopy()) - - switch r.For { - case ReconcilerForFunctionRunner: - controllerutil.RemoveFinalizer(obj, FunctionRunnerFinalizer) - case ReconcilerForServer: - controllerutil.RemoveFinalizer(obj, ServerFinalizer) - case ReconcilerForController: - controllerutil.RemoveFinalizer(obj, ControllerFinalizer) - } - - if err := r.Client.Patch(ctx, obj, patch); err != nil { - klog.Errorf("Failed to remove finalizer from FunctionConfig %q: %v", obj.Name, err) - return err - } - - return nil -} - -func (r *FunctionConfigReconciler) addFinalizer(ctx context.Context, obj *configapi.FunctionConfig) error { - patch := client.MergeFrom(obj.DeepCopy()) - - updated := false - switch r.For { - case ReconcilerForFunctionRunner: - updated = controllerutil.AddFinalizer(obj, FunctionRunnerFinalizer) - case ReconcilerForServer: - updated = controllerutil.AddFinalizer(obj, ServerFinalizer) - case ReconcilerForController: - updated = controllerutil.AddFinalizer(obj, ControllerFinalizer) - } - - if updated { - if err := r.Client.Patch(ctx, obj, patch); err != nil { - klog.Errorf("Failed to add finalizer to FunctionConfig %q: %v", obj.Name, err) - return err - } - } - - return nil -} diff --git a/controllers/functionconfigs/reconciler/functionconfigreconciler_test.go b/controllers/functionconfigs/reconciler_test.go similarity index 69% rename from controllers/functionconfigs/reconciler/functionconfigreconciler_test.go rename to controllers/functionconfigs/reconciler_test.go index 1a5086aa6..92a4527e6 100644 --- a/controllers/functionconfigs/reconciler/functionconfigreconciler_test.go +++ b/controllers/functionconfigs/reconciler_test.go @@ -1,4 +1,4 @@ -// Copyright 2025 The kpt Authors +// Copyright 2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -12,13 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -package reconciler +package functionconfigs import ( "context" "testing" "time" + "github.com/kptdev/krm-functions-catalog/functions/go/starlark/starlark" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -30,7 +31,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ) -const defaultImagePrefix = "ghcr.io/kptdev/krm-functions-catalog/" +const defaultImagePrefix = "ghcr.io/kptdev/krm-functions-catalog" const functionCacheDir = "/functions" const testNamespace = "porch-fn-system" @@ -57,6 +58,7 @@ func TestFunctionConfigReconciler(t *testing.T) { PodExecutor: &configapi.PodExecutorConfig{ Tags: []string{ "v0.1.1", + "", }, TimeToLive: metav1.Duration{Duration: 30 * time.Second}, MaxParallelExecutions: 2, @@ -124,14 +126,12 @@ func TestFunctionConfigReconciler(t *testing.T) { Tags: []string{ "v0.4.3", "v0.4", + "", }, }, }, } - preloadedFunctionConfigStore := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir) - preloadedFunctionConfigStore.UpsertFunctionConfig("set-image", sampleFunctionConfig) - tests := []testcase{ { name: "FunctionConfig object is stored in FunctionStore after reconciliation", @@ -139,13 +139,12 @@ func TestFunctionConfigReconciler(t *testing.T) { requests: []string{"set-image"}, check: func(t *testing.T, r *FunctionConfigReconciler) { // Check existence of the functionConfig in cluster - got, exists := r.FunctionConfigStore.GetFunctionConfig("set-image") - expectedNumberOfFunctions := 1 + got, exists := r.FunctionConfigStore.Get("set-image") expectedImage := "set-image" - assert.True(t, exists, "FunctionConfig %s should exist in the store", expectedImage) - assert.Equal(t, expectedImage, got.Spec.Image, "expected image %q, got %q", expectedImage, got.Spec.Image) - assert.Equal(t, expectedNumberOfFunctions, len(r.FunctionConfigStore.List()), "expect %d function configs in the store, but got %d", expectedNumberOfFunctions, len(r.FunctionConfigStore.List())) + assert.Truef(t, exists, "FunctionConfig %s should exist in the store", expectedImage) + assert.Equalf(t, expectedImage, got.Image, "expected image %q, got %q", expectedImage, got.Image) + assert.Equalf(t, 1, r.FunctionConfigStore.Len(), "expect %d function configs in the store, but got %d", 1, r.FunctionConfigStore.Len()) }, }, { @@ -154,7 +153,7 @@ func TestFunctionConfigReconciler(t *testing.T) { requests: []string{"set-image"}, check: func(t *testing.T, r *FunctionConfigReconciler) { // Check existence of the functionConfig in cluster - _, exists := r.FunctionConfigStore.GetFunctionConfig("set-image") + _, exists := r.FunctionConfigStore.Get("set-image") assert.False(t, exists, "FunctionConfig 'set-image' should not exist in the store") }, }, @@ -165,9 +164,10 @@ func TestFunctionConfigReconciler(t *testing.T) { check: func(t *testing.T, r *FunctionConfigReconciler) { expectedKey := "ghcr.io/kptdev/krm-functions-catalog/set-image:v0.1.4" expectedPath := "/functions/set-image" - binary, exists := r.FunctionConfigStore.GetBinaryFromCache(expectedKey) - assert.True(t, exists, "BinaryExecutorCache should have '%s'", expectedKey) - assert.Equal(t, expectedPath, binary, "BinaryExecutorCache entry is %q, want %q", binary, expectedPath) + config, exists := r.FunctionConfigStore.Get(expectedKey) + require.True(t, exists, "BinaryExecutorCache should have '%s'", expectedKey) + assert.Equal(t, expectedPath, config.BinaryExecutor.Path, + "BinaryExecutorCache entry is %q, want %q", config.BinaryExecutor.Path, expectedPath) }, }, { @@ -175,19 +175,17 @@ func TestFunctionConfigReconciler(t *testing.T) { objs: []client.Object{builtInSetNamespace, builtInApplyReplacements, builtInStarlarkWithId}, requests: []string{"apply-replacements", "set-namespace", "starlark"}, check: func(t *testing.T, r *FunctionConfigReconciler) { - expectedStarlarkKey := "starlark-id" - execFunctions := r.FunctionConfigStore.GetExecCache() - - got, ok := execFunctions[expectedStarlarkKey] - assert.True(t, ok, "BuiltInExecutorCache should have '%s'", expectedStarlarkKey) - assert.NotNil(t, got, "BuiltInExecutorCache entry is not the expected processor function") + proc, ok := r.FunctionConfigStore.GetProcessor("starlark") + assert.Truef(t, ok, "BuiltInExecutorCache should have %q", starlarkExecutorID) + assert.NotNil(t, proc, "BuiltInExecutorCache entry is not the expected processor function") }, }, } scheme := runtime.NewScheme() err := configapi.AddToScheme(scheme) + require.NoError(t, err) if err != nil { t.Fatalf("unable to add configapi to scheme: %v", err) } @@ -196,7 +194,8 @@ func TestFunctionConfigReconciler(t *testing.T) { t.Run(tt.name, func(t *testing.T) { c := fake.NewClientBuilder().WithObjects(tt.objs...).WithScheme(scheme).WithStatusSubresource(&configapi.FunctionConfig{}).Build() - functionConfigStore := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir) + functionConfigStore := NewStore(defaultImagePrefix, functionCacheDir) + functionConfigStore.processorMapping[starlarkExecutorID] = starlark.Process reconciler := &FunctionConfigReconciler{ Client: c, FunctionConfigStore: functionConfigStore, @@ -262,7 +261,7 @@ func TestFinalizersAdded(t *testing.T) { c := fake.NewClientBuilder().WithScheme(schemeWithFunctionConfig(t)).WithObjects(obj).WithStatusSubresource(&configapi.FunctionConfig{}).Build() r := &FunctionConfigReconciler{ Client: c, - FunctionConfigStore: NewFunctionConfigStore(defaultImagePrefix, functionCacheDir), + FunctionConfigStore: NewStore(defaultImagePrefix, functionCacheDir), For: tc.forValue, } @@ -279,141 +278,6 @@ func TestFinalizersAdded(t *testing.T) { } } -func TestGetProcessorFromCache(t *testing.T) { - store := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir) - - // Populate via UpdateExecCache (same path as reconciler) - obj := &configapi.FunctionConfig{ - ObjectMeta: metav1.ObjectMeta{Name: "set-namespace", Namespace: testNamespace}, - Spec: configapi.FunctionConfigSpec{ - Image: "set-namespace", - Prefixes: []string{""}, - GoExecutor: &configapi.GoExecutorConfig{ - Tags: []string{"v0.4.1"}, - }, - }, - } - store.UpdateExecCache(obj.Name, obj) - - // Found with full prefix - processor, found := store.GetProcessorFromCache("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1") - assert.True(t, found) - assert.NotNil(t, processor) - - // Found without prefix (short form) - processor, found = store.GetProcessorFromCache("set-namespace:v0.4.1") - assert.True(t, found) - assert.NotNil(t, processor) - - // Not found for unknown tag - _, found = store.GetProcessorFromCache("set-namespace:v9.9.9") - assert.False(t, found) - - // Not found for unknown image - _, found = store.GetProcessorFromCache("nonexistent:v1.0.0") - assert.False(t, found) -} - -func TestPrePopulationPattern(t *testing.T) { - // Simulates what setupFunctionConfigReconciler does on cold start: - // list all FunctionConfigs and populate the store synchronously - // without going through the reconcile loop. - store := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir) - - configs := []configapi.FunctionConfig{ - { - ObjectMeta: metav1.ObjectMeta{Name: "set-namespace", Namespace: testNamespace}, - Spec: configapi.FunctionConfigSpec{ - Image: "set-namespace", - Prefixes: []string{""}, - GoExecutor: &configapi.GoExecutorConfig{Tags: []string{"v0.4.1"}}, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{Name: "apply-replacements", Namespace: testNamespace}, - Spec: configapi.FunctionConfigSpec{ - Image: "apply-replacements", - Prefixes: []string{""}, - GoExecutor: &configapi.GoExecutorConfig{Tags: []string{"v0.1.1"}}, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{Name: "set-image", Namespace: testNamespace}, - Spec: configapi.FunctionConfigSpec{ - Image: "set-image", - Prefixes: []string{""}, - BinaryExecutor: &configapi.BinaryExecutorConfig{ - Tags: []string{"v0.1.4"}, - Path: "set-image", - }, - }, - }, - } - - // Pre-populate (mirrors the code in setupFunctionConfigReconciler) - for i := range configs { - obj := &configs[i] - store.UpsertFunctionConfig(obj.Name, obj) - if obj.Spec.GoExecutor != nil { - store.UpdateExecCache(obj.Name, obj) - } - if obj.Spec.BinaryExecutor != nil { - store.UpdateBinaryCache(obj.Name, obj) - } - } - - // Verify exec cache is populated - _, found := store.GetProcessorFromCache("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1") - assert.True(t, found, "set-namespace should be in exec cache after pre-population") - - _, found = store.GetProcessorFromCache("ghcr.io/kptdev/krm-functions-catalog/apply-replacements:v0.1.1") - assert.True(t, found, "apply-replacements should be in exec cache after pre-population") - - // Verify binary cache is populated - path, found := store.GetBinaryFromCache("ghcr.io/kptdev/krm-functions-catalog/set-image:v0.1.4") - assert.True(t, found, "set-image should be in binary cache after pre-population") - assert.Equal(t, "/functions/set-image", path) - - // Verify function configs are stored - assert.Len(t, store.List(), 3) -} - -func TestConcurrentAccessSafety(t *testing.T) { - // Verifies no data race when UpdateExecCache and GetProcessorFromCache - // are called concurrently (the fix for the data race bug). - store := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir) - - obj := &configapi.FunctionConfig{ - ObjectMeta: metav1.ObjectMeta{Name: "set-namespace", Namespace: testNamespace}, - Spec: configapi.FunctionConfigSpec{ - Image: "set-namespace", - Prefixes: []string{""}, - GoExecutor: &configapi.GoExecutorConfig{Tags: []string{"v0.4.1"}}, - }, - } - - done := make(chan struct{}) - - // Writer goroutine - go func() { - defer close(done) - for i := 0; i < 100; i++ { - store.UpdateExecCache(obj.Name, obj) - } - }() - - // Reader goroutine (concurrent with writer) - for i := 0; i < 100; i++ { - store.GetProcessorFromCache("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1") - } - - <-done - - // After all writes complete, the entry should be present - _, found := store.GetProcessorFromCache("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1") - assert.True(t, found) -} - func TestFinalizersRemoved(t *testing.T) { now := metav1.Now() const testFinalizer = "config.porch.kpt.dev/test-hold" @@ -454,8 +318,9 @@ func TestFinalizersRemoved(t *testing.T) { } c := fake.NewClientBuilder().WithScheme(schemeWithFunctionConfig(t)).WithObjects(obj).WithStatusSubresource(&configapi.FunctionConfig{}).Build() - store := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir) - store.UpsertFunctionConfig(objName, obj) + store := NewStore(defaultImagePrefix, functionCacheDir) + err := store.Store(obj) + require.NoError(t, err) r := &FunctionConfigReconciler{ Client: c, @@ -463,7 +328,7 @@ func TestFinalizersRemoved(t *testing.T) { For: tc.forValue, } - _, err := r.Reconcile(context.Background(), ctrl.Request{ + _, err = r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: types.NamespacedName{Name: objName, Namespace: testNamespace}, }) require.NoError(t, err) @@ -474,8 +339,67 @@ func TestFinalizersRemoved(t *testing.T) { assert.NotContains(t, got.Finalizers, tc.finalizer) assert.Contains(t, got.Finalizers, testFinalizer) - _, exists := store.GetFunctionConfig(objName) + _, exists := store.Get(objName) assert.False(t, exists, "FunctionConfig should be removed from the store when deletion completes") }) } } + +func TestPrePopulationPattern(t *testing.T) { + // Simulates what setupFunctionConfigReconciler does on cold start: + // list all FunctionConfigs and populate the store synchronously + // without going through the reconcile loop. + store := NewStore(defaultImagePrefix, functionCacheDir) + + configs := []configapi.FunctionConfig{ + { + ObjectMeta: metav1.ObjectMeta{Name: "set-namespace", Namespace: testNamespace}, + Spec: configapi.FunctionConfigSpec{ + Image: "set-namespace", + Prefixes: []string{""}, + GoExecutor: &configapi.GoExecutorConfig{Tags: []string{"v0.4.1"}}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "apply-replacements", Namespace: testNamespace}, + Spec: configapi.FunctionConfigSpec{ + Image: "apply-replacements", + Prefixes: []string{""}, + GoExecutor: &configapi.GoExecutorConfig{Tags: []string{"v0.1.1"}}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "set-image", Namespace: testNamespace}, + Spec: configapi.FunctionConfigSpec{ + Image: "set-image", + Prefixes: []string{""}, + BinaryExecutor: &configapi.BinaryExecutorConfig{ + Tags: []string{"v0.1.4"}, + Path: "set-image", + }, + }, + }, + } + + // Pre-populate (mirrors the code in setupFunctionConfigReconciler) + for i := range configs { + obj := &configs[i] + err := store.Store(obj) + require.NoError(t, err) + } + + // Verify exec cache is populated + _, found := store.GetProcessor("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1") + assert.True(t, found, "set-namespace should be in exec cache after pre-population") + + _, found = store.GetProcessor("ghcr.io/kptdev/krm-functions-catalog/apply-replacements:v0.1.1") + assert.True(t, found, "apply-replacements should be in exec cache after pre-population") + + // Verify binary cache is populated + config, found := store.Get("ghcr.io/kptdev/krm-functions-catalog/set-image:v0.1.4") + require.True(t, found, "set-image should be in binary cache after pre-population") + assert.Equal(t, "/functions/set-image", config.BinaryExecutor.Path) + + // Verify function configs are stored + assert.Equal(t, 3, store.Len(), "unexpected amount of function configs in cache") +} diff --git a/controllers/functionconfigs/store.go b/controllers/functionconfigs/store.go new file mode 100644 index 000000000..c0adb1f27 --- /dev/null +++ b/controllers/functionconfigs/store.go @@ -0,0 +1,293 @@ +// Copyright 2026 The kpt 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 functionconfigs + +import ( + "iter" + "maps" + "path/filepath" + "slices" + "strings" + "sync" + + applyreplacements "github.com/kptdev/krm-functions-catalog/functions/go/apply-replacements/replacements" + setnamespace "github.com/kptdev/krm-functions-catalog/functions/go/set-namespace/transformer" + "github.com/kptdev/krm-functions-catalog/functions/go/starlark/starlark" + "github.com/kptdev/krm-functions-sdk/go/fn" + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + imageutil "github.com/kptdev/porch/pkg/util/image" + pkgerrors "github.com/pkg/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/klog/v2" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + LatestTag = "latest" + AllPrefixes = "*" +) + +type InternalCacheEntry struct { + Entry map[string]map[string]configapi.FunctionConfigSpec + // objKey stores what cluster object the entry was read from. + // Used to check for conflicts/duplicate definitions. + objKey client.ObjectKey +} + +type FunctionConfigStore struct { + mu sync.RWMutex + + // image base name -> prefix -> tag + internalCache map[string]InternalCacheEntry + + // processorMapping contains all the built-in functions that can be executed as a Go function + processorMapping map[string]fn.ResourceListProcessorFunc + + defaultImagePrefix string + defaultBinaryDir string +} + +func NewStore(defaultImagePrefix, defaultBinaryDir string) *FunctionConfigStore { + procMap := map[string]fn.ResourceListProcessorFunc{ + "apply-replacements": applyreplacements.ApplyReplacements, + "set-namespace": setnamespace.Run, + "starlark": starlark.Process, + } + + return &FunctionConfigStore{ + defaultImagePrefix: strings.TrimRight(defaultImagePrefix, "/"), + defaultBinaryDir: strings.TrimRight(defaultBinaryDir, "/"), + + // processorMapping contains all the built-in functions that can be executed as a Go function + processorMapping: procMap, + + internalCache: make(map[string]InternalCacheEntry), + } +} + +func (s *FunctionConfigStore) Store(obj *configapi.FunctionConfig) error { + s.mu.Lock() + defer s.mu.Unlock() + + spec := &obj.Spec + + objKey := client.ObjectKeyFromObject(obj) + if entry, ok := s.internalCache[spec.Image]; ok && entry.objKey != objKey { + return apierrors.NewConflict( + configapi.TypeFunctionConfig.GroupResource(), + client.ObjectKeyFromObject(obj).String(), + pkgerrors.Errorf("Image %q is already configured from object %q", spec.Image, entry.objKey), + ) + } + + // remove any unnecessary data that will only be used for map keys + strippedSpec := *spec.DeepCopy() + // keep the first prefix for warmup TODO: remove once warmup is event based + //strippedSpec.Prefixes = nil + if len(strippedSpec.Prefixes) != 0 { + strippedSpec.Prefixes = strippedSpec.Prefixes[:1] + } + if strippedSpec.PodExecutor != nil { + // keep the first tag for warmup TODO: remove once warmup is event based + //strippedSpec.PodExecutor.Tags = nil + if len(strippedSpec.PodExecutor.Tags) > 0 { + strippedSpec.PodExecutor.Tags = strippedSpec.PodExecutor.Tags[:1] + } + } + if strippedSpec.BinaryExecutor != nil { + strippedSpec.BinaryExecutor.Tags = nil + if len(strippedSpec.BinaryExecutor.Path) > 0 && strippedSpec.BinaryExecutor.Path[0] != '/' { + var err error + strippedSpec.BinaryExecutor.Path, err = filepath.Abs(filepath.Join(s.defaultBinaryDir, spec.BinaryExecutor.Path)) + if err != nil { + klog.Warningf("Failed to cache %q: %v", spec.Image, err) + } + } + } + if strippedSpec.GoExecutor != nil { + strippedSpec.GoExecutor.Tags = nil + } + + prefixes := s.normalizePrefixes(spec.Prefixes) + + // if no prefixes are given, assume the user wants to apply the config to all prefixes + if len(prefixes) == 0 { + prefixes = append(prefixes, AllPrefixes) + } + + s.internalCache[spec.Image] = InternalCacheEntry{ + Entry: make(map[string]map[string]configapi.FunctionConfigSpec), + objKey: objKey, + } + for _, prefix := range prefixes { + // One tag can technically have multiple types of configurations, + // but handling the overlap would be less efficient than just doing multiple writes. + s.internalCache[spec.Image].Entry[prefix] = make(map[string]configapi.FunctionConfigSpec) + for _, conf := range []configapi.TagIterable{spec.GoExecutor, spec.BinaryExecutor, spec.PodExecutor} { + for tag := range conf.IterTags() { + switch tag { + case "": + s.internalCache[spec.Image].Entry[prefix][LatestTag] = strippedSpec + case LatestTag: + s.internalCache[spec.Image].Entry[prefix][""] = strippedSpec + } + s.internalCache[spec.Image].Entry[prefix][tag] = strippedSpec + } + } + } + + return nil +} + +// normalizePrefixes strips additional slashes, inlines the default image prefix and removes duplicates +func (s *FunctionConfigStore) normalizePrefixes(prefixes []string) []string { + prefixesSet := make(map[string]struct{}) + for _, prefix := range prefixes { + prefix = strings.Trim(prefix, "/") + if prefix == "" { + prefixesSet[s.defaultImagePrefix] = struct{}{} + } + if strings.Trim(prefix, "/") == s.defaultImagePrefix { + prefixesSet[""] = struct{}{} + } + prefixesSet[prefix] = struct{}{} + } + + return slices.Collect(maps.Keys(prefixesSet)) +} + +func (s *FunctionConfigStore) Delete(imageName string) { + s.mu.Lock() + defer s.mu.Unlock() + delete(s.internalCache, imageName) +} + +func (s *FunctionConfigStore) DeleteByObjName(key client.ObjectKey) { + s.mu.Lock() + defer s.mu.Unlock() + + for imageName, entry := range s.internalCache { + if entry.objKey == key { + delete(s.internalCache, imageName) + return + } + } +} + +func (s *FunctionConfigStore) Get(fullImageName string) (configapi.FunctionConfigSpec, bool) { + s.mu.RLock() + defer s.mu.RUnlock() + parsedImage := imageutil.Parse(fullImageName) + if imageEntry, ok := s.internalCache[parsedImage.BaseName]; ok { + prefixEntry, ok := imageEntry.Entry[parsedImage.Prefix()] + + if !ok { + prefixEntry, ok = imageEntry.Entry[AllPrefixes] + } + + if ok { + if tagEntry, ok := prefixEntry[parsedImage.Tag]; ok { + return tagEntry, true + } + } + } + return configapi.FunctionConfigSpec{}, false +} + +func (s *FunctionConfigStore) GetByConstraint(fullImageName, constraint string) (configapi.FunctionConfigSpec, bool) { + s.mu.RLock() + defer s.mu.RUnlock() + parsedImage := imageutil.Parse(fullImageName) + if imageEntry, ok := s.internalCache[parsedImage.BaseName]; ok { + prefixEntry, ok := imageEntry.Entry[parsedImage.Prefix()] + + if !ok { + prefixEntry, ok = imageEntry.Entry[AllPrefixes] + } + + if ok { + tags := slices.Collect(maps.Keys(prefixEntry)) + best, err := imageutil.FindBestSemverMatch(constraint, tags) + if err != nil { + klog.Warningf("Failed to find best semantic version for image %q by constraint %q: %v", fullImageName, constraint, err) + return configapi.FunctionConfigSpec{}, false + } + return prefixEntry[best], true + } + } + return configapi.FunctionConfigSpec{}, false +} + +// GetProcessor looks up a function processor by image, holding the read lock for the duration of the lookup. +func (s *FunctionConfigStore) GetProcessor(imageName string) (fn.ResourceListProcessor, bool) { + config, ok := s.Get(imageName) + if !ok { + return nil, false + } + + return s.getProcessorForConfig(&config) +} + +func (s *FunctionConfigStore) GetProcessorByConstraint(imageName, constraint string) (fn.ResourceListProcessor, bool) { + config, ok := s.GetByConstraint(imageName, constraint) + if !ok { + return nil, false + } + + return s.getProcessorForConfig(&config) +} + +func (s *FunctionConfigStore) getProcessorForConfig(config *configapi.FunctionConfigSpec) (fn.ResourceListProcessor, bool) { + if config.GoExecutor == nil { + return nil, false + } + + if config.GoExecutor.ID != nil { + proc, ok := s.processorMapping[*config.GoExecutor.ID] + return proc, ok + } + + proc, ok := s.processorMapping[config.Image] + return proc, ok +} + +// IterPodConfigSpecs iterates through function configs which contain a pod executor config +// +// TODO: remove when warmup is event based +func (s *FunctionConfigStore) IterPodConfigSpecs() iter.Seq[configapi.FunctionConfigSpec] { + return func(yield func(configapi.FunctionConfigSpec) bool) { + s.mu.RLock() + defer s.mu.RUnlock() + + for _, imageEntry := range s.internalCache { + for _, prefixEntry := range imageEntry.Entry { + for _, tagEntry := range prefixEntry { + if tagEntry.PodExecutor != nil { + if !yield(tagEntry) { + return + } + } + } + } + } + } +} + +func (s *FunctionConfigStore) Len() int { + s.mu.RLock() + defer s.mu.RUnlock() + + return len(s.internalCache) +} diff --git a/controllers/functionconfigs/store_test.go b/controllers/functionconfigs/store_test.go new file mode 100644 index 000000000..cbd71bbee --- /dev/null +++ b/controllers/functionconfigs/store_test.go @@ -0,0 +1,96 @@ +// Copyright 2026 The kpt 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 functionconfigs + +import ( + "testing" + + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestGetProcessorFromCache(t *testing.T) { + store := NewStore(defaultImagePrefix, functionCacheDir) + + // Populate via UpdateExecCache (same path as reconciler) + obj := &configapi.FunctionConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "set-namespace", Namespace: testNamespace}, + Spec: configapi.FunctionConfigSpec{ + Image: "set-namespace", + Prefixes: []string{""}, + GoExecutor: &configapi.GoExecutorConfig{ + Tags: []string{"v0.4.1"}, + }, + }, + } + require.NoError(t, store.Store(obj)) + + // Found with full prefix + processor, found := store.GetProcessor(defaultImagePrefix + "/set-namespace:v0.4.1") + assert.True(t, found) + assert.NotNil(t, processor) + + // Found without prefix (short form) + processor, found = store.GetProcessor("set-namespace:v0.4.1") + assert.True(t, found) + assert.NotNil(t, processor) + + // Not found for unknown tag + _, found = store.GetProcessor("set-namespace:v9.9.9") + assert.False(t, found) + + // Not found for unknown image + _, found = store.GetProcessor("nonexistent:v1.0.0") + assert.False(t, found) +} + +func TestConcurrentAccessSafety(t *testing.T) { + // Verifies no data race when Store and GetProcessor + // are called concurrently (the fix for the data race bug). + store := NewStore(defaultImagePrefix, functionCacheDir) + + obj := &configapi.FunctionConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "set-namespace", Namespace: testNamespace}, + Spec: configapi.FunctionConfigSpec{ + Image: "set-namespace", + Prefixes: []string{""}, + GoExecutor: &configapi.GoExecutorConfig{Tags: []string{"v0.4.1"}}, + }, + } + + done := make(chan struct{}) + + // Writer goroutine + go func() { + defer close(done) + for i := 0; i < 100; i++ { + // TODO: unsure if this can be replicated as intended with the new Store() method + _ = store.Store(obj) + } + }() + + // Reader goroutine (concurrent with writer) + for i := 0; i < 100; i++ { + store.GetProcessor("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1") + } + + <-done + + // After all writes complete, the entry should be present + _, found := store.GetProcessor("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1") + assert.True(t, found) +} diff --git a/controllers/main.go b/controllers/main.go index c8f956f26..c3a2985d8 100644 --- a/controllers/main.go +++ b/controllers/main.go @@ -39,7 +39,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "github.com/kptdev/kpt/pkg/lib/runneroptions" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" + "github.com/kptdev/porch/controllers/functionconfigs" "github.com/kptdev/porch/controllers/packagerevisions/pkg/controllers/packagerevision" "github.com/kptdev/porch/controllers/packagevariants/pkg/controllers/packagevariant" "github.com/kptdev/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset" @@ -299,17 +299,17 @@ func setupReconciler(mgr ctrl.Manager, enabled []string, r Reconciler, started [ return append(started, name), nil } -func setupFunctionConfigReconciler(mgr ctrl.Manager) (*reconciler.FunctionConfigStore, error) { +func setupFunctionConfigReconciler(mgr ctrl.Manager) (*functionconfigs.FunctionConfigStore, error) { prefix := os.Getenv("DEFAULT_IMAGE_PREFIX") if prefix == "" { prefix = runneroptions.GHCRImagePrefix } - functionConfigStore := reconciler.NewFunctionConfigStore(prefix, "") + functionConfigStore := functionconfigs.NewStore(prefix, "") - rec := &reconciler.FunctionConfigReconciler{ + rec := &functionconfigs.FunctionConfigReconciler{ Client: mgr.GetClient(), FunctionConfigStore: functionConfigStore, - For: reconciler.ReconcilerForController, + For: functionconfigs.ReconcilerForController, } if err := ctrl.NewControllerManagedBy(mgr). @@ -321,7 +321,7 @@ func setupFunctionConfigReconciler(mgr ctrl.Manager) (*reconciler.FunctionConfig prePopulateFunctionConfigStore(mgr.GetAPIReader(), functionConfigStore) - klog.Infof("FunctionConfig reconciler registered (for: %s)", reconciler.ReconcilerForController) + klog.Infof("FunctionConfig reconciler registered (for: %s)", functionconfigs.ReconcilerForController) return functionConfigStore, nil } @@ -329,7 +329,7 @@ func setupFunctionConfigReconciler(mgr ctrl.Manager) (*reconciler.FunctionConfig // synchronously so the exec cache is ready before the PR controller starts. // Without this, a pod restart leaves the cache empty until the async // informer triggers reconciliation. -func prePopulateFunctionConfigStore(reader client.Reader, store *reconciler.FunctionConfigStore) { +func prePopulateFunctionConfigStore(reader client.Reader, store *functionconfigs.FunctionConfigStore) { var fcList configapi.FunctionConfigList if err := reader.List(context.Background(), &fcList); err != nil { klog.Warningf("FunctionConfig pre-population failed (non-fatal): %v", err) @@ -337,12 +337,9 @@ func prePopulateFunctionConfigStore(reader client.Reader, store *reconciler.Func } for i := range fcList.Items { obj := &fcList.Items[i] - store.UpsertFunctionConfig(obj.Name, obj) - if obj.Spec.GoExecutor != nil { - store.UpdateExecCache(obj.Name, obj) - } - if obj.Spec.BinaryExecutor != nil { - store.UpdateBinaryCache(obj.Name, obj) + err := store.Store(obj) + if err != nil { + klog.Warningf("Failed to store %q during pre-populate (non-fatal): %v", client.ObjectKeyFromObject(obj), err) } } klog.Infof("FunctionConfig store pre-populated with %d configs", len(fcList.Items)) diff --git a/controllers/main_test.go b/controllers/main_test.go index da5005f3d..1f9ecc709 100644 --- a/controllers/main_test.go +++ b/controllers/main_test.go @@ -20,7 +20,7 @@ import ( "testing" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" + "github.com/kptdev/porch/controllers/functionconfigs" mockclient "github.com/kptdev/porch/test/mockery/mocks/external/sigs.k8s.io/controller-runtime/pkg/client" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -144,6 +144,7 @@ func TestPrePopulateFunctionConfigStore_Success(t *testing.T) { items := []configapi.FunctionConfig{ { Spec: configapi.FunctionConfigSpec{ + Image: "set-namespace", GoExecutor: &configapi.GoExecutorConfig{ Tags: []string{"v0.4.1"}, }, @@ -151,19 +152,16 @@ func TestPrePopulateFunctionConfigStore_Success(t *testing.T) { }, { Spec: configapi.FunctionConfigSpec{ + Image: "starlark", BinaryExecutor: &configapi.BinaryExecutorConfig{ Tags: []string{"v1.0.0"}, Path: "/usr/local/bin/starlark", }, }, }, - { - Spec: configapi.FunctionConfigSpec{}, - }, } items[0].Name = "set-namespace" items[1].Name = "starlark" - items[2].Name = "no-executor" mockReader := mockclient.NewMockReader(t) mockReader.EXPECT().List(mock.Anything, mock.AnythingOfType("*v1alpha1.FunctionConfigList"), mock.Anything). @@ -171,25 +169,23 @@ func TestPrePopulateFunctionConfigStore_Success(t *testing.T) { list.(*configapi.FunctionConfigList).Items = items }).Return(nil) - store := reconciler.NewFunctionConfigStore("ghcr.io/kptdev", "/tmp/bins") + store := functionconfigs.NewStore("ghcr.io/kptdev", "/tmp/bins") prePopulateFunctionConfigStore(mockReader, store) - _, ok := store.GetFunctionConfig("set-namespace") + _, ok := store.Get("set-namespace:v0.4.1") assert.True(t, ok, "set-namespace should be in store") - _, ok = store.GetFunctionConfig("starlark") + _, ok = store.Get("starlark:v1.0.0") assert.True(t, ok, "starlark should be in store") - _, ok = store.GetFunctionConfig("no-executor") - assert.True(t, ok, "no-executor should be in store") } func TestPrePopulateFunctionConfigStore_ListError(t *testing.T) { mockReader := mockclient.NewMockReader(t) mockReader.EXPECT().List(mock.Anything, mock.Anything, mock.Anything).Return(assert.AnError) - store := reconciler.NewFunctionConfigStore("ghcr.io/kptdev", "/tmp/bins") + store := functionconfigs.NewStore("ghcr.io/kptdev", "/tmp/bins") prePopulateFunctionConfigStore(mockReader, store) - _, ok := store.GetFunctionConfig("anything") + _, ok := store.Get("anything") assert.False(t, ok, "store should be empty after list error") } @@ -198,8 +194,8 @@ func TestPrePopulateFunctionConfigStore_EmptyList(t *testing.T) { mockReader.EXPECT().List(mock.Anything, mock.AnythingOfType("*v1alpha1.FunctionConfigList"), mock.Anything). Return(nil) - store := reconciler.NewFunctionConfigStore("ghcr.io/kptdev", "/tmp/bins") + store := functionconfigs.NewStore("ghcr.io/kptdev", "/tmp/bins") prePopulateFunctionConfigStore(mockReader, store) - assert.Equal(t, 0, len(store.List())) + assert.Equal(t, 0, store.Len()) } diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/config_test.go b/controllers/packagerevisions/pkg/controllers/packagerevision/config_test.go index 280cfba90..a35cf5a71 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/config_test.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/config_test.go @@ -4,7 +4,7 @@ import ( "flag" "testing" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" + "github.com/kptdev/porch/controllers/functionconfigs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -62,7 +62,7 @@ func TestInit_NilCache(t *testing.T) { r := &PackageRevisionReconciler{ RepoOperationRetryAttempts: 3, MaxGRPCMessageSize: defaultMaxGRPCMessageSize, - FunctionConfigStore: reconciler.NewFunctionConfigStore("", ""), + FunctionConfigStore: functionconfigs.NewStore("", ""), } err := r.Init(mgr) require.NoError(t, err) @@ -75,7 +75,7 @@ func TestInit_SetsCredResolverAndFetcher(t *testing.T) { r := &PackageRevisionReconciler{ RepoOperationRetryAttempts: 3, MaxGRPCMessageSize: defaultMaxGRPCMessageSize, - FunctionConfigStore: reconciler.NewFunctionConfigStore("", ""), + FunctionConfigStore: functionconfigs.NewStore("", ""), } err := r.Init(mgr) @@ -93,7 +93,7 @@ func TestInit_RendererEnabledWithFnRunner(t *testing.T) { r := &PackageRevisionReconciler{ RepoOperationRetryAttempts: 3, MaxGRPCMessageSize: defaultMaxGRPCMessageSize, - FunctionConfigStore: reconciler.NewFunctionConfigStore("", ""), + FunctionConfigStore: functionconfigs.NewStore("", ""), } t.Setenv("FUNCTION_RUNNER_ADDRESS", "localhost:0") diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go b/controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go index ab16759a4..b6aaf9669 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go @@ -20,7 +20,7 @@ import ( "time" porchv1alpha2 "github.com/kptdev/porch/api/porch/v1alpha2" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" + "github.com/kptdev/porch/controllers/functionconfigs" "github.com/kptdev/porch/pkg/repository" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -50,7 +50,7 @@ type PackageRevisionReconciler struct { Scheme *runtime.Scheme ContentCache repository.ContentCache ExternalPackageFetcher repository.ExternalPackageFetcher - FunctionConfigStore *reconciler.FunctionConfigStore + FunctionConfigStore *functionconfigs.FunctionConfigStore Renderer renderer // nil = skip rendering MaxConcurrentReconciles int diff --git a/func/Makefile b/func/Makefile index 01116b6dd..2c13763bb 100644 --- a/func/Makefile +++ b/func/Makefile @@ -16,19 +16,18 @@ IMAGE_TAG ?= latest IMAGE_REPO ?= ghcr.io/kptdev IMAGE_NAME ?= function-runner WRAPPER_SERVER_IMAGE_NAME ?= wrapper-server -COMPILED_PROTO=evaluator/evaluator_grpc.pb.go evaluator/evaluator.pb.go +COMPILED_PROTO=proto/evaluator_grpc.pb.go proto/evaluator.pb.go PORCHDIR = $(abspath $(CURDIR)/..) all: $(COMPILED_PROTO) -$(COMPILED_PROTO): evaluator/evaluator.proto +$(COMPILED_PROTO): proto/evaluator.proto protoc \ - -I /usr/local/include/google/protobuf \ - -I ./evaluator \ - --go_out=./evaluator --go_opt=paths=source_relative \ - --go-grpc_out=./evaluator --go-grpc_opt=paths=source_relative \ - ./evaluator/evaluator.proto + -I ./proto \ + --go_out=./proto --go_opt=paths=source_relative \ + --go-grpc_out=./proto --go-grpc_opt=paths=source_relative \ + ./proto/evaluator.proto .PHONY: build-image docker-build build-image docker-build: diff --git a/func/client/main.go b/func/client/main.go index ae031fe59..f4e5b0fa7 100644 --- a/func/client/main.go +++ b/func/client/main.go @@ -23,7 +23,7 @@ import ( "strings" "time" - pb "github.com/kptdev/porch/func/evaluator" + pb "github.com/kptdev/porch/func/proto" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" "sigs.k8s.io/kustomize/kyaml/kio" diff --git a/func/internal/executableevaluator.go b/func/evaluator/executableevaluator.go similarity index 70% rename from func/internal/executableevaluator.go rename to func/evaluator/executableevaluator.go index 5f45404f3..4b57a4d66 100644 --- a/func/internal/executableevaluator.go +++ b/func/evaluator/executableevaluator.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "bytes" @@ -22,8 +22,9 @@ import ( kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" "github.com/kptdev/kpt/pkg/fn" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" - pb "github.com/kptdev/porch/func/evaluator" + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + "github.com/kptdev/porch/controllers/functionconfigs" + pb "github.com/kptdev/porch/func/proto" regclientref "github.com/regclient/regclient/types/ref" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -36,20 +37,23 @@ type ExecutableEvaluatorOptions struct { type executableEvaluator struct { // Fast-path function cache - FunctionConfigStore *reconciler.FunctionConfigStore + FunctionConfigStore *functionconfigs.FunctionConfigStore } var _ Evaluator = &executableEvaluator{} -func NewExecutableEvaluator(FunctionConfigStore *reconciler.FunctionConfigStore) (Evaluator, error) { +func NewExecutableEvaluator(FunctionConfigStore *functionconfigs.FunctionConfigStore) (Evaluator, error) { return &executableEvaluator{ FunctionConfigStore: FunctionConfigStore, }, nil } func (e *executableEvaluator) EvaluateFunction(ctx context.Context, req *pb.EvaluateFunctionRequest) (*pb.EvaluateFunctionResponse, error) { - var selectedBinary string + var configSpec configapi.FunctionConfigSpec + var exists bool + if req.Tag != "" { + // TODO: use imageutil.Parse ref, err := regclientref.New(req.Image) if err != nil { return nil, fmt.Errorf("failed to parse image %q as reference: %w", req.Image, err) @@ -58,27 +62,21 @@ func (e *executableEvaluator) EvaluateFunction(ctx context.Context, req *pb.Eval ref.Digest = "" req.Image = ref.CommonName() - binary, exists := e.FunctionConfigStore.GetBinaryFromCacheByConstraint(req.Image, req.Tag) - if !exists { - return nil, &fn.NotFoundError{ - Function: kptfilev1.Function{Image: req.Image}, - } - } - selectedBinary = binary + configSpec, exists = e.FunctionConfigStore.GetByConstraint(req.Image, req.Tag) } else { - klog.Infof("Image tag is empty, using the image with explicit tag: %q", req.Image) - binary, exists := e.FunctionConfigStore.GetBinaryFromCache(req.Image) - if !exists { - return nil, &fn.NotFoundError{ - Function: kptfilev1.Function{Image: req.Image}, - } + klog.V(2).Infof("Image tag is empty, using the image with explicit tag: %q", req.Image) + configSpec, exists = e.FunctionConfigStore.Get(req.Image) + } + + if !exists || configSpec.BinaryExecutor == nil { + return nil, &fn.NotFoundError{ + Function: kptfilev1.Function{Image: req.Image}, } - selectedBinary = binary } klog.Infof("Evaluating %q in executable mode", req.Image) var stdout, stderr bytes.Buffer - cmd := exec.CommandContext(ctx, selectedBinary) // #nosec G204 -- variables controlled internally + cmd := exec.CommandContext(ctx, configSpec.BinaryExecutor.Path) // #nosec G204 -- variables controlled internally cmd.Stdin = bytes.NewReader(req.ResourceList) cmd.Stdout = &stdout cmd.Stderr = &stderr diff --git a/func/internal/executableevaluator_test.go b/func/evaluator/executableevaluator_test.go similarity index 79% rename from func/internal/executableevaluator_test.go rename to func/evaluator/executableevaluator_test.go index 88b307aa3..386dac2e4 100644 --- a/func/internal/executableevaluator_test.go +++ b/func/evaluator/executableevaluator_test.go @@ -12,32 +12,36 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "bytes" + "flag" "fmt" "os" + "path/filepath" "testing" kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" "github.com/kptdev/kpt/pkg/fn" + "github.com/kptdev/kpt/pkg/lib/runneroptions" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" - pb "github.com/kptdev/porch/func/evaluator" - "github.com/kptdev/porch/pkg/util" + "github.com/kptdev/porch/controllers/functionconfigs" + pb "github.com/kptdev/porch/func/proto" + imageutil "github.com/kptdev/porch/pkg/util/image" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/klog/v2" ) const ( - defaultKRMImagePrefix = "ghcr.io/kptdev/krm-functions-catalog/" + defaultKRMImagePrefix = runneroptions.GHCRImagePrefix setImageFunction = "set-image" starlarkFunction = "starlark" ) -func getFunctionConfigStore(binaryDir string) *reconciler.FunctionConfigStore { +func getFunctionConfigStore(t *testing.T, binaryDir string) *functionconfigs.FunctionConfigStore { + t.Helper() starlarkConfig := &configapi.FunctionConfig{ Spec: configapi.FunctionConfigSpec{ Image: starlarkFunction, @@ -68,9 +72,9 @@ func getFunctionConfigStore(binaryDir string) *reconciler.FunctionConfigStore { }, }, } - fstore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, binaryDir) - fstore.UpdateBinaryCache(starlarkFunction, starlarkConfig) - fstore.UpdateBinaryCache(setImageFunction, setImageConfig) + fstore := functionconfigs.NewStore(defaultKRMImagePrefix, binaryDir) + require.NoError(t, fstore.Store(starlarkConfig)) + require.NoError(t, fstore.Store(setImageConfig)) return fstore } @@ -80,13 +84,17 @@ func TestNewExecutableEvaluator(t *testing.T) { executableEvaluatorOptions := ExecutableEvaluatorOptions{ FunctionCacheDir: tempCacheDir, } - fStore := getFunctionConfigStore(executableEvaluatorOptions.FunctionCacheDir) + fStore := getFunctionConfigStore(t, executableEvaluatorOptions.FunctionCacheDir) _, err := NewExecutableEvaluator(fStore) assert.NoError(t, err) }) } func TestEvaluateExecutableFunction(t *testing.T) { + flagSet := flag.NewFlagSet("log-level", flag.ContinueOnError) + klog.InitFlags(flagSet) + _ = flagSet.Parse([]string{"--v", "5"}) + const tempCacheDir = "/tmp/func_cache" t.Run("invalid semver constraint will cause function not found error", func(t *testing.T) { ctx := t.Context() @@ -96,11 +104,11 @@ func TestEvaluateExecutableFunction(t *testing.T) { req := &pb.EvaluateFunctionRequest{ ResourceList: []byte("req-rl"), - Image: util.ImageJoin(defaultKRMImagePrefix, testImageName), + Image: imageutil.Join(defaultKRMImagePrefix, testImageName), Tag: ">> 0.1.3 < 0.2.0", // Invalid semver constraint, '>>' is not a valid operator } - fStore := getFunctionConfigStore(executableEvaluatorOptions.FunctionCacheDir) + fStore := getFunctionConfigStore(t, executableEvaluatorOptions.FunctionCacheDir) evaluator, _ := NewExecutableEvaluator(fStore) resp, err := evaluator.EvaluateFunction(ctx, req) @@ -118,11 +126,11 @@ func TestEvaluateExecutableFunction(t *testing.T) { req := &pb.EvaluateFunctionRequest{ ResourceList: []byte("req-rl"), // This image is not included in the config.yaml -> function not found - Image: util.ImageJoin(defaultKRMImagePrefix, testImageName), + Image: imageutil.Join(defaultKRMImagePrefix, testImageName), Tag: "> 0.1.3 < 0.2.0", // This is a valid semver constraint syntax } - fStore := getFunctionConfigStore(executableEvaluatorOptions.FunctionCacheDir) + fStore := getFunctionConfigStore(t, executableEvaluatorOptions.FunctionCacheDir) evaluator, _ := NewExecutableEvaluator(fStore) _, err := evaluator.EvaluateFunction(ctx, req) assert.Equal(t, fmt.Sprintf("function \"%s\" not found", req.Image), err.Error()) @@ -135,11 +143,11 @@ func TestEvaluateExecutableFunction(t *testing.T) { req := &pb.EvaluateFunctionRequest{ ResourceList: []byte("req-rl"), - Image: util.ImageJoin(defaultKRMImagePrefix, setImageFunction), + Image: imageutil.Join(defaultKRMImagePrefix, setImageFunction), Tag: "> 0.1.3 < 0.2.0", } - fStore := getFunctionConfigStore(executableEvaluatorOptions.FunctionCacheDir) + fStore := getFunctionConfigStore(t, executableEvaluatorOptions.FunctionCacheDir) evaluator, _ := NewExecutableEvaluator(fStore) _, err := evaluator.EvaluateFunction(ctx, req) assert.ErrorContains(t, err, fmt.Sprintf("function \"%s\" not found", req.Image), err.Error()) @@ -152,11 +160,11 @@ func TestEvaluateExecutableFunction(t *testing.T) { req := &pb.EvaluateFunctionRequest{ ResourceList: []byte("req-rl"), - Image: util.ImageJoin(defaultKRMImagePrefix, setImageFunction), + Image: imageutil.Join(defaultKRMImagePrefix, setImageFunction), Tag: ">= 0.1.2 < 0.2.0", } - fStore := getFunctionConfigStore(executableEvaluatorOptions.FunctionCacheDir) + fStore := getFunctionConfigStore(t, executableEvaluatorOptions.FunctionCacheDir) evaluator, _ := NewExecutableEvaluator(fStore) _, err := evaluator.EvaluateFunction(ctx, req) @@ -169,14 +177,14 @@ func TestEvaluateExecutableFunction(t *testing.T) { tmpDir := t.TempDir() // Create a simple test executable that echoes input as a valid KRM function - testBinary := util.ImageJoin(tmpDir, setImageFunction) + testBinary := filepath.Join(tmpDir, setImageFunction) const testScript = `#!/bin/sh # Emulating the KRM function execution by running this shell script cat exit 0 ` err := os.WriteFile(testBinary, []byte(testScript), 0755) - assert.NoError(t, err) + require.NoError(t, err) executableEvaluatorOptions := ExecutableEvaluatorOptions{ FunctionCacheDir: tmpDir, @@ -192,7 +200,7 @@ items: [] // We expect v0.1.3 to be selected as it's the greatest version req := &pb.EvaluateFunctionRequest{ ResourceList: []byte(resourceList), - Image: util.ImageJoin(defaultKRMImagePrefix, setImageFunction), + Image: imageutil.Join(defaultKRMImagePrefix, setImageFunction), Tag: ">= 0.1.2 < 0.2.0", } @@ -201,7 +209,7 @@ items: [] r, w, _ := os.Pipe() os.Stderr = w - fStore := getFunctionConfigStore(executableEvaluatorOptions.FunctionCacheDir) + fStore := getFunctionConfigStore(t, executableEvaluatorOptions.FunctionCacheDir) evaluator, err := NewExecutableEvaluator(fStore) require.NoError(t, err) @@ -221,9 +229,7 @@ items: [] assert.NotNil(t, resp) // Verify the klog message contains the expected version selection - assert.Contains(t, logOutput, `Selected image "ghcr.io/kptdev/krm-functions-catalog/set-image:v0.1.3"`) - assert.Contains(t, logOutput, `(version "0.1.3")`) - assert.Contains(t, logOutput, `for request "ghcr.io/kptdev/krm-functions-catalog/set-image"`) + assert.Contains(t, logOutput, `Selected tag "v0.1.3"`) }) t.Run("successful function execution with explicit tagging", func(t *testing.T) { ctx := t.Context() @@ -232,7 +238,7 @@ items: [] tmpDir := t.TempDir() // Create a simple test executable that echoes input as a valid KRM function - testBinary := util.ImageJoin(tmpDir, setImageFunction) + testBinary := filepath.Join(tmpDir, setImageFunction) const testScript = `#!/bin/sh # Emulating the KRM function execution by running this shell script cat @@ -254,7 +260,7 @@ items: [] // Explicit tagging req := &pb.EvaluateFunctionRequest{ ResourceList: []byte(resourceList), - Image: util.ImageJoin(defaultKRMImagePrefix, setImageFunction) + ":v0.1.3", + Image: imageutil.Join(defaultKRMImagePrefix, setImageFunction) + ":v0.1.3", } // Capture klog output by redirecting stderr @@ -262,7 +268,7 @@ items: [] r, w, _ := os.Pipe() os.Stderr = w - fStore := getFunctionConfigStore(executableEvaluatorOptions.FunctionCacheDir) + fStore := getFunctionConfigStore(t, executableEvaluatorOptions.FunctionCacheDir) evaluator, err := NewExecutableEvaluator(fStore) require.NoError(t, err) @@ -291,7 +297,7 @@ items: [] tmpDir := t.TempDir() // Create a simple test executable that echoes input as a valid KRM function - testBinary := util.ImageJoin(tmpDir, setImageFunction) + testBinary := filepath.Join(tmpDir, setImageFunction) const testScript = `#!/bin/sh # Emulating the KRM function execution by running this shell script cat @@ -312,7 +318,7 @@ items: [] req := &pb.EvaluateFunctionRequest{ ResourceList: []byte(resourceList), - Image: util.ImageJoin(defaultKRMImagePrefix, setImageFunction) + ":v0.0.1", + Image: imageutil.Join(defaultKRMImagePrefix, setImageFunction) + ":v0.0.1", Tag: ">= 0.1.2 < 0.2.0", } @@ -321,7 +327,7 @@ items: [] r, w, _ := os.Pipe() os.Stderr = w - fStore := getFunctionConfigStore(executableEvaluatorOptions.FunctionCacheDir) + fStore := getFunctionConfigStore(t, executableEvaluatorOptions.FunctionCacheDir) evaluator, err := NewExecutableEvaluator(fStore) require.NoError(t, err) @@ -341,8 +347,6 @@ items: [] assert.NotNil(t, resp) // Verify the klog message contains the expected version selection - assert.Contains(t, logOutput, `Selected image "ghcr.io/kptdev/krm-functions-catalog/set-image:v0.1.3"`) - assert.Contains(t, logOutput, `(version "0.1.3")`) - assert.Contains(t, logOutput, `for request "ghcr.io/kptdev/krm-functions-catalog/set-image"`) + assert.Contains(t, logOutput, `Selected tag "v0.1.3"`) }) } diff --git a/func/internal/multievaluator.go b/func/evaluator/multievaluator.go similarity index 96% rename from func/internal/multievaluator.go rename to func/evaluator/multievaluator.go index a299d37b9..42cd9e549 100644 --- a/func/internal/multievaluator.go +++ b/func/evaluator/multievaluator.go @@ -12,14 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "context" "errors" "github.com/kptdev/kpt/pkg/fn" - pb "github.com/kptdev/porch/func/evaluator" + pb "github.com/kptdev/porch/func/proto" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) diff --git a/func/internal/podcachemanager.go b/func/evaluator/podcachemanager.go similarity index 65% rename from func/internal/podcachemanager.go rename to func/evaluator/podcachemanager.go index 77e898bbb..ef7d151db 100644 --- a/func/internal/podcachemanager.go +++ b/func/evaluator/podcachemanager.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "context" @@ -23,9 +23,8 @@ import ( "sync/atomic" "time" - configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - fnconf "github.com/kptdev/porch/controllers/functionconfigs/reconciler" - "github.com/kptdev/porch/pkg/util" + "github.com/kptdev/porch/controllers/functionconfigs" + . "github.com/kptdev/porch/func/types" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/util/wait" @@ -45,52 +44,31 @@ type podCacheManager struct { podTTL time.Duration // connectionRequestCh receives requests for a connection to a KRM function evaluator pod - connectionRequestCh <-chan *connectionRequest + connectionRequestCh <-chan *ConnectionRequest // podReadyCh is a channel to receive the information when a pod is ready. - podReadyCh <-chan *podReadyResponse + podReadyCh <-chan *PodReadyResponse // functions maps KRM function image names to its pods and waitlist information. - functions map[string]*functionInfo + functions map[string]*FunctionInfo podManager *podManager maxWaitlistLength int maxParallelPodsPerFunction int - functionConfigMap *fnconf.FunctionConfigStore + functionConfigMap *functionconfigs.FunctionConfigStore } -// functionInfo holds the list of all pod instances for the same KRM function image. -type functionInfo struct { - // status of all pods belonging to the same KRM function image - pods []functionPodInfo - // roundRobinIdx is used to distribute requests across pods when all have equal load - roundRobinIdx int -} - -// functionPodInfo represents the state of a single pod instance. -type functionPodInfo struct { - // podData contains the information about the pod, returned by the podManager - // It is nil until the pod is actually started - *podData - // waitlist is used to temporarily store connection requests until the pod is started - waitlist []chan<- *connectionResponse - // time of last function evaluation, used by the garbage collector to identify idle pods - lastActivity time.Time - // the number of currently ongoing and waiting fn evaluations in the pod - concurrentEvaluations *atomic.Int32 -} - -func (pcm *podCacheManager) redistributeLoad(image string, fn *functionInfo, connections []chan<- *connectionResponse) bool { +func (pcm *podCacheManager) redistributeLoad(image string, fn *FunctionInfo, connections []chan<- *ConnectionResponse) bool { pcm.removeUnhealthyPods(fn, false) redistributed := false for _, ch := range connections { bestPodIndex, _ := pcm.findBestPod(fn) if bestPodIndex != -1 { - pod := pcm.functions[image].pods[bestPodIndex] - if pod.podData != nil { + pod := pcm.functions[image].Pods[bestPodIndex] + if pod.PodData != nil { pod.SendResponse(ch, nil) } else { - pod.waitlist = append(pod.waitlist, ch) + pod.Waitlist = append(pod.Waitlist, ch) } redistributed = true } @@ -109,90 +87,86 @@ func (pcm *podCacheManager) podCacheManager(ctx context.Context) { select { case req := <-pcm.connectionRequestCh: if pcm.podManager.imageResolver != nil { - req.image = pcm.podManager.imageResolver(req.image) + req.Image = pcm.podManager.imageResolver(req.Image) } - fn := pcm.FunctionInfo(req.image) + fn := pcm.FunctionInfo(req.Image) shouldScaleUp := false pcm.removeUnhealthyPods(fn, false) bestPodIndex, bestWaitlistLen := pcm.findBestPod(fn) - _, maxWaitlist, maxPods := pcm.getParamsForImage(req.image) + _, maxWaitlist, maxPods := pcm.getParamsForImage(req.Image) if bestPodIndex == -1 { shouldScaleUp = true } else { - if bestWaitlistLen >= maxWaitlist && len(fn.pods) < maxPods { + if bestWaitlistLen >= maxWaitlist && len(fn.Pods) < maxPods { shouldScaleUp = true } } if shouldScaleUp { - klog.Infof("Scaling up for image %s. No idle pods available. Starting a new pod.", req.image) + klog.Infof("Scaling up for image %s. No idle pods available. Starting a new pod.", req.Image) - fn.pods = append(fn.pods, NewPodInfo(req.responseCh)) + fn.Pods = append(fn.Pods, NewPodInfo(req.ResponseCh)) - functionConfig, exists := pcm.functionConfigMap.GetFunctionConfig(util.GetImageName(req.image)) - if !exists { - functionConfig = &configapi.FunctionConfig{} - } - - go pcm.podManager.getFuncEvalPodClient(context.Background(), req.image, len(fn.pods), functionConfig.Spec.PodExecutor, true) + config, _ := pcm.functionConfigMap.Get(req.Image) + go pcm.podManager.getFuncEvalPodClient(context.Background(), req.Image, len(fn.Pods), config.PodExecutor, true) } else { - pod := &fn.pods[bestPodIndex] - klog.Infof("Queuing request for %s on pod instance #%d (queue length will be %d)", req.image, bestPodIndex, bestWaitlistLen+1) - pod.lastActivity = time.Now() - pod.concurrentEvaluations.Add(1) - if pod.podData != nil { - pod.SendResponse(req.responseCh, nil) + pod := &fn.Pods[bestPodIndex] + klog.Infof("Queuing request for %s on pod instance #%d (queue length will be %d)", req.Image, bestPodIndex, bestWaitlistLen+1) + pod.LastActivity = time.Now() + pod.ConcurrentEvaluations.Add(1) + if pod.PodData != nil { + pod.SendResponse(req.ResponseCh, nil) } else { - pod.waitlist = append(pod.waitlist, req.responseCh) + pod.Waitlist = append(pod.Waitlist, req.ResponseCh) } } case podReadyMsg := <-pcm.podReadyCh: - if podReadyMsg.image == "" { + if podReadyMsg.Image == "" { klog.Error("Received a 'pod ready' message with an empty KRM image name. This indicates a logical error in the code.") continue } - fn, ok := pcm.functions[podReadyMsg.image] + fn, ok := pcm.functions[podReadyMsg.Image] if !ok { - klog.Errorf("Received a ready pod for %q, but the KRM function is missing from the pool! Ignoring.", podReadyMsg.image) + klog.Errorf("Received a ready pod for %q, but the KRM function is missing from the pool! Ignoring.", podReadyMsg.Image) continue } // Find the first pod with nil podData, which means it is pending creation. - toUpdate := slices.IndexFunc(fn.pods, func(pod functionPodInfo) bool { - return pod.podData == nil + toUpdate := slices.IndexFunc(fn.Pods, func(pod FunctionPodInfo) bool { + return pod.PodData == nil }) if toUpdate == -1 { - klog.Errorf("Received a ready pod for %q, but no pending instance was found in the pod pool. Total of %d pods was in the pool. Ignoring.", podReadyMsg.image, len(fn.pods)) + klog.Errorf("Received a ready pod for %q, but no pending instance was found in the pod pool. Total of %d pods was in the pool. Ignoring.", podReadyMsg.Image, len(fn.Pods)) continue } - if podReadyMsg.err != nil { - klog.Warningf("Pod creation failed for image %s: %v", podReadyMsg.image, podReadyMsg.err) - waitListToRedistribute := fn.pods[toUpdate].waitlist - failedPod := fn.pods[toUpdate] - fn.pods = slices.Delete(fn.pods, toUpdate, toUpdate+1) + if podReadyMsg.Err != nil { + klog.Warningf("Pod creation failed for image %s: %v", podReadyMsg.Image, podReadyMsg.Err) + waitListToRedistribute := fn.Pods[toUpdate].Waitlist + failedPod := fn.Pods[toUpdate] + fn.Pods = slices.Delete(fn.Pods, toUpdate, toUpdate+1) redistributed := false - if len(fn.pods) > 0 { - redistributed = pcm.redistributeLoad(podReadyMsg.image, fn, waitListToRedistribute) + if len(fn.Pods) > 0 { + redistributed = pcm.redistributeLoad(podReadyMsg.Image, fn, waitListToRedistribute) } if !redistributed { for _, ch := range waitListToRedistribute { - failedPod.SendResponse(ch, podReadyMsg.err) + failedPod.SendResponse(ch, podReadyMsg.Err) } } - pcm.DeletePodWithServiceInBackgroundByObjectKey(podReadyMsg.podData) + pcm.DeletePodWithServiceInBackgroundByObjectKey(podReadyMsg.PodData) continue } - pod := &fn.pods[toUpdate] - pod.podData = &podReadyMsg.podData - pod.lastActivity = time.Now() - klog.Infof("New pod %s is ready for image %s. Total number of pods for image: %d", podReadyMsg.podKey.Name, podReadyMsg.image, len(fn.pods)) - for _, ch := range pod.waitlist { + pod := &fn.Pods[toUpdate] + pod.PodData = &podReadyMsg.PodData + pod.LastActivity = time.Now() + klog.Infof("New pod %s is ready for image %s. Total number of pods for image: %d", podReadyMsg.PodKey.Name, podReadyMsg.Image, len(fn.Pods)) + for _, ch := range pod.Waitlist { pod.SendResponse(ch, nil) } - pod.waitlist = nil + pod.Waitlist = nil case <-tick: pcm.garbageCollector() @@ -208,8 +182,8 @@ func (pcm *podCacheManager) podCacheManager(ctx context.Context) { // If the image is present in the configMap, it returns the specific parameters for that image. // Otherwise, it falls back to the global defaults (pcm.podTTL, pcm.maxWaitlistLength, pcm.maxParallelPodsPerFunction). func (pcm *podCacheManager) getParamsForImage(image string) (ttl time.Duration, maxWaitlist, maxPods int) { - if entry, ok := pcm.functionConfigMap.GetFunctionConfig(util.GetImageName(image)); ok && entry.Spec.PodExecutor != nil { - podExecutorConfig := entry.Spec.PodExecutor + if entry, ok := pcm.functionConfigMap.Get(image); ok && entry.PodExecutor != nil { + podExecutorConfig := entry.PodExecutor parsedTTL := podExecutorConfig.TimeToLive.Duration if parsedTTL <= 0 { parsedTTL = pcm.podTTL @@ -227,10 +201,10 @@ func (pcm *podCacheManager) getParamsForImage(image string) (ttl time.Duration, return pcm.podTTL, pcm.maxWaitlistLength, pcm.maxParallelPodsPerFunction } -func (pcm *podCacheManager) FunctionInfo(image string) *functionInfo { +func (pcm *podCacheManager) FunctionInfo(image string) *FunctionInfo { fn, ok := pcm.functions[image] if !ok { - fn = &functionInfo{} + fn = &FunctionInfo{} pcm.functions[image] = fn } return fn @@ -278,14 +252,14 @@ func (pcm *podCacheManager) retrieveFunctionPods(ctx context.Context) error { image := pod.Spec.Containers[0].Image fn := pcm.FunctionInfo(image) - if len(fn.pods) < pcm.maxParallelPodsPerFunction && pod.Status.Phase == corev1.PodRunning { + if len(fn.Pods) < pcm.maxParallelPodsPerFunction && pod.Status.Phase == corev1.PodRunning { pData, err := pcm.podManager.createPodData(ctx, serviceKey, podKey, image) if err == nil { klog.Infof("retrieved function evaluator pod %s/%s for %s", pod.Namespace, pod.Name, image) - fn.pods = append(fn.pods, NewPodInfo(nil)) - pcm.podManager.podReadyCh <- &podReadyResponse{ - podData: *pData, - err: nil, + fn.Pods = append(fn.Pods, NewPodInfo(nil)) + pcm.podManager.podReadyCh <- &PodReadyResponse{ + PodData: *pData, + Err: nil, } continue } @@ -307,29 +281,27 @@ func (pcm *podCacheManager) warmupCache(defaultImagePrefix string) error { defer func() { klog.Infof("cache warming is completed and it took %v", time.Since(start)) }() - for _, entry := range pcm.functionConfigMap.List() { - if entry.Spec.PodExecutor != nil && len(entry.Spec.PodExecutor.Tags) > 0 { - image := entry.Spec.Image - if len(entry.Spec.PodExecutor.Tags[0]) > 0 { - image = fmt.Sprintf("%s:%s", entry.Spec.Image, entry.Spec.PodExecutor.Tags[0]) + for spec := range pcm.functionConfigMap.IterPodConfigSpecs() { + if spec.PodExecutor != nil { + image := spec.Image + if len(spec.PodExecutor.Tags) > 0 && len(spec.PodExecutor.Tags[0]) > 0 { + image += ":" + spec.PodExecutor.Tags[0] + } else { + image += ":latest" } - if len(entry.Spec.Prefixes) > 0 && entry.Spec.Prefixes[0] != "" { - image = ImageJoin(entry.Spec.Prefixes[0], image) + if len(spec.Prefixes) > 0 && spec.Prefixes[0] != "" { + image = ImageJoin(spec.Prefixes[0], image) } else { image = ImageJoin(defaultImagePrefix, image) } image = pcm.podManager.imageResolver(image) fn := pcm.FunctionInfo(image) - if len(fn.pods) == 0 { - fn.pods = append(fn.pods, NewPodInfo(nil)) + if len(fn.Pods) == 0 { + fn.Pods = append(fn.Pods, NewPodInfo(nil)) go func(fnImage string) { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() - functionConfig, exists := pcm.functionConfigMap.GetFunctionConfig(entry.Spec.Image) - if !exists { - functionConfig = &configapi.FunctionConfig{} - } - pcm.podManager.getFuncEvalPodClient(ctx, fnImage, 1, functionConfig.Spec.PodExecutor, false) + pcm.podManager.getFuncEvalPodClient(ctx, fnImage, 1, spec.PodExecutor, false) }(image) } } @@ -344,20 +316,20 @@ func ImageJoin(prefix, image string) string { // findBestPod returns with the index of the best pod for the given function. // It uses round-robin among pods with equal load to ensure even distribution. // If there are no suitable pods, it returns with -1. -func (pcm *podCacheManager) findBestPod(fn *functionInfo) (int, int) { +func (pcm *podCacheManager) findBestPod(fn *FunctionInfo) (int, int) { if fn == nil { return -1, 0 } - n := len(fn.pods) + n := len(fn.Pods) if n == 0 { return -1, 0 } minWaitlist := 0 // Find the minimum waitlist length across all pods - minWaitlist = fn.pods[0].WaitlistLen() + minWaitlist = fn.Pods[0].WaitlistLen() for i := 1; i < n; i++ { - wl := fn.pods[i].WaitlistLen() + wl := fn.Pods[i].WaitlistLen() if wl < minWaitlist { minWaitlist = wl } @@ -365,9 +337,9 @@ func (pcm *podCacheManager) findBestPod(fn *functionInfo) (int, int) { // Round-robin among pods that have the minimum waitlist length for i := 0; i < n; i++ { - idx := (fn.roundRobinIdx + i) % n - if fn.pods[idx].WaitlistLen() == minWaitlist { - fn.roundRobinIdx = (idx + 1) % n + idx := (fn.RoundRobinIdx + i) % n + if fn.Pods[idx].WaitlistLen() == minWaitlist { + fn.RoundRobinIdx = (idx + 1) % n return idx, minWaitlist } } @@ -378,57 +350,57 @@ func (pcm *podCacheManager) findBestPod(fn *functionInfo) (int, int) { // removeUnhealthyPods removes unhealthy pods from the function's pod list. // If removeIdle is true, it will also remove idle pods that have reached their TTL. -func (pcm *podCacheManager) removeUnhealthyPods(fn *functionInfo, removeIdle bool) { +func (pcm *podCacheManager) removeUnhealthyPods(fn *FunctionInfo, removeIdle bool) { if fn == nil { return } - fn.pods = slices.DeleteFunc(fn.pods, func(pod functionPodInfo) bool { + fn.Pods = slices.DeleteFunc(fn.Pods, func(pod FunctionPodInfo) bool { removeFromCache := false - if pod.podData == nil { + if pod.PodData == nil { // pod is under creation return false } k8sPod := &corev1.Pod{} - err := pcm.podManager.kubeClient.Get(context.Background(), *pod.podKey, k8sPod) + err := pcm.podManager.kubeClient.Get(context.Background(), *pod.PodKey, k8sPod) if err != nil { if apierrors.IsNotFound(err) { - klog.Infof("Removing deleted pod from cache for image %s", pod.image) + klog.Infof("Removing deleted pod from cache for image %s", pod.Image) } else { - klog.Errorf("Failed to get pod %v, removing from cache: %v", pod.podKey, err) + klog.Errorf("Failed to get pod %v, removing from cache: %v", pod.PodKey, err) } removeFromCache = true } service := &corev1.Service{} - err = pcm.podManager.kubeClient.Get(context.Background(), *pod.serviceKey, service) + err = pcm.podManager.kubeClient.Get(context.Background(), *pod.ServiceKey, service) if err != nil { if apierrors.IsNotFound(err) { - klog.Infof("Removing deleted service from cache for image %s", pod.image) + klog.Infof("Removing deleted service from cache for image %s", pod.Image) } else { - klog.Errorf("Failed to get service %v, removing from cache: %v", pod.serviceKey, err) + klog.Errorf("Failed to get service %v, removing from cache: %v", pod.ServiceKey, err) } removeFromCache = true } - err = pcm.podManager.kubeClient.Get(context.Background(), *pod.serviceKey, service) + err = pcm.podManager.kubeClient.Get(context.Background(), *pod.ServiceKey, service) if err != nil { - klog.Warningf("unable to find expected service %s namespace %s: %v", pod.serviceKey.Name, k8sPod.Namespace, err) + klog.Warningf("unable to find expected service %s namespace %s: %v", pod.ServiceKey.Name, k8sPod.Namespace, err) } if k8sPod.Status.Phase == corev1.PodFailed { - klog.Errorf("Evicting pod in failed state (%s/%s) from cache for image %s", k8sPod.Namespace, k8sPod.Name, pod.image) + klog.Errorf("Evicting pod in failed state (%s/%s) from cache for image %s", k8sPod.Namespace, k8sPod.Name, pod.Image) removeFromCache = true } serviceUrl := service.Name + "." + service.Namespace + serviceDnsNameSuffix - if net.JoinHostPort(serviceUrl, defaultWrapperServerPort) != pod.grpcConnection.Target() { - klog.Errorf("Evicting pod whose pod IP doesn't match with its grpc connection (%s/%s) from cache for image %s", k8sPod.Namespace, k8sPod.Name, pod.image) + if net.JoinHostPort(serviceUrl, defaultWrapperServerPort) != pod.GrpcConnection.Target() { + klog.Errorf("Evicting pod whose pod IP doesn't match with its grpc connection (%s/%s) from cache for image %s", k8sPod.Namespace, k8sPod.Name, pod.Image) removeFromCache = true } - ttl, _, _ := pcm.getParamsForImage(pod.image) - if removeIdle && pod.WaitlistLen() == 0 && time.Since(pod.lastActivity) > ttl { - klog.Infof("Removing idle pod %q that reached its TTL from cache for image %s", k8sPod.Name, pod.image) + ttl, _, _ := pcm.getParamsForImage(pod.Image) + if removeIdle && pod.WaitlistLen() == 0 && time.Since(pod.LastActivity) > ttl { + klog.Infof("Removing idle pod %q that reached its TTL from cache for image %s", k8sPod.Name, pod.Image) removeFromCache = true } @@ -450,27 +422,27 @@ func (pcm *podCacheManager) garbageCollector() { pcm.removeUnhealthyPods(fn, true) // Clean up empty slices - if len(fn.pods) == 0 { + if len(fn.Pods) == 0 { delete(pcm.functions, image) } } } -func (pcm *podCacheManager) DeletePodWithServiceInBackgroundByObjectKey(podData podData) { +func (pcm *podCacheManager) DeletePodWithServiceInBackgroundByObjectKey(podData PodData) { k8sPod := &corev1.Pod{} - if podData.podKey != nil { - err := pcm.podManager.kubeClient.Get(context.Background(), *podData.podKey, k8sPod) + if podData.PodKey != nil { + err := pcm.podManager.kubeClient.Get(context.Background(), *podData.PodKey, k8sPod) if err != nil { - klog.Warningf("unable to find pod %s in namespace: %s: %v", podData.podKey.Name, podData.podKey.Namespace, err) + klog.Warningf("unable to find pod %s in namespace: %s: %v", podData.PodKey.Name, podData.PodKey.Namespace, err) } pcm.DeletePodInBackground(k8sPod) } service := &corev1.Service{} - if podData.serviceKey != nil { - err := pcm.podManager.kubeClient.Get(context.Background(), *podData.serviceKey, service) + if podData.ServiceKey != nil { + err := pcm.podManager.kubeClient.Get(context.Background(), *podData.ServiceKey, service) if err != nil { - klog.Warningf("unable to find service %s in namespace %s: %v", podData.serviceKey.Name, podData.serviceKey.Namespace, err) + klog.Warningf("unable to find service %s in namespace %s: %v", podData.ServiceKey.Name, podData.ServiceKey.Namespace, err) } pcm.DeleteServiceInBackground(service) } @@ -519,43 +491,16 @@ func (pcm *podCacheManager) DeleteServiceInBackground(svc *corev1.Service) { }() } -func NewPodInfo(firstResponseCh chan<- *connectionResponse) functionPodInfo { - pod := functionPodInfo{ - waitlist: []chan<- *connectionResponse{}, - podData: nil, // This will be filled in when the pod is ready. - lastActivity: time.Now(), - concurrentEvaluations: &atomic.Int32{}, +func NewPodInfo(firstResponseCh chan<- *ConnectionResponse) FunctionPodInfo { + pod := FunctionPodInfo{ + Waitlist: []chan<- *ConnectionResponse{}, + PodData: nil, // This will be filled in when the pod is ready. + LastActivity: time.Now(), + ConcurrentEvaluations: &atomic.Int32{}, } if firstResponseCh != nil { - pod.waitlist = append(pod.waitlist, firstResponseCh) - pod.concurrentEvaluations.Add(1) + pod.Waitlist = append(pod.Waitlist, firstResponseCh) + pod.ConcurrentEvaluations.Add(1) } return pod } - -// SendResponse sends a reply to the connection request containing the pod data. -// If err != nil it sends `err` as an error response. -// It sends and error response if the pod is not ready yet (this shouldn't happen). -func (pod *functionPodInfo) SendResponse(responseCh chan<- *connectionResponse, err error) { - switch { - case err != nil: - responseCh <- &connectionResponse{ - err: err, - } - case pod.podData == nil: - responseCh <- &connectionResponse{ - err: fmt.Errorf("pod is not ready, connection response sent prematurely. This is logical error in the code"), - } - default: - responseCh <- &connectionResponse{ - podData: *pod.podData, - concurrentEvaluations: pod.concurrentEvaluations, - err: nil, - } - } -} - -// WaitlistLen returns with the number of fn evaluations currently handled by the pod -func (pod functionPodInfo) WaitlistLen() int { - return int(pod.concurrentEvaluations.Load()) -} diff --git a/func/internal/podcachemanager_eventloop_test.go b/func/evaluator/podcachemanager_eventloop_test.go similarity index 74% rename from func/internal/podcachemanager_eventloop_test.go rename to func/evaluator/podcachemanager_eventloop_test.go index 564a90bab..c9b237d01 100644 --- a/func/internal/podcachemanager_eventloop_test.go +++ b/func/evaluator/podcachemanager_eventloop_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "context" @@ -23,7 +23,8 @@ import ( "time" "github.com/kptdev/kpt/pkg/lib/runneroptions" - fnconf "github.com/kptdev/porch/controllers/functionconfigs/reconciler" + "github.com/kptdev/porch/controllers/functionconfigs" + . "github.com/kptdev/porch/func/types" "github.com/stretchr/testify/assert" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" @@ -38,18 +39,18 @@ import ( // newTestEventLoopPCM creates a podCacheManager with unbuffered channels suitable // for deterministic event loop testing. The podManager's podReadyCh is the same as // the pcm's podReadyCh so that getFuncEvalPodClient sends results to the event loop. -func newTestEventLoopPCM(kubeClient client.Client) (*podCacheManager, chan *connectionRequest, chan *podReadyResponse) { - reqCh := make(chan *connectionRequest) - readyCh := make(chan *podReadyResponse) +func newTestEventLoopPCM(kubeClient client.Client) (*podCacheManager, chan *ConnectionRequest, chan *PodReadyResponse) { + reqCh := make(chan *ConnectionRequest) + readyCh := make(chan *PodReadyResponse) pcm := &podCacheManager{ gcScanInterval: 5 * time.Minute, podTTL: 10 * time.Minute, connectionRequestCh: reqCh, podReadyCh: readyCh, - functions: map[string]*functionInfo{}, + functions: map[string]*FunctionInfo{}, maxWaitlistLength: 2, maxParallelPodsPerFunction: 1, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/functions"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/functions"), podManager: &podManager{ kubeClient: kubeClient, namespace: defaultNamespace, @@ -70,33 +71,33 @@ func TestEventLoop_PodReadyEmptyImage(t *testing.T) { pcm, _, readyCh := newTestEventLoopPCM(kubeClient) // Pre-populate a pending pod for "test-image" BEFORE starting the event loop - waitCh := make(chan *connectionResponse, 1) - pcm.functions["test-image"] = &functionInfo{ - pods: []functionPodInfo{NewPodInfo(waitCh)}, + waitCh := make(chan *ConnectionResponse, 1) + pcm.functions["test-image"] = &FunctionInfo{ + Pods: []FunctionPodInfo{NewPodInfo(waitCh)}, } go pcm.podCacheManager(t.Context()) // Send podReady with empty image → should be logged and skipped - readyCh <- &podReadyResponse{podData: podData{image: ""}} + readyCh <- &PodReadyResponse{PodData: PodData{Image: ""}} // Send valid podReady to prove the loop continued past the empty image conn, _ := grpc.NewClient("localhost:9446", grpc.WithTransportCredentials(insecure.NewCredentials())) podKey := client.ObjectKey{Name: "test-pod", Namespace: defaultNamespace} serviceKey := client.ObjectKey{Name: "test-svc", Namespace: defaultNamespace} - readyCh <- &podReadyResponse{ - podData: podData{ - image: "test-image", - grpcConnection: conn, - podKey: &podKey, - serviceKey: &serviceKey, + readyCh <- &PodReadyResponse{ + PodData: PodData{ + Image: "test-image", + GrpcConnection: conn, + PodKey: &podKey, + ServiceKey: &serviceKey, }, } select { case resp := <-waitCh: - assert.NoError(t, resp.err) - assert.Equal(t, "test-image", resp.image) + assert.NoError(t, resp.Err) + assert.Equal(t, "test-image", resp.Image) case <-time.After(5 * time.Second): t.Fatal("event loop did not process valid podReady after empty image") } @@ -107,9 +108,9 @@ func TestEventLoop_PodReadyUnknownFunction(t *testing.T) { pcm, _, readyCh := newTestEventLoopPCM(kubeClient) // Pre-populate a pending pod for "known-image" BEFORE starting the event loop - waitCh := make(chan *connectionResponse, 1) - pcm.functions["known-image"] = &functionInfo{ - pods: []functionPodInfo{NewPodInfo(waitCh)}, + waitCh := make(chan *ConnectionResponse, 1) + pcm.functions["known-image"] = &FunctionInfo{ + Pods: []FunctionPodInfo{NewPodInfo(waitCh)}, } go pcm.podCacheManager(t.Context()) @@ -118,29 +119,29 @@ func TestEventLoop_PodReadyUnknownFunction(t *testing.T) { conn, _ := grpc.NewClient("localhost:9446", grpc.WithTransportCredentials(insecure.NewCredentials())) podKey := client.ObjectKey{Name: "test-pod", Namespace: defaultNamespace} serviceKey := client.ObjectKey{Name: "test-svc", Namespace: defaultNamespace} - readyCh <- &podReadyResponse{ - podData: podData{ - image: "unknown-image", - grpcConnection: conn, - podKey: &podKey, - serviceKey: &serviceKey, + readyCh <- &PodReadyResponse{ + PodData: PodData{ + Image: "unknown-image", + GrpcConnection: conn, + PodKey: &podKey, + ServiceKey: &serviceKey, }, } // Send valid podReady for "known-image" to prove the loop continued - readyCh <- &podReadyResponse{ - podData: podData{ - image: "known-image", - grpcConnection: conn, - podKey: &podKey, - serviceKey: &serviceKey, + readyCh <- &PodReadyResponse{ + PodData: PodData{ + Image: "known-image", + GrpcConnection: conn, + PodKey: &podKey, + ServiceKey: &serviceKey, }, } select { case resp := <-waitCh: - assert.NoError(t, resp.err) - assert.Equal(t, "known-image", resp.image) + assert.NoError(t, resp.Err) + assert.Equal(t, "known-image", resp.Image) case <-time.After(5 * time.Second): t.Fatal("event loop did not continue after unknown function podReady") } @@ -166,29 +167,29 @@ func TestEventLoop_PodReadyNoPendingPod(t *testing.T) { pcm, reqCh, readyCh := newTestEventLoopPCM(kubeClient) readyPod := makeReadyPodInfo("test-image", podKey, serviceKey, conn, 0) - pcm.functions["test-image"] = &functionInfo{ - pods: []functionPodInfo{readyPod}, + pcm.functions["test-image"] = &FunctionInfo{ + Pods: []FunctionPodInfo{readyPod}, } go pcm.podCacheManager(t.Context()) // Send podReady for "test-image" — all pods are ready, no pending instance → logged, skipped - readyCh <- &podReadyResponse{ - podData: podData{ - image: "test-image", - grpcConnection: conn, - podKey: &podKey, - serviceKey: &serviceKey, + readyCh <- &PodReadyResponse{ + PodData: PodData{ + Image: "test-image", + GrpcConnection: conn, + PodKey: &podKey, + ServiceKey: &serviceKey, }, } // Verify the loop continues by sending a connectionRequest - responseCh := make(chan *connectionResponse, 1) - reqCh <- &connectionRequest{image: "test-image", responseCh: responseCh} + responseCh := make(chan *ConnectionResponse, 1) + reqCh <- &ConnectionRequest{Image: "test-image", ResponseCh: responseCh} select { case resp := <-responseCh: - assert.NoError(t, resp.err) + assert.NoError(t, resp.Err) case <-time.After(5 * time.Second): t.Fatal("event loop stopped after podReady with no pending pod") } @@ -199,41 +200,41 @@ func TestEventLoop_QueueOnPendingPod(t *testing.T) { pcm, reqCh, readyCh := newTestEventLoopPCM(kubeClient) // Pre-populate with pending pod that has one initial waiter - initialCh := make(chan *connectionResponse, 1) - pcm.functions["test-image"] = &functionInfo{ - pods: []functionPodInfo{NewPodInfo(initialCh)}, + initialCh := make(chan *ConnectionResponse, 1) + pcm.functions["test-image"] = &FunctionInfo{ + Pods: []FunctionPodInfo{NewPodInfo(initialCh)}, } go pcm.podCacheManager(t.Context()) // Send another connectionRequest — should queue on the existing pending pod - secondCh := make(chan *connectionResponse, 1) - reqCh <- &connectionRequest{image: "test-image", responseCh: secondCh} + secondCh := make(chan *ConnectionResponse, 1) + reqCh <- &ConnectionRequest{Image: "test-image", ResponseCh: secondCh} // Now complete the pod by sending podReady conn, _ := grpc.NewClient("localhost:9446", grpc.WithTransportCredentials(insecure.NewCredentials())) podKey := client.ObjectKey{Name: "test-pod", Namespace: defaultNamespace} serviceKey := client.ObjectKey{Name: "test-svc", Namespace: defaultNamespace} - readyCh <- &podReadyResponse{ - podData: podData{ - image: "test-image", - grpcConnection: conn, - podKey: &podKey, - serviceKey: &serviceKey, + readyCh <- &PodReadyResponse{ + PodData: PodData{ + Image: "test-image", + GrpcConnection: conn, + PodKey: &podKey, + ServiceKey: &serviceKey, }, } // Both waiters should receive successful responses select { case resp := <-initialCh: - assert.NoError(t, resp.err) + assert.NoError(t, resp.Err) case <-time.After(5 * time.Second): t.Fatal("initial waiter did not receive response") } select { case resp := <-secondCh: - assert.NoError(t, resp.err) + assert.NoError(t, resp.Err) case <-time.After(5 * time.Second): t.Fatal("second waiter did not receive response") } @@ -254,24 +255,24 @@ func TestEventLoop_PodFailedNoRedistribution(t *testing.T) { pcm, reqCh, _ := newTestEventLoopPCM(kubeClient) // Pre-populate imageMetadataCache so imageDigestAndEntrypoint returns instantly - pcm.podManager.imageMetadataCache.Store("ghcr.io/kptdev/krm-functions-catalog/test-fn:latest", &digestAndEntrypoint{ - digest: "abc123def456abc123def456abc123def456abc123def456abc123def456abc1", - entrypoint: []string{"/test-fn"}, + pcm.podManager.imageMetadataCache.Store("ghcr.io/kptdev/krm-functions-catalog/test-fn:latest", &DigestAndEntrypoint{ + Digest: "abc123def456abc123def456abc123def456abc123def456abc123def456abc1", + Entrypoint: []string{"/test-fn"}, }) go pcm.podCacheManager(t.Context()) // Send connectionRequest — triggers scale-up → goroutine → CreatePod fails → error response - responseCh := make(chan *connectionResponse, 1) - reqCh <- &connectionRequest{ - image: "ghcr.io/kptdev/krm-functions-catalog/test-fn:latest", - responseCh: responseCh, + responseCh := make(chan *ConnectionResponse, 1) + reqCh <- &ConnectionRequest{ + Image: "ghcr.io/kptdev/krm-functions-catalog/test-fn:latest", + ResponseCh: responseCh, } select { case resp := <-responseCh: - assert.Error(t, resp.err) - assert.Contains(t, resp.err.Error(), "fake pod create error") + assert.Error(t, resp.Err) + assert.Contains(t, resp.Err.Error(), "fake pod create error") case <-time.After(10 * time.Second): t.Fatal("timed out waiting for error response from failed pod creation") } @@ -291,7 +292,7 @@ func TestRetrieveFunctionPods_ListFails(t *testing.T) { }).Build() pcm := &podCacheManager{ - functions: map[string]*functionInfo{}, + functions: map[string]*FunctionInfo{}, podManager: &podManager{ kubeClient: kubeClient, namespace: defaultNamespace, @@ -307,7 +308,7 @@ func TestRetrieveFunctionPods_EmptyPodList(t *testing.T) { kubeClient := fake.NewClientBuilder().Build() pcm := &podCacheManager{ - functions: map[string]*functionInfo{}, + functions: map[string]*FunctionInfo{}, podManager: &podManager{ kubeClient: kubeClient, namespace: defaultNamespace, diff --git a/func/internal/podcachemanager_unit_test.go b/func/evaluator/podcachemanager_unit_test.go similarity index 78% rename from func/internal/podcachemanager_unit_test.go rename to func/evaluator/podcachemanager_unit_test.go index d23a3d95e..41b35241e 100644 --- a/func/internal/podcachemanager_unit_test.go +++ b/func/evaluator/podcachemanager_unit_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "fmt" @@ -24,7 +24,8 @@ import ( "github.com/kptdev/kpt/pkg/lib/runneroptions" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - fnconf "github.com/kptdev/porch/controllers/functionconfigs/reconciler" + "github.com/kptdev/porch/controllers/functionconfigs" + . "github.com/kptdev/porch/func/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc" @@ -35,31 +36,31 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ) -// makePodInfoWithLoad creates a functionPodInfo with the specified concurrent evaluation count. -func makePodInfoWithLoad(load int32) functionPodInfo { +// makePodInfoWithLoad creates a FunctionPodInfo with the specified concurrent evaluation count. +func makePodInfoWithLoad(load int32) FunctionPodInfo { counter := &atomic.Int32{} counter.Store(load) - return functionPodInfo{ - concurrentEvaluations: counter, - lastActivity: time.Now(), - waitlist: []chan<- *connectionResponse{}, + return FunctionPodInfo{ + ConcurrentEvaluations: counter, + LastActivity: time.Now(), + Waitlist: []chan<- *ConnectionResponse{}, } } -// makeReadyPodInfo creates a functionPodInfo with podData, for testing functions that require a ready pod. -func makeReadyPodInfo(image string, podKey, serviceKey client.ObjectKey, grpcConn *grpc.ClientConn, load int32) functionPodInfo { +// makeReadyPodInfo creates a FunctionPodInfo with PodData, for testing functions that require a ready pod. +func makeReadyPodInfo(image string, podKey, serviceKey client.ObjectKey, grpcConn *grpc.ClientConn, load int32) FunctionPodInfo { counter := &atomic.Int32{} counter.Store(load) - return functionPodInfo{ - podData: &podData{ - image: image, - podKey: &podKey, - serviceKey: &serviceKey, - grpcConnection: grpcConn, + return FunctionPodInfo{ + PodData: &PodData{ + Image: image, + PodKey: &podKey, + ServiceKey: &serviceKey, + GrpcConnection: grpcConn, }, - concurrentEvaluations: counter, - lastActivity: time.Now(), - waitlist: []chan<- *connectionResponse{}, + ConcurrentEvaluations: counter, + LastActivity: time.Now(), + Waitlist: []chan<- *ConnectionResponse{}, } } @@ -68,7 +69,7 @@ func TestFindBestPod(t *testing.T) { tests := []struct { name string - fn *functionInfo + fn *FunctionInfo expectedIdx int expectedWaitlist int }{ @@ -80,13 +81,13 @@ func TestFindBestPod(t *testing.T) { }, { name: "empty pods returns -1", - fn: &functionInfo{pods: []functionPodInfo{}}, + fn: &FunctionInfo{Pods: []FunctionPodInfo{}}, expectedIdx: -1, expectedWaitlist: 0, }, { name: "single pod with no load", - fn: &functionInfo{pods: []functionPodInfo{ + fn: &FunctionInfo{Pods: []FunctionPodInfo{ makePodInfoWithLoad(0), }}, expectedIdx: 0, @@ -94,7 +95,7 @@ func TestFindBestPod(t *testing.T) { }, { name: "single pod with load", - fn: &functionInfo{pods: []functionPodInfo{ + fn: &FunctionInfo{Pods: []FunctionPodInfo{ makePodInfoWithLoad(5), }}, expectedIdx: 0, @@ -102,7 +103,7 @@ func TestFindBestPod(t *testing.T) { }, { name: "multiple pods selects least loaded", - fn: &functionInfo{pods: []functionPodInfo{ + fn: &FunctionInfo{Pods: []FunctionPodInfo{ makePodInfoWithLoad(5), makePodInfoWithLoad(1), makePodInfoWithLoad(8), @@ -112,7 +113,7 @@ func TestFindBestPod(t *testing.T) { }, { name: "multiple pods with equal load selects first", - fn: &functionInfo{pods: []functionPodInfo{ + fn: &FunctionInfo{Pods: []FunctionPodInfo{ makePodInfoWithLoad(3), makePodInfoWithLoad(3), makePodInfoWithLoad(3), @@ -122,7 +123,7 @@ func TestFindBestPod(t *testing.T) { }, { name: "last pod is least loaded", - fn: &functionInfo{pods: []functionPodInfo{ + fn: &FunctionInfo{Pods: []FunctionPodInfo{ makePodInfoWithLoad(10), makePodInfoWithLoad(7), makePodInfoWithLoad(2), @@ -150,6 +151,7 @@ func makeFunctionConfig(name string, ttl time.Duration, maxWaitlistLength, maxPa runneroptions.GHCRImagePrefix, }, PodExecutor: &configapi.PodExecutorConfig{ + Tags: []string{""}, TimeToLive: metav1.Duration{Duration: ttl}, MaxParallelExecutions: maxParallelPodsPerFunction, PreferredMaxQueueLength: maxWaitlistLength, @@ -159,25 +161,25 @@ func makeFunctionConfig(name string, ttl time.Duration, maxWaitlistLength, maxPa } func TestGetParamsForImage(t *testing.T) { - functionConfigStore := fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/functions") - functionConfigStore.UpsertFunctionConfig("full-override", makeFunctionConfig( + functionConfigStore := functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/functions") + require.NoError(t, functionConfigStore.Store(makeFunctionConfig( "full-override", 5*time.Minute, 10, 5, - )) - functionConfigStore.UpsertFunctionConfig("partial-override", makeFunctionConfig( + ))) + require.NoError(t, functionConfigStore.Store(makeFunctionConfig( "partial-override", 3*time.Minute, 0, 0, - )) - functionConfigStore.UpsertFunctionConfig("zero-ttl", makeFunctionConfig( + ))) + require.NoError(t, functionConfigStore.Store(makeFunctionConfig( "zero-ttl", 0, 1, 1, - )) + ))) pcm := &podCacheManager{ podTTL: 10 * time.Minute, maxWaitlistLength: 2, @@ -235,48 +237,48 @@ func TestGetParamsForImage(t *testing.T) { func TestNewPodInfo(t *testing.T) { t.Run("nil channel creates pod with empty waitlist", func(t *testing.T) { pod := NewPodInfo(nil) - assert.Nil(t, pod.podData) - assert.Empty(t, pod.waitlist) - assert.Equal(t, int32(0), pod.concurrentEvaluations.Load()) + assert.Nil(t, pod.PodData) + assert.Empty(t, pod.Waitlist) + assert.Equal(t, int32(0), pod.ConcurrentEvaluations.Load()) }) t.Run("non-nil channel adds to waitlist and increments counter", func(t *testing.T) { - ch := make(chan *connectionResponse, 1) + ch := make(chan *ConnectionResponse, 1) pod := NewPodInfo(ch) - assert.Nil(t, pod.podData) - assert.Len(t, pod.waitlist, 1) - assert.Equal(t, int32(1), pod.concurrentEvaluations.Load()) + assert.Nil(t, pod.PodData) + assert.Len(t, pod.Waitlist, 1) + assert.Equal(t, int32(1), pod.ConcurrentEvaluations.Load()) }) } func TestSendResponse(t *testing.T) { t.Run("sends error when err is not nil", func(t *testing.T) { - pod := &functionPodInfo{ - podData: &podData{image: "test"}, - concurrentEvaluations: &atomic.Int32{}, + pod := &FunctionPodInfo{ + PodData: &PodData{Image: "test"}, + ConcurrentEvaluations: &atomic.Int32{}, } - ch := make(chan *connectionResponse, 1) + ch := make(chan *ConnectionResponse, 1) testErr := fmt.Errorf("test error") pod.SendResponse(ch, testErr) resp := <-ch - assert.Error(t, resp.err) - assert.Equal(t, "test error", resp.err.Error()) + assert.Error(t, resp.Err) + assert.Equal(t, "test error", resp.Err.Error()) }) t.Run("sends error when podData is nil", func(t *testing.T) { - pod := &functionPodInfo{ - podData: nil, - concurrentEvaluations: &atomic.Int32{}, + pod := &FunctionPodInfo{ + PodData: nil, + ConcurrentEvaluations: &atomic.Int32{}, } - ch := make(chan *connectionResponse, 1) + ch := make(chan *ConnectionResponse, 1) pod.SendResponse(ch, nil) resp := <-ch - assert.Error(t, resp.err) - assert.Contains(t, resp.err.Error(), "pod is not ready") + assert.Error(t, resp.Err) + assert.Contains(t, resp.Err.Error(), "pod is not ready") }) t.Run("sends success with podData", func(t *testing.T) { @@ -285,32 +287,32 @@ func TestSendResponse(t *testing.T) { podKey := client.ObjectKey{Name: "test-pod", Namespace: "test-ns"} serviceKey := client.ObjectKey{Name: "test-svc", Namespace: "test-ns"} - pod := &functionPodInfo{ - podData: &podData{ - image: "test-image", - grpcConnection: conn, - podKey: &podKey, - serviceKey: &serviceKey, + pod := &FunctionPodInfo{ + PodData: &PodData{ + Image: "test-image", + GrpcConnection: conn, + PodKey: &podKey, + ServiceKey: &serviceKey, }, - concurrentEvaluations: &atomic.Int32{}, + ConcurrentEvaluations: &atomic.Int32{}, } - ch := make(chan *connectionResponse, 1) + ch := make(chan *ConnectionResponse, 1) pod.SendResponse(ch, nil) resp := <-ch - assert.NoError(t, resp.err) - assert.Equal(t, "test-image", resp.podData.image) - assert.NotNil(t, resp.grpcConnection) - assert.NotNil(t, resp.concurrentEvaluations) + assert.NoError(t, resp.Err) + assert.Equal(t, "test-image", resp.PodData.Image) + assert.NotNil(t, resp.GrpcConnection) + assert.NotNil(t, resp.ConcurrentEvaluations) }) } func TestWaitlistLen(t *testing.T) { counter := &atomic.Int32{} counter.Store(7) - pod := functionPodInfo{ - concurrentEvaluations: counter, + pod := FunctionPodInfo{ + ConcurrentEvaluations: counter, } assert.Equal(t, 7, pod.WaitlistLen()) @@ -320,13 +322,13 @@ func TestWaitlistLen(t *testing.T) { func TestFunctionInfo(t *testing.T) { pcm := &podCacheManager{ - functions: map[string]*functionInfo{}, + functions: map[string]*FunctionInfo{}, } t.Run("creates new entry for unknown image", func(t *testing.T) { fn := pcm.FunctionInfo("new-image") assert.NotNil(t, fn) - assert.Empty(t, fn.pods) + assert.Empty(t, fn.Pods) // Verify it was stored in the map stored, ok := pcm.functions["new-image"] assert.True(t, ok) @@ -334,12 +336,12 @@ func TestFunctionInfo(t *testing.T) { }) t.Run("returns existing entry for known image", func(t *testing.T) { - existing := &functionInfo{pods: []functionPodInfo{makePodInfoWithLoad(1)}} + existing := &FunctionInfo{Pods: []FunctionPodInfo{makePodInfoWithLoad(1)}} pcm.functions["existing-image"] = existing fn := pcm.FunctionInfo("existing-image") assert.Equal(t, existing, fn) - assert.Len(t, fn.pods, 1) + assert.Len(t, fn.Pods, 1) }) } @@ -349,7 +351,7 @@ func TestRemoveUnhealthyPods(t *testing.T) { t.Run("nil function info is no-op", func(t *testing.T) { pcm := &podCacheManager{ podTTL: 10 * time.Minute, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function"), podManager: &podManager{ kubeClient: fake.NewClientBuilder().Build(), }, @@ -361,21 +363,21 @@ func TestRemoveUnhealthyPods(t *testing.T) { t.Run("pod under creation (nil podData) is kept", func(t *testing.T) { pcm := &podCacheManager{ podTTL: 10 * time.Minute, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function"), podManager: &podManager{ kubeClient: fake.NewClientBuilder().Build(), }, } - fn := &functionInfo{ - pods: []functionPodInfo{ + fn := &FunctionInfo{ + Pods: []FunctionPodInfo{ { - podData: nil, // under creation - concurrentEvaluations: &atomic.Int32{}, + PodData: nil, // under creation + ConcurrentEvaluations: &atomic.Int32{}, }, }, } pcm.removeUnhealthyPods(fn, false) - assert.Len(t, fn.pods, 1, "pod under creation should be kept") + assert.Len(t, fn.Pods, 1, "pod under creation should be kept") }) t.Run("pod not found in k8s is removed", func(t *testing.T) { @@ -388,19 +390,19 @@ func TestRemoveUnhealthyPods(t *testing.T) { pcm := &podCacheManager{ podTTL: 10 * time.Minute, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function"), podManager: &podManager{ kubeClient: fake.NewClientBuilder().Build(), // empty - no pods namespace: testNs, }, } - fn := &functionInfo{ - pods: []functionPodInfo{ + fn := &FunctionInfo{ + Pods: []FunctionPodInfo{ makeReadyPodInfo("test-image", podKey, serviceKey, conn, 0), }, } pcm.removeUnhealthyPods(fn, false) - assert.Empty(t, fn.pods, "pod not found in k8s should be removed") + assert.Empty(t, fn.Pods, "pod not found in k8s should be removed") }) t.Run("pod in Failed state is removed", func(t *testing.T) { @@ -420,19 +422,19 @@ func TestRemoveUnhealthyPods(t *testing.T) { pcm := &podCacheManager{ podTTL: 10 * time.Minute, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function"), podManager: &podManager{ kubeClient: fake.NewClientBuilder().WithObjects(k8sPod, k8sSvc).Build(), namespace: testNs, }, } - fn := &functionInfo{ - pods: []functionPodInfo{ + fn := &FunctionInfo{ + Pods: []FunctionPodInfo{ makeReadyPodInfo("test-image", podKey, serviceKey, conn, 0), }, } pcm.removeUnhealthyPods(fn, false) - assert.Empty(t, fn.pods, "pod in Failed state should be removed") + assert.Empty(t, fn.Pods, "pod in Failed state should be removed") }) t.Run("healthy pod is kept", func(t *testing.T) { @@ -452,19 +454,19 @@ func TestRemoveUnhealthyPods(t *testing.T) { pcm := &podCacheManager{ podTTL: 10 * time.Minute, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function"), podManager: &podManager{ kubeClient: fake.NewClientBuilder().WithObjects(k8sPod, k8sSvc).Build(), namespace: testNs, }, } - fn := &functionInfo{ - pods: []functionPodInfo{ + fn := &FunctionInfo{ + Pods: []FunctionPodInfo{ makeReadyPodInfo("test-image", podKey, serviceKey, conn, 0), }, } pcm.removeUnhealthyPods(fn, false) - assert.Len(t, fn.pods, 1, "healthy pod should be kept") + assert.Len(t, fn.Pods, 1, "healthy pod should be kept") }) t.Run("idle pod past TTL removed when removeIdle is true", func(t *testing.T) { @@ -484,7 +486,7 @@ func TestRemoveUnhealthyPods(t *testing.T) { pcm := &podCacheManager{ podTTL: 10 * time.Minute, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function"), podManager: &podManager{ kubeClient: fake.NewClientBuilder().WithObjects(k8sPod, k8sSvc).Build(), namespace: testNs, @@ -492,11 +494,11 @@ func TestRemoveUnhealthyPods(t *testing.T) { } podInfo := makeReadyPodInfo("test-image", podKey, serviceKey, conn, 0) - podInfo.lastActivity = time.Now().Add(-15 * time.Minute) // past TTL - fn := &functionInfo{pods: []functionPodInfo{podInfo}} + podInfo.LastActivity = time.Now().Add(-15 * time.Minute) // past TTL + fn := &FunctionInfo{Pods: []FunctionPodInfo{podInfo}} pcm.removeUnhealthyPods(fn, true) - assert.Empty(t, fn.pods, "idle pod past TTL should be removed when removeIdle=true") + assert.Empty(t, fn.Pods, "idle pod past TTL should be removed when removeIdle=true") }) t.Run("idle pod past TTL kept when removeIdle is false", func(t *testing.T) { @@ -516,7 +518,7 @@ func TestRemoveUnhealthyPods(t *testing.T) { pcm := &podCacheManager{ podTTL: 10 * time.Minute, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function"), podManager: &podManager{ kubeClient: fake.NewClientBuilder().WithObjects(k8sPod, k8sSvc).Build(), namespace: testNs, @@ -524,11 +526,11 @@ func TestRemoveUnhealthyPods(t *testing.T) { } podInfo := makeReadyPodInfo("test-image", podKey, serviceKey, conn, 0) - podInfo.lastActivity = time.Now().Add(-15 * time.Minute) // past TTL - fn := &functionInfo{pods: []functionPodInfo{podInfo}} + podInfo.LastActivity = time.Now().Add(-15 * time.Minute) // past TTL + fn := &FunctionInfo{Pods: []FunctionPodInfo{podInfo}} pcm.removeUnhealthyPods(fn, false) - assert.Len(t, fn.pods, 1, "idle pod past TTL should be kept when removeIdle=false") + assert.Len(t, fn.Pods, 1, "idle pod past TTL should be kept when removeIdle=false") }) } @@ -552,17 +554,17 @@ func TestGarbageCollectorUnit(t *testing.T) { t.Run("removes empty function entries from map", func(t *testing.T) { pcm := &podCacheManager{ podTTL: 1 * time.Minute, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function"), podManager: &podManager{ kubeClient: fake.NewClientBuilder().WithObjects(k8sPod, k8sSvc).Build(), namespace: testNs, }, - functions: map[string]*functionInfo{ + functions: map[string]*FunctionInfo{ "expired-image": { - pods: []functionPodInfo{ - func() functionPodInfo { + Pods: []FunctionPodInfo{ + func() FunctionPodInfo { p := makeReadyPodInfo("expired-image", podKey, serviceKey, conn, 0) - p.lastActivity = time.Now().Add(-5 * time.Minute) + p.LastActivity = time.Now().Add(-5 * time.Minute) return p }(), }, @@ -598,47 +600,47 @@ func TestRedistributeLoad(t *testing.T) { pcm := &podCacheManager{ podTTL: 10 * time.Minute, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function"), podManager: &podManager{ kubeClient: fake.NewClientBuilder().WithObjects(k8sPod, k8sSvc).Build(), namespace: testNs, }, - functions: map[string]*functionInfo{}, + functions: map[string]*FunctionInfo{}, } readyPod := makeReadyPodInfo("test-image", podKey, serviceKey, conn, 0) - fn := &functionInfo{pods: []functionPodInfo{readyPod}} + fn := &FunctionInfo{Pods: []FunctionPodInfo{readyPod}} pcm.functions["test-image"] = fn // Create connection channels to redistribute - ch1 := make(chan *connectionResponse, 1) - ch2 := make(chan *connectionResponse, 1) + ch1 := make(chan *ConnectionResponse, 1) + ch2 := make(chan *ConnectionResponse, 1) - result := pcm.redistributeLoad("test-image", fn, []chan<- *connectionResponse{ch1, ch2}) + result := pcm.redistributeLoad("test-image", fn, []chan<- *ConnectionResponse{ch1, ch2}) assert.True(t, result, "should redistribute successfully") // Both channels should receive responses resp1 := <-ch1 - assert.NoError(t, resp1.err) + assert.NoError(t, resp1.Err) resp2 := <-ch2 - assert.NoError(t, resp2.err) + assert.NoError(t, resp2.Err) }) t.Run("returns false when no pods available", func(t *testing.T) { pcm := &podCacheManager{ podTTL: 10 * time.Minute, - functionConfigMap: fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function"), + functionConfigMap: functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function"), podManager: &podManager{ kubeClient: fake.NewClientBuilder().Build(), namespace: testNs, }, - functions: map[string]*functionInfo{}, + functions: map[string]*FunctionInfo{}, } - fn := &functionInfo{pods: []functionPodInfo{}} + fn := &FunctionInfo{Pods: []FunctionPodInfo{}} pcm.functions["empty-image"] = fn - ch := make(chan *connectionResponse, 1) - result := pcm.redistributeLoad("empty-image", fn, []chan<- *connectionResponse{ch}) + ch := make(chan *ConnectionResponse, 1) + result := pcm.redistributeLoad("empty-image", fn, []chan<- *ConnectionResponse{ch}) assert.False(t, result, "should return false with no pods") }) } @@ -660,9 +662,9 @@ func TestDeletePodWithServiceInBackgroundByObjectKey(t *testing.T) { podKey := client.ObjectKeyFromObject(k8sPod) serviceKey := client.ObjectKeyFromObject(k8sSvc) - pd := podData{ - podKey: &podKey, - serviceKey: &serviceKey, + pd := PodData{ + PodKey: &podKey, + ServiceKey: &serviceKey, } pcm.DeletePodWithServiceInBackgroundByObjectKey(pd) @@ -683,9 +685,9 @@ func TestDeletePodWithServiceInBackgroundByObjectKey(t *testing.T) { kubeClient: fake.NewClientBuilder().Build(), }, } - pd := podData{ - podKey: nil, - serviceKey: nil, + pd := PodData{ + PodKey: nil, + ServiceKey: nil, } // Should not panic pcm.DeletePodWithServiceInBackgroundByObjectKey(pd) diff --git a/func/internal/podevaluator.go b/func/evaluator/podevaluator.go similarity index 76% rename from func/internal/podevaluator.go rename to func/evaluator/podevaluator.go index a29054def..768825ba6 100644 --- a/func/internal/podevaluator.go +++ b/func/evaluator/podevaluator.go @@ -12,20 +12,21 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "context" "fmt" - "sync/atomic" "time" "github.com/kptdev/kpt/pkg/fn/runtime" "github.com/kptdev/kpt/pkg/lib/runneroptions" - fnconf "github.com/kptdev/porch/controllers/functionconfigs/reconciler" - "github.com/kptdev/porch/func/evaluator" + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + "github.com/kptdev/porch/controllers/functionconfigs" + "github.com/kptdev/porch/func/proto" + . "github.com/kptdev/porch/func/types" "github.com/kptdev/porch/pkg/util" - "google.golang.org/grpc" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -44,10 +45,13 @@ const ( defaultRegistry = "ghcr.io/kptdev/krm-functions-catalog/" serviceDnsNameSuffix = ".svc.cluster.local" channelBufferSize = 128 + + // how long to wait for the FunctionConfig caches to be filled + warmupCacheWaitTimeout = 20 * time.Second ) type podEvaluator struct { - requestCh chan<- *connectionRequest + requestCh chan<- *ConnectionRequest podCacheManager *podCacheManager } @@ -71,39 +75,7 @@ type PodEvaluatorOptions struct { var _ Evaluator = &podEvaluator{} -type podData struct { - // the OCI image name of the KRM function - image string - // connection to the grpc server running in the fn evaluator pod - grpcConnection *grpc.ClientConn - // namespaced name of the pod - podKey *client.ObjectKey - // namespaced name of the service - serviceKey *client.ObjectKey -} - -type connectionRequest struct { - // the OCI image name of the KRM function - image string - // responseCh is the channel to send the response back. - responseCh chan<- *connectionResponse -} - -type connectionResponse struct { - podData - // the number of currently ongoing and waiting fn evaluations in the pod - concurrentEvaluations *atomic.Int32 - // err indicates the error that prevents us to allocate a pod for the fn evaluator - err error -} - -type podReadyResponse struct { - podData - // err indicates the error that prevents us to allocate a pod for the fn evaluator - err error -} - -func NewPodEvaluator(ctx context.Context, o PodEvaluatorOptions, cl client.Client, functionConfigStore *fnconf.FunctionConfigStore) (Evaluator, error) { +func NewPodEvaluator(ctx context.Context, o PodEvaluatorOptions, cl client.Client, functionConfigStore *functionconfigs.FunctionConfigStore) (Evaluator, error) { maxWaitlist := o.MaxWaitlistLength if maxWaitlist <= 0 { maxWaitlist = 2 @@ -120,8 +92,8 @@ func NewPodEvaluator(ctx context.Context, o PodEvaluatorOptions, cl client.Clien managerNs = defaultManagerNamespace } - reqCh := make(chan *connectionRequest, channelBufferSize) - readyCh := make(chan *podReadyResponse, channelBufferSize) + reqCh := make(chan *ConnectionRequest, channelBufferSize) + readyCh := make(chan *PodReadyResponse, channelBufferSize) pe := &podEvaluator{ requestCh: reqCh, @@ -130,7 +102,7 @@ func NewPodEvaluator(ctx context.Context, o PodEvaluatorOptions, cl client.Clien podTTL: o.PodTTL, connectionRequestCh: reqCh, podReadyCh: readyCh, - functions: map[string]*functionInfo{}, + functions: map[string]*FunctionInfo{}, maxWaitlistLength: maxWaitlist, maxParallelPodsPerFunction: maxPods, functionConfigMap: functionConfigStore, @@ -162,6 +134,11 @@ func NewPodEvaluator(ctx context.Context, o PodEvaluatorOptions, cl client.Clien } if o.WarmUpPodCacheOnStartup { + err = waitForFunctionConfigs(ctx, cl, functionConfigStore) + // We proceed even if not all CRs are reconciled + if err != nil { + klog.Warningf("Failed to wait for all FunctionConfigs to be cached, warmup may be partial: %v", err) + } // TODO(mengqiy): add watcher that support reloading the cache when the config file was changed. err = pe.podCacheManager.warmupCache(o.DefaultImagePrefix) // If we can't warm up the cache, we can still proceed without it. @@ -173,7 +150,24 @@ func NewPodEvaluator(ctx context.Context, o PodEvaluatorOptions, cl client.Clien return pe, nil } -func (pe *podEvaluator) EvaluateFunction(ctx context.Context, req *evaluator.EvaluateFunctionRequest) (*evaluator.EvaluateFunctionResponse, error) { +func waitForFunctionConfigs(ctx context.Context, cl client.Client, store *functionconfigs.FunctionConfigStore) error { + list := &configapi.FunctionConfigList{} + return wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, warmupCacheWaitTimeout, true, func(ctx context.Context) (bool, error) { + if err := cl.List(ctx, list); err != nil { + // if listing fails, something is wrong with the client or cluster + return false, err + } + + if len(list.Items) > store.Len() { + klog.Infof("[Cache Warmup]: Some FunctionConfigs have not yet been reconciled; CRs: %d, Cached: %d", len(list.Items), store.Len()) + return false, nil + } + + return true, nil + }) +} + +func (pe *podEvaluator) EvaluateFunction(ctx context.Context, req *proto.EvaluateFunctionRequest) (*proto.EvaluateFunctionResponse, error) { starttime := time.Now() var image string defer func() { @@ -188,23 +182,23 @@ func (pe *podEvaluator) EvaluateFunction(ctx context.Context, req *evaluator.Eva req.Image = image // make a buffer for the channel to prevent unnecessary blocking when the pod cache manager sends it to multiple waiting goroutine in batch. - responseChannel := make(chan *connectionResponse, 1) + responseChannel := make(chan *ConnectionResponse, 1) // Send a request to request a grpc client. - pe.requestCh <- &connectionRequest{ - image: req.Image, - responseCh: responseChannel, + pe.requestCh <- &ConnectionRequest{ + Image: req.Image, + ResponseCh: responseChannel, } // Waiting for the client from the channel. This step is blocking. select { case pod := <-responseChannel: - if pod == nil || pod.grpcConnection == nil || pod.err != nil { - return nil, fmt.Errorf("unable to get the grpc client to the pod for %v: %w", req.Image, pod.err) + if pod == nil || pod.GrpcConnection == nil || pod.Err != nil { + return nil, fmt.Errorf("unable to get the grpc client to the pod for %v: %w", req.Image, pod.Err) } - defer pod.concurrentEvaluations.Add(-1) + defer pod.ConcurrentEvaluations.Add(-1) - resp, err := evaluator.NewFunctionEvaluatorClient(pod.grpcConnection).EvaluateFunction(ctx, req) + resp, err := proto.NewFunctionEvaluatorClient(pod.GrpcConnection).EvaluateFunction(ctx, req) if err != nil { klog.V(4).Infof("Resource List: %s", req.ResourceList) return nil, fmt.Errorf("unable to evaluate %q with pod evaluator: %w", req.Image, err) diff --git a/func/internal/podevaluator_podcachemanager_test.go b/func/evaluator/podevaluator_podcachemanager_test.go similarity index 90% rename from func/internal/podevaluator_podcachemanager_test.go rename to func/evaluator/podevaluator_podcachemanager_test.go index e1481ea5a..824f14e32 100644 --- a/func/internal/podevaluator_podcachemanager_test.go +++ b/func/evaluator/podevaluator_podcachemanager_test.go @@ -14,7 +14,7 @@ limitations under the License. */ -package internal +package evaluator import ( "context" @@ -27,7 +27,8 @@ import ( "github.com/kptdev/kpt/pkg/lib/runneroptions" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - fnconf "github.com/kptdev/porch/controllers/functionconfigs/reconciler" + "github.com/kptdev/porch/controllers/functionconfigs" + . "github.com/kptdev/porch/func/types" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" "k8s.io/apimachinery/pkg/runtime" @@ -50,10 +51,10 @@ func TestPodCacheManager(t *testing.T) { const mockTemplateRV = "12345" - defaultImageMetadataCache := map[string]*digestAndEntrypoint{ + defaultImageMetadataCache := map[string]*DigestAndEntrypoint{ defaultImageName: { - digest: "5245a52778d684fa698f69861fb2e058b308f6a74fed5bf2fe77d97bad5e071c", - entrypoint: []string{"/" + defaultImageName}, + Digest: "5245a52778d684fa698f69861fb2e058b308f6a74fed5bf2fe77d97bad5e071c", + Entrypoint: []string{"/" + defaultImageName}, }, } @@ -231,24 +232,24 @@ func TestPodCacheManager(t *testing.T) { t.Fatalf("Failed to create grpc client: %v", err) } - blankCache := make(map[string]*functionInfo) + blankCache := make(map[string]*FunctionInfo) - pData := podData{ - image: defaultImageName, - grpcConnection: grpcClient, - podKey: ptr.To(client.ObjectKeyFromObject(defaultPodObject)), - serviceKey: ptr.To(client.ObjectKeyFromObject(defaultServiceObject)), + pData := PodData{ + Image: defaultImageName, + GrpcConnection: grpcClient, + PodKey: ptr.To(client.ObjectKeyFromObject(defaultPodObject)), + ServiceKey: ptr.To(client.ObjectKeyFromObject(defaultServiceObject)), } funcPodInfo := NewPodInfo(nil) - funcPodInfo.podData = &pData - funcPodInfo.concurrentEvaluations.Add(1) + funcPodInfo.PodData = &pData + funcPodInfo.ConcurrentEvaluations.Add(1) - funcInfo := functionInfo{ - pods: []functionPodInfo{funcPodInfo}, + funcInfo := FunctionInfo{ + Pods: []FunctionPodInfo{funcPodInfo}, } - functionWithDefaultPod := make(map[string]*functionInfo) + functionWithDefaultPod := make(map[string]*FunctionInfo) functionWithDefaultPod[defaultImageName] = &funcInfo // These tests focus on the pod management logic in the pod cache manager covering @@ -259,7 +260,7 @@ func TestPodCacheManager(t *testing.T) { expectFail bool skip bool kubeClient client.WithWatch - functions map[string]*functionInfo + functions map[string]*FunctionInfo expectedLog string skipRetrieve bool }{ @@ -325,8 +326,8 @@ func TestPodCacheManager(t *testing.T) { } //Set up the podmanager and podcachemanager - requestCh := make(chan *connectionRequest) - podReadyCh := make(chan *podReadyResponse) + requestCh := make(chan *ConnectionRequest) + podReadyCh := make(chan *PodReadyResponse) pm := &podManager{ namespace: defaultNamespace, @@ -342,7 +343,7 @@ func TestPodCacheManager(t *testing.T) { pm.imageMetadataCache.Store(k, v) } - functionConfigStore := fnconf.NewFunctionConfigStore(runneroptions.GHCRImagePrefix, "/function") + functionConfigStore := functionconfigs.NewStore(runneroptions.GHCRImagePrefix, "/function") pcm := &podCacheManager{ // Setting to 5 minutes to avoid GC invocation @@ -368,7 +369,7 @@ func TestPodCacheManager(t *testing.T) { defer klog.ClearLogger() // restore global logger after test klog.SetLogger(logger) - pcm.functions = make(map[string]*functionInfo) + pcm.functions = make(map[string]*FunctionInfo) for k, v := range tt.functions { pcm.functions[k] = v } @@ -382,17 +383,17 @@ func TestPodCacheManager(t *testing.T) { } - clientConn := make(chan *connectionResponse) - requestCh <- &connectionRequest{defaultImageName, clientConn} + clientConn := make(chan *ConnectionResponse) + requestCh <- &ConnectionRequest{defaultImageName, clientConn} select { case cc := <-clientConn: - if !tt.expectFail && cc.err != nil { - t.Errorf("Expected to get client connection, got error: %v", cc.err) - } else if tt.expectFail && cc.err == nil { + if !tt.expectFail && cc.Err != nil { + t.Errorf("Expected to get client connection, got error: %v", cc.Err) + } else if tt.expectFail && cc.Err == nil { t.Errorf("Expected to get error, got client connection") - } else if cc.err == nil { - if cc.podData.grpcConnection == nil { + } else if cc.Err == nil { + if cc.PodData.GrpcConnection == nil { t.Errorf("Expected to get grpc client, got nil") } } @@ -418,14 +419,14 @@ func TestPodCacheManager(t *testing.T) { deepCopyObject(defaultPodObject, podObjectInvalidRetentionTimestamp) funcExpiredPodInfo := NewPodInfo(nil) - funcExpiredPodInfo.podData = &pData - funcExpiredPodInfo.lastActivity = time.Now().Add(-11 * time.Minute) + funcExpiredPodInfo.PodData = &pData + funcExpiredPodInfo.LastActivity = time.Now().Add(-11 * time.Minute) - funcExpiredInfo := functionInfo{ - pods: []functionPodInfo{funcExpiredPodInfo}, + funcExpiredInfo := FunctionInfo{ + Pods: []FunctionPodInfo{funcExpiredPodInfo}, } - functionWithExpiredPod := make(map[string]*functionInfo) + functionWithExpiredPod := make(map[string]*FunctionInfo) functionWithExpiredPod[defaultImageName] = &funcExpiredInfo // These tests focus on the pod cleanup / garbage collection logic in the pod @@ -435,7 +436,7 @@ func TestPodCacheManager(t *testing.T) { expectFail bool skip bool kubeClient client.WithWatch - functions map[string]*functionInfo + functions map[string]*FunctionInfo podShouldBeDeleted bool serviceShouldBeDeleted bool cacheShouldBeEmpty bool @@ -488,7 +489,7 @@ func TestPodCacheManager(t *testing.T) { t.SkipNow() } - pcm.functions = make(map[string]*functionInfo) + pcm.functions = make(map[string]*FunctionInfo) for k, v := range tt.functions { pcm.functions[k] = v } diff --git a/func/internal/podevaluator_podmanager_test.go b/func/evaluator/podevaluator_podmanager_test.go similarity index 97% rename from func/internal/podevaluator_podmanager_test.go rename to func/evaluator/podevaluator_podmanager_test.go index 2a5832ac6..5cc7a4c2b 100644 --- a/func/internal/podevaluator_podmanager_test.go +++ b/func/evaluator/podevaluator_podmanager_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "bytes" @@ -27,10 +27,11 @@ import ( "time" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + . "github.com/kptdev/porch/func/types" "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog/v2" - pb "github.com/kptdev/porch/func/evaluator" + pb "github.com/kptdev/porch/func/proto" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc" @@ -119,10 +120,10 @@ func TestPodManager(t *testing.T) { return &pb.EvaluateFunctionResponse{ResourceList: []byte("thisShouldBeKRM"), Log: []byte("Success")}, nil } - defaultImageMetadataCache := map[string]*digestAndEntrypoint{ + defaultImageMetadataCache := map[string]*DigestAndEntrypoint{ defaultImageName: { - digest: "5245a52778d684fa698f69861fb2e058b308f6a74fed5bf2fe77d97bad5e071c", - entrypoint: []string{"/" + defaultImageName}, + Digest: "5245a52778d684fa698f69861fb2e058b308f6a74fed5bf2fe77d97bad5e071c", + Entrypoint: []string{"/" + defaultImageName}, }, } @@ -309,7 +310,7 @@ func TestPodManager(t *testing.T) { kubeClient client.WithWatch namespace string wrapperServerImage string - imageMetadataCache map[string]*digestAndEntrypoint + imageMetadataCache map[string]*DigestAndEntrypoint evalFunc func(ctx context.Context, req *pb.EvaluateFunctionRequest) (*pb.EvaluateFunctionResponse, error) functionImage string managerNamespace string @@ -590,7 +591,7 @@ func TestPodManager(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() //Set up the pod manager - podReadyCh := make(chan *podReadyResponse) + podReadyCh := make(chan *PodReadyResponse) pm := &podManager{ kubeClient: tt.kubeClient, namespace: tt.namespace, @@ -622,14 +623,14 @@ func TestPodManager(t *testing.T) { go pm.getFuncEvalPodClient(ctx, tt.functionImage, 1, podConfig, false) cc := <-podReadyCh - if cc.err != nil && !tt.expectFail { - assert.NoError(t, cc.err, "Expected to get ready pod") - } else if cc.err == nil { + if cc.Err != nil && !tt.expectFail { + assert.NoError(t, cc.Err, "Expected to get ready pod") + } else if cc.Err == nil { if tt.expectFail { assert.Fail(t, "Expected to get error, got ready pod") } var pod corev1.Pod - err := tt.kubeClient.Get(ctx, *cc.podKey, &pod) + err := tt.kubeClient.Get(ctx, *cc.PodKey, &pod) assert.NoError(t, err, "Failed to get pod") assert.True(t, strings.HasPrefix(pod.Labels[krmFunctionImageLabel], tt.functionImage), diff --git a/func/internal/podevaluator_porch_parallel_execution_test.go b/func/evaluator/podevaluator_porch_parallel_execution_test.go similarity index 90% rename from func/internal/podevaluator_porch_parallel_execution_test.go rename to func/evaluator/podevaluator_porch_parallel_execution_test.go index e68802948..9c370dbb3 100644 --- a/func/internal/podevaluator_porch_parallel_execution_test.go +++ b/func/evaluator/podevaluator_porch_parallel_execution_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "context" @@ -23,7 +23,8 @@ import ( "testing" "time" - pb "github.com/kptdev/porch/func/evaluator" + pb "github.com/kptdev/porch/func/proto" + . "github.com/kptdev/porch/func/types" "github.com/stretchr/testify/require" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" @@ -86,20 +87,20 @@ func TestPodEvaluatorExecutionParallel(t *testing.T) { t.Fatalf("grpc dial failed: %v", err) } - reqCh := make(chan *connectionRequest, 2) + reqCh := make(chan *ConnectionRequest, 2) go func() { counter := &atomic.Int32{} for req := range reqCh { // Increment counter to simulate single pod with limited concurrency counter.Add(1) - req.responseCh <- &connectionResponse{ - podData: podData{ - image: req.image, - grpcConnection: conn, - podKey: ptr.To(client.ObjectKey{}), + req.ResponseCh <- &ConnectionResponse{ + PodData: PodData{ + Image: req.Image, + GrpcConnection: conn, + PodKey: ptr.To(client.ObjectKey{}), }, - concurrentEvaluations: counter, - err: nil, + ConcurrentEvaluations: counter, + Err: nil, } } }() diff --git a/func/internal/podevaluator_tag_resolution_test.go b/func/evaluator/podevaluator_tag_resolution_test.go similarity index 83% rename from func/internal/podevaluator_tag_resolution_test.go rename to func/evaluator/podevaluator_tag_resolution_test.go index bf050cf6f..4aac86f4d 100644 --- a/func/internal/podevaluator_tag_resolution_test.go +++ b/func/evaluator/podevaluator_tag_resolution_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "context" @@ -23,12 +23,13 @@ import ( "time" "github.com/kptdev/kpt/pkg/fn/runtime" - pb "github.com/kptdev/porch/func/evaluator" + pb "github.com/kptdev/porch/func/proto" + . "github.com/kptdev/porch/func/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" - ptr "k8s.io/utils/ptr" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -78,18 +79,18 @@ func TestTagResolution(t *testing.T) { ) require.NoError(t, err, "grpc dial failed") - reqCh := make(chan *connectionRequest, 2) + reqCh := make(chan *ConnectionRequest, 2) go func() { counter := &atomic.Int32{} for req := range reqCh { - req.responseCh <- &connectionResponse{ - podData: podData{ - image: req.image, - grpcConnection: conn, - podKey: ptr.To(client.ObjectKey{}), + req.ResponseCh <- &ConnectionResponse{ + PodData: PodData{ + Image: req.Image, + GrpcConnection: conn, + PodKey: ptr.To(client.ObjectKey{}), }, - concurrentEvaluations: counter, - err: nil, + ConcurrentEvaluations: counter, + Err: nil, } } }() @@ -126,18 +127,18 @@ func TestTagResolution(t *testing.T) { ) require.NoError(t, err, "grpc dial failed") - reqCh := make(chan *connectionRequest, 2) + reqCh := make(chan *ConnectionRequest, 2) go func() { counter := &atomic.Int32{} for req := range reqCh { - req.responseCh <- &connectionResponse{ - podData: podData{ - image: req.image, - grpcConnection: conn, - podKey: ptr.To(client.ObjectKey{}), + req.ResponseCh <- &ConnectionResponse{ + PodData: PodData{ + Image: req.Image, + GrpcConnection: conn, + PodKey: ptr.To(client.ObjectKey{}), }, - concurrentEvaluations: counter, - err: nil, + ConcurrentEvaluations: counter, + Err: nil, } } }() diff --git a/func/internal/podevaluator_unit_test.go b/func/evaluator/podevaluator_unit_test.go similarity index 84% rename from func/internal/podevaluator_unit_test.go rename to func/evaluator/podevaluator_unit_test.go index 2143b6144..0dc91778e 100644 --- a/func/internal/podevaluator_unit_test.go +++ b/func/evaluator/podevaluator_unit_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "context" @@ -22,7 +22,8 @@ import ( "testing" "github.com/kptdev/kpt/pkg/fn/runtime" - pb "github.com/kptdev/porch/func/evaluator" + pb "github.com/kptdev/porch/func/proto" + . "github.com/kptdev/porch/func/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc" @@ -47,7 +48,7 @@ func startFakeEvalServer(t *testing.T, evalFunc func(ctx context.Context, req *p } func TestEvaluateFunction_ErrorInResponse(t *testing.T) { - reqCh := make(chan *connectionRequest, 1) + reqCh := make(chan *ConnectionRequest, 1) pe := &podEvaluator{requestCh: reqCh, podCacheManager: &podCacheManager{ podManager: &podManager{ @@ -58,8 +59,8 @@ func TestEvaluateFunction_ErrorInResponse(t *testing.T) { go func() { req := <-reqCh - req.responseCh <- &connectionResponse{ - err: fmt.Errorf("fake pod allocation error"), + req.ResponseCh <- &ConnectionResponse{ + Err: fmt.Errorf("fake pod allocation error"), } }() @@ -72,7 +73,7 @@ func TestEvaluateFunction_ErrorInResponse(t *testing.T) { } func TestEvaluateFunction_NilGrpcConnection(t *testing.T) { - reqCh := make(chan *connectionRequest, 1) + reqCh := make(chan *ConnectionRequest, 1) pe := &podEvaluator{requestCh: reqCh, podCacheManager: &podCacheManager{ podManager: &podManager{ @@ -83,8 +84,8 @@ func TestEvaluateFunction_NilGrpcConnection(t *testing.T) { go func() { req := <-reqCh - req.responseCh <- &connectionResponse{ - podData: podData{grpcConnection: nil}, + req.ResponseCh <- &ConnectionResponse{ + PodData: PodData{GrpcConnection: nil}, } }() @@ -108,7 +109,7 @@ func TestEvaluateFunction_GrpcCallFails(t *testing.T) { counter := &atomic.Int32{} counter.Store(1) - reqCh := make(chan *connectionRequest, 1) + reqCh := make(chan *ConnectionRequest, 1) pe := &podEvaluator{requestCh: reqCh, podCacheManager: &podCacheManager{ podManager: &podManager{ @@ -119,9 +120,9 @@ func TestEvaluateFunction_GrpcCallFails(t *testing.T) { go func() { req := <-reqCh - req.responseCh <- &connectionResponse{ - podData: podData{image: "test-image", grpcConnection: conn}, - concurrentEvaluations: counter, + req.ResponseCh <- &ConnectionResponse{ + PodData: PodData{Image: "test-image", GrpcConnection: conn}, + ConcurrentEvaluations: counter, } }() @@ -148,7 +149,7 @@ func TestEvaluateFunction_SuccessWithStderr(t *testing.T) { counter := &atomic.Int32{} counter.Store(1) - reqCh := make(chan *connectionRequest, 1) + reqCh := make(chan *ConnectionRequest, 1) pe := &podEvaluator{requestCh: reqCh, podCacheManager: &podCacheManager{ podManager: &podManager{ @@ -159,9 +160,9 @@ func TestEvaluateFunction_SuccessWithStderr(t *testing.T) { go func() { req := <-reqCh - req.responseCh <- &connectionResponse{ - podData: podData{image: "test-image", grpcConnection: conn}, - concurrentEvaluations: counter, + req.ResponseCh <- &ConnectionResponse{ + PodData: PodData{Image: "test-image", GrpcConnection: conn}, + ConcurrentEvaluations: counter, } }() @@ -188,7 +189,7 @@ func TestEvaluateFunction_SuccessClean(t *testing.T) { counter := &atomic.Int32{} counter.Store(1) - reqCh := make(chan *connectionRequest, 1) + reqCh := make(chan *ConnectionRequest, 1) pe := &podEvaluator{requestCh: reqCh, podCacheManager: &podCacheManager{ podManager: &podManager{ @@ -199,9 +200,9 @@ func TestEvaluateFunction_SuccessClean(t *testing.T) { go func() { req := <-reqCh - req.responseCh <- &connectionResponse{ - podData: podData{image: "test-image", grpcConnection: conn}, - concurrentEvaluations: counter, + req.ResponseCh <- &ConnectionResponse{ + PodData: PodData{Image: "test-image", GrpcConnection: conn}, + ConcurrentEvaluations: counter, } }() @@ -226,7 +227,7 @@ func TestEvaluateFunction_CounterDecrement(t *testing.T) { counter := &atomic.Int32{} counter.Store(1) - reqCh := make(chan *connectionRequest, 1) + reqCh := make(chan *ConnectionRequest, 1) pe := &podEvaluator{requestCh: reqCh, podCacheManager: &podCacheManager{ podManager: &podManager{ @@ -237,9 +238,9 @@ func TestEvaluateFunction_CounterDecrement(t *testing.T) { go func() { req := <-reqCh - req.responseCh <- &connectionResponse{ - podData: podData{image: "test-image", grpcConnection: conn}, - concurrentEvaluations: counter, + req.ResponseCh <- &ConnectionResponse{ + PodData: PodData{Image: "test-image", GrpcConnection: conn}, + ConcurrentEvaluations: counter, } }() diff --git a/func/internal/podmanager.go b/func/evaluator/podmanager.go similarity index 96% rename from func/internal/podmanager.go rename to func/evaluator/podmanager.go index 6f68e0966..195a14e08 100644 --- a/func/internal/podmanager.go +++ b/func/evaluator/podmanager.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "context" @@ -36,6 +36,7 @@ import ( "github.com/kptdev/kpt/pkg/fn/runtime" "github.com/kptdev/kpt/pkg/lib/runneroptions" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + . "github.com/kptdev/porch/func/types" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" @@ -192,11 +193,11 @@ type podManager struct { wrapperServerImage string // podReadyCh is a channel to receive requests to get GRPC client from each function evaluation request handler. - podReadyCh chan<- *podReadyResponse + podReadyCh chan<- *PodReadyResponse - // imageMetadataCache is a cache of image name to digestAndEntrypoint. + // imageMetadataCache is a cache of image name to DigestAndEntrypoint. // Only podManager is allowed to touch this cache. - // Its underlying type is map[string]*digestAndEntrypoint. + // Its underlying type is map[string]*DigestAndEntrypoint. imageMetadataCache sync.Map // podReadyTimeout is the timeout podManager will wait for the pod to be ready before reporting an error @@ -221,18 +222,11 @@ type podManager struct { tagResolver runtime.TagResolver } -type digestAndEntrypoint struct { - // digest is a hex string - digest string - // entrypoint is the entrypoint of the image - entrypoint []string -} - -func (pm *podManager) createPodData(ctx context.Context, serviceKey client.ObjectKey, podKey client.ObjectKey, image string) (*podData, error) { - podData := &podData{ - image: image, - podKey: &podKey, - serviceKey: &serviceKey, +func (pm *podManager) createPodData(ctx context.Context, serviceKey client.ObjectKey, podKey client.ObjectKey, image string) (*PodData, error) { + podData := &PodData{ + Image: image, + PodKey: &podKey, + ServiceKey: &serviceKey, } serviceUrl, err := pm.getServiceUrlOnceEndpointActive(ctx, serviceKey, podKey) if err != nil { @@ -254,7 +248,7 @@ func (pm *podManager) createPodData(ctx context.Context, serviceKey client.Objec if err != nil { return podData, fmt.Errorf("failed to dial grpc function evaluator on %q for pod %s/%s: %w", address, serviceKey.Namespace, serviceKey.Name, err) } - podData.grpcConnection = cc + podData.GrpcConnection = cc return podData, err } @@ -264,9 +258,9 @@ func (pm *podManager) createPodData(ctx context.Context, serviceKey client.Objec // create a pod with a fixed name. Otherwise, it will create a pod and let the // apiserver to generate the name from a template. func (pm *podManager) getFuncEvalPodClient(ctx context.Context, image string, postFix int, podConfig *configapi.PodExecutorConfig, useGenerateName bool) { - c, err := func() (*podData, error) { - podData := &podData{ - image: image, + c, err := func() (*PodData, error) { + podData := &PodData{ + Image: image, } pod, err := pm.CreatePod(ctx, image, postFix, podConfig, useGenerateName) if err != nil { @@ -275,7 +269,7 @@ func (pm *podManager) getFuncEvalPodClient(ctx context.Context, image string, po } podKey := client.ObjectKeyFromObject(pod) - podData.podKey = &podKey + podData.PodKey = &podKey // Service name is Image Label set on Pod manifest serviceName := pod.Labels[krmFunctionImageLabel] @@ -289,9 +283,9 @@ func (pm *podManager) getFuncEvalPodClient(ctx context.Context, image string, po return podData, err }() - pm.podReadyCh <- &podReadyResponse{ - podData: *c, - err: err, + pm.podReadyCh <- &PodReadyResponse{ + PodData: *c, + Err: err, } } @@ -353,16 +347,11 @@ func (pm *podManager) InspectOrCreateSecret(ctx context.Context, registryAuthSec return nil } -// DockerConfig represents the structure of Docker config.json -type DockerConfig struct { - Auths map[string]authn.AuthConfig `json:"auths"` -} - // imageDigestAndEntrypoint gets the entrypoint of a container image by looking at its metadata. -func (pm *podManager) imageDigestAndEntrypoint(ctx context.Context, image string) (*digestAndEntrypoint, error) { +func (pm *podManager) imageDigestAndEntrypoint(ctx context.Context, image string) (*DigestAndEntrypoint, error) { val, found := pm.imageMetadataCache.Load(image) if found { - return val.(*digestAndEntrypoint), nil + return val.(*DigestAndEntrypoint), nil } start := time.Now() @@ -423,7 +412,7 @@ func (pm *podManager) getCustomAuth(ref name.Reference, registryAuthSecretPath s } // getImageMetadata retrieves the image digest and entrypoint. -func (pm *podManager) getImageMetadata(ctx context.Context, ref name.Reference, auth authn.Authenticator, image string) (*digestAndEntrypoint, error) { +func (pm *podManager) getImageMetadata(ctx context.Context, ref name.Reference, auth authn.Authenticator, image string) (*DigestAndEntrypoint, error) { img, err := pm.getImage(ctx, ref, auth, image) if err != nil { return nil, err @@ -442,9 +431,9 @@ func (pm *podManager) getImageMetadata(ctx context.Context, ref name.Reference, if len(entrypoint) == 0 { entrypoint = cfg.Cmd } - de := &digestAndEntrypoint{ - digest: hash.Hex, - entrypoint: entrypoint, + de := &DigestAndEntrypoint{ + Digest: hash.Hex, + Entrypoint: entrypoint, } pm.imageMetadataCache.Store(image, de) return de, nil @@ -515,7 +504,7 @@ func createTransport(tlsConfig *tls.Config) *http.Transport { // CreatePod creates a pod for an image. func (pm *podManager) CreatePod(ctx context.Context, image string, postFix int, config *configapi.PodExecutorConfig, useGenerateName bool) (*corev1.Pod, error) { - var de *digestAndEntrypoint + var de *DigestAndEntrypoint var err error if pm.imageResolver != nil { image = pm.imageResolver(image) @@ -525,7 +514,7 @@ func (pm *podManager) CreatePod(ctx context.Context, image string, postFix int, return nil, fmt.Errorf("unable to get the entrypoint for %v: %w", image, err) } - podId, err := podID(image, de.digest, strconv.Itoa(postFix)) + podId, err := podID(image, de.Digest, strconv.Itoa(postFix)) if err != nil { return nil, err } @@ -767,7 +756,7 @@ func (pm *podManager) appendImagePullSecret(image string, podTemplate *corev1.Po } // Patches the expected port, and the original entrypoint and image of the kpt function into the function container -func (pm *podManager) patchNewPodContainer(pod *corev1.PodTemplateSpec, de digestAndEntrypoint, image string) error { +func (pm *podManager) patchNewPodContainer(pod *corev1.PodTemplateSpec, de DigestAndEntrypoint, image string) error { var patchedContainer bool for i := range pod.Spec.Containers { container := &pod.Spec.Containers[i] @@ -777,7 +766,7 @@ func (pm *podManager) patchNewPodContainer(pod *corev1.PodTemplateSpec, de diges "--max-request-body-size", strconv.Itoa(pm.maxGrpcMessageSize), "--", ) - container.Args = append(container.Args, de.entrypoint...) + container.Args = append(container.Args, de.Entrypoint...) container.Image = image patchedContainer = true } diff --git a/func/internal/podmanager_unit_test.go b/func/evaluator/podmanager_unit_test.go similarity index 98% rename from func/internal/podmanager_unit_test.go rename to func/evaluator/podmanager_unit_test.go index 1f5bea6a6..7ce07de5f 100644 --- a/func/internal/podmanager_unit_test.go +++ b/func/evaluator/podmanager_unit_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package evaluator import ( "crypto/ecdsa" @@ -29,6 +29,7 @@ import ( "time" "github.com/google/go-containerregistry/pkg/name" + . "github.com/kptdev/porch/func/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -230,9 +231,9 @@ func TestPatchNewPodContainer(t *testing.T) { }, }, } - de := digestAndEntrypoint{ - digest: "abc123", - entrypoint: []string{"/my-function"}, + de := DigestAndEntrypoint{ + Digest: "abc123", + Entrypoint: []string{"/my-function"}, } err := pm.patchNewPodContainer(podTemplateSpec, de, "ghcr.io/kptdev/krm-functions-catalog/my-function:latest") @@ -257,9 +258,9 @@ func TestPatchNewPodContainer(t *testing.T) { }, }, } - de := digestAndEntrypoint{ - digest: "abc123", - entrypoint: []string{"/entry"}, + de := DigestAndEntrypoint{ + Digest: "abc123", + Entrypoint: []string{"/entry"}, } err := pm.patchNewPodContainer(podTemplateSpec, de, "test-image") @@ -280,9 +281,9 @@ func TestPatchNewPodContainer(t *testing.T) { }, }, } - de := digestAndEntrypoint{ - digest: "abc123", - entrypoint: []string{"/fn"}, + de := DigestAndEntrypoint{ + Digest: "abc123", + Entrypoint: []string{"/fn"}, } err := pm.patchNewPodContainer(podTemplateSpec, de, "my-image") diff --git a/func/internal/testdata/config.yaml b/func/evaluator/testdata/config.yaml similarity index 100% rename from func/internal/testdata/config.yaml rename to func/evaluator/testdata/config.yaml diff --git a/func/internal/testdata/config_bad_format.yaml b/func/evaluator/testdata/config_bad_format.yaml similarity index 100% rename from func/internal/testdata/config_bad_format.yaml rename to func/evaluator/testdata/config_bad_format.yaml diff --git a/func/evaluator/evaluator.pb.go b/func/proto/evaluator.pb.go similarity index 97% rename from func/evaluator/evaluator.pb.go rename to func/proto/evaluator.pb.go index 6d122f5ea..c28c30beb 100644 --- a/func/evaluator/evaluator.pb.go +++ b/func/proto/evaluator.pb.go @@ -15,19 +15,17 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: // protoc-gen-go v1.36.11 -// protoc v6.30.2 +// protoc v3.21.12 // source: evaluator.proto -package evaluator +package proto import ( + protoreflect "google.golang.org/protobuf/reflect/protoreflect" + protoimpl "google.golang.org/protobuf/runtime/protoimpl" reflect "reflect" sync "sync" unsafe "unsafe" - - protoreflect "google.golang.org/protobuf/reflect/protoreflect" - protoimpl "google.golang.org/protobuf/runtime/protoimpl" - _ "google.golang.org/protobuf/types/known/structpb" ) const ( @@ -204,7 +202,7 @@ var File_evaluator_proto protoreflect.FileDescriptor const file_evaluator_proto_rawDesc = "" + "\n" + - "\x0fevaluator.proto\x12\tevaluator\x1a\fstruct.proto\"f\n" + + "\x0fevaluator.proto\x12\tevaluator\"f\n" + "\x17EvaluateFunctionRequest\x12#\n" + "\rresource_list\x18\x01 \x01(\fR\fresourceList\x12\x14\n" + "\x05image\x18\x02 \x01(\tR\x05image\x12\x10\n" + @@ -218,7 +216,7 @@ const file_evaluator_proto_rawDesc = "" + "\rresource_list\x18\x01 \x01(\fR\fresourceList\x12\x10\n" + "\x03log\x18\x02 \x01(\fR\x03log2r\n" + "\x11FunctionEvaluator\x12]\n" + - "\x10EvaluateFunction\x12\".evaluator.EvaluateFunctionRequest\x1a#.evaluator.EvaluateFunctionResponse\"\x00B0Z.github.com/kptdev/porch/func/evaluatorb\x06proto3" + "\x10EvaluateFunction\x12\".evaluator.EvaluateFunctionRequest\x1a#.evaluator.EvaluateFunctionResponse\"\x00B$Z\"github.com/kptdev/porch/func/protob\x06proto3" var ( file_evaluator_proto_rawDescOnce sync.Once diff --git a/func/evaluator/evaluator.proto b/func/proto/evaluator.proto similarity index 92% rename from func/evaluator/evaluator.proto rename to func/proto/evaluator.proto index 229fb7b83..8515857cc 100644 --- a/func/evaluator/evaluator.proto +++ b/func/proto/evaluator.proto @@ -14,11 +14,9 @@ syntax = "proto3"; -package evaluator; +package evaluator; // TODO: should this be "proto" as well? -import "struct.proto"; - -option go_package = "github.com/kptdev/porch/func/evaluator"; +option go_package = "github.com/kptdev/porch/func/proto"; // Evaluator of kpt functions service FunctionEvaluator { diff --git a/func/evaluator/evaluator_grpc.pb.go b/func/proto/evaluator_grpc.pb.go similarity index 97% rename from func/evaluator/evaluator_grpc.pb.go rename to func/proto/evaluator_grpc.pb.go index 6b6df9c48..bd765065d 100644 --- a/func/evaluator/evaluator_grpc.pb.go +++ b/func/proto/evaluator_grpc.pb.go @@ -1,4 +1,4 @@ -// Copyright 2025-2026 The kpt Authors +// Copyright 2022, 2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -14,15 +14,14 @@ // Code generated by protoc-gen-go-grpc. DO NOT EDIT. // versions: -// - protoc-gen-go-grpc v1.6.1 -// - protoc v6.30.2 +// - protoc-gen-go-grpc v1.6.2 +// - protoc v3.21.12 // source: evaluator.proto -package evaluator +package proto import ( context "context" - grpc "google.golang.org/grpc" codes "google.golang.org/grpc/codes" status "google.golang.org/grpc/status" diff --git a/func/server/server.go b/func/server/server.go index 416dd1a05..45d369169 100644 --- a/func/server/server.go +++ b/func/server/server.go @@ -26,10 +26,10 @@ import ( "github.com/kptdev/kpt/pkg/lib/runneroptions" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" - pb "github.com/kptdev/porch/func/evaluator" + "github.com/kptdev/porch/controllers/functionconfigs" + "github.com/kptdev/porch/func/evaluator" "github.com/kptdev/porch/func/healthchecker" - "github.com/kptdev/porch/func/internal" + pb "github.com/kptdev/porch/func/proto" porchotel "github.com/kptdev/porch/internal/otel" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "google.golang.org/grpc" @@ -65,9 +65,9 @@ type options struct { defaultImagePrefix string // Parameters of ExecEvaluator - exec internal.ExecutableEvaluatorOptions + exec evaluator.ExecutableEvaluatorOptions // Parameters of PodEvaluator - pod internal.PodEvaluatorOptions + pod evaluator.PodEvaluatorOptions } func main() { @@ -147,11 +147,11 @@ func run(o *options) error { return err } - runtimes := []internal.Evaluator{} + runtimes := []evaluator.Evaluator{} for rt := range availableRuntimes { switch rt { case execRuntime: - execEval, err := internal.NewExecutableEvaluator(fnConfigReconciler.FunctionConfigStore) + execEval, err := evaluator.NewExecutableEvaluator(fnConfigReconciler.FunctionConfigStore) if err != nil { return fmt.Errorf("failed to initialize executable evaluator: %w", err) } @@ -162,7 +162,7 @@ func run(o *options) error { return fmt.Errorf("environment variable %v must be set to use pod function evaluator runtime", wrapperServerImageEnv) } o.pod.DefaultImagePrefix = o.defaultImagePrefix - podEval, err := internal.NewPodEvaluator(ctx, o.pod, fnConfigReconciler.Client, fnConfigReconciler.FunctionConfigStore) + podEval, err := evaluator.NewPodEvaluator(ctx, o.pod, fnConfigReconciler.Client, fnConfigReconciler.FunctionConfigStore) if err != nil { return fmt.Errorf("failed to initialize pod evaluator: %w", err) } @@ -172,7 +172,7 @@ func run(o *options) error { if len(runtimes) == 0 { klog.Warning("no runtime is enabled in function-runner") } - evaluator := internal.NewMultiEvaluator(runtimes...) + evaluator := evaluator.NewMultiEvaluator(runtimes...) klog.Infof("Listening on %s", address) @@ -222,7 +222,7 @@ func buildScheme() (*runtime.Scheme, error) { return scheme, nil } -func buildFnConfigReconciler(o *options, scheme *runtime.Scheme) (*reconciler.FunctionConfigReconciler, error) { +func buildFnConfigReconciler(o *options, scheme *runtime.Scheme) (*functionconfigs.FunctionConfigReconciler, error) { restCfg, err := getRestConfig() if err != nil { return nil, err @@ -243,12 +243,12 @@ func buildFnConfigReconciler(o *options, scheme *runtime.Scheme) (*reconciler.Fu return nil, err } - functionConfigStore := reconciler.NewFunctionConfigStore(o.defaultImagePrefix, o.exec.FunctionCacheDir) + functionConfigStore := functionconfigs.NewStore(o.defaultImagePrefix, o.exec.FunctionCacheDir) - rec := &reconciler.FunctionConfigReconciler{ + rec := &functionconfigs.FunctionConfigReconciler{ Client: mgr.GetClient(), FunctionConfigStore: functionConfigStore, - For: reconciler.ReconcilerForFunctionRunner, + For: functionconfigs.ReconcilerForFunctionRunner, } if err := ctrl.NewControllerManagedBy(mgr). diff --git a/func/types/common_types.go b/func/types/common_types.go new file mode 100644 index 000000000..6a8c9ae44 --- /dev/null +++ b/func/types/common_types.go @@ -0,0 +1,117 @@ +// Copyright 2026 The kpt 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 types + +import ( + "fmt" + "sync/atomic" + "time" + + "github.com/google/go-containerregistry/pkg/authn" + "google.golang.org/grpc" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type PodData struct { + // the OCI image name of the KRM function + Image string + // connection to the grpc server running in the fn evaluator pod + GrpcConnection *grpc.ClientConn + // namespaced name of the pod + PodKey *client.ObjectKey + // namespaced name of the service + ServiceKey *client.ObjectKey +} + +type ConnectionRequest struct { + // the OCI image name of the KRM function + Image string + // ResponseCh is the channel to send the response back. + ResponseCh chan<- *ConnectionResponse +} + +type ConnectionResponse struct { + PodData + // the number of currently ongoing and waiting fn evaluations in the pod + ConcurrentEvaluations *atomic.Int32 + // Err indicates the error that prevents us to allocate a pod for the fn evaluator + Err error +} + +type PodReadyResponse struct { + PodData + // Err indicates the error that prevents us to allocate a pod for the fn evaluator + Err error +} + +type DigestAndEntrypoint struct { + // Digest is a hex string + Digest string + // Entrypoint is the Entrypoint of the image + Entrypoint []string +} + +// DockerConfig represents the structure of Docker config.json +type DockerConfig struct { + Auths map[string]authn.AuthConfig `json:"auths"` +} + +// FunctionInfo holds the list of all pod instances for the same KRM function image. +type FunctionInfo struct { + // status of all Pods belonging to the same KRM function image + Pods []FunctionPodInfo + // RoundRobinIdx is used to distribute requests across Pods when all have equal load + RoundRobinIdx int +} + +// FunctionPodInfo represents the state of a single pod instance. +type FunctionPodInfo struct { + // PodData contains the information about the pod, returned by the podManager + // It is nil until the pod is actually started + *PodData + // Waitlist is used to temporarily store connection requests until the pod is started + Waitlist []chan<- *ConnectionResponse + // time of last function evaluation, used by the garbage collector to identify idle pods + LastActivity time.Time + // the number of currently ongoing and waiting fn evaluations in the pod + ConcurrentEvaluations *atomic.Int32 +} + +// SendResponse sends a reply to the connection request containing the pod data. +// If err != nil it sends `err` as an error response. +// It sends an error response if the pod is not ready yet (this shouldn't happen). +func (pod *FunctionPodInfo) SendResponse(responseCh chan<- *ConnectionResponse, err error) { + switch { + case err != nil: + responseCh <- &ConnectionResponse{ + Err: err, + } + case pod.PodData == nil: + responseCh <- &ConnectionResponse{ + Err: fmt.Errorf("pod is not ready, connection response sent prematurely. This is logical error in the code"), + } + default: + responseCh <- &ConnectionResponse{ + PodData: *pod.PodData, + ConcurrentEvaluations: pod.ConcurrentEvaluations, + Err: nil, + } + } +} + +// WaitlistLen returns with the number of fn evaluations currently handled by the pod +func (pod *FunctionPodInfo) WaitlistLen() int { + return int(pod.ConcurrentEvaluations.Load()) +} diff --git a/func/wrapper-server/main.go b/func/wrapper-server/main.go index 0030af3c2..4bf016ccf 100644 --- a/func/wrapper-server/main.go +++ b/func/wrapper-server/main.go @@ -27,8 +27,8 @@ import ( "strconv" "github.com/kptdev/krm-functions-sdk/go/fn" - pb "github.com/kptdev/porch/func/evaluator" "github.com/kptdev/porch/func/healthchecker" + pb "github.com/kptdev/porch/func/proto" porchotel "github.com/kptdev/porch/internal/otel" "github.com/spf13/cobra" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" diff --git a/func/wrapper-server/wrapper_server_test.go b/func/wrapper-server/wrapper_server_test.go index 5705e3981..b18db666c 100644 --- a/func/wrapper-server/wrapper_server_test.go +++ b/func/wrapper-server/wrapper_server_test.go @@ -20,7 +20,7 @@ import ( "flag" "testing" - pb "github.com/kptdev/porch/func/evaluator" + pb "github.com/kptdev/porch/func/proto" "github.com/stretchr/testify/assert" "k8s.io/klog/v2" "sigs.k8s.io/kustomize/kyaml/kio" diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index c55ed486e..afa6e60b4 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -27,7 +27,7 @@ import ( porchapi "github.com/kptdev/porch/api/porch/v1alpha1" porchv1alpha2 "github.com/kptdev/porch/api/porch/v1alpha2" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" + "github.com/kptdev/porch/controllers/functionconfigs" internalapi "github.com/kptdev/porch/internal/api/porchinternal/v1alpha1" "github.com/kptdev/porch/pkg/cache" cachetypes "github.com/kptdev/porch/pkg/cache/types" @@ -94,7 +94,7 @@ type ExtraConfig struct { CacheOptions cachetypes.CacheOptions PodNameSpace string - FunctionStore *reconciler.FunctionConfigStore + FunctionStore *functionconfigs.FunctionConfigStore } // Config defines the config for the apiserver @@ -286,7 +286,7 @@ func (c completedConfig) getCoreV1Client() (*corev1client.CoreV1Client, error) { return corev1Client, nil } -func (c completedConfig) buildFunctionConfigReconciler(ctx context.Context, scheme *runtime.Scheme, withIndex bool) (*reconciler.FunctionConfigReconciler, error) { +func (c completedConfig) buildFunctionConfigReconciler(ctx context.Context, scheme *runtime.Scheme, withIndex bool) (*functionconfigs.FunctionConfigReconciler, error) { restConfig, err := c.getRestConfig() if err != nil { return nil, err @@ -321,12 +321,12 @@ func (c completedConfig) buildFunctionConfigReconciler(ctx context.Context, sche } } - functionConfigStore := reconciler.NewFunctionConfigStore(c.ExtraConfig.GRPCRuntimeOptions.DefaultImagePrefix, "") + functionConfigStore := functionconfigs.NewStore(c.ExtraConfig.GRPCRuntimeOptions.DefaultImagePrefix, "") - rec := &reconciler.FunctionConfigReconciler{ + rec := &functionconfigs.FunctionConfigReconciler{ Client: mgr.GetClient(), FunctionConfigStore: functionConfigStore, - For: reconciler.ReconcilerForServer, + For: functionconfigs.ReconcilerForServer, } c.ExtraConfig.FunctionStore = functionConfigStore diff --git a/pkg/engine/builtinruntime.go b/pkg/engine/builtinruntime.go index e507e9771..c2d189ea7 100644 --- a/pkg/engine/builtinruntime.go +++ b/pkg/engine/builtinruntime.go @@ -24,17 +24,16 @@ import ( "github.com/kptdev/kpt/pkg/fn" "github.com/kptdev/kpt/pkg/lib/kptops" fnsdk "github.com/kptdev/krm-functions-sdk/go/fn" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" - "github.com/kptdev/porch/pkg/util" + "github.com/kptdev/porch/controllers/functionconfigs" regclientref "github.com/regclient/regclient/types/ref" "k8s.io/klog/v2" ) type builtinRuntime struct { - store *reconciler.FunctionConfigStore + store *functionconfigs.FunctionConfigStore } -func newBuiltinRuntime(functionConfigStore *reconciler.FunctionConfigStore) *builtinRuntime { +func newBuiltinRuntime(functionConfigStore *functionconfigs.FunctionConfigStore) *builtinRuntime { return &builtinRuntime{ store: functionConfigStore, } @@ -47,12 +46,9 @@ func (br *builtinRuntime) GetRunner(ctx context.Context, funct *kptfilev1.Functi ctx: ctx, } - cache := br.store.GetExecCache() - if funct.Tag != "" { ref, err := regclientref.New(funct.Image) if err != nil { - klog.Infof("????") return nil, fmt.Errorf("failed to parse image %q as reference: %w", funct.Image, err) } // If the image already carries an inline tag, strip it @@ -64,21 +60,17 @@ func (br *builtinRuntime) GetRunner(ctx context.Context, funct *kptfilev1.Functi funct.Image = stripped } } - baseName := util.GetImageName(funct.Image) - builtinEntry := cache[baseName] - cacheKeys := make([]string, 0, len(builtinEntry.Tags)) - cacheKeys = append(cacheKeys, builtinEntry.Tags...) - _, err = util.FindBestSemverMatch(funct.Tag, funct.Image, cacheKeys) - if err != nil { + processor, ok := br.store.GetProcessorByConstraint(funct.Image, funct.Tag) + if !ok { return nil, &fn.NotFoundError{ - Function: kptfilev1.Function{Image: funct.Image}, + Function: *funct, } } - builtinRunner.processor = builtinEntry.Process + builtinRunner.processor = processor } else { klog.Infof("Image tag is empty, using the image with explicit tag: %q", funct.Image) - processor, found := br.store.GetProcessorFromCache(funct.Image) + processor, found := br.store.GetProcessor(funct.Image) if !found { return nil, &fn.NotFoundError{Function: *funct} } diff --git a/pkg/engine/builtinruntime_test.go b/pkg/engine/builtinruntime_test.go index 0f0434bac..30e8e459c 100644 --- a/pkg/engine/builtinruntime_test.go +++ b/pkg/engine/builtinruntime_test.go @@ -16,16 +16,18 @@ package engine import ( "bytes" + "flag" "os" "path/filepath" "testing" kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" "github.com/kptdev/kpt/pkg/fn" + "github.com/kptdev/kpt/pkg/lib/runneroptions" fnsdk "github.com/kptdev/krm-functions-sdk/go/fn" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" - "github.com/kptdev/porch/pkg/util" + "github.com/kptdev/porch/controllers/functionconfigs" + imageutil "github.com/kptdev/porch/pkg/util/image" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -34,7 +36,7 @@ import ( const ( customImagePrefix = "test.io/kptdev/krm-functions-catalog" - defaultKRMImagePrefix = "ghcr.io/kptdev/krm-functions-catalog" + defaultKRMImagePrefix = runneroptions.GHCRImagePrefix testImageName = "test-image" setNamespaceFunction = "set-namespace" @@ -48,6 +50,7 @@ func TestNewBuiltinRuntime(t *testing.T) { Name: applyReplacementsFunction, }, Spec: configapi.FunctionConfigSpec{ + Image: applyReplacementsFunction, Prefixes: []string{ "", customImagePrefix, @@ -58,33 +61,33 @@ func TestNewBuiltinRuntime(t *testing.T) { }, } - functionConfigStore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, "") - functionConfigStore.UpdateExecCache(applyReplacementsFunction, &functionConfig) + functionConfigStore := functionconfigs.NewStore(defaultKRMImagePrefix, "") + require.NoError(t, functionConfigStore.Store(&functionConfig)) br := newBuiltinRuntime(functionConfigStore) - assert.NotNil(t, br) - assert.NotNil(t, br.store) + require.NotNil(t, br) + require.NotNil(t, br.store) // Verify that functions are registered with the custom image prefix - processor, exists := functionConfigStore.GetProcessorFromCache(customImagePrefix + "/" + applyReplacementsFunction + ":v0.1.1") + processor, exists := functionConfigStore.GetProcessor(customImagePrefix + "/" + applyReplacementsFunction + ":v0.1.1") assert.True(t, exists, "Expected function to be registered with custom image prefix: %s", customImagePrefix) assert.NotNil(t, processor) // Verify that functions are also registered with default GHCR prefix - processor, exists = functionConfigStore.GetProcessorFromCache(defaultKRMImagePrefix + "/" + applyReplacementsFunction + ":v0.1.1") + processor, exists = functionConfigStore.GetProcessor(defaultKRMImagePrefix + "/" + applyReplacementsFunction + ":v0.1.1") assert.True(t, exists, "Expected function to be registered with default GHCR prefix") assert.NotNil(t, processor) }) t.Run("custom image prefix is not specified", func(t *testing.T) { functionConfig := configapi.FunctionConfig{ - ObjectMeta: v1.ObjectMeta{ Name: applyReplacementsFunction, }, Spec: configapi.FunctionConfigSpec{ + Image: applyReplacementsFunction, Prefixes: []string{ "", }, @@ -93,21 +96,21 @@ func TestNewBuiltinRuntime(t *testing.T) { }, }, } - functionConfigStore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, "") - functionConfigStore.UpdateExecCache(applyReplacementsFunction, &functionConfig) + functionConfigStore := functionconfigs.NewStore(defaultKRMImagePrefix, "") + require.NoError(t, functionConfigStore.Store(&functionConfig)) br := newBuiltinRuntime(functionConfigStore) assert.NotNil(t, br) assert.NotNil(t, br.store) // Verify that functions are not registered with the custom image prefix - processor, exists := functionConfigStore.GetProcessorFromCache(customImagePrefix + applyReplacementsFunction + ":v0.1.1") + processor, exists := functionConfigStore.GetProcessor(customImagePrefix + applyReplacementsFunction + ":v0.1.1") assert.False(t, exists, "Expected function to not be registered with custom image prefix: %s", customImagePrefix) assert.Nil(t, processor) // Verify that functions are registered with default GHCR prefix - processor, exists = functionConfigStore.GetProcessorFromCache(defaultKRMImagePrefix + "/" + applyReplacementsFunction + ":v0.1.1") + processor, exists = functionConfigStore.GetProcessor(defaultKRMImagePrefix + "/" + applyReplacementsFunction + ":v0.1.1") assert.True(t, exists, "Expected function to be registered with default GHCR prefix") assert.NotNil(t, processor) @@ -115,6 +118,10 @@ func TestNewBuiltinRuntime(t *testing.T) { } func TestBuiltinRuntime(t *testing.T) { + flagSet := flag.NewFlagSet("log-level", flag.ContinueOnError) + klog.InitFlags(flagSet) + _ = flagSet.Parse([]string{"--v", "5"}) + t.Run("invalid semver constraint syntax", func(t *testing.T) { ctx := t.Context() functionConfig := configapi.FunctionConfig{ @@ -122,13 +129,14 @@ func TestBuiltinRuntime(t *testing.T) { Name: setNamespaceFunction, }, Spec: configapi.FunctionConfigSpec{ + Image: setNamespaceFunction, GoExecutor: &configapi.GoExecutorConfig{ Tags: []string{"v0.4.1"}, }, }, } - functionConfigStore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, "") - functionConfigStore.UpdateExecCache(setNamespaceFunction, &functionConfig) + functionConfigStore := functionconfigs.NewStore(defaultKRMImagePrefix, "") + require.NoError(t, functionConfigStore.Store(&functionConfig)) br := newBuiltinRuntime(functionConfigStore) funct := &kptfilev1.Function{ Image: filepath.Join(defaultKRMImagePrefix, testImageName), @@ -137,9 +145,7 @@ func TestBuiltinRuntime(t *testing.T) { Tag: ">> 0.4.0 < 0.5.0", } _, err := br.GetRunner(ctx, funct) - assert.Equal(t, &fn.NotFoundError{ - Function: kptfilev1.Function{Image: funct.Image}, - }, err) + assert.Equal(t, &fn.NotFoundError{Function: *funct}, err) }) t.Run("builtinrutime not found", func(t *testing.T) { ctx := t.Context() @@ -148,13 +154,14 @@ func TestBuiltinRuntime(t *testing.T) { Name: setNamespaceFunction, }, Spec: configapi.FunctionConfigSpec{ + Image: setNamespaceFunction, GoExecutor: &configapi.GoExecutorConfig{ Tags: []string{"v0.4.1"}, }, }, } - functionConfigStore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, "") - functionConfigStore.UpdateExecCache(setNamespaceFunction, &functionConfig) + functionConfigStore := functionconfigs.NewStore(defaultKRMImagePrefix, "") + require.NoError(t, functionConfigStore.Store(&functionConfig)) br := newBuiltinRuntime(functionConfigStore) funct := &kptfilev1.Function{ Image: filepath.Join(defaultKRMImagePrefix, testImageName), @@ -163,9 +170,7 @@ func TestBuiltinRuntime(t *testing.T) { Tag: ">= 0.4.0 < 0.5.0", } _, err := br.GetRunner(ctx, funct) - assert.Equal(t, &fn.NotFoundError{ - Function: kptfilev1.Function{Image: funct.Image}, - }, err) + assert.Equal(t, &fn.NotFoundError{Function: *funct}, err) }) t.Run("function does not match the semantic version constraints", func(t *testing.T) { ctx := t.Context() @@ -174,13 +179,14 @@ func TestBuiltinRuntime(t *testing.T) { Name: setNamespaceFunction, }, Spec: configapi.FunctionConfigSpec{ + Image: setNamespaceFunction, GoExecutor: &configapi.GoExecutorConfig{ Tags: []string{"v0.4.1"}, }, }, } - functionConfigStore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, "") - functionConfigStore.UpdateExecCache(setNamespaceFunction, &functionConfig) + functionConfigStore := functionconfigs.NewStore(defaultKRMImagePrefix, "") + require.NoError(t, functionConfigStore.Store(&functionConfig)) br := newBuiltinRuntime(functionConfigStore) funct := &kptfilev1.Function{ Image: filepath.Join(defaultKRMImagePrefix, setNamespaceFunction), @@ -190,9 +196,7 @@ func TestBuiltinRuntime(t *testing.T) { Tag: "> 0.2.0 < 0.3.0", } _, err := br.GetRunner(ctx, funct) - assert.Equal(t, &fn.NotFoundError{ - Function: kptfilev1.Function{Image: funct.Image}, - }, err) + assert.Equal(t, &fn.NotFoundError{Function: *funct}, err) }) t.Run("function not found using explicit tagging", func(t *testing.T) { ctx := t.Context() @@ -201,23 +205,22 @@ func TestBuiltinRuntime(t *testing.T) { Name: setNamespaceFunction, }, Spec: configapi.FunctionConfigSpec{ + Image: setNamespaceFunction, GoExecutor: &configapi.GoExecutorConfig{ Tags: []string{"v0.4.1"}, }, }, } - functionConfigStore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, "") - functionConfigStore.UpdateExecCache(setNamespaceFunction, &functionConfig) + functionConfigStore := functionconfigs.NewStore(defaultKRMImagePrefix, "") + require.NoError(t, functionConfigStore.Store(&functionConfig)) br := newBuiltinRuntime(functionConfigStore) funct := &kptfilev1.Function{ - Image: util.ImageJoin(defaultKRMImagePrefix, setNamespaceFunction) + ":v0.4.2", + Image: imageutil.Join(defaultKRMImagePrefix, setNamespaceFunction) + ":v0.4.2", // Image is explicitly tagged with v0.4.2, however, // there is no function with this explicit tag in the cache } _, err := br.GetRunner(ctx, funct) - assert.Equal(t, &fn.NotFoundError{ - Function: kptfilev1.Function{Image: funct.Image}, - }, err) + assert.Equal(t, &fn.NotFoundError{Function: *funct}, err) }) t.Run("function execution error", func(t *testing.T) { ctx := t.Context() @@ -226,22 +229,23 @@ func TestBuiltinRuntime(t *testing.T) { Name: applyReplacementsFunction, }, Spec: configapi.FunctionConfigSpec{ + Image: applyReplacementsFunction, GoExecutor: &configapi.GoExecutorConfig{ Tags: []string{"v0.1.0"}, }, }, } - functionConfigStore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, "") - functionConfigStore.UpdateExecCache(applyReplacementsFunction, &functionConfig) + functionConfigStore := functionconfigs.NewStore(defaultKRMImagePrefix, "") + require.NoError(t, functionConfigStore.Store(&functionConfig)) br := newBuiltinRuntime(functionConfigStore) - fn := &kptfilev1.Function{ + function := &kptfilev1.Function{ // Wrong function is specified for namespace setting, // which will cause an execution error when the function tries to run - Image: filepath.Join(defaultKRMImagePrefix, applyReplacementsFunction), + Image: imageutil.Join(defaultKRMImagePrefix, applyReplacementsFunction), Tag: ">= 0.1.0 < 0.2.0", } - fr, err := br.GetRunner(ctx, fn) - assert.Nil(t, err) + fr, err := br.GetRunner(ctx, function) + require.NoError(t, err) reader := bytes.NewReader([]byte(`apiVersion: config.kubernetes.io/v1alpha1 kind: ResourceList items: @@ -260,7 +264,7 @@ functionConfig: `)) var buf bytes.Buffer err = fr.Run(reader, &buf) - assert.Equal(t, "error: function failure", err.Error()) + assert.ErrorContains(t, err, "function failure") }) t.Run("successful execution with semantic versioning", func(t *testing.T) { ctx := t.Context() @@ -269,15 +273,16 @@ functionConfig: Name: setNamespaceFunction, }, Spec: configapi.FunctionConfigSpec{ + Image: setNamespaceFunction, GoExecutor: &configapi.GoExecutorConfig{ Tags: []string{"v0.4.1"}, }, }, } - functionConfigStore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, "") - functionConfigStore.UpdateExecCache(setNamespaceFunction, &functionConfig) + functionConfigStore := functionconfigs.NewStore(defaultKRMImagePrefix, "") + require.NoError(t, functionConfigStore.Store(&functionConfig)) br := newBuiltinRuntime(functionConfigStore) - fn := &kptfilev1.Function{ + function := &kptfilev1.Function{ Image: filepath.Join(defaultKRMImagePrefix, setNamespaceFunction), // This semver constraint matches the version of the apply-replacements function in builtin runtime, // so it should successfully find the function and run it @@ -289,8 +294,8 @@ functionConfig: r, w, _ := os.Pipe() os.Stderr = w - fr, err := br.GetRunner(ctx, fn) - assert.Nil(t, err) + fr, err := br.GetRunner(ctx, function) + require.NoError(t, err) // Flush klog and restore stderr klog.Flush() @@ -303,9 +308,7 @@ functionConfig: logOutput := logBuffer.String() // Verify the klog message contains the expected version selection - assert.Contains(t, logOutput, `Selected image "ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1"`) - assert.Contains(t, logOutput, `version "0.4.1"`) - assert.Contains(t, logOutput, `for request "ghcr.io/kptdev/krm-functions-catalog/set-namespace"`) + assert.Contains(t, logOutput, `Selected tag "v0.4.1"`) reader := bytes.NewReader([]byte(`apiVersion: config.kubernetes.io/v1alpha1 kind: ResourceList @@ -340,6 +343,7 @@ functionConfig: Name: setNamespaceFunction, }, Spec: configapi.FunctionConfigSpec{ + Image: setNamespaceFunction, Prefixes: []string{ "", }, @@ -348,10 +352,10 @@ functionConfig: }, }, } - functionConfigStore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, "") - functionConfigStore.UpdateExecCache(setNamespaceFunction, &functionConfig) + functionConfigStore := functionconfigs.NewStore(defaultKRMImagePrefix, "") + require.NoError(t, functionConfigStore.Store(&functionConfig)) br := newBuiltinRuntime(functionConfigStore) - fn := &kptfilev1.Function{ + function := &kptfilev1.Function{ Image: filepath.Join(defaultKRMImagePrefix, setNamespaceFunction) + ":v0.4.1", } @@ -360,7 +364,7 @@ functionConfig: r, w, _ := os.Pipe() os.Stderr = w - fr, err := br.GetRunner(ctx, fn) + fr, err := br.GetRunner(ctx, function) require.NoError(t, err) // Flush klog and restore stderr @@ -409,15 +413,16 @@ functionConfig: Name: setNamespaceFunction, }, Spec: configapi.FunctionConfigSpec{ + Image: setNamespaceFunction, GoExecutor: &configapi.GoExecutorConfig{ Tags: []string{"v0.4.1"}, }, }, } - functionConfigStore := reconciler.NewFunctionConfigStore(defaultKRMImagePrefix, "") - functionConfigStore.UpdateExecCache(setNamespaceFunction, &functionConfig) + functionConfigStore := functionconfigs.NewStore(defaultKRMImagePrefix, "") + require.NoError(t, functionConfigStore.Store(&functionConfig)) br := newBuiltinRuntime(functionConfigStore) - fn := &kptfilev1.Function{ + function := &kptfilev1.Function{ Image: filepath.Join(defaultKRMImagePrefix, setNamespaceFunction) + ":v0.3.0", Tag: ">= 0.4.0 < 0.5.0", } @@ -427,7 +432,7 @@ functionConfig: r, w, _ := os.Pipe() os.Stderr = w - fr, err := br.GetRunner(ctx, fn) + fr, err := br.GetRunner(ctx, function) require.NoError(t, err) // Flush klog and restore stderr @@ -441,9 +446,7 @@ functionConfig: logOutput := logBuffer.String() // Verify the klog message contains the expected version selection - assert.Contains(t, logOutput, `Selected image "ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1"`) - assert.Contains(t, logOutput, `(version "0.4.1")`) - assert.Contains(t, logOutput, `for request "ghcr.io/kptdev/krm-functions-catalog/set-namespace"`) + assert.Contains(t, logOutput, `Selected tag "v0.4.1"`) reader := bytes.NewReader([]byte(`apiVersion: config.kubernetes.io/v1alpha1 kind: ResourceList diff --git a/pkg/engine/grpcruntime.go b/pkg/engine/grpcruntime.go index 126623cf1..f219325dd 100644 --- a/pkg/engine/grpcruntime.go +++ b/pkg/engine/grpcruntime.go @@ -22,8 +22,8 @@ import ( kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" "github.com/kptdev/kpt/pkg/fn" "github.com/kptdev/kpt/pkg/lib/kptops" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" - "github.com/kptdev/porch/func/evaluator" + "github.com/kptdev/porch/controllers/functionconfigs" + "github.com/kptdev/porch/func/proto" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" @@ -38,7 +38,7 @@ type GRPCRuntimeOptions struct { type grpcRuntime struct { cc *grpc.ClientConn - client evaluator.FunctionEvaluatorClient + client proto.FunctionEvaluatorClient } func newGRPCFunctionRuntime(options GRPCRuntimeOptions) (*grpcRuntime, error) { @@ -62,7 +62,7 @@ func newGRPCFunctionRuntime(options GRPCRuntimeOptions) (*grpcRuntime, error) { return &grpcRuntime{ cc: cc, - client: evaluator.NewFunctionEvaluatorClient(cc), + client: proto.NewFunctionEvaluatorClient(cc), }, err } @@ -91,7 +91,7 @@ func (gr *grpcRuntime) Close() error { type grpcRunner struct { ctx context.Context - client evaluator.FunctionEvaluatorClient + client proto.FunctionEvaluatorClient image string tag string } @@ -104,7 +104,7 @@ func (gr *grpcRunner) Run(r io.Reader, w io.Writer) error { return fmt.Errorf("failed to read function runner input: %w", err) } - res, err := gr.client.EvaluateFunction(gr.ctx, &evaluator.EvaluateFunctionRequest{ + res, err := gr.client.EvaluateFunction(gr.ctx, &proto.EvaluateFunctionRequest{ ResourceList: in, Image: gr.image, Tag: gr.tag, @@ -120,7 +120,7 @@ func (gr *grpcRunner) Run(r io.Reader, w io.Writer) error { // NewMultiFunctionRuntime creates a FunctionRuntime that tries builtin functions // first, then falls back to the gRPC fn-runner. -func NewMultiFunctionRuntime(grpcAddress string, maxGrpcMessageSize int, functionConfigStore *reconciler.FunctionConfigStore) (fn.FunctionRuntime, error) { +func NewMultiFunctionRuntime(grpcAddress string, maxGrpcMessageSize int, functionConfigStore *functionconfigs.FunctionConfigStore) (fn.FunctionRuntime, error) { builtin := newBuiltinRuntime(functionConfigStore) if grpcAddress == "" { diff --git a/pkg/engine/grpcruntime_test.go b/pkg/engine/grpcruntime_test.go index 1fb9643f5..be465c534 100644 --- a/pkg/engine/grpcruntime_test.go +++ b/pkg/engine/grpcruntime_test.go @@ -24,8 +24,8 @@ import ( "testing" v1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" - "github.com/kptdev/porch/func/evaluator" + "github.com/kptdev/porch/controllers/functionconfigs" + "github.com/kptdev/porch/func/proto" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc" @@ -136,8 +136,8 @@ func TestGRPCRuntimeCloseMultipleCalls(t *testing.T) { func TestGRPCRunnerRunSuccess(t *testing.T) { client := &mockClient{ - evaluateFunc: func(ctx context.Context, req *evaluator.EvaluateFunctionRequest) (*evaluator.EvaluateFunctionResponse, error) { - return &evaluator.EvaluateFunctionResponse{ + evaluateFunc: func(ctx context.Context, req *proto.EvaluateFunctionRequest) (*proto.EvaluateFunctionResponse, error) { + return &proto.EvaluateFunctionResponse{ ResourceList: []byte(`apiVersion: config.kubernetes.io/v1alpha1 kind: ResourceList items: @@ -184,7 +184,7 @@ functionConfig: func TestGRPCRunnerRunEvaluationError(t *testing.T) { client := &mockClient{ - evaluateFunc: func(ctx context.Context, req *evaluator.EvaluateFunctionRequest) (*evaluator.EvaluateFunctionResponse, error) { + evaluateFunc: func(ctx context.Context, req *proto.EvaluateFunctionRequest) (*proto.EvaluateFunctionResponse, error) { return nil, status.Error(codes.Internal, "evaluation failed") }, } @@ -232,8 +232,8 @@ func TestGRPCRunnerRunReadError(t *testing.T) { func TestGRPCRunnerRunWriteError(t *testing.T) { client := &mockClient{ - evaluateFunc: func(ctx context.Context, req *evaluator.EvaluateFunctionRequest) (*evaluator.EvaluateFunctionResponse, error) { - return &evaluator.EvaluateFunctionResponse{ + evaluateFunc: func(ctx context.Context, req *proto.EvaluateFunctionRequest) (*proto.EvaluateFunctionResponse, error) { + return &proto.EvaluateFunctionResponse{ ResourceList: []byte(`apiVersion: config.kubernetes.io/v1alpha1 kind: ResourceList items: @@ -275,26 +275,26 @@ items: } type mockClient struct { - evaluateFunc func(context.Context, *evaluator.EvaluateFunctionRequest) (*evaluator.EvaluateFunctionResponse, error) + evaluateFunc func(context.Context, *proto.EvaluateFunctionRequest) (*proto.EvaluateFunctionResponse, error) } -func (m *mockClient) EvaluateFunction(ctx context.Context, req *evaluator.EvaluateFunctionRequest, opts ...grpc.CallOption) (*evaluator.EvaluateFunctionResponse, error) { +func (m *mockClient) EvaluateFunction(ctx context.Context, req *proto.EvaluateFunctionRequest, opts ...grpc.CallOption) (*proto.EvaluateFunctionResponse, error) { return m.evaluateFunc(ctx, req) } type mockFunctionEvaluator struct { - evaluator.UnimplementedFunctionEvaluatorServer - evaluateFunc func(context.Context, *evaluator.EvaluateFunctionRequest) (*evaluator.EvaluateFunctionResponse, error) + proto.UnimplementedFunctionEvaluatorServer + evaluateFunc func(context.Context, *proto.EvaluateFunctionRequest) (*proto.EvaluateFunctionResponse, error) } -func (m *mockFunctionEvaluator) EvaluateFunction(ctx context.Context, req *evaluator.EvaluateFunctionRequest) (*evaluator.EvaluateFunctionResponse, error) { +func (m *mockFunctionEvaluator) EvaluateFunction(ctx context.Context, req *proto.EvaluateFunctionRequest) (*proto.EvaluateFunctionResponse, error) { return m.evaluateFunc(ctx, req) } func startMockServer(t *testing.T) (string, func()) { mock := &mockFunctionEvaluator{} server := grpc.NewServer() - evaluator.RegisterFunctionEvaluatorServer(server, mock) + proto.RegisterFunctionEvaluatorServer(server, mock) lis, err := net.Listen("tcp", "localhost:0") require.NoError(t, err) go func() { @@ -350,6 +350,6 @@ func TestNewMultiFunctionRuntime_NilStorePanics(t *testing.T) { }) } -func newTestFunctionConfigStore() *reconciler.FunctionConfigStore { - return reconciler.NewFunctionConfigStore("", "") +func newTestFunctionConfigStore() *functionconfigs.FunctionConfigStore { + return functionconfigs.NewStore("", "") } diff --git a/pkg/engine/options.go b/pkg/engine/options.go index b6b9dcade..27c3eb5d7 100644 --- a/pkg/engine/options.go +++ b/pkg/engine/options.go @@ -19,7 +19,7 @@ import ( "github.com/kptdev/kpt/pkg/fn" "github.com/kptdev/kpt/pkg/lib/runneroptions" - "github.com/kptdev/porch/controllers/functionconfigs/reconciler" + "github.com/kptdev/porch/controllers/functionconfigs" cachetypes "github.com/kptdev/porch/pkg/cache/types" "github.com/kptdev/porch/pkg/repository" ) @@ -44,7 +44,7 @@ func WithCache(cache cachetypes.Cache) EngineOption { }) } -func WithBuiltinFunctionRuntime(functionConfigStore *reconciler.FunctionConfigStore) EngineOption { +func WithBuiltinFunctionRuntime(functionConfigStore *functionconfigs.FunctionConfigStore) EngineOption { return EngineOptionFunc(func(engine *cadEngine) error { runtime := newBuiltinRuntime(functionConfigStore) if engine.taskHandler.GetRuntime() == nil { diff --git a/pkg/util/image/image.go b/pkg/util/image/image.go new file mode 100644 index 000000000..75b7ece22 --- /dev/null +++ b/pkg/util/image/image.go @@ -0,0 +1,120 @@ +// Copyright 2026 The kpt 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 image + +import ( + "fmt" + "slices" + "strings" + + "github.com/Masterminds/semver/v3" + "k8s.io/klog/v2" +) + +// FindBestSemverMatch selects the cache key whose semver tag best satisfies +// the constraint for the given imageName. It returns the full cache key +// (e.g. "ghcr.io/foo/bar:v1.2.3") of the highest matching version. +func FindBestSemverMatch(constraint string, cachedTags []string) (string, error) { + c, err := semver.NewConstraint(constraint) + if err != nil { + return "", fmt.Errorf("invalid semver constraint %q: %w", constraint, err) + } + + type candidate struct { + key string + version *semver.Version + } + + var matches []candidate + for _, tag := range cachedTags { + v, err := semver.NewVersion(tag) + if err != nil { + klog.V(2).Infof("Failed to parse version %q: %v", tag, err) + continue + } + + if c.Check(v) { + matches = append(matches, candidate{key: tag, version: v}) + } + } + + if len(matches) == 0 { + return "", fmt.Errorf("no tag matching constraint %q found among %+v", constraint, cachedTags) + } + + slices.SortFunc(matches, func(a, b candidate) int { + return a.version.Compare(b.version) + }) + + selected := matches[len(matches)-1] + klog.V(3).Infof("Selected tag %q", selected.key) + + return selected.key, nil +} + +func Join(parts ...string) string { + if len(parts) == 0 { + return "" + } + + sb := strings.Builder{} + sb.WriteString(strings.Trim(parts[0], "/")) + + for _, part := range parts[1:] { + part = strings.Trim(part, "/") + sb.WriteString("/") + sb.WriteString(part) + } + return sb.String() +} + +// Parse creates a ParsedImage object from the full image name. +// Does not guarantee that the input string is a valid reference, unlike regclientref.New(). +func Parse(fullImageName string) ParsedImage { + output := ParsedImage{Original: fullImageName} + + firstSlash := strings.Index(fullImageName, "/") + lastSlash := strings.LastIndex(fullImageName, "/") + + if firstSlash != -1 { + str := fullImageName[:firstSlash] + if registryRE.MatchString(str) { + output.Registry = strings.TrimRight(str, "/") + fullImageName = fullImageName[firstSlash+1:] + + lastSlash = strings.LastIndex(fullImageName, "/") + if lastSlash != -1 { + output.SubPath = strings.Trim(fullImageName[:lastSlash], "/") + fullImageName = fullImageName[lastSlash+1:] + } + } else { + output.SubPath = strings.Trim(fullImageName[:lastSlash], "/") + fullImageName = fullImageName[lastSlash+1:] + } + } + + if lastAt := strings.LastIndex(fullImageName, "@"); lastAt != -1 { + output.Digest = fullImageName[lastAt+1:] + fullImageName = fullImageName[:lastAt] + } + + if lastColon := strings.LastIndex(fullImageName, ":"); lastColon != -1 { + output.Tag = fullImageName[lastColon+1:] + fullImageName = fullImageName[:lastColon] + } + + output.BaseName = strings.TrimLeft(fullImageName, "/") + return output +} diff --git a/pkg/util/image/image_test.go b/pkg/util/image/image_test.go new file mode 100644 index 000000000..079e46873 --- /dev/null +++ b/pkg/util/image/image_test.go @@ -0,0 +1,225 @@ +// Copyright 2026 The kpt 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 image + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFindBestSemverMatch(t *testing.T) { + testCases := map[string]struct { + constraint string + tags []string + expected string + expectedErr string + }{ + "selects highest matching version": { + constraint: ">= 0.4.0 < 0.5.0", + tags: []string{ + "v0.4.1", + "v0.4", + "@sha256:abcdef123456", + }, + expected: "v0.4.1", + }, + "exact version match": { + constraint: "0.1.1", + tags: []string{ + "v0.1.1", + "v0.1", + }, + expected: "v0.1.1", + }, + "no matching version for valid constraint": { + constraint: "> 1.0.0", + tags: []string{ + "v0.1.1", + "v0.1", + }, + expectedErr: "no tag matching", + }, + "invalid semver constraint": { + constraint: ">> 1.0.0", + tags: []string{ + "v1.1.0", + }, + expectedErr: "invalid semver constraint", + }, + "skips sha256-tagged entries": { + constraint: ">= 0.4.0", + tags: []string{ + "v0.4.1", + "v0.4", + "@sha256:abcdef123456", + }, + expected: "v0.4.1", + }, + "matches without registry prefix": { + constraint: ">= 0.4.0", + tags: []string{ + "v0.4.1", + "v0.4", + "@sha256:abcdef123456", + }, + expected: "v0.4.1", + }, + "empty cache keys": { + constraint: ">= 0.1.0", + tags: []string{}, + expectedErr: "no tag matching", + }, + "selects greatest from multiple matches": { + constraint: ">= 1.0.0 < 2.0.0", + tags: []string{ + "v1.0.0", + "v1.1.0", + "v1.2.0", + "v2.0.0", + }, + expected: "v1.2.0", + }, + "skips entries with unparseable versions": { + constraint: ">= 1.0.0", + tags: []string{ + "v1.0.0", + "vnotaversion", + }, + expected: "v1.0.0", + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + best, err := FindBestSemverMatch(tc.constraint, tc.tags) + if tc.expectedErr != "" { + assert.ErrorContains(t, err, tc.expectedErr) + } else { + require.NoError(t, err) + assert.Equal(t, tc.expected, best) + } + }) + } +} + +func TestImageParse(t *testing.T) { + const ( + registry = "ghcr.io" + registryWithPort = "my-registry.com:5000" + subpath = "kptdev/krm-functions-catalog" + image = "apply-setters" + tag = "v0.2.3" + digest = "sha256:7d89a74f106241391f687fc2985c8e6de597bb21f0d0014def5edc730618d9cc" + ) + + testCases := map[string]struct { + input string + want ParsedImage + }{ + "empty": { + input: "", + want: ParsedImage{}, + }, + "base name only": { + input: image, + want: ParsedImage{ + Original: image, + BaseName: image, + }, + }, + "tag only": { + input: fmt.Sprintf("%s:%s", image, tag), + want: ParsedImage{ + Original: fmt.Sprintf("%s:%s", image, tag), + BaseName: image, + Tag: tag, + }, + }, + "registry no path": { + input: fmt.Sprintf("%s/%s", registry, image), + want: ParsedImage{ + Original: fmt.Sprintf("%s/%s", registry, image), + Registry: registry, + BaseName: image, + }, + }, + "registry with path": { + input: fmt.Sprintf("%s/%s:%s", subpath, image, tag), + want: ParsedImage{ + Original: fmt.Sprintf("%s/%s:%s", subpath, image, tag), + SubPath: subpath, + BaseName: image, + Tag: tag, + }, + }, + "fully qualified, no digest": { + input: fmt.Sprintf("%s/%s/%s:%s", registry, subpath, image, tag), + want: ParsedImage{ + Original: fmt.Sprintf("%s/%s/%s:%s", registry, subpath, image, tag), + Registry: registry, + SubPath: subpath, + BaseName: image, + Tag: tag, + }, + }, + "digest without tag": { + input: fmt.Sprintf("%s@%s", image, digest), + want: ParsedImage{ + Original: fmt.Sprintf("%s@%s", image, digest), + BaseName: image, + Digest: digest, + }, + }, + "tag and digest": { + input: fmt.Sprintf("%s:%s@%s", image, tag, digest), + want: ParsedImage{ + Original: fmt.Sprintf("%s:%s@%s", image, tag, digest), + BaseName: image, + Tag: tag, + Digest: digest, + }, + }, + "registry with port": { + input: fmt.Sprintf("%s/%s/%s:%s", registryWithPort, subpath, image, tag), + want: ParsedImage{ + Original: fmt.Sprintf("%s/%s/%s:%s", registryWithPort, subpath, image, tag), + Registry: registryWithPort, + SubPath: subpath, + BaseName: image, + Tag: tag, + }, + }, + "digest with nested repository": { + input: fmt.Sprintf("%s/%s/%s@%s", registry, subpath, image, digest), + want: ParsedImage{ + Original: fmt.Sprintf("%s/%s/%s@%s", registry, subpath, image, digest), + Registry: registry, + SubPath: subpath, + BaseName: image, + Digest: digest, + }, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + got := Parse(tc.input) + assert.Equal(t, tc.want, got) + }) + } +} diff --git a/pkg/util/image/regex.go b/pkg/util/image/regex.go new file mode 100644 index 000000000..2cef57af4 --- /dev/null +++ b/pkg/util/image/regex.go @@ -0,0 +1,40 @@ +// Copyright 2020 The regclient 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. + +// taken from https://github.com/regclient/regclient/blob/b413945eb6f65d26be7cbbda4ddba6249e823a8b/types/ref/ref.go + +package image + +import "regexp" + +var ( + hostPartS = `(?:[a-zA-Z0-9](?:[a-zA-Z0-9-]*[a-zA-Z0-9])?)` + portS = `(?:` + regexp.QuoteMeta(`:`) + `[0-9]+)` + ipv6PartS = `(?:[0-9a-fA-F]{1,4}:){0,7}[0-9a-fA-F]{1,4}` + ipv6S = `(?:` + regexp.QuoteMeta(`[`) + `(?:` + + ipv6PartS + `|` + // uncompressed + regexp.QuoteMeta(`::`) + ipv6PartS + `|` + // prefix compressed + ipv6PartS + regexp.QuoteMeta(`::`) + ipv6PartS + `|` + // middle compressed + ipv6PartS + regexp.QuoteMeta(`::`) + // suffix compressed + `)` + regexp.QuoteMeta(`]`) + `)` + localhostS = `localhost` + hostDomainS = `(?:` + hostPartS + `(?:(?:` + regexp.QuoteMeta(`.`) + hostPartS + `)+` + regexp.QuoteMeta(`.`) + `?|` + regexp.QuoteMeta(`.`) + `))` + hostUpperS = `(?:[a-zA-Z0-9]*[A-Z][a-zA-Z0-9-]*[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[A-Z][a-zA-Z0-9]*)` + registryS = `(?:` + + `(?:` + hostDomainS + `|` + hostUpperS + `|` + ipv6S + `|` + localhostS + `)` + portS + `?|` + // name with dotted domain, upper case, or IPv6 with optional port + hostPartS + portS + // a short name with required port + `)` + + registryRE = regexp.MustCompile(`^(` + registryS + `)$`) +) diff --git a/pkg/util/image/types.go b/pkg/util/image/types.go new file mode 100644 index 000000000..b447ad214 --- /dev/null +++ b/pkg/util/image/types.go @@ -0,0 +1,83 @@ +// Copyright 2026 The kpt 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 image + +import "strings" + +// ParsedImage is a structured representation of a container image reference, +// broken into registry, sub-path, base name, tag, and digest components. +type ParsedImage struct { + // The registry part of the image name without trailing slash. + // Example: ghcr.io + Registry string + // The part of the image name between Registry and BaseName without leading or trailing slashes. + // Example: kptdev/krm-functions-catalog + SubPath string + // The last part of the image name, after all slashes, without the leading slash. + // Example: apply-setters + BaseName string + // The tag of the image without the leading colon. + // Example: v0.2.3 + Tag string + // The sha256 digest of the image, without the leading @, but containing the `sha256` prefix. + // Example: sha256:7d89a74f106241391f687fc2985c8e6de597bb21f0d0014def5edc730618d9cc + Digest string + // Original contains the unparsed image name. Intended for testing. + // Should be the same as the output of Full(). + Original string +} + +// Full reconstructs the full image name from the parsed parts. +func (p *ParsedImage) Full() string { + sb := strings.Builder{} + + if p.Registry != "" { + sb.WriteString(p.Registry) + sb.WriteString("/") + } + + if p.SubPath != "" { + sb.WriteString(p.SubPath) + sb.WriteString("/") + } + + sb.WriteString(p.BaseName) + if p.Tag != "" { + sb.WriteString(":") + sb.WriteString(p.Tag) + } + + if p.Digest != "" { + sb.WriteString("@") + sb.WriteString(p.Digest) + } + + return sb.String() +} + +// Prefix returns the part before BaseName with *no* trailing slash. +func (p *ParsedImage) Prefix() string { + sb := strings.Builder{} + + if p.Registry != "" { + sb.WriteString(p.Registry) + sb.WriteString("/") + } + if p.SubPath != "" { + sb.WriteString(p.SubPath) + } + + return sb.String() +} diff --git a/pkg/util/util.go b/pkg/util/util.go index 4310f5556..c25cedc41 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -26,7 +26,6 @@ import ( "slices" "strings" - semver "github.com/Masterminds/semver/v3" "github.com/google/uuid" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -346,88 +345,3 @@ func RetryOnError(retries int, f func(retryNumber int) error) error { klog.Errorf("Failed to fetch remote repository after %d retries: %v", retries, err) return err } - -// FindBestSemverMatch selects the cache key whose semver tag best satisfies -// the constraint for the given imageName. It returns the full cache key -// (e.g. "ghcr.io/foo/bar:v1.2.3") of the highest matching version. -func FindBestSemverMatch(constraint string, imageName string, cachedTags []string) (string, error) { - c, err := semver.NewConstraint(constraint) - if err != nil { - return "", fmt.Errorf("invalid semver constraint %q: %w", constraint, err) - } - - type candidate struct { - key string - version *semver.Version - } - - var matches []candidate - for _, tag := range cachedTags { - v, err := semver.NewVersion(tag) - if err != nil { - klog.Infof("Failed to parse version %q from cached image %q: %v", tag, imageName, err) - continue - } - - if c.Check(v) { - matches = append(matches, candidate{key: tag, version: v}) - } - } - - if len(matches) == 0 { - klog.Infof("Image %q with constraint %q is not found in the cache", imageName, constraint) - return "", fmt.Errorf("no image matching %q with constraint %q found in the cache", imageName, constraint) - } - - slices.SortFunc(matches, func(a, b candidate) int { - return a.version.Compare(b.version) - }) - - selected := matches[len(matches)-1] - klog.Infof("Selected image %q (version %q) for request %q", - imageName+":"+selected.key, selected.version, imageName) - - return selected.key, nil -} - -func GetImageName(image string) string { - if i := strings.Index(image, "@"); i != -1 { - image = image[:i] - } - - if i := strings.LastIndex(image, ":"); i != -1 && !strings.Contains(image[i+1:], "/") { - image = image[:i] - } - - if i := strings.LastIndex(image, "/"); i != -1 { - image = image[i+1:] - } - return image -} - -func GetImageRepository(image string) string { - lastSlash := strings.LastIndex(image, "/") - if lastSlash == -1 { - return "" - } - return image[:lastSlash] -} - -func GetImageTag(image string) string { - if strings.Contains(image, "@sha256:") { - return "" - } - - lastSlash := strings.LastIndex(image, "/") - lastColon := strings.LastIndex(image, ":") - - if lastColon == -1 || lastColon < lastSlash { - return "latest" - } - - return image[lastColon+1:] -} - -func ImageJoin(prefix, image string) string { - return strings.TrimRight(prefix, "/") + "/" + strings.TrimLeft(image, "/") -} diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index c5195d6be..64db738b0 100644 --- a/pkg/util/util_test.go +++ b/pkg/util/util_test.go @@ -473,136 +473,3 @@ func getPartErrMsg(errorSlice []string, start string) string { return "" } - -func TestFindBestSemverMatch(t *testing.T) { - cacheKeys := []string{ - "ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1", - "ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4", - "ghcr.io/kptdev/krm-functions-catalog/set-namespace@sha256:abcdef123456", - "ghcr.io/kptdev/krm-functions-catalog/apply-replacements:v0.1.1", - "ghcr.io/kptdev/krm-functions-catalog/apply-replacements:v0.1", - "ghcr.io/kptdev/krm-functions-catalog/starlark:v0.4.3", - "set-namespace:v0.4.1", - } - - t.Run("selects highest matching version", func(t *testing.T) { - cacheKeys := []string{ - "v0.4.1", - "v0.4", - "@sha256:abcdef123456", - } - key, err := FindBestSemverMatch( - ">= 0.4.0 < 0.5.0", - "ghcr.io/kptdev/krm-functions-catalog/set-namespace", - cacheKeys, - ) - assert.NoError(t, err) - assert.Equal(t, "v0.4.1", key) - }) - - t.Run("exact version match", func(t *testing.T) { - cacheKeys := []string{ - "v0.1.1", - "v0.1", - } - key, err := FindBestSemverMatch( - "0.1.1", - "ghcr.io/kptdev/krm-functions-catalog/apply-replacements", - cacheKeys, - ) - assert.NoError(t, err) - assert.Equal(t, "v0.1.1", key) - }) - - t.Run("no matching version for valid constraint", func(t *testing.T) { - _, err := FindBestSemverMatch( - "> 1.0.0", - "ghcr.io/kptdev/krm-functions-catalog/set-namespace", - cacheKeys, - ) - assert.Error(t, err) - assert.Contains(t, err.Error(), "no image matching") - }) - - t.Run("image not in cache", func(t *testing.T) { - _, err := FindBestSemverMatch( - ">= 0.1.0", - "ghcr.io/kptdev/krm-functions-catalog/nonexistent", - cacheKeys, - ) - assert.Error(t, err) - assert.Contains(t, err.Error(), "no image matching") - }) - - t.Run("invalid semver constraint", func(t *testing.T) { - _, err := FindBestSemverMatch( - ">> 1.0.0", - "ghcr.io/kptdev/krm-functions-catalog/set-namespace", - cacheKeys, - ) - assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid semver constraint") - }) - - t.Run("skips sha256-tagged entries", func(t *testing.T) { - cacheKeys := []string{ - "v0.4.1", - "v0.4", - "@sha256:abcdef123456", - } - key, err := FindBestSemverMatch( - ">= 0.4.0", - "ghcr.io/kptdev/krm-functions-catalog/set-namespace", - cacheKeys, - ) - assert.NoError(t, err) - assert.NotContains(t, key, "@sha256:") - }) - - t.Run("matches without registry prefix", func(t *testing.T) { - cacheKeys := []string{ - "v0.4.1", - "v0.4", - "@sha256:abcdef123456", - } - key, err := FindBestSemverMatch( - ">= 0.4.0", - "set-namespace", - cacheKeys, - ) - assert.NoError(t, err) - assert.Equal(t, "v0.4.1", key) - }) - - t.Run("empty cache keys", func(t *testing.T) { - _, err := FindBestSemverMatch( - ">= 0.1.0", - "ghcr.io/kptdev/krm-functions-catalog/set-namespace", - []string{}, - ) - assert.Error(t, err) - assert.Contains(t, err.Error(), "no image matching") - }) - - t.Run("selects greatest from multiple matches", func(t *testing.T) { - keys := []string{ - "v1.0.0", - "v1.1.0", - "v1.2.0", - "v2.0.0", - } - key, err := FindBestSemverMatch(">= 1.0.0 < 2.0.0", "myimage", keys) - assert.NoError(t, err) - assert.Equal(t, "v1.2.0", key) - }) - - t.Run("skips entries with unparseable versions", func(t *testing.T) { - keys := []string{ - "v1.0.0", - "vnotaversion", - } - key, err := FindBestSemverMatch(">= 1.0.0", "myimage", keys) - assert.NoError(t, err) - assert.Equal(t, "v1.0.0", key) - }) -}