/test

ping @aditighag @tommyp1ckles

/test

ping @aditighag @tommyp1ckles

/test

(2) Can you expand on this? Maybe give some examples of non-relevant updates? Why would we not be interested in every backend that either has been deleted or has become non-viable due to health checking? The only thing I can think of that's "not relevant" is when a backend that has become non-viable (e.g. only has quarantined backends) and then there's a change that still keeps it non-relevant, e.g. one of the quarantined backends is removed. But is there harm in that situation to go and try find sockets to terminate?

There are multiple backend attributes, state being one of them - https://github.com/cilium/cilium/blob/main/pkg/loadbalancer/backend.go#L25-L25. Do we want to run the socket termination loop for backend updates corresponding to changes to attributes such as weight, clusterID, UnhealthyUpdatedAt, etc that don't change its state?

(2) Can you expand on this? Maybe give some examples of non-relevant updates? Why would we not be interested in every backend that either has been deleted or has become non-viable due to health checking? The only thing I can think of that's "not relevant" is when a backend that has become non-viable (e.g. only has quarantined backends) and then there's a change that still keeps it non-relevant, e.g. one of the quarantined backends is removed. But is there harm in that situation to go and try find sockets to terminate?

There are multiple backend attributes, state being one of them - https://github.com/cilium/cilium/blob/main/pkg/loadbalancer/backend.go#L25-L25. Do we want to run the socket termination loop for backend updates corresponding to changes to attributes such as weight, clusterID, UnhealthyUpdatedAt, etc that don't change its state?

It waits for changes to take a look at for 50ms and then does a tree traversal to go through them to find relevant changes. This is fast and lockless, so that should be fine. If we want to reduce the overhead of context switching and tree traversal we can bump the waiting up by a bit to make this even cheaper.

Alternatively we could tie this into the reconciler and when it deletes a backend it could also queue up the socket destruction.

You got me now questioning whether this might be burning enough cycles looking through irrelevant changes that it’s worth saving those. I’ll do a benchmark today to quantify.

(2) Can you expand on this? Maybe give some examples of non-relevant updates? Why would we not be interested in every backend that either has been deleted or has become non-viable due to health checking? The only thing I can think of that's "not relevant" is when a backend that has become non-viable (e.g. only has quarantined backends) and then there's a change that still keeps it non-relevant, e.g. one of the quarantined backends is removed. But is there harm in that situation to go and try find sockets to terminate?

There are multiple backend attributes, state being one of them - https://github.com/cilium/cilium/blob/main/pkg/loadbalancer/backend.go#L25-L25. Do we want to run the socket termination loop for backend updates corresponding to changes to attributes such as weight, clusterID, UnhealthyUpdatedAt, etc that don't change its state?

It waits for changes to take a look at for 50ms and then does a tree traversal to go through them to find relevant changes. This is fast and lockless, so that should be fine. If we want to reduce the overhead of context switching and tree traversal we can bump the waiting up by a bit to make this even cheaper.

Alternatively we could tie this into the reconciler and when it deletes a backend it could also queue up the socket destruction.

You got me now questioning whether this might be burning enough cycles looking through irrelevant changes that it’s worth saving those. I’ll do a benchmark today to quantify.

Ok, I've added a benchmark for the iteration. We're spending 300ns per backend change, which let's say for 100 backend changes would cost 600us, which is about 0.06% of one CPU core (see the comment for more details). So if we'd have a very very busy cluster it's still very little spent on this and it's unlikely that we'd have this sort of constant churn.

My argument here is that it's better to sacrifice few CPU cycles to have an implementation that is decoupled: the other code has no control coupling to the UDP socket termination and the more we implement in this style the easier the code is to work with => we strive to have each component in the system to have a single concern, referential transparency and less control coupling. This in fact is the core design principle around StateDB: we move away from tight coupling of components and imperative callbacks that not end up with a big ball of mud (e.g. see pkg/service, pkg/endpoint etc.) or reduce latency/throughput in unexpected ways, but also to avoid production incidents like dead locks when one component directly calls into the black box of another (control coupling). That is why I find the trade-off of doing little bit extra work here much better than the alternatives (e.g. BPF reconciler directly coupled to this thing when it deletes a backend).

EDIT: Ah, it's the backend instances lookup that IsAlive() does that takes most of the time. Adding a check for the protocol and skipping non-UDP backends brings the time down to 24ns/backend. So this can now process 41 million backend changes per second if the changes are all non-UDP. That seems fine?

/test

@aditighag PTAL

/test

There are multiple backend attributes, state being one of them - https://github.com/cilium/cilium/blob/main/pkg/loadbalancer/backend.go#L25-L25. Do we want to run the socket termination loop for backend updates corresponding to changes to attributes such as weight, clusterID, UnhealthyUpdatedAt, etc that don't change its state?

Missed following up -- mainly to highlight the potential overhead of iterating kernel sockets unnecessarily. We can revisit this if users start reporting noticeable overhead.
The benchmarks you added should also help us keep track of the processing overhead in cilium.