Server-side Apply!

[DirectXMan12]

Server-side apply will be landing in 1.14 (code is merged 🎉 and all), so we should come up with a plan to support server-side apply.

Talking with @apelisse and @jennybuckley, it looks like the main constraints on server-side apply from a controller perspective are that you must set all the fields you care about when submitting an object, which is what we encourage anyway (declare the state of the world that you want to see, instead of just making changes).

This should ultimately (in many cases) allow people to write code like:

if err := r.Client.Ensure(ctx, &grp.MyKind{
   fieldA: "foo",
   fieldB: "bar",
}); err != nil {
    return ctrl.Result{}, err
}

instead of using CreateOrUpdate.

It would be neat to call server-side apply from CreateOrUpdate, but it's not clear to me that we can safely do so without violating an implicit contract -- if people have special logic that they do when fields aren't set (not a good pattern, but people almost certainly do it), it could break them. That being said, it's not clear to me that we explicitly write that as a contract anywhere.

[DirectXMan12]

/kind feature
/priority important-longterm

[DirectXMan12]

We also need to come up with a reasonable fallback approach, and detection. We should be able to do a dry-run patch to check if server-side apply is supported (there doesn't appear to be a more declarative way ATM).

[apelisse]

We also need to come up with a reasonable fallback approach, and detection.

OpenAPI has a "route" section describing the end-points, you can check if PATCH for the specific type supports application/apply-patch+yaml.

[DirectXMan12]

cool, that's way better.

[JoelSpeed]

Will this need a client-side implementation to fall back to? A three way merge in a similar way to Kubectl does in K8s 1.13 and below?

Is there any documentation on how Kubectl will fall back if the server-side apply isn't present? Should this be the behaviour that we try to mimic?

[apelisse]

kubectl right now is going to fall-back to client-side apply if the feature isn't enabled on the server, but I'm not sure that's a behavior we'll want to keep. I think it is different enough that they can't be blindly replaced.

[DirectXMan12]

I think we may just have to explicitly fail if server-side apply is not available -- the chance for really subtle bugs may be too high.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[DirectXMan12]

/remove-lifecycle rotten
/lifecycle frozen

[texascloud]

Any movement on this?

[apelisse]

We're missing that PR and its extension that @mariantalla is working on.

[DirectXMan12]

FWIW, basic support is in right now -- it looks like

cl.Patch(ctx, obj, client.Apply, client.ForceOwnership, client.FieldOwner("my-controller"))

but that's a bit less elegant than it could be

[apelisse]

That's not terrible. We could have a Apply method to get rid of the parameter patch-type parameter.

[DirectXMan12]

I'm thinking the eventual interface should not need the patch type, and should just keep the field manager in some internal state so that you don't have to type it every time. We can probably also default to forceownership.

[apelisse]

We can probably also default to forceownership.

I think it's a great default, but needs to be overridable.

[DirectXMan12]

basically:

type Applier struct {
  patcher client.Patcher

  name string
}

func (a *Applier) Ensure(ctx context.Context, obj runtime.Object, opts ...client.PatchOption) error {
  // put the default options first so that they can be overridden
  opts = append([]client.PatchOption{client.ForceOwnership, client.FieldOwner(a.name)}, opts...)
  return a.patcher.Patch(ctx, obj, client.Apply, opts...)
}

[apelisse]

Also, the "owner" is required, so I would surface that somehow.

[DirectXMan12]

(that's why I have the name field)

[apelisse]

Discussed offline, we're on the same page :-)

[apelisse]

if err := r.Client.Ensure(ctx, &grp.MyKind{
   fieldA: "foo",
   fieldB: "bar",
}); err != nil {
    return ctrl.Result{}, err
}

The big problem we have with that is that Server-Side Apply cares about the difference between fields set at zero-value and fields that are not set. I'm assuming this won't work :-/

[vincepri]

/help

Initially we want to focus on a non-breaking change for v0.5.x, in the future as we learn from use cases we can put in Client.

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Initially we want to focus on a non-breaking change for v0.5.x, in the future as we learn from use cases we can put in Client.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[kwiesmueller]

/wg api-expression

[allenhaozi]

I met an error when I use Server-side Apply

#1311

[coderanger]

@DirectXMan12 How would you feel about closing this? I think it's been long enough that SSA is pretty broadly available so we could maybe just skip the client-side abstraction entirely. We can maybe close this and open more specific tickets for any lingering issues (controller-gen support for the new typed builders? better docs?)

[m1o1]

@coderanger do you mean that since SSA is broadly available, we should use cl.Patch(ctx, obj, client.Apply, client.ForceOwnership, client.FieldOwner("my-controller")) as shown above? Or something else? About to switch some stuff to use SSA instead of CreateOrUpdate, just want to make sure I'm headed in the right direction.

Edit* link to slack for anyone interested: here

[coderanger]

@m1o1 This isn't a good place for user support, please ask in Slack.

[feloy]

That's not terrible. We could have a Apply method to get rid of the parameter patch-type parameter.

I would like to propose the implementation(1) the Apply method that would have the same effect as the following call:

cl.Patch(ctx, obj, client.Apply, client.ForceOwnership, client.FieldOwner("my-controller"))

The signaure of the Apply method would be:

Apply(ctx context.Context, obj Object, opts ...ApplyOption) error

The ApplyOption implementations would be DryRunAll, FieldOwner and ForceOwnership,as for the PatchOption.

An Apply method would also be added to the StatusWriter interface, to patch Status of resources.

@apelisse what's your opinion?

(1) I would be happy to work on this implementation

[EraYaN]

This could also borrow from client-go's Apply method I suppose. It actually directly accepts the *ApplyConfigurations types. Which saves a bunch of wrangling those objects, which would be very helpful.

[yhrn]

This could also borrow from client-go's Apply method I suppose. It actually directly accepts the *ApplyConfigurations types. Which saves a bunch of wrangling those objects, which would be very helpful.

Yes! Based on the discussion here I believe that it is important that an Apply method should not accept API Object but rather ApplyConfigurations. The serialization of API Objects will often include "empty values" for some properties which will very likely result in managing fields that was not intended. The most obvious example is metadata.creationTimestamp which you should not be setting yourself but will be serialized as null in that case. This in turn means that you are signaling the intent of managing that field, and even though the API server won't clear the field it will always increment resourceVersion because of this, even if nothing else changed, breaking SSA idempotency.

My suggestion would be:

1. Include generation of ApplyConfigurations in kubebuilder projects - not sure if it should be built into controller-gen or if applyconfiguration-gen should be used directly. We probably also need the ApplyConfigurations for the top level API Objects to implement some interface for exposing the TypeMeta/ObjectMeta information needed by the Client implementation (lets call this interface ObjectApplyConfigurations for now).
2. Implement an Apply method with the following signature Apply(ctx context.Context, applyConfig ObjectApplyConfiguration, result Object, opts ...ApplyOption) error
3. Deprecate Patch with the Apply type since it likely does not yield the desired results.

[apelisse]

cc @Jefftree has started some work on this, but if someone wants to take over, happy to help.

[alvaroaleman]

For the first point, there is an open PR to add applyconfiguration generation: kubernetes-sigs/controller-tools#536

For the second point, last time I checked there was no common interface that is implemented by all applyconfigurations and that would allow us to type things and make mistakes harder - Has that changed in the meantime?

[Jefftree]

Summarizing the state of applyconfigurations in controller-tools:

There is an open PR to add applyconfiguration generation to controller-tools: kubernetes-sigs/controller-tools#536 I don't think we want to move forward with this PR because it duplicates a lot of code found in the kubernetes client-go applyconfigurations and could lead to drift and extra burden on maintenance, please refer to this comment

@JoelSpeed has put together an integration with using the upstream client-go generator for kubebuilder types, and I would heavily recommend that approach over merging the existing PR. I think there are still some things that need to be worked out like loading the openapi file, but the structure should be there.

This solves step 1 in the proposal by @yhrn. Steps 2 and 3 should be much simpler once 1 is completed. I had a previous controller-runtime branch that showed a POC of how the new Apply interface would be implemented. #1365

cl.Patch(context.TODO(), fooObj, client.ApplyFrom(applyConfig), owner);

I currently don't have the bandwidth to work on this, but would love for someone to take over and am available for guidance/collaboration.
/cc @cannonpalms since you showed some interest in reviving the PRs.

[alvaroaleman]

I opened kubernetes/kubernetes#118138 which we would need to have in order to be able to provide proper first-class support for SSA in controller-runtime.