feat(design): project and resource simulate - #181
Conversation
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
We demonstrated the end to end flow, the CLI driving provider plan servers against a live control plane, in the Crossplane community meeting: see the |
| preview of the revision advance rather than of the apply, and counted | ||
| separately in the summary. crossplane-diff reached the same conclusion from | ||
| the other direction: its composition diffing excludes Manual-policy | ||
| composites by default behind an `--include-manual` flag. This keeps that |
There was a problem hiding this comment.
is there a reason why we wouldn't keep the same nomenclature?
I went with manual for the flag because Manual is the enumeration value for the compositionUpdatePolicy.
There was a problem hiding this comment.
Open for other names, but feels natural to see intent for the action that we include all manuals
| 1. **Discover.** Parse the project file, find the Compositions under the | ||
| project's API paths, and list the XRs on the target cluster whose type | ||
| each Composition composes. `--namespace` and `--name` scope the run. | ||
| 2. **Prepare.** Build the project's embedded functions locally, exactly as |
There was a problem hiding this comment.
I think we should have a variant of this that pulls these from the live cluster (at least, any not overridden locally) -- unless you mean to scope resource simulate to include -f composition.yaml as well?
I don't think everyone is using projects (yet?) and the folks who aren't will likely still want to diff a composition.
There was a problem hiding this comment.
is composition diff mean for Inline resource configuration ?, same is working with conpositon render from projects already and yes include -f Composition.yaml
| review. The existing maintainers stay code owners of what they built, the | ||
| standalone repo enters maintenance with a pointer forward, and every | ||
| Crossplane user gets the preview by default: one binary, one release | ||
| train, one CI matrix. |
There was a problem hiding this comment.
one thing i am attached to that saves a bunch of time in diff development is the integration test suite with setup-envtest.
it's about 7 seconds to run a full IT through kong input -> machine output against a sort-of-fake cluster, versus minutes to spin up the e2e kind cluster. because it's so (relatively) quick, it covers a lot of surface area and will be valuable for keeping regressions to a minimum during the transition.
we'll probably want to port that over for any CLI commands that benefit from testing against a cluster. it could form a foundation of the e2e/IT testing work we've been meaning to tackle.
There was a problem hiding this comment.
for simulate we only using an real controlplane to get resources - the other parts of the machinery running with render and diff gRPC Server - but good to think for Testing ( Composition + e2e ) about Implementation Details
| fetches what functions ask for from the live cluster), the diff | ||
| calculator (rendered against live, including removal detection and nested | ||
| XRs), the server-side dry-run that keeps defaulting and webhook mutation | ||
| out of the diff, and the composition impact discovery behind |
There was a problem hiding this comment.
these already build on c/cli: the impact discovery uses the xrmClient of crossplane trace. and we run all the XRs through crossplane validate.
this will probably mean promoting trace and validate to GA (or at least the libraries they build on)
There was a problem hiding this comment.
true let's do this If we have more commands using thr libraries
| rendered counterpart means the change would remove it, and it enters the | ||
| plan as a delete instead of silently disappearing from the preview. | ||
|
|
||
| Printing then follows one rule: show users their own manifest, annotated. |
There was a problem hiding this comment.
it will be a conscious choice to switch from kubectl diff style syntax to terraform plan syntax.
maybe we should see which way consumers would prefer.
since my use case is largely for CI, i tried to build diff to use an output format that would be compatible with markdown's diff interpreter syntax for code blocks, so line by line replacement:
- foo: bar
+ foo: bazThere was a problem hiding this comment.
perhaps an output flag ?
| (@tampakrap), Adam Wolfe-Gordon (@adamwg) | ||
| * Status: Draft | ||
|
|
||
| ## Background |
There was a problem hiding this comment.
one thing i don't see mentioned here is claims. we have a lot of people still on crossplane v1's object model; any specific goal or non-goal about those?
diff does try pretty hard to work against legacy objects.
There was a problem hiding this comment.
for projects we have v2 support, this should be our target for all commands V2 under project ... we introduced some migration Checks in v1.20 cli - so my feeling is we should not adding more v1 features here.. but open for suggestions...
There was a problem hiding this comment.
I don't have a strong opinion about support or non-support for claims. Two things to consider, though:
- Claims are supported in Crossplane v2 via the
LegacyClusterscope on v1 XRDs and there's nothing stopping users from putting v1 XRDs in a project (thoughcrossplane xrd generatealways generates v2). - We already have machinery in the CLI to convert claims to XRs, so seems like it would be a small lift to support claims here; on the other hand, it would also simple enough for users to convert their claims to XRs before running simulate.
| discovery. Both commands enter the tree with the CLI's `maturity:"alpha"` | ||
| tag and graduate the way every other command does. | ||
|
|
||
| ### Running Plan Servers Like Render Runs Functions |
There was a problem hiding this comment.
one thing i found out the hard way is that diff is slow unless you keep containers alive across runs. every run might stand up a crossplane container, all function containers, now provider containers, and then tear them down. we should explicitly account for container orchestration: strong names, maybe even a top level --orphan-containers flag in case you need to run some things serially.
There was a problem hiding this comment.
true - like functions we should have an option to have providers alive across runs
| render and crossplane-diff already share. Functions that require extra | ||
| resources get them the way crossplane-diff resolves them today: | ||
| iteratively, fetching what the pipeline requests from the live cluster | ||
| and re-rendering until requirements stabilize, with an iteration cap. |
There was a problem hiding this comment.
"iteration cap" reminds me of failure modes. diff is designed to fail fast and not emit partial results (for a given resource's tree) to avoid giving a false sense of confidence. is that a design principle we want to adhere to, here, or do we think "best effort" instead?
we should also agree on an error contract for structured mode. diff writes both stderr and the structured payload OutputError but that's a relatively new change for structured mode.
| The CLI treats provider packages the way render treats function packages: as | ||
| images it can run locally and talk to over gRPC. | ||
|
|
||
| From the project's `dependsOn` (or an explicit `--provider-images` override, |
There was a problem hiding this comment.
how does a provider-images override work? how do i know which provider serves a given api group without a project? are we looking into the cluster for this (it's what diff would do)? and if we're querying the cluster anyway, shouldn't we just query it and keep the image override optional?
(i suppose we could also query the provider registry, but that seems like an added dependency we don't want)
There was a problem hiding this comment.
providers will have an gRPC Info endpoint to tell which api-groups are available per gRPC server
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
I like this a lot overall, and have no major concerns. A bunch of comments inline on the details - no blockers, just some considerations.
| (@tampakrap), Adam Wolfe-Gordon (@adamwg) | ||
| * Status: Draft | ||
|
|
||
| ## Background |
There was a problem hiding this comment.
I don't have a strong opinion about support or non-support for claims. Two things to consider, though:
- Claims are supported in Crossplane v2 via the
LegacyClusterscope on v1 XRDs and there's nothing stopping users from putting v1 XRDs in a project (thoughcrossplane xrd generatealways generates v2). - We already have machinery in the CLI to convert claims to XRs, so seems like it would be a small lift to support claims here; on the other hand, it would also simple enough for users to convert their claims to XRs before running simulate.
| crossplane/upjet proposes exposing that knowledge: a `PlanService` gRPC protocol | ||
| that upjet providers serve from an `internal plan-server` subcommand, computing |
There was a problem hiding this comment.
Does this need to be a full networked gRPC server, or could it be a one-shot command that accepts a protobuf request on stdin and prints a protobuf response to stdout (like crossplane internal render)? I think the stdin/stdout scheme is overall simpler from the CLI perspective (in particular, less sensitive to different container runtime setups and other details of user environments).
| would do. | ||
|
|
||
| ```console | ||
| $ crossplane resource simulate -f resources.yaml |
There was a problem hiding this comment.
How does the CLI know which provider(s) to use in this mode? Is --provider-images required, or are providers discovered from the running cluster?
| (When Crossplane records required-resource references on XRs, | ||
| [crossplane/crossplane#7351][issue-7351], discovery collapses to a | ||
| single read; the loop is the interim.) |
There was a problem hiding this comment.
I think a loop would still be necessary even if we had required resource references on XRs, since the functions may require different resources than were previously recorded. Fetching the last recorded required resources up-front would certainly be an optimization, though.
| One class of matched XRs needs special honesty: those pinned to an older | ||
| CompositionRevision (`compositionUpdatePolicy: Manual`). Applying the |
There was a problem hiding this comment.
Until we've finished the progressive function rollout work (crossplane/crossplane#6139), function changes will still change the behavior of old composition revisions, and that might be worth simulating. Any reason not to fetch the referenced composition revision from the cluster and use it for simulation?
| a direct call, and its loading and validation glue merges with the CLI's | ||
| own instead of wrapping it. The move is the moment to shed that | ||
| scaffolding, not to carry it along. | ||
| * **What it becomes.** One simulate surface, not parallel commands. What |
There was a problem hiding this comment.
Is the current crossplane-diff functionality subsumed by the new simulation commands? Should we have something like crossplane resource diff that does exactly what diff does today, without the provider-level simulation?
| Simulate's cost scales with what the user asks to preview, not with the size | ||
| of the control plane. Per XR: the same render work `crossplane render` does | ||
| (plus requirement-resolution iterations, capped), one server-side dry-run | ||
| per composed resource, and one gRPC plan call per rendered MR, each of |
There was a problem hiding this comment.
Could we batch MRs into plan calls to the providers?
3 participants
Description of your changes
This proposes
crossplane project simulateandcrossplane resource simulate: a terraform plan style preview of what a Composition, embedded function, XR, MR, or provider version change would do, both to the cluster and to the cloud behind it.Rendering and cluster diffing build on crossplane-contrib/crossplane-diff rather than reimplementing it, and the proposal is explicit that converging that work into the CLI happens together with its maintainers, who are proposed reviewers here. The cloud layer drives per-provider plan servers over gRPC, the way render drives function runtimes.
Companion proposal in crossplane/upjet#694 defines the PlanService protocol and the provider-side implementation.
Fixes #
I have:
./nix.sh flake checkto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.