[jira] [Comment Edited] (YUNIKORN-2220) pod.DeepCopy() is called twice in Task

["Craig Condit (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/YUNIKORN-2220?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17791829#comment-17791829 ] 

Craig Condit edited comment on YUNIKORN-2220 at 11/30/23 11:33 PM:
-------------------------------------------------------------------

I don't think this is safe. The event recorder actually modifies the object passed to it, hence the need to defensively copy ahead of time. Since the events process asyncronously, we can't rely on the object being private without a clone.

In this case, there are two different events, so the object may be modified by two different goroutines. It's not safe to assume they will not collide.


was (Author: ccondit):
I don't think this is safe. The event recorder actually modifies the object passed to it, hence the need to defensively copy ahead of time. Since the events process asyncronously, we can't rely on the object being private without a clone.

> pod.DeepCopy() is called twice in Task
> --------------------------------------
>
>                 Key: YUNIKORN-2220
>                 URL: https://issues.apache.org/jira/browse/YUNIKORN-2220
>             Project: Apache YuniKorn
>          Issue Type: Improvement
>          Components: shim - kubernetes
>            Reporter: Peter Bacsko
>            Assignee: Peter Bacsko
>            Priority: Minor
>
> A small improvement is possible in {{task.go}}.
> In {{handleSubmitTaskEvent()}} and {{{}postTaskAllocated(){}}}, we call {{pod.DeepCopy()}} twice to avoid possible race conditions, but a single copy is enough. Once we have a copy, it's local to the method.
> {noformat}
> events.GetRecorder().Eventf(task.pod.DeepCopy(), nil, v1.EventTypeNormal, "Scheduling", "Scheduling",
> 		"%s is queued and waiting for allocation", task.alias)
> 	// if this task belongs to a task group, that means the app has gang scheduling enabled
> 	// in this case, post an event to indicate the task is being gang scheduled
> 	if !task.placeholder && task.taskGroupName != "" {
> 		events.GetRecorder().Eventf(task.pod.DeepCopy(), nil,
> 			v1.EventTypeNormal, "GangScheduling", "GangScheduling",
> 			"Pod belongs to the taskGroup %s, it will be scheduled as a gang member", task.taskGroupName) <-- second copy if GS is used
> 	}
> {noformat}
> {noformat}
> events.GetRecorder().Eventf(task.pod.DeepCopy(),
> 	nil, v1.EventTypeNormal, "Scheduled", "Scheduled",
> 	"Successfully assigned %s to node %s", task.alias, task.nodeName)
> ...
> events.GetRecorder().Eventf(task.pod.DeepCopy(), nil,
> 	v1.EventTypeNormal, "PodBindSuccessful", "PodBindSuccessful",
> 	"Pod %s is successfully bound to node %s", task.alias, task.nodeName)
> {noformat}



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: issues-help@yunikorn.apache.org