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

YUNIKORN-2196 Optimize aggregated resources tracking feature to not add overhead to scheduling #739

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

zhuqi-lucas
Copy link
Contributor

@zhuqi-lucas zhuqi-lucas commented Nov 28, 2023

What is this PR for?

Currently, all tracking calculations are done in the scheduling cycle, it will add overhead to scheduling, we need to rethink and optimize this!

We may use the events that are generated to allow calculating this outside of the scheduler.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Task

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@zhuqi-lucas zhuqi-lucas marked this pull request as draft November 28, 2023 14:06
@zhuqi-lucas
Copy link
Contributor Author

zhuqi-lucas commented Nov 28, 2023

Hi @wilfred-s @pbacsko This is a draft PR for us to move resource tracking from scheduling cycle, i want to know your opinions about if it's the right way for us to do this in event publish cycle?

And more questions:

  1. The event storage seems will lose some events when it's reach the limit size, if we need to change it to accurate storage which will not lose any events since we want the resource tracking progress more accurate?
  2. If we need to add more fields to the EventRecord, because now i add some app resources tracking fields into EventRecord message field, but it seems not performance good?

@pbacsko
Copy link
Contributor

pbacsko commented Nov 28, 2023

@zhuqi-lucas I think we should hold off with this for a while. We need to benchmark and see how expensive resource tracking is. In this case, I'm not sure that benchmarking is actually needed.

As I can see, aggregation is called from three places:
application.go L#1780
application.go L#1787
application.go L#1839

When does this happen? Allocation removal or placeholder replacement. It's not a very frequent operation. TrackedResource.AggregateTrackedResource() is not a particularly expensive method, there's a loop for the resources and some basic math.

The overhead of tracking is very low, I think almost unmeasurable. My performance unit test does not trigger it because it does not send pod completed events, but I'd be surpised if the call stack for this were visible.

I'd skip this one completely.

@@ -68,6 +77,43 @@ func (sp *EventPublisher) Stop() {
sp.stop.Store(true)
}

func (sp *EventPublisher) AggregateAppTrackedResourceFromEvents(messages []*si.EventRecord) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very complicated for something which isn't even a problem in my opinion.

I'm a strong -1 for optimizing tracking this way.

Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

I'm -1 on this.

We've talked about this recently. After checking&understanding the actual costs of the tracking, I would not touch the existing code at all. The new, event based aggregation is VERY complicated.

@zhuqi-lucas
Copy link
Contributor Author

zhuqi-lucas commented Nov 29, 2023

Hi @pbacsko , thank you for review, i agree the application tracking now does not have much impact on the scheduling performance.

  • And for further resource aggregation such as user/group tracking, if we can have a separate service to do all the tracking async calculation? I am not sure if it's a good option.

  • And besides the external access from REST API for event, if we want to do some internal aggregation and or filter for the events?

@wilfred-s
Copy link
Contributor

I think we can simply use the existing events and push the whole aggregation out of YuniKorn. The only thing that seems to be missing is the node instance type. If that is available just listening to the app and node events would allow creating the summary outside of the scheduler.
When we have the user and group links to the applications as events we can do the same for those without any impact on the scheduling cycle.

So instead of adding that inside the event system we should push this out. Keep the scheduler for scheduling. Make all this an add on...

That is where my remarks came from when I looked at the aggregation for users and groups.

@wilfred-s
Copy link
Contributor

And for further resource aggregation such as user/group tracking, if we can have a separate service to do all the tracking async calculation? I am not sure if it's a good option.

That is the best option. A scheduler should schedule. It is not for providing statistics.

And besides the external access from REST API for event, if we want to do some internal aggregation and or filter for the events?

No, because whatever we come up with will not fit all use cases. The only thing we can think of, which has been discussed before, is splitting the streams into just one type i.e. only application events. Not even sure that it will help much.
If filtering or something needs to happen

@zhuqi-lucas
Copy link
Contributor Author

zhuqi-lucas commented Nov 29, 2023

Thank you @wilfred-s , so i am thinking the passing flow for event, if we can extend the event to use for trackingService, we can just do: cc @pbacsko

  1. When we add event, we pass to eventSystem and have storage for pushing events to shim.
  2. When we add event, we notify to trackingService to aggregate.
  3. So the same event, can pass to both eventSystem and trackingService

So we don't need a storage for trackingService, we don't need to push events to shim, but if we want to have a storage for REST API can query aggregated result? Maybe require a design for this also, and we can expose some history aggregation result in UI, and we can provide the duration for history data?

@zhuqi-lucas
Copy link
Contributor Author

Anyway, i created a jira to extend the EventRecord first, so we can depend on that:
https://issues.apache.org/jira/browse/YUNIKORN-2208

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.

3 participants