-
Notifications
You must be signed in to change notification settings - Fork 668
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
Migrate RemovePodsHavingTooManyRestarts to plugin #902
Migrate RemovePodsHavingTooManyRestarts to plugin #902
Conversation
05dbfdc
to
17402cd
Compare
17402cd
to
cb972b5
Compare
@@ -41,6 +42,14 @@ func ValidateRemoveFailedPodsArgs(args *componentconfig.RemoveFailedPodsArgs) er | |||
) | |||
} | |||
|
|||
// ValidateRemovePodsHavingTooManyRestartsArgs validates RemovePodsHavingTooManyRestarts arguments | |||
func ValidateRemovePodsHavingTooManyRestartsArgs(args *componentconfig.RemovePodsHavingTooManyRestartsArgs) error { | |||
return errorsAggregate( |
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.
The validation is missing check for PodRestartThreshold
:
if args == nil || args.PodRestartThreshold < 1 {
return fmt.Errorf("PodsHavingTooManyRestarts threshold not set")
}
IncludingInitContainers: tooManyRestartsParams.IncludingInitContainers, | ||
} | ||
if err := validation.ValidateRemovePodsHavingTooManyRestartsArgs(args); err != nil { | ||
klog.V(1).ErrorS(err, "unable to validate plugin arguments", "pluginName", removefailedpods.PluginName) |
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.
s/removefailedpods/removepodshavingtoomanyrestarts
} | ||
|
||
if restarts < tooManyRestartsArgs.PodRestartThreshold { | ||
errs = append(errs, fmt.Errorf("pod's restarts %v is excluded", restarts)) |
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.
There's at most a single error. No need to store the error into an array:
var err error
...
err = fmt.Errorf("pod's restarts %v is excluded", restarts)
...
return err
will do as well.
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.
s/"pod's restarts %v is excluded"/"number of container restarts (%v) not exceeding the threshold"
nodes: []*v1.Node{node1}, | ||
expectedEvictedPodCount: 0, | ||
nodeFit: false, |
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.
This line can be omitted, false
is the default value for nodeFit
. The same for other lines.
test/e2e/e2e_toomanyrestarts_test.go
Outdated
|
||
// Run RemovePodsHavingTooManyRestarts strategy | ||
t.Log("Running RemovePodsHavingTooManyRestarts strategy") | ||
plugin.(framework.DeschedulePlugin).Deschedule(ctx, nodes) |
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.
s/nodes/workerNodes
test/e2e/e2e_toomanyrestarts_test.go
Outdated
defer close(stopCh) | ||
|
||
nodeList, err := clientSet.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) | ||
if err != nil { | ||
t.Errorf("Error listing node with %v", err) | ||
} | ||
|
||
nodes, workerNodes := splitNodesAndWorkerNodes(nodeList.Items) | ||
nodes, _ := splitNodesAndWorkerNodes(nodeList.Items) |
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.
It is still important to run the plugin over worker nodes only. Some of the control plane pod containers may get restarted too many times and evicted by the descheduler. Changing the assumption about the master nodes being protected against eviction and making the debugging step more complicated.
@BinacsLee thank You for helping us with the migration to plugins. It's great to see improvements in the code. |
cb972b5
to
1a05724
Compare
hi @ingvagabund , thanks for your review! I have fixed the code, PTAL. :) |
1a05724
to
3dbe0bf
Compare
@BinacsLee thank You for addressing the comments. Would you please rebase the PR as well? |
3dbe0bf
to
ca270db
Compare
@ingvagabund sure, rebase done. PTAL |
sorry, will fix the test soon |
ca270db
to
3a9c897
Compare
@@ -57,3 +57,13 @@ type RemovePodsViolatingNodeAffinityArgs struct { | |||
LabelSelector *metav1.LabelSelector | |||
NodeAffinityType []string | |||
} | |||
|
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.
Missing // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
line
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.
Thanks a lot! pull-descheduler-verify-master
can be passed now
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.
All tests passed :)
3a9c897
to
d798e7d
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund 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 |
No description provided.