feat :Add GetActivePods to handle/datastore and remove deleted pod from prefix-cache scorer #1376
Conversation
Signed-off-by: Kfir Toledo <[email protected]>
Signed-off-by: Kfir Toledo <[email protected]>
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kfirtoledo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @kfirtoledo. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
|
/assign @nirrozenbaum |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
@elevran is this aligned with the new datalayer APIs? |
I don't see a conflict - the datastore API is minimally affected. |
| @@ -27,6 +29,9 @@ type Handle interface { | |||
| Context() context.Context | |||
|
|
|||
| HandlePlugins | |||
|
|
|||
| // GetActivePods returns a list of all active pods | |||
| GetActivePods() []types.NamespacedName | |||
There was a problem hiding this comment.
nit: In go, it's more idiomatic to use ActivePods
There was a problem hiding this comment.
Consider returning a map[types.NamespacedName]bool instead of a list so the plugin doesn't need to convert a list to a map/set
| @@ -61,6 +61,7 @@ type Datastore interface { | |||
| PodList(predicate func(backendmetrics.PodMetrics) bool) []backendmetrics.PodMetrics | |||
| PodUpdateOrAddIfNotExist(pod *corev1.Pod) bool | |||
| PodDelete(namespacedName types.NamespacedName) | |||
| GetActivePods() []types.NamespacedName | |||
There was a problem hiding this comment.
No need to add this, we can use PodList.
| @@ -239,6 +248,45 @@ func (m *Plugin) matchLongestPrefix(ctx context.Context, hashes []BlockHash) map | |||
| return res | |||
| } | |||
|
|
|||
| // StartPodActiveWatcher starts a goroutine that watches for active pods. | |||
| func (m *Plugin) StartPodActiveWatcher(ctx context.Context, handle plugins.Handle) { | |||
There was a problem hiding this comment.
Can we call this something like CleanUpInactivePods to be more descriptive?
| ticker := time.NewTicker(PodActiveCheckInterval) | ||
| defer ticker.Stop() | ||
|
|
||
| podLastSeen := make(map[ServerID]time.Time) |
There was a problem hiding this comment.
I am thinking of a much simpler implementation: iterate over current pods in the podToLRU in the indexer, and if that pod doesn't exist in ActivePods, remove. And this approach does not need a PodInactivityTimeout.
|
PR needs rebase. 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-sigs/prow repository. |
k8s-ci-robot
added
the
needs-rebase
This PR introduces two main enhancements:
These changes allow plugins to query active pods directly from the handle and support cache cleanup for inactive pods in the prefix-cache scorer (Fix #1289)