You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "yzhangal (via GitHub)" <gi...@apache.org> on 2023/02/17 22:15:10 UTC
[GitHub] [yunikorn-core] yzhangal opened a new pull request, #507: [YUNIKORN-1385] To provide visibility to application's aggregated res…
yzhangal opened a new pull request, #507:
URL: https://github.com/apache/yunikorn-core/pull/507
…ource consumption
### What is this PR for?
A few sentences describing the overall goals of the pull request's commits.
First time? Check out the contributing guide - http://yunikorn.apache.org/community/how_to_contribute
### What type of PR is it?
* [ ] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/YUNIKORN/
* Put link here, and add [YUNIKORN-*Jira number*] in PR title, eg. `[YUNIKORN-2] Gang scheduling interface parameters`
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1116169961
##########
pkg/metrics/init.go:
##########
@@ -229,3 +236,7 @@ func formatMetricName(metricName string) string {
}
return string(newBytes)
}
+
+func (m *Metrics) LogAppSummary(appSummary string) {
+ log.Println(AppSummaryHeader + appSummary)
+}
Review Comment:
This method is called from `PartitionContext`. It has a very specific purpose, it only prints something else to the console which is completely unrelated to metrics. Should be moved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190337570
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
+ defer urt.Unlock()
+ urm, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
This also needs to be able to be turned off via zap logger levels. Right now, every user will pay the cost of marshalling this complex object every time, even if logging of these events is disabled. That's part of what the zap logger framework gets you. It also standardizes representation of objects in the log so that the log formats are consistent throughout the application. I'll say again: This MUST not be a string returned here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190539127
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
+ defer urt.Unlock()
+ urm, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
HI @craigcondit , wonder if this would work:
I renamed UsedResourceTracker to UsedResource here, and added Clone() method in UsedResource,
```
func (ur *UsedResource) Clone() *UsedResource {
if ur == nil:
return nil
ret := NewUsedResource()
ur.RLock()
defer ur.RUnlock()
for k, v := range ur.UsedResourceMap {
sourceEntry := map[string]int64(v)
destEntry := make(map[string]int64)
for key, element := range sourceEntry {
destEntry[key] = element
}
ret.UsedResourceMap[k] = destEntry
}
return ret
}
func (sa *Application) GetApplicationSummary(rmID string) *ApplicationSummary {
sa.RLock()
defer sa.RUnlock()
appSummary := &ApplicationSummary{
ApplicationID: sa.ApplicationID,
SubmissionTime: sa.SubmissionTime,
StartTime: sa.startTime,
FinishTime: sa.finishedTime,
User: sa.user.User,
Queue: sa.queuePath,
State: sa.stateMachine.Current(),
RmID: rmID,
ResourceUsage: sa.usedResource.Clone(),
}
return appSummary
}
func (as *ApplicationSummary) GetReport() string {
res := "{"
res += "\"appId\":\"" + as.ApplicationID + "\","
res += "\"submissionTime\":\"" + strconv.FormatInt(as.SubmissionTime.UnixMilli(), 10) + "\","
res += "\"startTime\":\"" + strconv.FormatInt(as.StartTime.UnixMilli(), 10) + "\","
res += "\"finishTime\":\"" + strconv.FormatInt(as.FinishTime.UnixMilli(), 10) + "\","
res += "\"user\":\"" + as.User + "\","
res += "\"queue\":\"" + as.Queue + "\","
res += "\"state\":\"" + as.State + "\","
res += "\"rmID\":\"" + as.RmID + "\","
res += "\"resourceUsage\":" + as.ResourceUsage.GetResourceUsageJson()
res += "}"
return res
}
```
Above I introduced a GetResourceUsageJson() because I was doing different processing with the different fields you can see in the above method, which abstract what information we need to report in what format.
```
func (sa *Application) GetAppSummary(rmID string) string {
appSummary := sa.GetApplicationSummary(rmID)
ret := appSummary.GetReport()
appSummary.ResourceUsage = nil
return ret
```
And the final report is created with:
```
log.logger().Info(
zap.String("YK_APP_SUMMARY", app.GetAppSummary(pc.RmID)))
```
inside func (pc *PartitionContext) moveTerminatedApp(appID string) {
If user chooses to use log level higher than info, then, s/he may not get the report.
To be honest, we actually hope we can always report this without being impacted by the log level.
Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202915441
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
We do not need the instance type for all nodes, only those that might be candidates. The code in initWorkingState() that can be updated:
```
allocationsByNode := make(map[string][]*Allocation)
queueByAlloc := make(map[string]*QueuePreemptionSnapshot)
nodeAvailableMap := make(map[string]*resources.Resource)
nodeInstanceTypeMap := make(map[string]string) // <-- TODO add this
// build a map from NodeID to allocation and from allocationID to queue capacities
for _, victims := range p.allocationsByQueue {
for _, allocation := range victims.PotentialVictims {
nodeID := allocation.GetNodeID()
allocations, ok := allocationsByNode[nodeID]
if !ok {
allocations = make([]*Allocation, 0)
}
allocationsByNode[nodeID] = append(allocations, allocation)
queueByAlloc[allocation.GetAllocationKey()] = victims
nodeInstanceTypeMap[nodeID] = node.GetInstanceType() // <-- TODO add this
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1116153680
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,55 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ var aggregatedResourceTime map[string]int64
+ var timeDiff int64
+ var releaseTime time.Time
+ var resourceSecondsKey string
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime = time.Now()
+ timeDiff = int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey = key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff //resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ jsonStr, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
`jsonStr` suggests that it's a string, but it's a byte array.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190324164
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
Review Comment:
Good point, will address in next rev.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190339189
##########
pkg/scheduler/objects/application.go:
##########
@@ -107,6 +112,50 @@ type Application struct {
sync.RWMutex
}
+type ApplicationSummary struct {
+ ApplicationID string
+ SubmissionTime time.Time
+ StartTime time.Time
+ FinishTime time.Time
+ User string
+ Queue string
+ State string
+ RmID string
+ ResourceUsage string
+}
+
+func (as *ApplicationSummary) GetReport() string {
+ res := "{"
+ res += "\"appId\":\"" + as.ApplicationID + "\","
+ res += "\"submissionTime\":\"" + strconv.FormatInt(as.SubmissionTime.UnixMilli(), 10) + "\","
+ res += "\"startTime\":\"" + strconv.FormatInt(as.StartTime.UnixMilli(), 10) + "\","
+ res += "\"finishTime\":\"" + strconv.FormatInt(as.FinishTime.UnixMilli(), 10) + "\","
+ res += "\"user\":\"" + as.User + "\","
+ res += "\"queue\":\"" + as.Queue + "\","
+ res += "\"state\":\"" + as.State + "\","
+ res += "\"rmID\":\"" + as.RmID + "\","
+ res += "\"resourceUsage\":" + as.ResourceUsage
+ res += "}"
+ return res
+}
+
+func (sa *Application) GetApplicationSummary(rmID string) *ApplicationSummary {
+ sa.RLock()
+ defer sa.RUnlock()
+ appSummary := &ApplicationSummary{
+ ApplicationID: sa.ApplicationID,
+ SubmissionTime: sa.SubmissionTime,
+ StartTime: sa.startTime,
+ FinishTime: sa.finishedTime,
+ User: sa.user.User,
+ Queue: sa.queuePath,
+ State: sa.stateMachine.Current(),
+ RmID: rmID,
+ ResourceUsage: sa.usedResourceTracker.GetResourceUsageSummary(),
Review Comment:
As this will be output to the same destination as other logs, it MUST be formatted consistently. This is not negotiable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190331788
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
Review Comment:
Thanks, that's very good point. In order to populate the pod binding time, we need to change the shim to pass the binding time information to core, right? Or how to get the exact binding time within core?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192685790
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
Hi @craigcondit Another question, I saw that all callers of the following method pass "Reserved" as the value for the first parameter, wonder if it's intended to pass a different value at all, given that it's meant to be a reserved allocation.
```
func newReservedAllocation(result AllocationResult, nodeID string, instType string, ask *AllocationAsk) *Allocation {
alloc := NewAllocation("", nodeID, instType, ask)
```
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192604759
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
Thanks @craigcondit
I have some questions about the startTime.
Currently I implemented the startTime in the app state machine as:
```
fmt.Sprintf("enter_%s", Starting.String()): func(_ context.Context, event *fsm.Event) {
app := event.Args[0].(*Application) //nolint:errcheck
app.startTime = time.Now()
```
I guess the Start state in this diagram (https://yunikorn.apache.org/docs/next/design/scheduler_object_states/) means the scheduler started working on allocation for the app. Wonder if it makes sense to have a state in the diagram to represent the first pod binding? If there is such a state, we can set the startTime in the state machine when entering this state.
Otherwise, I studied the code and saw that we can possibly have a field bindingTime in the Allocation structure to represent the binding time, and set it in the following method:
`func (sa *Application) tryNodes(ask *AllocationAsk, iterator NodeIterator) *Allocation {`
would you please confirm?
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202923988
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
Thanks, when I said "all nodes", I meant outside the preemption code, when we do an allocation, we need similar information. Doing a partial map for preemption code makes it a bit different than the handling outside the preemption code. If you think this is what we should do, I will give it a try.
Do you think it's worth to have a global nodeId to instanceType mapping? we discussed this before and chose to use the alternative solution that's currently in the PR.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1203004254
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
Added review comments - I think this can work, but we need to add some defensive checks in case the instance type is somehow not populated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1204611229
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -155,7 +155,10 @@ func (p *Preemptor) initWorkingState() {
}
allocationsByNode[nodeID] = append(allocations, allocation)
queueByAlloc[allocation.GetAllocationKey()] = victims
- nodeInstanceTypeMap[nodeID] = allocation.GetInstanceType()
+ instType := allocation.GetInstanceType()
+ if instType != UnknownInstanceType {
Review Comment:
Just to code defensively, I would skip the update if the allocation type is either unknown *or* empty. This should ensure we don't clobber the instance type. I know we're confident all the allocation instance types have been set, but there may be edge cases (recovery for instance).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1191259273
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
+ defer urt.Unlock()
+ urm, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
BTW, currently Hadoop YARN reports something similar in rm-appsummary.log file, as one json line per app, and I'm trying to do the same for the convenience of ingestion processing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192609360
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
The right way to do this would be to track the timestamp per allocation when the allocation is associated with a node, and when an allocation terminates, report that usage as ((allocationEndTime - allocationBoundTime) * resources.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated res…
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#issuecomment-1435443834
I have done local test with docker desktop and real cluster testing, the with ./deployments/examples/sleep/sleeppods.yaml,
It can report the following:
{“appId”:“application-sleep-0002-test13”,“submissionTime”:“1675494465465”,“startTime”:“1675494467467”,“finishTime”:“1675494529037”,“user”:“kubernetes-admin”,“queue”:“root.sandbox”,“state”:“Completed”,“rmID”:“mycluster”,“resourceUsage”:{“”:{“memorySeconds”:15500000000,“vcoreSeconds”:3100}}}
{“appId”:“application-sleep-0001-test13”,“submissionTime”:“1675494465465”,“startTime”:“1675494467466”,“finishTime”:“1675494529563”,“user”:“kubernetes-admin”,“queue”:“root.sandbox”,“state”:“Completed”,“rmID”:“mycluster”,“resourceUsage”:{“”:{“memorySeconds”:16000000000,“vcoreSeconds”:3200}}}
{“appId”:“application-sleep-0003-test13”,“submissionTime”:“1675494466465”,“startTime”:“1675494468467”,“finishTime”:“1675494529576”,“user”:“kubernetes-admin”,“queue”:“root.sandbox”,“state”:“Completed”,“rmID”:“mycluster”,“resourceUsage”:{“”:{“memorySeconds”:15500000000,“vcoreSeconds”:3100}}}
The two level map introduced by this diff will have one entry at the top level map for each instance type used by the application, then one entry at the second level for each resource type (CPU, memory etc) used.
Number of top level buckets: the number of instance types
Second level, for each instance type: the number of resource types, currently vcore and Memory
I did basic memory foot print testing with pprof, due to the small scale of apps that can run in parallel, the resource usage data added to Application struct is not noticeable. Based on the analysis done in https://levelup.gitconnected.com/memory-allocation-and-performance-in-golang-maps-b267b5ad9217,
"A map in Go is a hash table that stores its key/value pairs into buckets. Each bucket is an array that holds up to 8 entries. The default number of buckets is 1. Once the number of entries across each bucket reaches an average load of buckets (aka load factor), the map gets bigger by doubling the number of buckets. Every time a map grows, it allocates memory for the new-coming entries. In practice, every time the load of the buckets reaches 6.5 or more, the map grows. This value is hardcoded and was chosen to optimize memory usage."
Then I did a measurement with a standalone program, by creating 1000 such maps, with 5 instance types, and 2 resource types each instance type. The memory consumption is measured to be about 256 bytes per map (or per Yunikorn application with this PR). I used "top" command to observe the memory usage change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190338451
##########
pkg/metrics/init.go:
##########
@@ -166,6 +169,10 @@ type CoreEventMetrics interface {
Reset()
}
+func (m *Metrics) LogAppSummary(appSummary string) {
Review Comment:
As before, this needs to be consistent with other logging used throughout the system, and be configurable via log levels in zap. Doing it this way forces ALL users of logging to have to deal with a log message that is formatted completely differently from the rest of the application log messages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192637574
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
Thanks @craigcondit , that boils down to the last question in my previous comment:
Otherwise, I studied the code and saw that we can possibly have a field bindingTime in the Allocation structure to represent the binding time, and set it in the following method:
`func (sa *Application) tryNodes(ask *AllocationAsk, iterator NodeIterator) *Allocation {`
I think the allocationEndTime of your term is the time when the allocation is released, which I currently use and the pod finish time.
Would you please advice?
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#issuecomment-1561820228
Thank you so much @craigcondit @pbacsko @wilfred-s @yangwwei @zhuqi-lucas and other community folks! I learned a lot working with you on adding this feature. Look forward to more collaboration moving forward!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202828517
##########
pkg/scheduler/objects/allocation.go:
##########
@@ -99,16 +103,17 @@ func NewAllocation(uuid, nodeID string, ask *AllocationAsk) *Allocation {
}
}
-func newReservedAllocation(result AllocationResult, nodeID string, ask *AllocationAsk) *Allocation {
- alloc := NewAllocation("", nodeID, ask)
+func newReservedAllocation(result AllocationResult, nodeID string, instType string, ask *AllocationAsk) *Allocation {
Review Comment:
What I meant was have you done an exhaustive check for all code paths that use the allocation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1203038606
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -174,6 +177,20 @@ func (p *Preemptor) initWorkingState() {
p.allocationsByNode = allocationsByNode
p.queueByAlloc = queueByAlloc
p.nodeAvailableMap = nodeAvailableMap
+ p.nodeInstanceTypeMap = nodeInstanceTypeMap
+}
+
+// getInstanceType retrieves instance type from the nodeInstanceTypeMap for given nodeId
+// if not found, issue error log and return UnknownInstanceType
+func (p *Preemptor) getInstanceType(nodeId string) string {
Review Comment:
This doesn't really need its own method.
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -153,6 +155,7 @@ func (p *Preemptor) initWorkingState() {
}
allocationsByNode[nodeID] = append(allocations, allocation)
queueByAlloc[allocation.GetAllocationKey()] = victims
+ nodeInstanceTypeMap[nodeID] = allocation.GetInstanceType()
Review Comment:
We should only set the node value if the allocation was populated. If the instance type is empty, we might clobber a valid instance type from another allocation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1117814571
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,55 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ var aggregatedResourceTime map[string]int64
+ var timeDiff int64
+ var releaseTime time.Time
+ var resourceSecondsKey string
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime = time.Now()
+ timeDiff = int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey = key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff //resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ jsonStr, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
Good, will be addressed in next rev.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] zhuqi-lucas commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated res…
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1113821964
##########
pkg/metrics/init.go:
##########
@@ -229,3 +236,7 @@ func formatMetricName(metricName string) string {
}
return string(newBytes)
}
+
+func (m *Metrics) LogAppSummary(appSummary string) {
+ log.Println(AppSummaryHeader + appSummary)
+}
Review Comment:
Should add a newline at the end of file.
##########
pkg/scheduler/objects/application_test.go:
##########
@@ -1061,6 +1064,125 @@ func TestCompleted(t *testing.T) {
assert.Equal(t, log[3].ApplicationState, Expired.String())
}
+func assertResourceUsage(t *testing.T, appSummary string, memorySeconds int64, vcoresSecconds int64) {
+ var jsonMap map[string]interface{}
+ var resource interface{}
+ var detailedResource map[string]interface{}
+
+ json.Unmarshal([]byte(appSummary), &jsonMap)
+
+ resource = jsonMap["resourceUsage"]
+
+ resourceUsageMap := resource.(map[string]interface{})
+ //fmt.Print("resourceMap: ")
Review Comment:
Remove debug print.
##########
pkg/scheduler/objects/application_test.go:
##########
@@ -1061,6 +1064,125 @@ func TestCompleted(t *testing.T) {
assert.Equal(t, log[3].ApplicationState, Expired.String())
}
+func assertResourceUsage(t *testing.T, appSummary string, memorySeconds int64, vcoresSecconds int64) {
+ var jsonMap map[string]interface{}
+ var resource interface{}
+ var detailedResource map[string]interface{}
+
+ json.Unmarshal([]byte(appSummary), &jsonMap)
+
+ resource = jsonMap["resourceUsage"]
+
+ resourceUsageMap := resource.(map[string]interface{})
+ //fmt.Print("resourceMap: ")
+ //fmt.Println(resourceUsageMap)
+
+ itypeResource, ok := resourceUsageMap["itype-1"]
+ if !ok {
+ assert.Assert(t, memorySeconds == -1 || vcoresSecconds == -1, "no resource usage")
+ return
+ }
+
+ //fmt.Print("itypeResource: ")
Review Comment:
Remove debug print.
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,55 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
Review Comment:
If we have a time to clean up the UsedResourceMap, if it will cause resource leak.
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
One question, when we are starting state the app for gang scheduler enabled, it means the gang scheduler placeholder allocations are all allocated, if these placeholder allocations need to calculate the resource usage, because before the startTime here, the gang scheduler allocations are all bound in k8s shim side.
And if the gang scheduler timeout, if the placeholder allocations need to calculate even the app failed, because some placeholder allocations have been occupied resources for some time, and the default timeout is 15 mins, it's not a short time for resources occupied.
cc @wilfred-s @craigcondit
##########
pkg/scheduler/objects/node.go:
##########
@@ -117,6 +121,15 @@ func (sn *Node) GetAttribute(key string) string {
return sn.attributes[key]
}
+// Get inst type from attributes passed to core from shim.
+func (sn *Node) GetInstType() string {
+ itype := sn.GetAttribute(YunikornInstanceType)
Review Comment:
Change after instanceType merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1116154881
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,55 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ var aggregatedResourceTime map[string]int64
+ var timeDiff int64
+ var releaseTime time.Time
+ var resourceSecondsKey string
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime = time.Now()
+ timeDiff = int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey = key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff //resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ jsonStr, err := json.Marshal(urt.UsedResourceMap)
+ if err != nil {
+ return string(err.Error())
+ } else {
+ return string(jsonStr)
+ }
+}
Review Comment:
Simplify:
```
if err != nil {
return err.Error() // already a string
}
return string(jsonStr)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1116152830
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,55 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ var aggregatedResourceTime map[string]int64
+ var timeDiff int64
+ var releaseTime time.Time
+ var resourceSecondsKey string
Review Comment:
Declare the variables close to their usage, increasing readability. Most of the time you can use `:=`.
Eg.
```
releaseTime := time.Now()
timeDiff := int64(releaseTime.Sub(createTime).Seconds())
resourceSecondsKey := key + "Seconds"
```
It's preferred to start the method with Lock/Unlock pair if there's no reason to do otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192645803
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
Thanks, agreed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190328853
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
+ defer urt.Unlock()
+ urm, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
HI Craig, that means we need to create a snapshot (deep copy) of the UsedResourceMap and return this snapshot here instead of the marshalled string, and then have zap logger report the map as json. That's what you suggested right?
@wilfred-s raised the concern that map uses more memory than one would expect earlier, and I tested it, it's indeed a lot of more memory consumption than I expected. Plus the runtime to do deep copy. I wonder if we really want to go this route now, or we can defer to later when there is a consumer requiring map instead of string? This is internal to yunikorn-core right now and the app summary is only used for the logging added by this ticket.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190331524
##########
pkg/scheduler/objects/application.go:
##########
@@ -107,6 +112,50 @@ type Application struct {
sync.RWMutex
}
+type ApplicationSummary struct {
+ ApplicationID string
+ SubmissionTime time.Time
+ StartTime time.Time
+ FinishTime time.Time
+ User string
+ Queue string
+ State string
+ RmID string
+ ResourceUsage string
+}
+
+func (as *ApplicationSummary) GetReport() string {
+ res := "{"
+ res += "\"appId\":\"" + as.ApplicationID + "\","
+ res += "\"submissionTime\":\"" + strconv.FormatInt(as.SubmissionTime.UnixMilli(), 10) + "\","
+ res += "\"startTime\":\"" + strconv.FormatInt(as.StartTime.UnixMilli(), 10) + "\","
+ res += "\"finishTime\":\"" + strconv.FormatInt(as.FinishTime.UnixMilli(), 10) + "\","
+ res += "\"user\":\"" + as.User + "\","
+ res += "\"queue\":\"" + as.Queue + "\","
+ res += "\"state\":\"" + as.State + "\","
+ res += "\"rmID\":\"" + as.RmID + "\","
+ res += "\"resourceUsage\":" + as.ResourceUsage
+ res += "}"
+ return res
+}
+
+func (sa *Application) GetApplicationSummary(rmID string) *ApplicationSummary {
+ sa.RLock()
+ defer sa.RUnlock()
+ appSummary := &ApplicationSummary{
+ ApplicationID: sa.ApplicationID,
+ SubmissionTime: sa.SubmissionTime,
+ StartTime: sa.startTime,
+ FinishTime: sa.finishedTime,
+ User: sa.user.User,
+ Queue: sa.queuePath,
+ State: sa.stateMachine.Current(),
+ RmID: rmID,
+ ResourceUsage: sa.usedResourceTracker.GetResourceUsageSummary(),
Review Comment:
This structure is meant to produce a string app summary, there is no other tooling downstream except for the one line logging I added with this ticket. One can always get the resource usage structure from the application struct. And based on the map memory footprint concern @wilfred-s raised, can we use string for now, and change to map later when we really need a map?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190331077
##########
pkg/scheduler/objects/application.go:
##########
@@ -107,6 +112,50 @@ type Application struct {
sync.RWMutex
}
+type ApplicationSummary struct {
+ ApplicationID string
+ SubmissionTime time.Time
+ StartTime time.Time
+ FinishTime time.Time
+ User string
+ Queue string
+ State string
+ RmID string
+ ResourceUsage string
+}
+
+func (as *ApplicationSummary) GetReport() string {
Review Comment:
See my comment above about json or separated fields, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202869538
##########
pkg/scheduler/objects/allocation.go:
##########
@@ -99,16 +103,17 @@ func NewAllocation(uuid, nodeID string, ask *AllocationAsk) *Allocation {
}
}
-func newReservedAllocation(result AllocationResult, nodeID string, ask *AllocationAsk) *Allocation {
- alloc := NewAllocation("", nodeID, ask)
+func newReservedAllocation(result AllocationResult, nodeID string, instType string, ask *AllocationAsk) *Allocation {
Review Comment:
Thanks @craigcondit yes I did. What I did was, I made sure all code paths that set an allocation to Allocated/AllocatedReserved/Replaced also set the bindTime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1193009065
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,59 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResource struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+func (ur *UsedResource) Clone() *UsedResource {
+ if ur == nil {
+ return nil
+ }
+ ret := NewUsedResource()
+ ur.RLock()
+ defer ur.RUnlock()
+ for k, v := range ur.UsedResourceMap {
+ sourceEntry := map[string]int64(v)
Review Comment:
This is very complicated... If I'm reading this correctly, this is unnecessary. The variable itself and the type conversion too. Just range through "v".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192608186
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
If you're going to account for usage accurately, you need to track each task from start to finish, not at the app level. If you have 1000 tasks, but can only run 10 at a time, you're going to have a stair-step chart as the first batch of 10 run and complete, then the next 10, etc. If each task takes 1 minute to complete, an accurate assessment of total time should be 1000 minutes. But tracking each task from the start of the application, you will account for 1 minute for each of the first 10 pods, 2 minutes for each of the next 10, etc. The final batch of 10 (100 batches total) will be tracked as 100 minutes each. On average, 50 minutes will be accounted to each task, so you will over-report actual usage by a factor of 50.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192609360
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
The right way to do this would be to track the timestamp per allocation when the allocation is associated with a node, and when an allocation terminates, report that usage as ((allocationEndTime - allocationStartTime) * resources.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192639907
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
A future enhancement could be to track total accumulated resources per unit time in the application for released allocations, and then provide an API to also get "in-progress" allocations. This could be used to get better granularity than task termination for long-running applications.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1203002116
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -505,7 +507,7 @@ func (p *Preemptor) tryNodes() (string, string, []*Allocation, bool) {
// call predicates to evaluate each node
result := p.checkPreemptionPredicates(predicateChecks, victimsByNode)
if result != nil && result.success {
- return result.nodeID, result.instType, result.victims, true
+ return result.nodeID, p.nodeInstanceTypeMap[result.nodeID], result.victims, true
Review Comment:
Use unknown if not found.
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -153,6 +155,7 @@ func (p *Preemptor) initWorkingState() {
}
allocationsByNode[nodeID] = append(allocations, allocation)
queueByAlloc[allocation.GetAllocationKey()] = victims
+ nodeInstanceTypeMap[nodeID] = allocation.GetInstanceType()
Review Comment:
This *should* work in theory, however, it's probably best to check that allocation.GetInstanceType() was in fact populated. If not, ignore it and move on. We can use unknown if the map entry ends up not being populated.
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -505,7 +507,7 @@ func (p *Preemptor) tryNodes() (string, string, []*Allocation, bool) {
// call predicates to evaluate each node
result := p.checkPreemptionPredicates(predicateChecks, victimsByNode)
if result != nil && result.success {
- return result.nodeID, result.instType, result.victims, true
+ return result.nodeID, p.nodeInstanceTypeMap[result.nodeID], result.victims, true
Review Comment:
Always check the result of a map lookup, just in case. If it's not found, log a BUG message (see elsewhere for examples).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202829873
##########
pkg/scheduler/objects/node.go:
##########
@@ -33,6 +33,10 @@ import (
"github.com/apache/yunikorn-scheduler-interface/lib/go/si"
)
+const (
+ UnknownInstanceType = "unknown"
Review Comment:
Instance type is optional; it may not be specified in any or all cases. I'd like to see uppercase to differentiate it; I think either OTHER or UNKNOWN should suffice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1203073684
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -174,6 +177,20 @@ func (p *Preemptor) initWorkingState() {
p.allocationsByNode = allocationsByNode
p.queueByAlloc = queueByAlloc
p.nodeAvailableMap = nodeAvailableMap
+ p.nodeInstanceTypeMap = nodeInstanceTypeMap
+}
+
+// getInstanceType retrieves instance type from the nodeInstanceTypeMap for given nodeId
+// if not found, issue error log and return UnknownInstanceType
+func (p *Preemptor) getInstanceType(nodeId string) string {
Review Comment:
Very much appreciated @craigcondit uploaded new rev again to address both latest points you made. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated res…
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#issuecomment-1435444431
This PR is meant to be added on top of https://issues.apache.org/jira/browse/YUNIKORN-1549. But I hope to get some review of the first version while we are waiting for YUNIKORN-1549.
Hi @yangwwei @wilfred-s @craigcondit @zhuqi-lucas would you please help taking a look? thanks a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192608186
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
If you're going to account for usage accurately, you need to track each task from start to finish, not from the app start time. If you have 1000 tasks, but can only run 10 at a time, you're going to have a stair-step chart as the first batch of 10 run and complete, then the next 10, etc. If each task takes 1 minute to complete, an accurate assessment of total time should be 1000 minutes. But tracking each task from the start of the application, you will account for 1 minute for each of the first 10 pods, 2 minutes for each of the next 10, etc. The final batch of 10 (100 batches total) will be tracked as 100 minutes each. On average, 50 minutes will be accounted to each task, so you will over-report actual usage by a factor of 50.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192604759
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
Thanks @craigcondit
I have some questions about the startTime.
Currently I implemented the startTime in the app state machine as:
```
fmt.Sprintf("enter_%s", Starting.String()): func(_ context.Context, event *fsm.Event) {
app := event.Args[0].(*Application) //nolint:errcheck
app.startTime = time.Now()
```
My understanding is that the Start state in this diagram (https://yunikorn.apache.org/docs/next/design/scheduler_object_states/) means the scheduler started working on allocation for the app (correct?).
Wonder if it makes sense to have a state in the diagram to represent the first pod binding? If there is such a state, we can set the startTime in the state machine when entering this state.
Otherwise, I studied the code and saw that we can possibly have a field bindingTime in the Allocation structure to represent the binding time, and set it in the following method:
`func (sa *Application) tryNodes(ask *AllocationAsk, iterator NodeIterator) *Allocation {`
would you please confirm?
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1204617584
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -155,7 +155,10 @@ func (p *Preemptor) initWorkingState() {
}
allocationsByNode[nodeID] = append(allocations, allocation)
queueByAlloc[allocation.GetAllocationKey()] = victims
- nodeInstanceTypeMap[nodeID] = allocation.GetInstanceType()
+ instType := allocation.GetInstanceType()
+ if instType != UnknownInstanceType {
Review Comment:
Thanks a lot @craigcondit I'm working on a new rev.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit closed pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit closed pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
URL: https://github.com/apache/yunikorn-core/pull/507
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1199517288
##########
pkg/scheduler/objects/allocation.go:
##########
@@ -99,16 +103,17 @@ func NewAllocation(uuid, nodeID string, ask *AllocationAsk) *Allocation {
}
}
-func newReservedAllocation(result AllocationResult, nodeID string, ask *AllocationAsk) *Allocation {
- alloc := NewAllocation("", nodeID, ask)
+func newReservedAllocation(result AllocationResult, nodeID string, instType string, ask *AllocationAsk) *Allocation {
Review Comment:
HI @craigcondit thanks for the review and this is a great question. My understanding was that the bind time is set when the allocation result is changed / set to "Allocated", "AllocatedReserved" and "Replaced", which I did.
```
const (
None AllocationResult = iota
Allocated
AllocatedReserved
Reserved
Unreserved
Replaced
)
```
My understanding of the above states is:
**Allocated**: bound
**AllocatedReserved**: bound (and previously reserved)
**Reserved**: reserved but not bound
**Unreserved**: unbound and unreserved
**Replaced**: bound (placeholder replaced with real allocate)
If my understanding is correct, then I think all code paths are covered by my change.
Would you please correct me if my understanding is wrong, and help highlighting the places that I may possibly missed?
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202832513
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -349,6 +349,7 @@ func (p *Preemptor) checkPreemptionPredicates(predicateChecks []*si.PreemptionPr
result := &predicateCheckResult{
allocationKey: check.AllocationKey,
nodeID: check.NodeID,
+ instType: UnknownInstanceType,
Review Comment:
As I indicated above: Only the instance type of the node assigned to the NEW allocation matters -- this has nothing to do with the preemption logic. Refactor this so that the instance type is handled outside the preemption code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202900578
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
It was via a discussion with @wilfred-s @yangwwei that we decided to add an instanceType in Allocation object, because we don't have a global map between nodeId and instanceType. We not only need instanceType for preemption code (which is new), but also need instanceType for all nodes. I wish we have a global map, but we think it's more of a management hassle, so we decided to have the instType info included together with nodeId when creating Allocation object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202903294
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
The down side is with my current implementation is that whenever we create an Allocation object, we need to have both the nodeId and instType information available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1117818163
##########
pkg/metrics/init.go:
##########
@@ -229,3 +236,7 @@ func formatMetricName(metricName string) string {
}
return string(newBytes)
}
+
+func (m *Metrics) LogAppSummary(appSummary string) {
+ log.Println(AppSummaryHeader + appSummary)
+}
Review Comment:
Hi Qi, Println itself will append newline. Thanks.
HI Pbacsko, I did spend good amount of time to identify a suitable place for this, I put it here thinking that appsummary is part of the application (aggregated) metrics, but if you have other place to recommend, I will be happy to give it a try. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1117830838
##########
pkg/scheduler/objects/application.go:
##########
@@ -1920,6 +1947,26 @@ func (sa *Application) cleanupAsks() {
sa.sortedRequests = nil
}
+func (sa *Application) GetAppSummary(rmID string) string {
+ var res string
+ sa.RLock()
+ defer sa.RUnlock()
+
+ res = "{"
+
+ res += "\"appId\":\"" + sa.ApplicationID + "\","
+ res += "\"submissionTime\":\"" + strconv.FormatInt(sa.SubmissionTime.UnixMilli(), 10) + "\","
+ res += "\"startTime\":\"" + strconv.FormatInt(sa.StartTime().UnixMilli(), 10) + "\","
+ res += "\"finishTime\":\"" + strconv.FormatInt(sa.FinishedTime().UnixMilli(), 10) + "\","
+ res += "\"user\":\"" + sa.GetUser().User + "\","
+ res += "\"queue\":\"" + sa.GetQueuePath() + "\","
+ res += "\"state\":\"" + sa.stateMachine.Current() + "\","
+ res += "\"rmID\":\"" + rmID + "\","
+ res += "\"resourceUsage\":" + sa.usedResourceTracker.GetResourceUsageSummary()
+ res += "}"
+ return res
Review Comment:
Thanks, will be addressed in next rev.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] zhuqi-lucas commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated res…
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1113858442
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
One question, when we are starting state the app for gang scheduler enabled, it means the gang scheduler placeholder allocations are all allocated, if these placeholder allocations need to calculate the resource usage, because before the startTime here, the gang scheduler allocations are all bound in k8s shim side.
And if the gang scheduler timeout, if the placeholder allocations need to calculate even the app failed, because some placeholder allocations have been occupied resources for some time, and the default timeout is 15 mins, it's not a short time for resources occupied.
cc @wilfred-s @craigcondit @yangwwei
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1116162565
##########
pkg/scheduler/objects/application.go:
##########
@@ -1920,6 +1947,26 @@ func (sa *Application) cleanupAsks() {
sa.sortedRequests = nil
}
+func (sa *Application) GetAppSummary(rmID string) string {
+ var res string
+ sa.RLock()
+ defer sa.RUnlock()
+
+ res = "{"
+
+ res += "\"appId\":\"" + sa.ApplicationID + "\","
+ res += "\"submissionTime\":\"" + strconv.FormatInt(sa.SubmissionTime.UnixMilli(), 10) + "\","
+ res += "\"startTime\":\"" + strconv.FormatInt(sa.StartTime().UnixMilli(), 10) + "\","
+ res += "\"finishTime\":\"" + strconv.FormatInt(sa.FinishedTime().UnixMilli(), 10) + "\","
+ res += "\"user\":\"" + sa.GetUser().User + "\","
+ res += "\"queue\":\"" + sa.GetQueuePath() + "\","
+ res += "\"state\":\"" + sa.stateMachine.Current() + "\","
+ res += "\"rmID\":\"" + rmID + "\","
+ res += "\"resourceUsage\":" + sa.usedResourceTracker.GetResourceUsageSummary()
+ res += "}"
+ return res
Review Comment:
It's weird to see all these formatting here. I do believe the `Application` object should not be concerned about formatting, that should be delegated to the caller.
One approach is to just call the Get* methods to retrieve the relevant piece of data. Missing getters can be added.
If it's necessary to return a snapshot of these data guarded by locks, you can create an `AppSummary` type which contains all these information and return that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1188924259
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
Review Comment:
This should be RLock() / RUnlock()
##########
pkg/scheduler/objects/application.go:
##########
@@ -107,6 +112,50 @@ type Application struct {
sync.RWMutex
}
+type ApplicationSummary struct {
+ ApplicationID string
+ SubmissionTime time.Time
+ StartTime time.Time
+ FinishTime time.Time
+ User string
+ Queue string
+ State string
+ RmID string
+ ResourceUsage string
+}
+
+func (as *ApplicationSummary) GetReport() string {
+ res := "{"
+ res += "\"appId\":\"" + as.ApplicationID + "\","
+ res += "\"submissionTime\":\"" + strconv.FormatInt(as.SubmissionTime.UnixMilli(), 10) + "\","
+ res += "\"startTime\":\"" + strconv.FormatInt(as.StartTime.UnixMilli(), 10) + "\","
+ res += "\"finishTime\":\"" + strconv.FormatInt(as.FinishTime.UnixMilli(), 10) + "\","
+ res += "\"user\":\"" + as.User + "\","
+ res += "\"queue\":\"" + as.Queue + "\","
+ res += "\"state\":\"" + as.State + "\","
+ res += "\"rmID\":\"" + as.RmID + "\","
+ res += "\"resourceUsage\":" + as.ResourceUsage
+ res += "}"
+ return res
+}
+
+func (sa *Application) GetApplicationSummary(rmID string) *ApplicationSummary {
+ sa.RLock()
+ defer sa.RUnlock()
+ appSummary := &ApplicationSummary{
+ ApplicationID: sa.ApplicationID,
+ SubmissionTime: sa.SubmissionTime,
+ StartTime: sa.startTime,
+ FinishTime: sa.finishedTime,
+ User: sa.user.User,
+ Queue: sa.queuePath,
+ State: sa.stateMachine.Current(),
+ RmID: rmID,
+ ResourceUsage: sa.usedResourceTracker.GetResourceUsageSummary(),
Review Comment:
Keep this as a real structure. Flattening to a string will make this difficult to parse by tooling downstream.
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
Review Comment:
I don't think that createTime is appropriate here. That time (normally) gets populated from the AllocationAsk object, which corresponds to the creation time of the Pod in Kubernetes. This is NOT when the pod begins executing, so in the case where say a queue with room for 10 pods gets 1000 submitted to it, all 1000 will have the same createTime. This is clearly not what we want.
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
Review Comment:
Why mess with the resource name here? Just document that each resource is aggregated in terms of seconds but leave the keys unmodified. This gets very messy with custom resource types...
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
To clarify: The startTime should be when the first allocation (not ask) is assigned.
##########
pkg/scheduler/objects/application.go:
##########
@@ -107,6 +112,50 @@ type Application struct {
sync.RWMutex
}
+type ApplicationSummary struct {
+ ApplicationID string
+ SubmissionTime time.Time
+ StartTime time.Time
+ FinishTime time.Time
+ User string
+ Queue string
+ State string
+ RmID string
+ ResourceUsage string
+}
+
+func (as *ApplicationSummary) GetReport() string {
Review Comment:
This really needs to be run through zap.Logger and formatted as separate fields within that. Dumping a random, difficult to parse string here is not good.
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
+ defer urt.Unlock()
+ urm, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
We should not be marshalling into JSON here, as the zap logger can do that for us. Instead, we should log individual fields.
##########
pkg/metrics/init.go:
##########
@@ -166,6 +169,10 @@ type CoreEventMetrics interface {
Reset()
}
+func (m *Metrics) LogAppSummary(appSummary string) {
Review Comment:
Use the standard zap logger with separated out fields, not a flattened json-as-string representation.
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
I would consider the startTime to be when the first pod (placeholder or otherwise) is bound. This should still be considered resources used by the application, as they are unavailable for others to utilize.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192672950
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
Hi Craig,
I found that we can not recover the bindTime for recovered allocation because si.Allocation come from the shim, unless we change the protocol of for this structure:
// Create a new Allocation from a node recovered allocation.
// Also creates an AllocationAsk to maintain backward compatible behaviour
// This returns a nil Allocation on nil input or errors
func NewAllocationFromSI(alloc *si.Allocation, instType string) *Allocation {
Or we simply set the current time to bind time for this kind of allocation? It seems that if the recovered allocation is because of the yunikorn restart, then we do need to let the shim to pass this info.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190334022
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
Review Comment:
Thanks for your detailed review.
What custom resource types you were referring too? are they not going to be based on seconds?
To explain why I did this:
I'm trying to make it easy for viewer to grasp the meaning of the numbers reported when one looks at the app summary reported.
Plus, the measurement of resource usage should be "resourceAmount * timeLength", not just "resourceAmount". Without a time unit ("Seconds" here) in the name, one can misread it as amount of resource, instead of "amount of resource * timeLength". If you notice, current yunikorn UI does report memory/vcore as "resources used", which is a misnomer in the UI, because what the UI displayed is a snapshot of currently used resourceAmount, not "resourceAmount * timeLength".
In addition, having the "Seconds" string in it makes it easy to adapt to since we are migrating from YARN to yunikorn, as YARN does report memorySeconds and vcoreSeconds too.
Wonder with the above explanation, would you still suggest removing the "Seconds"?
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1193009065
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,59 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResource struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+func (ur *UsedResource) Clone() *UsedResource {
+ if ur == nil {
+ return nil
+ }
+ ret := NewUsedResource()
+ ur.RLock()
+ defer ur.RUnlock()
+ for k, v := range ur.UsedResourceMap {
+ sourceEntry := map[string]int64(v)
Review Comment:
If I'm reading this correctly, this is completely unnecessary and very complicated read. The variable itself and the type conversion too. Just range through "v".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202830631
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
The instance type of the node used for the NEW allocation is what's needed. This has nothing at all to do with the preemption logic. Please fix this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202999936
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
Thanks a lot @craigcondit just updated with new rev. In the code change you suggested, I had to do:
nodeInstanceTypeMap[nodeID] = allocation.GetInstanceType()
instead of
nodeInstanceTypeMap[nodeID] = node.GetInstanceType()
Would you please take a look again when you get chance? thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1203039285
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
Many thanks @craigcondit good point, I just updated the PR with a few changes, including your above comment, assigning the map
`p.nodeInstanceTypeMap = nodeInstanceTypeMap`
which I forgot in last rev, and using "UNKNOWN",
Thanks again for taking further look!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1204467322
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -349,6 +349,7 @@ func (p *Preemptor) checkPreemptionPredicates(predicateChecks []*si.PreemptionPr
result := &predicateCheckResult{
allocationKey: check.AllocationKey,
nodeID: check.NodeID,
+ instType: UnknownInstanceType,
Review Comment:
Thanks Craig, addressed in the new rev yesterday.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190330192
##########
pkg/metrics/init.go:
##########
@@ -166,6 +169,10 @@ type CoreEventMetrics interface {
Reset()
}
+func (m *Metrics) LogAppSummary(appSummary string) {
Review Comment:
I log out json format (like YARN does) in one line for each app for the ease of fluentBit filtering. Would you please explain why we have to report separate fields? To me, json is more compact, and fluentBit filtering will be much more harder with individually reported fields. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190338730
##########
pkg/scheduler/objects/application.go:
##########
@@ -107,6 +112,50 @@ type Application struct {
sync.RWMutex
}
+type ApplicationSummary struct {
+ ApplicationID string
+ SubmissionTime time.Time
+ StartTime time.Time
+ FinishTime time.Time
+ User string
+ Queue string
+ State string
+ RmID string
+ ResourceUsage string
+}
+
+func (as *ApplicationSummary) GetReport() string {
Review Comment:
And see my replies; this is non-negotiable. We MUST be consistent in our logging practices.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1191278853
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
+ defer urt.Unlock()
+ urm, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
This is not YARN, we don't do the same things. We have to play nice within the Kubernetes ecosystem, where logging all goes through a single output (stdout). Because of this, we MUST be consistent in how we log, or we make it difficult for ALL downstream consumers of log events. We can't just change things up for the convenience of your consumer code.
As for this:
`log.logger().Info(zap.String("YK_APP_SUMMARY", app.GetAppSummary(pc.RmID)))`
You will end up evaluating `app.GetSummary(pc.RmID)` whether or not logging is enabled. To avoid this, one method would be to use `Stringer()` and provide a deferred function that retrieves the app summary as a string. However, this breaks the structured logging as you are logging a complex object. It's better to log this as individual fields so that the zap log line can be parsed appropriately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1193009065
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,59 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResource struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+func (ur *UsedResource) Clone() *UsedResource {
+ if ur == nil {
+ return nil
+ }
+ ret := NewUsedResource()
+ ur.RLock()
+ defer ur.RUnlock()
+ for k, v := range ur.UsedResourceMap {
+ sourceEntry := map[string]int64(v)
Review Comment:
This is very complicated to read... If I'm reading this correctly, this is unnecessary. The variable itself and the type conversion too. Just range through "v".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192639022
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
Yes, tracking the time (per allocation) between allocated and released will work. The application doesn't really impact this at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1199271018
##########
pkg/scheduler/objects/allocation.go:
##########
@@ -99,16 +103,17 @@ func NewAllocation(uuid, nodeID string, ask *AllocationAsk) *Allocation {
}
}
-func newReservedAllocation(result AllocationResult, nodeID string, ask *AllocationAsk) *Allocation {
- alloc := NewAllocation("", nodeID, ask)
+func newReservedAllocation(result AllocationResult, nodeID string, instType string, ask *AllocationAsk) *Allocation {
Review Comment:
Question: Have you verified that all the code paths which use this allocation object actually set the bind time? It would be unfortunate for the bind time to remain at 0 when the allocation is scheduled...
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -349,6 +349,7 @@ func (p *Preemptor) checkPreemptionPredicates(predicateChecks []*si.PreemptionPr
result := &predicateCheckResult{
allocationKey: check.AllocationKey,
nodeID: check.NodeID,
+ instType: UnknownInstanceType,
Review Comment:
Let's keep the instance type out of the preemption code. It's only needed on allocations. Once a node is chosen for the allocation we can apply the instance type.
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
This is just incorrect. Just because preemptions happened on a node does not ensure that node will ultimately be where the new allocation goes.
##########
pkg/scheduler/objects/node.go:
##########
@@ -33,6 +33,10 @@ import (
"github.com/apache/yunikorn-scheduler-interface/lib/go/si"
)
+const (
+ UnknownInstanceType = "unknown"
Review Comment:
Maybe we want unknown nodes to stand out a bit more -- perhaps "OTHER" in uppercase?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202926512
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
I think the solution I suggested should be sufficient for now. A global mapping further complicates the context, and requires additional locking at multiple levels, which the node iterator prevents currently. This is something that could be revisited in the future, but that's where we are at today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1204637827
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -155,7 +155,10 @@ func (p *Preemptor) initWorkingState() {
}
allocationsByNode[nodeID] = append(allocations, allocation)
queueByAlloc[allocation.GetAllocationKey()] = victims
- nodeInstanceTypeMap[nodeID] = allocation.GetInstanceType()
+ instType := allocation.GetInstanceType()
+ if instType != UnknownInstanceType {
Review Comment:
Hi @craigcondit new rev uploaded. Many thanks again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202893207
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
The Preemptor stores mappings between node and various information needed for preemption. See initWorkingState() -- you can add a nodeInstanceTypeMap there and populate it. Then when a node is chosen for the allocation, look it up in that map. That's far simpler than what you have done here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202869764
##########
pkg/scheduler/objects/node.go:
##########
@@ -33,6 +33,10 @@ import (
"github.com/apache/yunikorn-scheduler-interface/lib/go/si"
)
+const (
+ UnknownInstanceType = "unknown"
Review Comment:
Sure, will change in next rev.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1193030548
##########
pkg/scheduler/objects/application.go:
##########
@@ -107,6 +111,48 @@ type Application struct {
sync.RWMutex
}
+type ApplicationSummary struct {
+ ApplicationID string
+ SubmissionTime time.Time
+ StartTime time.Time
+ FinishTime time.Time
+ User string
+ Queue string
+ State string
+ RmID string
+ ResourceUsage* resources.UsedResource
+}
+
+func (as *ApplicationSummary) DoLogging() {
+ log.Logger().Info("YK_APP_SUMMARY:",
+ zap.String("appID", as.ApplicationID),
+ zap.Int64("submissionTime", as.SubmissionTime.UnixMilli()),
+ zap.Int64("startTime", as.StartTime.UnixMilli()),
+ zap.Int64("finishTime", as.FinishTime.UnixMilli()),
+ zap.String("user", as.User),
+ zap.String("queue", as.Queue),
+ zap.String("state", as.State),
+ zap.String("rmID", as.RmID),
+ zap.Any("resourceUsage", as.ResourceUsage.UsedResourceMap))
+}
+
+func (sa *Application) GetApplicationSummary(rmID string) *ApplicationSummary {
+ sa.RLock()
+ defer sa.RUnlock()
+ appSummary := &ApplicationSummary{
+ ApplicationID: sa.ApplicationID,
+ SubmissionTime: sa.SubmissionTime,
+ StartTime: sa.startTime,
+ FinishTime: sa.finishedTime,
+ User: sa.user.User,
+ Queue: sa.queuePath,
+ State: sa.stateMachine.Current(),
Review Comment:
This is guarded by a read lock inside the state machine. It's most likely not an issue, but it's best not to have multiple locks grabbed this way. So move this out before `sa.Lock()` and save it to a variable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1117812794
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,55 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ var aggregatedResourceTime map[string]int64
+ var timeDiff int64
+ var releaseTime time.Time
+ var resourceSecondsKey string
Review Comment:
Thanks for the review and very good feedback Pbacsko! Go language is new to me and I'm still learning, will address in next rev.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1117809604
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,55 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
Review Comment:
Thanks for the review Qi, very good feedback, will be addressed in new rev.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated res…
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#issuecomment-1438984474
The 256 bytes per map is higher than what I expected so I'm still taking further look to see if it can be reduced. But Would appreciate if you can take a look at the PR. Thanks @wilfred-s @yangwwei @craigcondit @zhuqi-lucas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1117815170
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,55 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ var aggregatedResourceTime map[string]int64
+ var timeDiff int64
+ var releaseTime time.Time
+ var resourceSecondsKey string
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime = time.Now()
+ timeDiff = int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey = key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff //resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ jsonStr, err := json.Marshal(urt.UsedResourceMap)
+ if err != nil {
+ return string(err.Error())
+ } else {
+ return string(jsonStr)
+ }
+}
Review Comment:
Thanks, to be addressed in next rev.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1117820136
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
Hi Qi, thanks for the thoughts. My understanding is that placeholder allocations as treated as being used, see
func (sa *Application) removeAllocationInternal(
only when the placeholder allocations are replaced with "real" allocations, they will be considered used.
I may have misunderstanding here, further comments are welcome.
Thanks,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#issuecomment-1542707588
> I think there's still some fundamental issues here to be addressed.
Thanks a lot for the detailed review. Please see my reply/comments below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] codecov[bot] commented on pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#issuecomment-1555118565
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#507](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (321b5e6) into [master](https://app.codecov.io/gh/apache/yunikorn-core/commit/9516d20f8a63a2cf3bcfeabb1a2b7c24d0ce7154?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9516d20) will **decrease** coverage by `0.13%`.
> The diff coverage is `63.35%`.
```diff
@@ Coverage Diff @@
## master #507 +/- ##
==========================================
- Coverage 74.93% 74.81% -0.13%
==========================================
Files 70 70
Lines 11400 11510 +110
==========================================
+ Hits 8543 8611 +68
- Misses 2553 2595 +42
Partials 304 304
```
| [Impacted Files](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [pkg/common/resources/resources.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `92.24% <0.00%> (-5.49%)` | :arrow_down: |
| [pkg/scheduler/context.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `30.42% <75.00%> (+0.10%)` | :arrow_up: |
| [pkg/scheduler/objects/application.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `65.04% <78.57%> (+0.68%)` | :arrow_up: |
| [pkg/scheduler/objects/preemption.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3ByZWVtcHRpb24uZ28=) | `76.32% <90.00%> (+0.15%)` | :arrow_up: |
| [pkg/scheduler/objects/allocation.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `90.24% <100.00%> (+0.66%)` | :arrow_up: |
| [pkg/scheduler/objects/application\_state.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/scheduler/objects/node.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `82.32% <100.00%> (+0.19%)` | :arrow_up: |
| [pkg/scheduler/partition.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/507?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `77.23% <100.00%> (+0.04%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1192889143
##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
execTimeout time.Duration // execTimeout for the application run
placeholderTimer *time.Timer // placeholder replace timer
gangSchedulingStyle string // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+ startTime time.Time // the time that the application starts running. Default is zero.
Review Comment:
HI @craigcondit I uploaded a new rev to address all of your comments, except for one: the app startTime. Ideally we should get the first pod's bind time, however, I think for multiple reasons it's not so important:
1. the resource usage calculation doesn't use the startTime
2. the startTime is the time that Yunikorn starts scheduling the app
3. if yunikorn died, and restarted, how to identify the bind time of the first pod for a recovered app?
If we really need to do something about this, we can make a change as follow-up to this jira I think.
Thanks.
##########
pkg/scheduler/objects/application.go:
##########
@@ -107,6 +112,50 @@ type Application struct {
sync.RWMutex
}
+type ApplicationSummary struct {
+ ApplicationID string
+ SubmissionTime time.Time
+ StartTime time.Time
+ FinishTime time.Time
+ User string
+ Queue string
+ State string
+ RmID string
+ ResourceUsage string
+}
+
+func (as *ApplicationSummary) GetReport() string {
Review Comment:
Thanks @craigcondit addressed in latest PR update posted.
##########
pkg/scheduler/objects/node.go:
##########
@@ -33,6 +33,10 @@ import (
"github.com/apache/yunikorn-scheduler-interface/lib/go/si"
)
+const (
+ UnknownInstanceType = "unknown"
Review Comment:
I used "unknown" because the instance type information was not passed or extracted correctly (see https://issues.apache.org/jira/browse/YUNIKORN-1549), and not because it's other instance types. Based on this, do you still think "OTHER" is better? I can make it "UNKNOWN" if you'd prefer. Thanks
##########
pkg/metrics/init.go:
##########
@@ -229,3 +236,7 @@ func formatMetricName(metricName string) string {
}
return string(newBytes)
}
+
+func (m *Metrics) LogAppSummary(appSummary string) {
+ log.Println(AppSummaryHeader + appSummary)
+}
Review Comment:
Thanks @craigcondit addressed in latest PR update https://github.com/apache/yunikorn-core/pull/507/commits/3d87097c05467398d269d3471e977136fdc53c4e
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
Review Comment:
Thanks @craigcondit addressed in latest PR update posted
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
+ defer urt.Unlock()
+ urm, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
Thanks @craigcondit addressed in latest PR update posted.
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
Review Comment:
Thanks @craigcondit addressed in latest PR update posted.
##########
pkg/metrics/init.go:
##########
@@ -166,6 +169,10 @@ type CoreEventMetrics interface {
Reset()
}
+func (m *Metrics) LogAppSummary(appSummary string) {
Review Comment:
Thanks @craigcondit addressed in latest PR update posted.
##########
pkg/scheduler/objects/application.go:
##########
@@ -107,6 +112,50 @@ type Application struct {
sync.RWMutex
}
+type ApplicationSummary struct {
+ ApplicationID string
+ SubmissionTime time.Time
+ StartTime time.Time
+ FinishTime time.Time
+ User string
+ Queue string
+ State string
+ RmID string
+ ResourceUsage string
+}
+
+func (as *ApplicationSummary) GetReport() string {
+ res := "{"
+ res += "\"appId\":\"" + as.ApplicationID + "\","
+ res += "\"submissionTime\":\"" + strconv.FormatInt(as.SubmissionTime.UnixMilli(), 10) + "\","
+ res += "\"startTime\":\"" + strconv.FormatInt(as.StartTime.UnixMilli(), 10) + "\","
+ res += "\"finishTime\":\"" + strconv.FormatInt(as.FinishTime.UnixMilli(), 10) + "\","
+ res += "\"user\":\"" + as.User + "\","
+ res += "\"queue\":\"" + as.Queue + "\","
+ res += "\"state\":\"" + as.State + "\","
+ res += "\"rmID\":\"" + as.RmID + "\","
+ res += "\"resourceUsage\":" + as.ResourceUsage
+ res += "}"
+ return res
+}
+
+func (sa *Application) GetApplicationSummary(rmID string) *ApplicationSummary {
+ sa.RLock()
+ defer sa.RUnlock()
+ appSummary := &ApplicationSummary{
+ ApplicationID: sa.ApplicationID,
+ SubmissionTime: sa.SubmissionTime,
+ StartTime: sa.startTime,
+ FinishTime: sa.finishedTime,
+ User: sa.user.User,
+ Queue: sa.queuePath,
+ State: sa.stateMachine.Current(),
+ RmID: rmID,
+ ResourceUsage: sa.usedResourceTracker.GetResourceUsageSummary(),
Review Comment:
Thanks @craigcondit addressed in latest PR update posted.
##########
pkg/scheduler/objects/application.go:
##########
@@ -107,6 +111,48 @@ type Application struct {
sync.RWMutex
}
+type ApplicationSummary struct {
+ ApplicationID string
+ SubmissionTime time.Time
+ StartTime time.Time
+ FinishTime time.Time
+ User string
+ Queue string
+ State string
+ RmID string
+ ResourceUsage* resources.UsedResource
+}
+
+func (as *ApplicationSummary) DoLogging() {
+ log.Logger().Info("YK_APP_SUMMARY:",
+ zap.String("appID", as.ApplicationID),
+ zap.Int64("submissionTime", as.SubmissionTime.UnixMilli()),
+ zap.Int64("startTime", as.StartTime.UnixMilli()),
+ zap.Int64("finishTime", as.FinishTime.UnixMilli()),
+ zap.String("user", as.User),
+ zap.String("queue", as.Queue),
+ zap.String("state", as.State),
+ zap.String("rmID", as.RmID),
+ zap.Any("resourceUsage", as.ResourceUsage.UsedResourceMap))
+}
+
+func (sa *Application) GetApplicationSummary(rmID string) *ApplicationSummary {
+ sa.RLock()
+ defer sa.RUnlock()
+ appSummary := &ApplicationSummary{
+ ApplicationID: sa.ApplicationID,
+ SubmissionTime: sa.SubmissionTime,
+ StartTime: sa.startTime,
+ FinishTime: sa.finishedTime,
+ User: sa.user.User,
+ Queue: sa.queuePath,
+ State: sa.stateMachine.Current(),
Review Comment:
Thanks much for the review @pbacsko just updated the PR to address your comments above. Would you please take a look again?
BTW, when doing test, I printed out the application tags,
2023-05-13T15:32:08.729-0700 INFO objects/application.go:2019 App tags {"App tags": {"namespace":"default","yunikorn.apache.org/creationTime":"1683930188","yunikorn.apache.org/schedulingPolicyParameters":"","yunikorn.apache.org/task-groups":""}}
Wonder if you or @craigcondit can share how we can pass more tags to the app when we submit an application.
For example, for a spark job
kind: SparkApplication
metadata:
name: x
namespace: default
Wonder if we can get both of the meta data entries "name" and "namespace" in the above example as tags?
Where to control what are passed as application tags?
Thanks.
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
The instance type here just meant the instance type of the current node, and it doesn't mean that the new allocation will go to instance of this type. The result is returned only when successful:
```
// call predicates to evaluate each node
result := p.checkPreemptionPredicates(predicateChecks, victimsByNode)
if result != nil && result.success {
return result.nodeID, result.instType, result.victims, true
}
```
Do you think this explanation makes sense? thanks
##########
pkg/scheduler/objects/allocation.go:
##########
@@ -99,16 +103,17 @@ func NewAllocation(uuid, nodeID string, ask *AllocationAsk) *Allocation {
}
}
-func newReservedAllocation(result AllocationResult, nodeID string, ask *AllocationAsk) *Allocation {
- alloc := NewAllocation("", nodeID, ask)
+func newReservedAllocation(result AllocationResult, nodeID string, instType string, ask *AllocationAsk) *Allocation {
Review Comment:
HI @craigcondit thanks for the review and this is a great question. My understanding was that the bind time is set when the allocation result is changed / set to "Allocated", "AllocatedReserved" and "Replaced", which I did.
```
const (
None AllocationResult = iota
Allocated
AllocatedReserved
Reserved
Unreserved
Replaced
)
```
My understanding of the above states is:
**Allocated**: bound
**AllocatedReserved**: bound (and previously reserved)
**Reserved**: reserved but not bound
**Unreserved**: unbound and unreserved
**Replaced**: bound (placeholder replaced with real allocate)
If my understanding is correct, then I think all code paths are covered by my change.
Would you please correct me if my understanding is wrong, and help highlighting the places that I may possibly missed?
I will look at other comments soon, but this one is very important, I'd like to get it right first.
Thanks.
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
Review Comment:
Thanks @craigcondit addressed in latest PR update posted.
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -349,6 +349,7 @@ func (p *Preemptor) checkPreemptionPredicates(predicateChecks []*si.PreemptionPr
result := &predicateCheckResult{
allocationKey: check.AllocationKey,
nodeID: check.NodeID,
+ instType: UnknownInstanceType,
Review Comment:
We need to have instance type info to be returned in the following code
```
func (p *Preemptor) tryNodes() (string, string, []*Allocation, bool) {
...
// call predicates to evaluate each node
result := p.checkPreemptionPredicates(predicateChecks, victimsByNode)
if result != nil && result.success {
return result.nodeID, result.instType, result.victims, true
}
```
That's why I added instanceType in
```
type predicateCheckResult struct {
allocationKey string
nodeID string
instType string
success bool
index int
victims []*Allocation
}
```
However, in the above code I passed "UnknownInstanceType", because PreemptionPredicatesArgs structure passed via RPC doesn't have the instance type info for the node, and fortunately the populateVictims method can fill it in right afterwards:
```
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
// Take the instance type info from the first victim Allocation of this given nodeID
if i == 0 {
pcr.instType = victim.GetInstanceType()
}
}
```
I think the above code block works because all victims in the victimList is from the same node:
`victimList, ok := victimsByNode[pcr.nodeID]
`
Wonder if you think my explanation makes sense?
Thanks a lot for the detailed review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202889790
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
Sorry I did not make it clear previously,
the `tryNodes()` function below is called by
```
func (p *Preemptor) TryPreemption() (*Allocation, bool) {
...
// try to find a node to schedule on and victims to preempt
nodeID, instType, victims, ok := p.tryNodes() // The nodeId, instType info returned here is used to construct
// a new allocation below
if !ok {
// no preemption possible
return nil, false
}
...
if p.headRoom.FitInMaxUndef(p.ask.GetAllocatedResource()) {
log.Logger().Info("Reserving node for ask after preemption",
zap.String("allocationKey", p.ask.GetAllocationKey()),
zap.String("nodeID", nodeID),
zap.Int("victimCount", len(victims)))
return newReservedAllocation(Reserved, nodeID, instType, p.ask), true
}
...
```
which in turn is called by
```
// tryAllocate will perform a regular allocation of a pending request, includes placeholders.
func (sa *Application) tryAllocate(headRoom *resources.Resource, preemptionDelay time.Duration, preemptAttemptsRemaining *int, nodeIterator func() NodeIterator, fullNodeIterator func() NodeIterator, getNodeFn func(string) *Node) *Allocation {
```
which returns Allocation object, which need the instance type info.
```
func (p *Preemptor) tryNodes() (string, string, []*Allocation, bool) {
// calculate victim list for each node
predicateChecks := make([]*si.PreemptionPredicatesArgs, 0)
victimsByNode := make(map[string][]*Allocation)
for nodeID, nodeAvailable := range p.nodeAvailableMap {
allocations, ok := p.allocationsByNode[nodeID]
if !ok {
// no allocations present, but node may still be available for scheduling
allocations = make([]*Allocation, 0)
}
// identify which victims and in which order should be tried
if idx, victims := p.calculateVictimsByNode(nodeAvailable, allocations); victims != nil {
victimsByNode[nodeID] = victims
keys := make([]string, 0)
for _, victim := range victims {
keys = append(keys, victim.GetAllocationKey())
}
// only check this node if there are victims or we have not already tried scheduling
if len(victims) > 0 || !p.nodesTried {
predicateChecks = append(predicateChecks, &si.PreemptionPredicatesArgs{
AllocationKey: p.ask.GetAllocationKey(),
NodeID: nodeID,
PreemptAllocationKeys: keys,
StartIndex: int32(idx),
})
}
}
}
// call predicates to evaluate each node
result := p.checkPreemptionPredicates(predicateChecks, victimsByNode)
if result != nil && result.success {
return result.nodeID, result.instType, result.victims, true
}
return "", UnknownInstanceType, nil, false
}
```
which calls
```
// call predicates to evaluate each node
result := p.checkPreemptionPredicates(predicateChecks, victimsByNode)
if result != nil && result.success {
return result.nodeID, result.instType, result.victims, true
}
```
My understanding is that we choose an candidate node (after identifying preemption vicitims) for new allocation, so I need to make
` result := p.checkPreemptionPredicates(predicateChecks, victimsByNode)
`
populate the instanceType of the successfully chosen node candidate.
Wonder if we can have a screen share session for us to walk through a bit.
Thanks a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1202903294
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -658,6 +660,10 @@ func (pcr *predicateCheckResult) populateVictims(victimsByNode map[string][]*All
for i := 0; i <= pcr.index; i++ {
victim := victimList[i]
pcr.victims = append(pcr.victims, victim)
+ // Take the instance type info from the first victim Allocation of this given nodeID
Review Comment:
The down side with my current implementation is that whenever we create an Allocation object, we need to have both the nodeId and instType information available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190539127
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
+ defer urt.Unlock()
+ urm, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
HI @craigcondit , wonder if this would work:
I renamed UsedResourceTracker to UsedResource here, and added Clone() method in UsedResource,
func (ur *UsedResource) Clone() *UsedResource {
if ur == nil:
return nil
ret := NewUsedResource()
ur.RLock()
defer ur.RUnlock()
for k, v := range ur.UsedResourceMap {
sourceEntry := map[string]int64(v)
destEntry := make(map[string]int64)
for key, element := range sourceEntry {
destEntry[key] = element
}
ret.UsedResourceMap[k] = destEntry
}
return ret
}
func (sa *Application) GetApplicationSummary(rmID string) *ApplicationSummary {
sa.RLock()
defer sa.RUnlock()
appSummary := &ApplicationSummary{
ApplicationID: sa.ApplicationID,
SubmissionTime: sa.SubmissionTime,
StartTime: sa.startTime,
FinishTime: sa.finishedTime,
User: sa.user.User,
Queue: sa.queuePath,
State: sa.stateMachine.Current(),
RmID: rmID,
**ResourceUsage: sa.usedResource.Clone(),**
}
return appSummary
}
func (as *ApplicationSummary) GetReport() string {
res := "{"
res += "\"appId\":\"" + as.ApplicationID + "\","
res += "\"submissionTime\":\"" + strconv.FormatInt(as.SubmissionTime.UnixMilli(), 10) + "\","
res += "\"startTime\":\"" + strconv.FormatInt(as.StartTime.UnixMilli(), 10) + "\","
res += "\"finishTime\":\"" + strconv.FormatInt(as.FinishTime.UnixMilli(), 10) + "\","
res += "\"user\":\"" + as.User + "\","
res += "\"queue\":\"" + as.Queue + "\","
res += "\"state\":\"" + as.State + "\","
res += "\"rmID\":\"" + as.RmID + "\","
**res += "\"resourceUsage\":" + as.ResourceUsage.GetResourceUsageJson()**
res += "}"
return res
}
Above I introduced a GetResourceUsageJson() because I was doing different processing with the different fields you can see in the above method, which abstract what information we need to report in what format.
func (sa *Application) GetAppSummary(rmID string) string {
appSummary := sa.GetApplicationSummary(rmID)
ret := appSummary.GetReport()
appSummary.ResourceUsage = nil
return ret
And the final report is created with:
log.logger().Info(
zap.String("YK_APP_SUMMARY", app.GetAppSummary(pc.RmID)))
inside func (pc *PartitionContext) moveTerminatedApp(appID string) {
If user chooses to use log level higher than info, then, s/he may not get the report.
To be honest, we actually hope we can always report this without being impacted by the log level.
Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] yzhangal commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1191428958
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
+ curUsage, ok := aggregatedResourceTime[resourceSecondsKey]
+ if !ok {
+ curUsage = 0
+ }
+ curUsage += int64(element) * timeDiff // resource size times timeDiff
+ aggregatedResourceTime[resourceSecondsKey] = curUsage
+ }
+ urt.UsedResourceMap[instType] = aggregatedResourceTime
+}
+
+func (urt *UsedResourceTracker) GetResourceUsageSummary() string {
+ urt.Lock()
+ defer urt.Unlock()
+ urm, err := json.Marshal(urt.UsedResourceMap)
Review Comment:
Thanks Craig, I will give it a try.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190340525
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
+ if !ok {
+ aggregatedResourceTime = map[string]int64{}
+ }
+ for key, element := range resource.Resources {
+ resourceSecondsKey := key + "Seconds"
Review Comment:
Keep the names consistent as this way a downstream user doesn't need to subtract off "Seconds" to get the original resource type. It's simply unnecessary, and adds additional complexity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1190339993
##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,52 @@ func (q Quantity) string() string {
return strconv.FormatInt(int64(q), 10)
}
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+ // Two level map for aggregated resource usage
+ // With instance type being the top level key, the mapped value is a map:
+ // resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+ //
+ UsedResourceMap map[string]map[string]int64
+
+ sync.RWMutex
+}
+
+// Aggregate the resource usage to UsedResourceMap[instType]
+// The time the given resource used is the delta between the resource createTime and currentTime
+func (urt *UsedResourceTracker) AggregateUsedResource(instType string,
+ resource *Resource, createTime time.Time) {
+ urt.Lock()
+ defer urt.Unlock()
+
+ releaseTime := time.Now()
+ timeDiff := int64(releaseTime.Sub(createTime).Seconds())
+ aggregatedResourceTime, ok := urt.UsedResourceMap[instType]
Review Comment:
The core alone is needed here. We need to record a timestamp into the allocation at the same time the usage is added to the queue (i.e. when a node is assigned).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated resource consumption
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1188920805
##########
pkg/metrics/init.go:
##########
@@ -229,3 +236,7 @@ func formatMetricName(metricName string) string {
}
return string(newBytes)
}
+
+func (m *Metrics) LogAppSummary(appSummary string) {
+ log.Println(AppSummaryHeader + appSummary)
+}
Review Comment:
This needs to go through the zap Logger or we risk corrupting output. The standard log package goes directly to stdout/stderr and does not use a consistent format. The AppSummaryHeader should be a zap.String("key", "value") as well, this allows for JSON-filtering of the resulting message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org