nodetopologymatch: allow to track only pods with exclusive resources #590
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
|
/hold
|
k8s-ci-robot
added
the
do-not-merge/hold
|
/retest looks like a infra glitch |
ffromani
force-pushed
the
resync-exclusive-resources
branch
from
2a01ad5
to
f38583b
Compare
ffromani
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/hold cancel the PR is reviewable |
k8s-ci-robot
removed
the
do-not-merge/hold
ffromani
commented
May 24, 2023
|
/hold see inline comment |
k8s-ci-robot
added
the
do-not-merge/hold
PiotrProkop
reviewed
May 24, 2023
ffromani
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
back to WIP as I revisit the approach and address reviewer's comments |
ffromani
force-pushed
the
resync-exclusive-resources
branch
from
f38583b
to
c4ff2f7
Compare
k8s-ci-robot
added
size/XXL
ffromani
changed the title
ffromani
force-pushed
the
resync-exclusive-resources
branch
2 times, most recently
from
a77432b
to
c91ce9b
Compare
|
/hold cancel marking it WIP is enough. Still not reviewable atm (because still WIP) |
k8s-ci-robot
removed
the
do-not-merge/hold
The `InformerFromHandle` helper was meant to abstract the creation of pod(Shared)Informer and anything related (podLister) from the framework Handle. Having podLister instantiated outside is just a historical accident. Let's fix this with no intended change of behavior. Signed-off-by: Francesco Romani <fromani@redhat.com>
ffromani
force-pushed
the
resync-exclusive-resources
branch
from
91357c3
to
74ba154
Compare
add helper package to identify pods which require some specific resources, most notably exclusive resources (CPU, devices...). We have code already which depends on detecting this pods, and we want to add more logic depending on exclusive resources. Additionally, we add (much more) test coverage for this code. Signed-off-by: Francesco Romani <fromani@redhat.com>
"foreign pods" are pods handled by other schedulers, namely the primary default scheduler. "foreign pods" need to be tracked to make sure the overreserve cache is accurate and trusted. When a foreign pod is detected, the relevant node cache is marked untrusty and verified periodically using the standard resync method. Expose in the configuration the foreign pods setting. Previously, we didn't had this option in the configuration because the foreign pod detection was simple, thus always enabled when the cache was in turn enabled. Tuning the foreign pods detection (to reduce the scheduling latency) benefits now from configurability. The default is the existing behavior, and the cluster admin can now disable entirely the foreign pods detection (perhaps because the NRT plugin is consumed by the main/only scheduler, thus is redundant) or enable it only for pods which have exclusive resources, which is indeed a way to greatly reduce the resync churn and thus the scheduling latency. Finally, the last addition is the option to only consider pods which have exclusive resources assigned to them. This is because only pods which have exclusive resources assigned to them can have NUMA-affine resources, thus contribute to the overreserve accounting. If we ignore pods with non-exclusive resources, we can significantly reduce noise, churn and CPU time. Signed-off-by: Francesco Romani <fromani@redhat.com>
We need to add more fields to pick the right method to compute the PFP, so we replace `types.NamespacedName` with a custom `podData` struct, which will be a superset of the former. Signed-off-by: Francesco Romani <fromani@redhat.com>
ffromani
force-pushed
the
resync-exclusive-resources
branch
from
74ba154
to
fb7236b
Compare
ffromani
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/hold trying to avoid premature merges |
k8s-ci-robot
added
the
do-not-merge/hold
ffromani
force-pushed
the
resync-exclusive-resources
branch
from
fb7236b
to
48b753c
Compare
|
currently hitting: kubernetes/test-infra#29622 |
ffromani
force-pushed
the
resync-exclusive-resources
branch
from
48b753c
to
97728ec
Compare
|
/hold cancel all issues seems to be gone |
k8s-ci-robot
removed
the
do-not-merge/hold
PiotrProkop
reviewed
Jun 1, 2023
| // the provided Node Resource Topology object. | ||
| func podFingerprintForNodeTopology(nrt *topologyv1alpha2.NodeResourceTopology) string { | ||
| // the provided Node Resource Topology object. Returns the expected fingerprint and the method to compute it. | ||
| func podFingerprintForNodeTopology(nrt *topologyv1alpha2.NodeResourceTopology, method apiconfig.CacheResyncMethod) (string, apiconfig.CacheResyncMethod) { |
There was a problem hiding this comment.
maybe change signature to:
func podFingerprintForNodeTopology(nrt *topologyv1alpha2.NodeResourceTopology, method apiconfig.CacheResyncMethod) (string, bool) {and the second return param will be wantsOnlyExclRes.
Then when passing to checkPodFingerprintForNode we can pass only boolean and not pfpMethod
There was a problem hiding this comment.
good point. That was the original version of the code. I'm still torn between YAGNI and keep the signature simple (and don't leak apiconfig too much all around the codebase) and be generic and forward looking.
Now that you highlight this, I'm leaning towards reverting to YAGNI and implement your suggestion
When computing the expected node pod fingerprint (PFP), we now look for the attribute describing the computation method provided by the topology-updater agent. If not provided, we fall back to the previous backward-compatible method, which is consider all pods. If the method is supplied, the scheduler plugin now computes the PFP in the same way advertised as the node agent. Finally, in order to keep the user fully in control, add config tunables to force the global behavior of the resync loop. The cluster admin can let the plugin autodetect (default behavior) or force one of the existing methods. Signed-off-by: Francesco Romani <fromani@redhat.com>
Rewrite TestFingerprintFromNRT as tabular test. The test grown a bit wild and it's time to cleanup. No expected change (bar a slight increase by side effect) in coverage. Signed-off-by: Francesco Romani <fromani@redhat.com>
ffromani
force-pushed
the
resync-exclusive-resources
branch
from
97728ec
to
f5d7100
Compare
k8s-ci-robot
added
the
lgtm
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Optimize both the foreign pods detection and the overreserve resync loop considering only pods which have exclusive resources assigned (vs considering all of them) because only exclusive resources have NUMA affinity, so only those contribute to per-NUMA accounting. This reduces churn, noise and computing power required. This is also expected to improve the scheduling performance by reducing the time for the resync loop to converge
Which issue(s) this PR fixes:
Fixes N/A
Special notes for your reviewer:
Does this PR introduce a user-facing change?