-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Adding more callbacks in kubedrain helper #117502
Adding more callbacks in kubedrain helper #117502
Conversation
Welcome @adilGhaffarDev! |
Hi @adilGhaffarDev. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/hold |
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
cef5e21
to
85eb2c8
Compare
/unhold |
a6e1b62
to
1c5a4d0
Compare
1c5a4d0
to
94187dc
Compare
/test pull-kubernetes-node-e2e-containerd |
/triage accepted |
@@ -274,8 +282,14 @@ func (d *Helper) evictPods(pods []corev1.Pod, evictionGroupVersion schema.GroupV | |||
for { | |||
switch d.DryRunStrategy { | |||
case cmdutil.DryRunServer: | |||
if d.OnPodDeletionOrEvictionStarted != nil { |
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.
Are you sure you want to have the hook invoked even in dry-run mode? Even if it's a server one.
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.
other hooks are not invoked in DryRunServer, I will remove this one too.
@@ -157,6 +158,8 @@ func NewDrainCmdOptions(f cmdutil.Factory, ioStreams genericiooptions.IOStreams) | |||
}, | |||
} | |||
o.drainer.OnPodDeletedOrEvicted = o.onPodDeletedOrEvicted |
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'd expect drain command not to use the old methods, but only the new hooks. Especially that if you have both OnPodDeletedOrEvicted
and OnPodDeletionOrEvictionFinished
defined, only the latter is being used, 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.
With your current implementation if I invoke kubectl drain -v=2
you'll get pod printed more than necessary. I don't think that's necessary, I'd like to maintain the current behavior of the drain command, so you need to make sure that whatever changes under the covers the command invocation stays the same.
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.
checking verbosity level while creating NewDrainCmdOptions
should fix this right? like this:
func NewDrainCmdOptions(f cmdutil.Factory, ioStreams genericiooptions.IOStreams) *DrainCmdOptions {
o := &DrainCmdOptions{
PrintFlags: genericclioptions.NewPrintFlags("drained").WithTypeSetter(scheme.Scheme),
IOStreams: ioStreams,
drainer: &drain.Helper{
GracePeriodSeconds: -1,
Out: ioStreams.Out,
ErrOut: ioStreams.ErrOut,
ChunkSize: cmdutil.DefaultChunkSize,
},
}
if !klog.V(2).Enabled() {
o.drainer.OnPodDeletedOrEvicted = o.onPodDeletedOrEvicted
} else {
o.drainer.OnPodDeletionOrEvictionFinished = o.onPodDeletionOrEvictionFinished
o.drainer.OnPodDeletionOrEvictionStarted = o.onPodDeletionOrEvictionStarted
}
return o
}
7640df6
to
95f90d7
Compare
95f90d7
to
0323fe4
Compare
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.
/lgtm
/approve
LGTM label has been added. Git tree hash: ac6af520f274643dfcebbd6abef2c8dd63b06b4e
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adilGhaffarDev, soltysh 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 |
Changelog suggestion - Added kubectl node drain helper callbacks `OnPodDeletionOrEvictionStarted` and `OnPodDeletionOrEvictionFailed`; people extending `kubectl` can use these new callbacks for more granularity.
- Deprecated the `OnPodDeletedOrEvicted` node drain helper callback. |
@sftim should I create separate PR for this? |
You can edit the PR description for this PR @adilGhaffarDev |
can we cherrypick this to v1.28? |
What type of PR is this?
/kind feature
Which issue(s) this PR fixes:
Fixes #116210
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: