Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

Support Equivalence failure cache by template-uuid #538

Closed
k82cn opened this issue Jan 5, 2019 · 25 comments
Closed

Support Equivalence failure cache by template-uuid #538

k82cn opened this issue Jan 5, 2019 · 25 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.
Milestone

Comments

@k82cn
Copy link
Contributor

k82cn commented Jan 5, 2019

Is this a BUG REPORT or FEATURE REQUEST?:

/kind feature

Description:

For batch workload, there're several "similar" tasks in one job; if one task failed to fit one node, the other tasks will have the same result (except pod affinity/anti-affinity). In kube-batch, we don-t know which tasks are similar, but we can support customized indexed, e.g. an annotation of task template uuid.

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 5, 2019
@k82cn
Copy link
Contributor Author

k82cn commented Jan 5, 2019

xref kubernetes/kubernetes#72322

@k82cn k82cn added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jan 5, 2019
@k82cn k82cn added this to the v0.4 milestone Jan 5, 2019
@k82cn k82cn added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Jan 5, 2019
@k82cn
Copy link
Contributor Author

k82cn commented Jan 5, 2019

NOTE: the result of pod affinity/ant-affinity can not be re-used, let's handle other predicates in v0.4 .

@zionwu
Copy link
Contributor

zionwu commented Mar 12, 2019

I would like to work on this issue. Per my understanding, we have to wait for the upstream issue kubernetes/kubernetes#72322 to be fixed, right?

And the change will be adding a map to the session. The map will cache the result that if a template-uuid fits a node. The map will be used in actions like allocate to improve the performance.

@k82cn
Copy link
Contributor Author

k82cn commented Mar 12, 2019

It's not necessary to wait for kubernetes/kubernetes#72322 ; we can have that feature here, and propose it to upstream :)

@k82cn
Copy link
Contributor Author

k82cn commented Mar 12, 2019

And the change will be adding a map to the session. The map will cache the result that if a template-uuid fits a node. The map will be used in actions like allocate to improve the performance.

Yes, we need to add a map in session; and this map is used by PredicatesFn directly, we do not need to change any actions :)

@zionwu
Copy link
Contributor

zionwu commented Mar 12, 2019

we can have that feature here, and propose it to upstream

So we will calculate the pod-template-hash for the pod. How do we store the hash? I can come up with two approaches:

  1. Set it as the pod's annotation, and call API to update the pod, maybe in SchedulerCache.AddPod. So that we don't have to re-calculate the hash everytime.
  2. Add a new field in TaskInfo, calculate and set the field in NewTaskInfo.

Which one do you prefer? Or any better approach?

@k82cn k82cn modified the milestones: v0.4, v0.5 Mar 12, 2019
@k82cn
Copy link
Contributor Author

k82cn commented Mar 12, 2019

So we will calculate the pod-template-hash for the pod. How do we store the hash? I can come up with two approaches:

Calculating hash is really heavy, prefer to let operation/controller set an annotation for it. In kube-batch, only the pods from the same PodGroup will check eCache.

@k82cn
Copy link
Contributor Author

k82cn commented Mar 12, 2019

And we need to pay attention on pod affinity/anti-affinity.

@zionwu
Copy link
Contributor

zionwu commented Mar 13, 2019

let operation/controller set an annotation for it

Do you mean the controllers in controller manager?
If so then we will have two PR, one for controller manager, another for kube-batch, right?

@k82cn
Copy link
Contributor Author

k82cn commented Mar 13, 2019

If so then we will have two PR, one for controller manager, another for kube-batch, right?

nop, we only support this feature in kube-batch; other operator, e.g. tf-operator, will decide whether use it or not. And user can aslo add annotation in PodTemplate which is not convience.

For the proposal in upstream, I'd suggest to add this annotation into pod template in controller manager.

@zionwu
Copy link
Contributor

zionwu commented Mar 13, 2019

Ok, got it, thanks.
I will work on the PR and let you review it once it's ready.

@zionwu
Copy link
Contributor

zionwu commented Mar 13, 2019

/assign

@k8s-ci-robot
Copy link
Contributor

@zionwu: GitHub didn't allow me to assign the following users: zionwu.

Note that only kubernetes-sigs members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k82cn
Copy link
Contributor Author

k82cn commented Mar 13, 2019

@zionwu , thanks very much! Please continue your work :)

@k82cn k82cn removed the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Mar 13, 2019
@hex108
Copy link
Contributor

hex108 commented Apr 2, 2019

I think it is a good solution to Add a new field in TaskInfo, calculate and set the field in NewTaskInfo. Although Calculating hash is really heavy, we only calculates it once when we put it to TaskInfo.

@k82cn
Copy link
Contributor Author

k82cn commented Apr 2, 2019

regarding "hash", how to resolve conflict?

@hex108
Copy link
Contributor

hex108 commented Apr 2, 2019

@k82cn In my option the conflict is very rare, even it happens, kubelet could deal with it well.

@k82cn
Copy link
Contributor Author

k82cn commented Apr 2, 2019

kubelet could deal with it well.

if hash conflict, scheduler may not bind pod.

@hex108
Copy link
Contributor

hex108 commented Apr 2, 2019

kubelet could deal with it well.

if hash conflict, scheduler may not bind pod.

The hash is stored in TaskInfo, it is just used for equivalence cache. We do not need to add it to pod.

@k82cn
Copy link
Contributor Author

k82cn commented Apr 22, 2019

/sig apps

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Apr 22, 2019
@zionwu
Copy link
Contributor

zionwu commented Jul 20, 2019

@k82cn @hex108 Do we have a conclusion on how to get the hash? do we let controller to set it as annotation or calculate it by kube-batch?

I also checked the latest implementation of eCache in default scheduler, it is using the UID of pod's controller as hash :

func GetEquivHash(pod *v1.Pod) types.UID {
	ownerReferences := pod.GetOwnerReferences()
	if ownerReferences != nil {
		return ownerReferences[0].UID
	}

	// If pod's UID of controllerRef is nil, return nil.
	return ""
}

In my opinion this is also a good approach, even for controller with multiple pod specs, like tf-operator. For TFJob, if one of its pod specs is unschedulable, the whole job is unschedulable and we don't have to schedule other pod spec of this job.

What do you think of this approach?

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 18, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 17, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.
Projects
None yet
Development

No branches or pull requests

5 participants