Skip to content
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

pkg/drain: Provide hooks for intermediate eviction errors #1582

Open
ahmetb opened this issue Apr 5, 2024 · 2 comments
Open

pkg/drain: Provide hooks for intermediate eviction errors #1582

ahmetb opened this issue Apr 5, 2024 · 2 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/cli Categorizes an issue or PR as relevant to SIG CLI. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@ahmetb
Copy link
Member

ahmetb commented Apr 5, 2024

What would you like to be added

Currently https://pkg.go.dev/k8s.io/kubectl/pkg/drain offers hooks that callers can use to get notified when a Pod eviction is starting or finished. For example:

// OnPodDeletionOrEvictionFinished is called when a pod is eviction/deletetion is failed;
// for printing progress output
OnPodDeletionOrEvictionFinished func(pod *corev1.Pod, usingEviction bool, err error)

This method signature (and Godoc) made me think my code will get intermediate errors in the err variable that I can use to make decisions, or print progress.

However, that did not turn out to be the case.

Currently, the OnPodDeletionOrEvictionFinished() is only called when:

  1. Evict or Delete Pod call succeeded + polling for Pod to be deleted succeeds.
  2. Evict or Delete Pod call succeeded + there was an error in polling for waiting Pod to be fully deleted.

Proposal

Extend the semantics around OnPodDeletionOrEvictionFinished() hook to include intermediate errors.

Why is this needed

  1. If my code gets an err that is of code=429 (indicating PDB disallowing eviction), I can cancel() the ctx I passed to RunNodeDrain(ctx), because waiting for many more minutes and retrying in 5s likely won't fix the issue.

  2. The documentation of OnPodDeletionOrEvictionFinished() hook says for printing progress output. However, if the drain operation is currently failing due to a single Pod refusing to Evict (say, due to PDB), this can provide the callers with an option to print progress

    Currently, this hook only works for printing progress if the eviction is successful, which makes it basically the same as OnPodDeletedOrEvicted func(pod *corev1.Pod, usingEviction bool) hook.

/kind feature
/sig cli
/cc @adilGhaffarDev
/cc soltysh

@ahmetb ahmetb added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 5, 2024
@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 5, 2024
@ardaguclu
Copy link
Member

After looking at the signature of OnPodDeletionOrEvictionFinished, I've inferred the same that function also errors out intermediate errors. In my opinion, it makes sense to extend it, unless there won't be any problem in terms of backwards-compatibility. As far as I understand, err error in hook does not return anything?.

@mpuckett159
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/cli Categorizes an issue or PR as relevant to SIG CLI. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

4 participants