Resolve functions using function objects registered in apiserver #3633
Conversation
justinsb
changed the title
justinsb
force-pushed
the
function_resolver
branch
from
90783a9 to
2885bdf
Compare
justinsb
changed the title
porch/pkg/apiserver/apiserver.go
Outdated
| @@ -147,6 +148,13 @@ func (c completedConfig) getCoreClient() (client.WithWatch, error) { | |||
| if err := configapi.AddToScheme(scheme); err != nil { | |||
| return nil, fmt.Errorf("error building scheme: %w", err) | |||
| } | |||
|
|
|||
| // We query the apiserver, even if we could query directly; this will then work with CRDs etc. | |||
| // TODO: We need to think about priority-and-fairness with loopback queries | |||
There was a problem hiding this comment.
This comments seems out of place. Registering porchapi to current scheme doesn't involve querying apiserver.
Perhaps, misplaced ?
There was a problem hiding this comment.
This is where we enable it, but fair point - I moved to the callsite!
| Namespace: r.namespace, | ||
| Name: name, | ||
| } | ||
| if err := r.client.Get(ctx, key, &function); err != nil { |
There was a problem hiding this comment.
Some clarifications:
How's the functions registry API work today ?
I am assuming by default, functions won't be there in any porch installation, so this resolving should be opt-in. Simplify because it is now calling network call for every function with out /. WDYT ?
There was a problem hiding this comment.
How it works today: You can register an OCI Repository with spec.content: Function, and then any discovered images are registered as Functions.
Performance: Currently I don't think the client.Client is caching (but it's always hard to know with controller-runtime!), but we could make it caching if we're concerned about the performance.
Opt-in: I think this should be the default behaviour and the default way we use functions. It's somewhat akin to the Function Catalog idea from kustomize. That said, I expect it to evolve over time (not least because we want to do something on the client side as well). If we have a flag and we set it for users then everyone will be running with anyway :-) I think we have a degree of opt-in because the user has to register a Repository of spec.content=Function to activate the behaviour (otherwise we fall through).
I do think I should write a doc about how this works today, we can keep it updated even as it changes...
There was a problem hiding this comment.
I think function registry is not widely discussed or shown in any of the demos, so I always considered as aspirational :) but this discussion suggests it is not the case.
Re: performance, I think functions are always beaten for slow performance, so just being careful with adding new latencies :)
Yes, I think documentation will certainly help.
For now, let's go with this and see if it causes any trouble in the field.
| @@ -225,5 +227,6 @@ func (o *PorchServerOptions) AddFlags(fs *pflag.FlagSet) { | |||
| } | |||
|
|
|||
| fs.StringVar(&o.FunctionRunnerAddress, "function-runner", "", "Address of the function runner gRPC service.") | |||
| fs.StringVar(&o.DefaultImagePrefix, "default-image-prefix", "gcr.io/kpt-fn/", "Default prefix for unqualified function names") | |||
There was a problem hiding this comment.
should we make the API based function resolution opt-in ?
justinsb
force-pushed
the
function_resolver
branch
from
2885bdf to
01ea958
Compare
The intent is that many users will run a function mirror or similar.
justinsb
force-pushed
the
function_resolver
branch
from
01ea958 to
17132d4
Compare
The intent is that many users will run a function mirror or similar.