Skip to content
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

Use server-side apply #4047

Closed
11 of 13 tasks
negz opened this issue May 4, 2023 · 14 comments
Closed
11 of 13 tasks

Use server-side apply #4047

negz opened this issue May 4, 2023 · 14 comments
Assignees
Labels
composition enhancement New feature or request roadmap Issues that have priority and are included in the roadmap, or are candidates to add to the roadmap
Milestone

Comments

@negz
Copy link
Member

negz commented May 4, 2023

What problem are you facing?

Most core Crossplane reconcilers use an Applicator to create and update resources in the API server. For example, we use an Applicator to keep claims synced with XRs, and to create or update an XR's composed resources.

An Applicator provides similar "upsert" semantics to kubectl apply. It allows you to simply "apply" your desired state - the Applicator then figures out whether the desired state affects a resource that already exists and if so how it should update it. When we began the Crossplane project the API server and Go Kubernetes client tooling did not support this - you had to either Create a new resource, or Get, mutate, and then Update an existing resource.

There are currently two implementations of the Applicator interface:

Both implementations simply call the underlying Kubernetes client's Create method if the relevant resource doesn't exist. Where they differ is what they do to update existing resources.

The APIPatchingApplicator uses the underlying client's Patch method. It creates a JSON merge patch where the patch is simply the entire new, desired resource. This has a few downsides:

  • Patches will never remove a field from an object; they're only additive.
  • Arrays are always replaced, not merged.

The APIUpdatingApplicator uses the underlying client's Update method. It circumvents the typical read, mutate, update approach by simply passing the new desired state to Update. Typically the Update method guards against this by using the passed resource's meta.resourceVersion as a form of eventual consistency. It expects to be passed a resource version equal to the latest version passed in the API server, to ensure clients don't accidentally revert state by writing an update based on a stale read. This is the major issue with the APIUpdatingApplicator - it manually sets the resource version to the latest read from the API server even if the state its passing is from an older version. This leads to the APIUpdatingApplicator unintentionally reverting changes made by other controllers.

Currently neither Applicator works for all situations. This results in us picking the least-bad option for a particular use case. We suspect these applicators may be the cause of several issues in Crossplane, including:

Related issues

  1. bug composition
    negz ravilr
  2. bug
    pedjak turkenh
  3. bug composition stale
    muvaf
  4. bug composition stale triage
  5. bug composition
    pedjak
  6. bug composition functions
    bradkwadsworth-mw negz
  7. enhancement stale
  8. bug composition
    pedjak
  9. enhancement stale
  10. bug composition
  11. bug composition stale
  12. bug composition exempt-from-stale v2
  13. bug composition
    negz pedjak

How could Crossplane help solve your problem?

Some time ago the Kubernetes API server introduced server-side-apply. This functionality intends to do the same thing as our Applicator types, deferring the "hard work" of figuring out how to make an update to the API server. Clients simply send their desired state. The API server supports more advanced merge semantics for arrays of objects, and also tracks ownership of fields to better support multiple controllers reconciling a single resource.

I believe it would be worth our while to use server-side-apply. Ideally we could deprecate our bespoke Applicator implementations entirely.

@negz negz added enhancement New feature or request composition labels May 4, 2023
@negz
Copy link
Member Author

negz commented May 4, 2023

Note that there is a proof-of-concept implementation in crossplane/crossplane-runtime#358

@jbw976 jbw976 added this to the v1.14 milestone May 8, 2023
@turkenh
Copy link
Member

turkenh commented May 18, 2023

@jbw976 jbw976 added the roadmap Issues that have priority and are included in the roadmap, or are candidates to add to the roadmap label Jun 15, 2023
@jbw976 jbw976 modified the milestones: v1.14, v1.15 Jun 28, 2023
@blakebarnett
Copy link

As we start to move more and more resources into production with Crossplane this issue is quickly becoming a very big problem.

We are often faced with a situation where the only good way to make a change to a composition is to delete the claims, otherwise any fields or data that we’ve removed never goes away. Orphaning the resources is a nerve-wracking and error-prone thing to do in production. I’m hoping this issue can get prioritized higher.

@jeanduplessis
Copy link
Contributor

@blakebarnett server-side apply is only a target for XP 1.15 at the moment.
1.14 is a stacked release with lots of features already. One way to get this moved forward is if someone in the community (hint hint) is willing to put some effort into driving this forward.

@negz
Copy link
Member Author

negz commented Jul 10, 2023

A few thoughts on tackling this:

  • Can we keep the Applicator interface, but introduce SSA based one? We "apply" in a lot of places. It would be nice to be able to switch out applicators without too much refactoring.
  • Will SSA let us deprecate the fields mentioned in Patch mergeOptions are written from the perspective of Go code, not YAML #2581?
  • Presumably part of this work will involve adding the right annotations to all of our CRDs (i.e. all managed resources). How will we generate that? I kind of like the approach meta controller takes here. I don't think they use SSA, but they have some sane defaults - e.g. assuming that a name field uniquely identifies an object in in an array.

@negz
Copy link
Member Author

negz commented Jul 11, 2023

Another thought: I'm guilty of doing something I ask other folks not to do here - leading with the solution and not the problem.

So, to be clear, we shouldn't use SSA just for the sake of using SSA. The broad-strokes problems we want to address are:

  • Supporting merging (not replacing) arrays of named objects when the XR reconciler overlays it desired state on top of an existing composed resource (e.g. strategic-merge-patch semantics).
  • Supporting deleting fields from composed resources when they're deleted from the corresponding XR templates.
  • Keeping claims in-sync with XRs. These two objects doesn't have the exact same schema (some of their 'Crossplane machinery' fields differ) but all of their user-specified fields are identical. We want to sync claim spec -> XR spec, and XR status -> claim status. This includes deleting fields in the destination when a field is deleted from the source.

The reason I pointed to SSA as a potential solution here is because on the surface "the API server figures it all out for you" seems nice. 😄 If client-side code is better that's totally fine.

@turkenh
Copy link
Member

turkenh commented Aug 23, 2023

PR adding Server Side Apply to provider kubernetes as an alpha feature: crossplane-contrib/provider-kubernetes#134

It uses a slightly different implementation for APIServerSideApplicator than what is proposed here which aims backward compatibility with the existing applicators.

It leverages the NewUnstructuredExtractor to extract fields managed by provider-kubernetes in order to decide whether an update is needed or not.

@negz
Copy link
Member Author

negz commented Sep 13, 2023

I ended up using server-side apply to implement the new FunctionComposer in #4500. Specifically what this means is that if your XR uses a Composition in mode: Pipeline, Crossplane will use SSA to:

  • Create/update composed resources.
  • Set spec.resourceRefs on the XR.
  • Update status on the XR (if the Function pipeline returns status to set)

After playing around with this, I opted to not use the Applicator interface and just use client.Patch(..., patch.Apply). It didn't feel like the Applicator interface was adding much value. Notably the most common ApplyOptions we use like ResourceMustBeControllableBy are natively handled by SSA.

@negz
Copy link
Member Author

negz commented Sep 13, 2023

It seems like it would be pretty feasible to update anything that uses *unstructured.Unstructured to use SSA. So that would include mostly the claim controller and the rest of the XR controller (e.g. the reconciler and PTComposer).

Per kubernetes-sigs/controller-runtime#347 (comment) it seems like SSA is currently not possible with 'real' structured types (e.g. *ProviderRevision) when using the controller-runtime client, because they'll generate an incorrect SSA patch.

Copy link

github-actions bot commented Feb 1, 2024

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.

@github-actions github-actions bot added the stale label Feb 1, 2024
@jbw976
Copy link
Member

jbw976 commented Feb 1, 2024

/fresh this is ongoing 😁

@github-actions github-actions bot removed the stale label Feb 1, 2024
@jbw976 jbw976 modified the milestones: v1.16, v1.17 Feb 22, 2024
Copy link

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.

@github-actions github-actions bot added the stale label Jun 25, 2024
@jbw976
Copy link
Member

jbw976 commented Jun 25, 2024

This docs entry provides the current status of server side apply support within the crossplane ecosystem:

https://docs.crossplane.io/latest/concepts/server-side-apply/#use-server-side-apply-to-sync-claims-end-to-end

TL;DR: full server side apply from claims -> composite resource (XR) -> composed resources is possible right now with:

  • --enable-ssa-claims alpha feature flag
  • composition functions (i.e. composition mode pipeline)

@github-actions github-actions bot removed the stale label Jun 26, 2024
@jbw976 jbw976 modified the milestones: v1.17, v1.15 Jun 27, 2024
@jbw976
Copy link
Member

jbw976 commented Jun 27, 2024

Recalling that we have #5656 to further track the maturity of the SSA feature out of alpha and to Beta, I'm going to go ahead and close this issue as the initial feature is done (as outlined in the previous comment 😉).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
composition enhancement New feature or request roadmap Issues that have priority and are included in the roadmap, or are candidates to add to the roadmap
Projects
Status: Done
Development

No branches or pull requests

7 participants