FunctionConfig controller improvements 1#1019
Draft
mozesl-nokia wants to merge 15 commits into
Draft
Conversation
…or now Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
mozesl-nokia
added
go
✅ Deploy Preview for kpt-porch ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the FunctionConfig controller, store, and surrounding infrastructure. The previous multi-cache FunctionConfigStore is replaced with a single multi-layer (image → prefix → tag) cache; image parsing is moved into a new pkg/util/image package; the func/internal package is renamed func/evaluator, the generated proto code moves to func/proto, and shared evaluator types move to func/types. Imports across the repository are updated accordingly.
Changes:
- Unify the function-config cache and rewrite
Store/Getpaths around a parsed-image lookup, replacing the priorUpdateExecCache/UpdateBinaryCache/GetBinaryFromCache*API. - Introduce
pkg/util/imagewith a newParsedImage,Parse,Join, andFindBestSemverMatch; remove the older helpers frompkg/util/util.go. - Reorganise
func/: rename packageinternal→evaluator, move generated proto tofunc/proto, extract shared types tofunc/types, and add a startup wait forFunctionConfigwarmup.
Reviewed changes
Copilot reviewed 43 out of 48 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/util.go, util_test.go | Removes image helpers and semver match (moved to pkg/util/image). |
| pkg/util/image/{image,types,regex}.go, image_test.go | New image parsing/joining/semver helpers and tests. |
| controllers/functionconfigs/store.go, store_test.go | New unified FunctionConfigStore implementation and tests. |
| controllers/functionconfigs/reconciler.go, reconciler_test.go | New reconciler using Store, replaces deleted reconciler/functionconfigreconciler.go. |
| controllers/functionconfigs/reconciler/functionconfigreconciler.go | Deleted (logic split into store.go/reconciler.go). |
| controllers/main.go, main_test.go | Pre-population uses the new Store API; minor test updates. |
| controllers/packagerevisions/.../packagerevision_controller.go, config_test.go | Import path/type updates for the new package. |
| api/porchconfig/v1alpha1/function_config_types.go | Adds TagIterable interface and IterTags methods on executor configs. |
| pkg/apiserver/apiserver.go | Updated imports/types for the renamed package. |
| pkg/engine/{builtinruntime,grpcruntime,options}.go (+tests) | Use the new store API and imageutil. |
| func/server/server.go, func/client/main.go, func/wrapper-server/* | Import-path updates after the package renames. |
| func/proto/evaluator.{proto,pb.go,_grpc.pb.go} | Generated proto files relocated. |
| func/types/common_types.go | Newly-shared evaluator types (PodData, ConnectionRequest, etc.). |
| func/evaluator/*.go (+ tests) | Package renamed internal → evaluator; uses new shared types and store API; adds startup warmup wait. |
| func/evaluator/testdata/config*.yaml | Test fixtures added. |
| func/Makefile | Proto source path partially updated. |
| .golangci.json | Allow dot-import (ST1001) under func/evaluator/. |
Comments suppressed due to low confidence (2)
func/Makefile:31
- The Makefile recipe was only partially updated. The prerequisite was changed to
proto/evaluator.proto, butCOMPILED_PROTO(line 19) still targets files underevaluator/, and the recipe (lines 28–31) still uses-I ./evaluator, writes outputs to./evaluator, and compiles./evaluator/evaluator.proto. With the proto file moved toproto/, runningmakewill fail to find the source. TheCOMPILED_PROTOvariable, the-Iinclude path, the--go_out/--go-grpc_outpaths, and the finalprotocargument all need to point toproto/.
$(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
func/evaluator/podcachemanager.go:296
spec.PodExecutor.Tags[0]will panic ifTagsis empty. The previous code (removed) gated this withlen(entry.Spec.PodExecutor.Tags) > 0. Furthermore,IterPodConfigSpecsiterates over every prefix×tag entry in the cache, so the samespecwill be yielded multiple times here, causing redundant warm-up goroutines for the same image. A length check onTagsis required, and the iteration semantics likely need to be reconsidered (e.g. yield each unique image once).
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
mozesl-nokia
force-pushed
the
fnconf-improv-1
branch
from
924f550 to
6ea3b9a
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 48 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
func/evaluator/podcachemanager.go:291
spec.PodExecutor.Tags[0]is indexed without checkinglen(spec.PodExecutor.Tags), which will panic when a FunctionConfig has a PodExecutor but no tags (allowed by the API comment). Treat an empty tag list aslatest(or skip warmup for that entry).
pkg/engine/builtinruntime.go:54- Leftover debug log
klog.Infof("????")should be removed (or replaced with a meaningful message). As-is it adds noisy, unhelpful output on parse failures.
Comment on lines
+87
to
+92
| if entry, ok := s.internalCache[spec.Image]; ok && entry.objName != objKey { | ||
| return apierrors.NewConflict( | ||
| configapi.TypeFunctionConfig.GroupResource(), | ||
| client.ObjectKeyFromObject(obj).String(), | ||
| pkgerrors.Errorf("Image %q is already configured from object %q", spec.Image, objKey), | ||
| ) |
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Comment on lines
+26
to
+28
| // 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Title
FunctionConfig controller improvements 1
Description
func/.Storemethod has been made "smarter", improved theGetmethods.Related Issue(s)
None
Type of Change
Checklist
Additional Notes (Optional)
make destroydoes not seem to work 100% of the time because porch-server gets deleted before the finalizers can be removed from the functionconfigsAI Disclosure
I have not used AI in the creation of this PR.