This can help a lot in the performance of the endpoint controller which is currently using PUTs instead of PATCHs. Related issue: #47597

Maybe we can use the 'Addresses' list itself as the key but I'm not sure if that's going to be unique (though I'd hope so) and if it's fine to have a list as a key.
Moreover, seems like some previous discussions favoured using maps with keys for objects than having lists (ref: #4889) in such cases.
@smarterclayton WDYT?

without contention, patch is actually more expensive both client-side (because patch computation is required) and server-side (because patch application and conflict-detection is required).

do we have contention on writing endpoints? I expected the endpoints controller to largely be the only writer

I thought the main issue was the extremely short resync interval, not write conflicts

I suggested PATCH to do blind overwrite, which avoids having to round trip between client and server, which reduces latency, which means the endpoints controller clears work faster, and moves more CPU to the master, where there is less latency to etcd.

I agree with @smarterclayton - we want to avoid unnecessary GETs. The issue here is that we'd need to do some API changes to accomplish that. The question here is - are we doing things like this?

cc @lavalamp @wojtek-t

The issue here is that we'd need to do some API changes to accomplish that.

does a json merge patch that overwrites the entire subsets list not work as expected?

@liggitt We are currently PUT'ing the endpoints object which IIUC is as good as merging the entire subsets list (as that's pretty much the whole endpoints object). This is needing us to send huge endpoints objects over the wire each time. Also (as @smarterclayton pointed out), this could be bad if just a single endpoint is flapping states.

Seems like it's not possible to merge patch individual elements of arrays as per RFC 7386 (the one that our jsonpatch vendored package follows). Quoting from the document:
Also, it is not possible to patch part of a target that is not an object, such as to replace just some if the values in an array.

However IIUC we've solved this problem in k8s by using some field of the array elements' struct as the patchMergeKey, so individual array elements can be keyed on that field.
This means adding a 'key' field to the EndpointSubset struct should work. Does this make sense @smarterclayton / @liggitt ?

This means adding a 'key' field to the EndpointSubset struct should work.

I think that would be a breaking API change

That's true. However if we want to be able to merge patch, we need the elements of the array to be somehow keyed.

Another option I could think of was to use the index of the array element as it's key. This won't need any API change, but it's not a very clean way. Because a patch operation like "update element at index 5" might end up reaching after some other element got inserted there. What's worse is it won't be detected as a conflict and you'll end up wrongly updating unless there's some other safety mechanism.
Could there be some better option or we just live without patches? :)

Closing this issue as it is infeasible without breaking API change. Let's reopen it in future if need be.

Isn't that pretty much equivalent to normal update? There's not much data except those addresses.

@smarterclayton Yes, the reason for starting the issue was to do smarter patching than patching the whole list (which is basically the entire ep object). Unless there are some other benefits of patching I might be missing?

OK, I clearly don't understand what you mean by "send all of subsets as one patch". Isn't it pretty much equivalent to plain update (assuming that metadata didn't change), except that instead of using protobuf we'd use JSON?