Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

agent,resmgr: merge PodResources{List,Map}, cache last List() result. #423

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

klihub
Copy link
Collaborator

@klihub klihub commented Dec 5, 2024

Merge PodResourceMap into PodResourcesList. Avoid unnecessary re-listing of pod resources by caching the result of the last List() and trying to reuse it to look up resources.

Merge PodResourceMap into PodResourcesList. Avoid unnecessary
re-listing of pod resources by caching the result of the last
list and trying to reuse it to look up resources.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the devel/podresapi/cache-list-result branch from 9f7ccf7 to 45e24cd Compare December 13, 2024 06:49
Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see need and natural point for cache invalidation? Now we are caching pod resources based on namespace + pod name, if I'm reading this correctly. Should we clear the cache on removing pod sandbox, or perhaps in some other events that could make us suspicious that pod has changed....

@klihub
Copy link
Collaborator Author

klihub commented Dec 13, 2024

Do you see need and natural point for cache invalidation? Now we are caching pod resources based on namespace + pod name, if I'm reading this correctly. Should we clear the cache on removing pod sandbox, or perhaps in some other events that could make us suspicious that pod has changed....

In the current version we tried to implement (and use) this so that both caching and invalidation would be transparent/implicit, with the trigger for invalidation being if we fail to find a pod in the cached last list result. To be honest, I am not sure if this is enough, we'll need to dig in more to understand better.

If we use the pod resource API at all, we use it to generate extra topology hints for device/memory+CPU alignment. My bigger fear than inaccuracy was the extra cost, especially in increased latency for decision making. That is why if enabled, we now trigger an asynchronous pod resource API lookup during pod creation and use the result during container creation, if it is available by that time.

It might be that we'd need also explicit invalidation, which we could then trigger, for instance, whenever a container is stopped. But we'll need to dig in more to understand it better.

@askervin
Copy link
Collaborator

It might be that we'd need also explicit invalidation, which we could then trigger, for instance, whenever a container is stopped. But we'll need to dig in more to understand it better.

As this feature is behind an off-by-default option, I suppose we could start with it, experiment, and consider explicit invalidation in a separate PR.

Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@askervin askervin merged commit 3fb1ab2 into containers:main Dec 17, 2024
3 checks passed
@klihub klihub deleted the devel/podresapi/cache-list-result branch December 17, 2024 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants