-
Notifications
You must be signed in to change notification settings - Fork 669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for degradation #301
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,7 @@ func RemovePodsViolatingNodeAffinity(ctx context.Context, client clientset.Inter | |
|
||
for _, pod := range pods { | ||
if pod.Spec.Affinity != nil && pod.Spec.Affinity.NodeAffinity != nil && pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil { | ||
if !nodeutil.PodFitsCurrentNode(pod, node) && nodeutil.PodFitsAnyNode(pod, nodes) { | ||
if !nodeutil.PodFitsCurrentNode(pod, node) && (nodeutil.PodFitsAnyNode(pod, nodes) || podEvictor.DegradationAllowed == true) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't want to ignore |
||
klog.V(1).Infof("Evicting pod: %v", pod.Name) | ||
if _, err := podEvictor.EvictPod(ctx, pod, node); err != nil { | ||
klog.Errorf("Error evicting pod: (%#v)", err) | ||
|
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
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DegradationAllowed
field is not used anywhere inside PodEvictor methods. Is this PR still WIP?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's used by the eviction strategy, which is passed in the pod evictor. I opted to make this exportable so we could access the flag there, rather than having to modify every single eviction strategy callsite to add in another argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see:
podEvictor.DegradationAllowed
. It's not a good practice.However, it's the right thing to do from the maintainability perspective. If you need to access DegradationAllowed through
PodEvictor
, introducing new method is more practical than accessingDegradationAllowed
directly. Also,DegradationAllowed
is not related to evicting pod itself, rather to relaxing constraint of selecting pods to be evicted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that changing every function every time we add a new flag is worse from a maintainability point of view, but it's not my code base, so I'm happy to follow whatever convention people like. In terms of the degradation, you are correct, we are presently doing this through a global flag and applying it across all pods that satisfy the criteria. We could also consider pushing this down to the pod level through an additional annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. The
strategyFunction
data type is still evolving. I am fine with changing the signature [1] to:where
Options
can be defined as:as long as
Options
contains fields generic for any strategy.[1] https://github.com/kubernetes-sigs/descheduler/blob/master/pkg/descheduler/descheduler.go#L63
If I understand the intention correctly,
DegradationAllowed
is a strategy level configuration. As such, the option needs to be passed into the strategy before it's ran.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmundt if this is only used by the
nodeAffinity
(and, in your new strategynodeSelector
), does it have to be a global setting? It could just be aStrategyParam
field for those right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in the other PR (sorry for the run-around, I mention it here for posterity), we would also plan to evaluate it within
IsEvictable
in cases where we know the pod can be explicitly degraded. I'm therefore not sure that theStrategyParam
would be sufficient, unless we're also able to access this from the pod package somehow. In terms of the global setting, I don't mind if we get rid of this and use a Pod annotation or similar, the important thing is simply that we have a mechanism to degrade specific node-local Pods - I'll defer to you on whichever option you find more palatable! If we leave it as an annotation, we could presumably also leave it as aStrategyParam
, as the annotation could be tested independently withinIsEvictable
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that the existing
descheduler.alpha.kubernetes.io/evict
annotation could solve? It is already checked inIsEvictable
and effectively bypasses that check for specific pods.With the above annotation^ I think this is what you want. In the other PR thread you mentioned wanting to specifically bypass DaemonSets too, which that annotation supports.
Sorry if it seems like I'm being difficult, I just don't yet see the need to pass this information to every strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, for some reason when I first took a look at the evict annotation I missed this. I think this would do the job, yes.
No problem, it's not always obvious what the preferred direction is when twiddling in someone else's code base, and as it turns out, I misread what the evict annotation actually does, so I'm happy for another set of eyes while I come to grips with things.
If you're happy with the
StrategyParam
direction, I'll give this a go with the evict annotation and see how it goes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes let's do that -- I agree that there are cases when these affinity strategies may want to evict even if the pod can't fit anywhere else, so adding another Param for these strategies makes sense to me.
I'm also assuming that if you enable that Param the user must know what they are doing, because as @ingvagabund pointed out if you're not properly handling such an evict-at-all-costs case you will end up with stuck Pending pods. I don't think that's the descheduler's concern though (as long as the possibility is clearly documented)