Ensure euqiv hash calculation is per schedule #59245
Conversation
resouer
added
the
sig/scheduling
k8s-ci-robot
added
release-note
| @@ -404,22 +430,18 @@ func podFitsOnNode( | |||
| ecache *EquivalenceCache, | |||
| queue SchedulingQueue, | |||
| alwaysCheckAllPredicates bool, | |||
| equivCacheInfo equivalenceClasseInfo, | |||
There was a problem hiding this comment.
I think a pointer to the struct would be better. Then you can pass nil instead of empty struct when no equivalence hash is available. eCacheAvailable should be changed to:
eCacheAvailable = equivCacheInfo != nil && equivCacheInfo.isAvailable && !podsAdded
There was a problem hiding this comment.
Aha, I was intentionally use struct to make this eCacheAvailable line here simpler, never mind, let's use pointer.
| equivCacheInfo := equivalenceClasseInfo{} | ||
| if ecache != nil { | ||
| // getHashEquivalencePod will return immediately if no equivalence pod found | ||
| hash, available := ecache.getHashEquivalencePod(pod) |
There was a problem hiding this comment.
nit: you can directly assign to struct members.
| @@ -388,6 +406,14 @@ func addNominatedPods(podPriority int32, meta algorithm.PredicateMetadata, | |||
| return true, metaOut, nodeInfoOut | |||
| } | |||
|
|
|||
| // equivalenceClasseInfo holds information of equivalence cache for specific pod. | |||
| type equivalenceClasseInfo struct { | |||
There was a problem hiding this comment.
Is it a typo error? equivalenceClasseInfo -> equivalenceClassInfo
| @@ -388,7 +404,18 @@ func addNominatedPods(podPriority int32, meta algorithm.PredicateMetadata, | |||
| return true, metaOut, nodeInfoOut | |||
| } | |||
|
|
|||
| // equivalenceClassInfo holds information of equivalence cache for specific pod. | |||
| // We will pass this to podFitsOnNode to ensure equivalence hash is only calculated per schedule. | |||
| type equivalenceClassInfo struct { | |||
There was a problem hiding this comment.
Could you please move this to equivalence_cache.go and change getHashEquivalencePod to return a pointer to this struct?
There was a problem hiding this comment.
Fixed. In this case, we can use getHashEquivalencePod() return nil or not as available flag.
| @@ -312,9 +312,25 @@ func findNodesThatFit( | |||
|
|
|||
| // We can use the same metadata producer for all nodes. | |||
| meta := metadataProducer(pod, nodeNameToInfo) | |||
|
|
|||
| equivCacheInfo := &equivalenceClassInfo{} | |||
There was a problem hiding this comment.
If you decided to follow my other suggestion, you could write this as:
var equivCacheInfo *equivalenceClassInfo
if eCached != nil {
// getHashEquivalencePod will return immediately if no equivalence pod found
equivCacheInfo = ecache.getHashEquivalencePod(pod)
}
|
kindly ping @bsalamat back for review :) |
| // 1. equivalenceHash | ||
| // 2. if equivalence hash is valid | ||
| func (ec *EquivalenceCache) getHashEquivalencePod(pod *v1.Pod) (uint64, bool) { | ||
| // equivalenceClassInfo holds information of equivalence cache for specific pod. |
There was a problem hiding this comment.
nit: equivalenceClassInfo holds equivalence hash which is used for checking equivalence cache.
|
LGTM. Just a nit. |
|
nits fixed, thanks for quick response. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, resouer The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-ci-robot
added
the
approved
|
Automatic merge from submit-queue (batch tested with PRs 59394, 58769, 59423, 59363, 59245). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Currently, equiv hash is calculated per schedule, but also, per node. This is a potential cause of dragging integration test, see #58881
We should ensure this only happens once during scheduling of specific pod no matter how many nodes we have.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #58989
Special notes for your reviewer:
Release note: