controller-runtime
controller-runtime copied to clipboard
kubernetes-sigs
Server-side Apply!
Server-side apply will be landing in 1.14 (code is merged :tada: 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.
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).
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.
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?
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.
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.
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
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
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
That's not terrible. We could have a Apply method to get rid of the parameter patch-type parameter.
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.
We can probably also default to forceownership.
I think it's a great default, but needs to be overridable.
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...)
}
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 :-/
/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.
@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.
I met an error when I use Server-side Apply
https://github.com/kubernetes-sigs/controller-runtime/issues/1311
@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?)
@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
That's not terrible. We could have a
Applymethod 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
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.
