-
Notifications
You must be signed in to change notification settings - Fork 960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Composition Functions Beta: Long-running Functions #4306
Conversation
If this design is accepted I'll need to update the plan laid out in the beta tracking issue. A lot of the issues it tracks are specific to the I believe the very high-level implementation plan for this design would be:
|
* Reviewers: Crossplane Maintainers | ||
* Status: Draft | ||
* Owners: Nic Cope (@negz) | ||
* Reviewers: Hasan Turken (@turkenh), Jared Watts (@jbw976) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
// The resource's connection details. | ||
map<string, bytes> connection_details = 2; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under the alpha design a Function can return a composed resource that specifies P&T style readiness checks and ways to derive connection details (e.g. connectionSecretFromField
etc). I've removed that under this design. I don't think I've seen anyone use it, and I don't think we actually need it. We can always add it back if there's a demand.
It's possible for a Function to just derive and write whatever XR connection details it needs, and to set the XR's as Ready whenever it feels it is. I'm pretty sure @turkenh identified we didn't need this in the original design and I think I agreed but must have forgotten and built it anyway? See #2886 (comment).
with ControllerConfig, but don't yet have a suitable alternative. Rather than | ||
propagating a ControllerConfig-like pattern I propose we prioritize finding an | ||
alternative. I intend to open a separate, simultaneous design to address this | ||
since it will affect Provider packages as well as Functions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: Here's the proposal:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, I like the direction we are heading toward!
Especially the possibility of converging to the same machinery with the Patch-and-Transform as a Function. instead of treating them as two different ways to compose resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @negz!! I really like the goals of this document around making functions easier to write and to use 🙇 🤩
A few questions for your consideration to help explain a few gaps in my understanding, nothing major blocking from my side!
Despite the name, a 'Function' is actually more like a 'function server'. Under | ||
this proposal, Functions are long-running processes. When you install one, the | ||
package manager deploys it using a Kubernetes Deployment - the same way it would | ||
deploy a Provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about calling the "function server" FunctionRunner
and Function
an actual configuration to be run with a function runner? I mean, a function to me is something that takes an input and produce some output, so something like the following could work:
---
apiVersion: pkg.crossplane.io/v1beta1
kind: FunctionRunner
metadata:
name: go-templates
spec:
package: xpkg.upbound.io/negz/go-templates:v0.1.0
status:
# The gRPC endpoint where Crossplane will send RunFunctionRequests.
endpoint: https://go-templates-9sdfn2
---
apiVersion: pkg.crossplane.io/v1beta1
kind: Function
metadata:
name: my-xpostgresql-go-templates
spec:
functionRunnerRef:
name: go-template
config:
apiVersion: example.org/v1
kind: GoTemplate
source: Remote
remote: git://github.com/example/my-xpostgresql-go-templates
---
apiVersion: apiextensions.crossplane.io/v2alpha1
kind: Composition
metadata:
name: example
spec:
compositeTypeRef:
apiVersion: database.example.org/v1
kind: XPostgreSQLInstance
functions:
- functionRef:
name: my-xpostgresql-go-templates
which could be inlined as follows:
---
apiVersion: apiextensions.crossplane.io/v2alpha1
kind: Composition
metadata:
name: example
spec:
compositeTypeRef:
apiVersion: database.example.org/v1
kind: XPostgreSQLInstance
functions:
- functionRunnerRef:
name: go-templates
config:
apiVersion: example.org/v1
kind: GoTemplate
source: Remote
remote: git://github.com/example/my-xpostgresql-go-templates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, a function to me is something that takes an input and produce some output
I think that's the "trick" here. We're really trying to frame these things (that are in practice more like servers) as "functions". We want folks to feel like they're building, installing support for, and calling "just a function" - not a server or runner. The fact that we make it a server at build time is a bit of an "implementation detail".
So to that end I think I prefer sticking with just "function" rather than introducing a runner type.
I see the (optional) config block in a Composition as kind of like an argument to a function call. In pseudocode Crossplane is making a call like this:
run_function(xr, composedResources, config) (xr, composedResources)
I think there will be a desire to avoid repeating the same config across multiple function calls, but I'd rather do that by allowing Functions to deliver a Config CRD, which could be instantiated and referenced. This has the added benefit of allowing Functions to deliver fully (OpenAPI) schemafied config types that the API server could validate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my confusion comes from the fact that I would think to be writing more “runtime” based functions than full-fledged ones, hence my point of separating functions and function runners (a.k.a. runtimes).
Maybe it’s because I’m thinking of something similar to this which I used in the past, but as a user I would expect to be writing more quick “scripts” in Python, Starlark or some other scripting language, rather than really complex “functions” that would require to be wrapped in a grpc server on their own.
But I feel your focus is instead more on full-fledged functions, so it makes sense to consider “runtime” ones as an exception/hack w.r.t. the concept of a function, in which one of the arguments to the function being called is actually custom logic/code, and therefore don’t deserve being first-class citizens of the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I share @phisco's view on Functions
and FunctionRunners
. Also calling it config suggests that the function is the thing that has consumed the config:
function(resources) := functionRunner(config)(resources)
But this is clearly cosmetics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to avoid introducing a new FunctionRunner
concept to our users and keep it as implementation details as long as it makes sense.
I agree with @phisco's concerns around the need for reusable config's for functions, however, would like to see it as Function
(as package) + FunctionConfig
(as CRD), which is consistent with what we have today with Provider
(as package) + ProviderConfig
(as CRD). (This could also help with the passing credential problem in #3718 /cc @ezgidemirel).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some discussion with @turkenh I've renamed config
to input
. This better communicates what this type is for. It doesn't configure the Function
type in any global way - it just specifies optional input passed with the Function call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn’t this mean that a function would have two types of inputs? The config and the actual input coming from the P&T pipeline or the previous function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn’t this mean that a function would have two types of inputs? The config and the actual input coming from the P&T pipeline or the previous function?
Yes - that's what I was trying to communicate before. The input to a "function call" is like function(xr, composed_resources, input)
.
The mental model of a call being more like runner(input)(XR, composed_resources)
does conceptually make sense to me, especially given that we think some "inputs" will be repeated a lot - i.e. N Compositions might all call the same function with the same input (fka config).
I still prefer to not expose "runner" as a distinct concept just because:
- There's fewer concepts to think about overall. You just install ("define") a Function, then call it (from a Composition).
- Similarly, there's fewer API types to worry about. In many cases you just install a
Function
(often as a dependency of your Configuration) and call it from your Composition. You don't need to install aFunctionRunner
, then define aFunction
, then call that from your Composition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think most people are going to use “runtime” based functions, at least that’s what I would do. So the two options are actually Function (+ PackageRuntimeConfig) + FunctionConfig or FunctionRunner (+ PackageRuntimeConfig) + Function. I still think the latter would be clearer to me, and it’s actually more similar to the original proposal, just with less focus on OCI images. However as we said it’s mostly cosmetic, so I’m ok marking this thread as resolved given that it looks like there is definitely consensus around the proposed solution.
XRs rely on poll-triggered reconciliation to promptly correct drift of their | ||
composed resources. The poll interval can be set using the `--poll-interval` | ||
flag. The XR controller does not know what kinds of resources it will compose | ||
when started, so it cannot start a watch for them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function invocation should not depend on the composed objects.
We know that the first function in the pipeline only depends on the XR, claim (we make that available too, don't we?) and the composition, and with that the whole pipeline.
What we need is a way to detect drift of the composed objects in order to trigger another run of the pipeline on demand. Technically, we don't even need a cached image for the trigger (we have to detect deletions and updates to the spec of composed objects).
With that said, it would be good to clearly call out the assumptions a function author can depend on. Being called every 60s should not be one of them IMO. Determinism for a defined set of inputs should be a requirement for every function. That keeps the freedom for us to improve the implementation later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned today that compositions are not well-founded in the sense that they can depend on siblings from the live system through ToCompositeFieldPath
. We are actually not really building just compositions, but at the same time they are transitions of a state machine. Mind blowing. We don't call that out anywhere properly or I didn't find it.
Our innnocent looking sentence in the docs:
FromCompositeFieldPath and ToCompositeFieldPath patches can also take a wildcarded field path in the toFieldPath parameter and patch each array element in the toFieldPath with the singular value provided in the fromFieldPath.
In that sense the inputs of a function include the live system MRs, and maybe some day we watch them too. Would be better IMO than the tag+ttl idea you are mentioning further down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't both work?
There's a section on potentially watching composed resources under "Future Considerations". Also #4316.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing all of my feedback @negz, this is looking good to me with the feedback you've incorporated! 💪
This one-pager proposes a lightweight implementation of the PRI proposal, along with an alternative for ControllerConfig. I believe this will be relevant for the in-flight Composition Functions beta design, as well as for Providers. crossplane#3601 crossplane#2671 crossplane#4306 Signed-off-by: Nic Cope <[email protected]>
This commit introduces an updated design for Composition Functions. Under this design Functions are long-running processes, like Providers. Crossplane makes RunFunctionRequest RPCs to a Function using gRPC. This updated design also sketches out an improved Function development experience, as well as a few hypothetical 'generic' Functions. A generic Function is a Function that works with any XR. Generic Functions extend Crossplane with new ways to express Composition logic without actually needing to develop a Function of your own. Signed-off-by: Nic Cope <[email protected]>
Forgot to hit save in my editor. :D Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Nic Cope <[email protected]>
This reduces the amount of times 'name' appears and clarifies that the Functions array is a pipeline of functions. Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Nic Cope <[email protected]>
i.e. Be explicit that we want to asbtract away the fact that these are servers. Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Nic Cope <[email protected]>
This was mostly covered already, but now easier to find and with a touch more detail (e.g. on dependencies). Signed-off-by: Nic Cope <[email protected]>
This could be useful when running 'composition render'. Signed-off-by: Nic Cope <[email protected]>
This was thrown in during the original design while we toyed with Functions as being a way to do "day two stuff" (e.g. trigger backups). I don't think anyone is confident that this is a good idea, so let's not mention it for now. Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Nic Cope <[email protected]>
Mixing up my CS terms. :) Signed-off-by: Nic Cope <[email protected]>
This better frames it as what is is - input to a particular Function call. The downside with calling if 'config' was that folks could mistake it to mean it was configuring the Function _globally_, not just configuring how one particular entry in one Composition's pipeline of steps calls a Function. Signed-off-by: Nic Cope <[email protected]>
// A Result of running a Function. | ||
message Result { | ||
// Severity of this result. | ||
Severity severity = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the expected behavior of Crossplane when a gRPC call returns an error? For example, should it trigger an event, modify the Sync condition etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably want it to fail the whole pipeline and return an error. Doing so would cause us to surface the error in a status condition and an event.
There's an entry in #3751 that mentions we should return errors with meaningful gRPC status codes, which will still be relevant under this design. In future we could potentially use this to better understand what errors are fatal vs returnable.
I believe we have good consensus that this is a better technical direction for Composition Functions. I'm going to merge this design so we can start moving in this direction in time for Crossplane v1.14. I do want to call out again that the user and developer experience parts of this design should be considered examples of potential experiences. We need to test them with community members and will surely iterate on them between now, beta, and beyond. What we do have confidence on is that it's better to build Functions as packages, and deploy them as long-running processes. |
@negz late comment regarding generic functions and Starlak example: Carvel ytt (https://carvel.dev/ytt/) is doing a great job of leveraging Starlak for specifically supporting a wide range of yaml files manipulation use-cases: mixing plain yaml and starlak fragments. I have successfully used ytt to craft existing compositions while leveraging full IDE support (see #3197 (comment)). I planned to test an alpha xfn using ytt cli (https://github.com/orange-cloudfoundry/paas-templates/issues/1813 ) before I learned about this new proposal. Depending on roadmap for this proposal to be implemented, a containerized Function leveraging ytt cli might provide good feedback on how useful a ytt generic function could be, and help draft possible workflows such as "git-ops function authoring". A ytt generic function might need to leverage
By "git-ops function authoring" workflow, I mean the practice of iteratively go through authoring/local-test/git push/deploy/trigger E2E test. I have fullfilled such workflow currently using ytt and kapp controller with 90 seconds cycle iterations (between push and start of kuttl test in the cluster). It seems important to keeping in mind the requirement of short feedback loop during composition function authoring. I wonder how the containerized function workflow described at https://github.com/crossplane/crossplane/blob/master/design/design-doc-composition-functions.md#use-an-oci-container could accomodate "gitops workflows", with the following potential challenges
See also related exchange into vfarcic/devops-toolkit-crossplane#4 (comment)
At that time @vfarcic opted for golang programming as a way to automate "tedious" crossplane authoring packages workflows. |
Another late idea related to this proposal
I was wondering whether there are other examples of such pattern in the kubernetes ecosystem. The admission control system seems to share many characteristics with crossplane composition functions. https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/ Was it considered to leverage the mutating webhook infrastructure to invoke composition functions developped in GPL or existing contemporary "Patch and Transform" libraries such as kyberno ? This approach may have the potential to reuse and leverage many of the existing k8s infrastructure for invocating webhooks that crossplane would have to implement when issuing direct grpc calls to composition functions. Examples benefits:
I have not found traces of such discussion in the design proposal or from memory in the alpha composition design. Did I miss it ? Ps: I'm currently out-of-office with limited connectivity. My responses on this thread will be late. |
FunctionIO
type and update the RunFunctionRequest
and RunFunctionResponse
RPC types
#4419
This one-pager proposes a lightweight implementation of the PRI proposal, along with an alternative for ControllerConfig. I believe this will be relevant for the in-flight Composition Functions beta design, as well as for Providers. crossplane#3601 crossplane#2671 crossplane#4306 Signed-off-by: Nic Cope <[email protected]>
This one-pager proposes a lightweight implementation of the PRI proposal, along with an alternative for ControllerConfig. I believe this will be relevant for the in-flight Composition Functions beta design, as well as for Providers. crossplane#3601 crossplane#2671 crossplane#4306 Signed-off-by: Nic Cope <[email protected]> Signed-off-by: Rohit31201 <[email protected]>
Description of your changes
This pull request introduces an updated design for Composition Functions. Under this design Functions are long-running processes, like Providers. Crossplane makes RunFunctionRequest RPCs to a Function using gRPC.
This updated design also sketches out an improved Function development experience, as well as a few hypothetical 'generic' Functions. A generic Function is a Function that works with any XR. Generic Functions extend Crossplane with new ways to express Composition logic without actually needing to develop a Function of your own.
My intention is that if this design is approved, we'll implement it as
v1beta1
support for Functions in Crossplane v1.14 (due in October).I have:
Added or updated unit and E2E tests for my change.Runmake reviewable
to ensure this PR is ready for review.Addedbackport release-x.y
labels to auto-backport this PR if necessary.