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

Refactor most of `kubectl drain` as a library #72827

Merged
merged 1 commit into from Feb 25, 2019

Conversation

@errordeveloper
Copy link
Member

errordeveloper commented Jan 11, 2019

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Currently draining a node is only possible with kubectl drain. Other projects, e.g. cluster autoscaler and openshift have reinvented some of this code. This PR moves what can be moved easily from pkg/kubectl/cmd/drain into pkg/util/node/drain.

This is intended as a simple code move, there are no changes to in behaviour.

Does this PR introduce a user-facing change?:

NONE
return []corev1.Pod{}, errors.New(fs.Message())
}
if len(ws) > 0 {
fmt.Fprintf(o.ErrOut, "WARNING: %s\n", ws.Message())

This comment has been minimized.

@errordeveloper

errordeveloper Jan 11, 2019

Author Member

This is a little odd side-effect, I'll see if I can get rid of it easily.

@@ -640,7 +430,7 @@ func (o *DrainOptions) deletePods(pods []corev1.Pod, getPodFn func(namespace, na
return err
}

func (o *DrainOptions) waitForDelete(pods []corev1.Pod, interval, timeout time.Duration, usingEviction bool, getPodFn func(string, string) (*corev1.Pod, error)) ([]corev1.Pod, error) {
func (o *DrainCmdOptions) waitForDelete(pods []corev1.Pod, interval, timeout time.Duration, usingEviction bool, getPodFn func(string, string) (*corev1.Pod, error)) ([]corev1.Pod, error) {

This comment has been minimized.

@errordeveloper

errordeveloper Jan 11, 2019

Author Member

This function wants to write output a lot, I've not moved it cause I believe ideally libraries shouldn't write output. I've not figured out how alternative version of this would look like, and decide to not do that for the purpose of first PR.

This comment has been minimized.

@neolit123

neolit123 Jan 22, 2019

Member

backends shouldn't be necessary on mute; they should allow piping to a custom stream.

i see this is already possible using o.Out/ErrOut, yet if this is always printing to the standard streams and if there is no easy way to customize it, it needs a minor refactor.
but given o.Out/ErrOut exists i'd consider moving this function too.

This comment has been minimized.

@errordeveloper

errordeveloper Jan 23, 2019

Author Member

Yes, but the streams are genericclioptions.IOStreams, and those are not convenient to use outside of Kubernetes project itself, as an additional wrapping is needed to attach those to a logger or something else.

This comment has been minimized.

@neolit123

neolit123 Jan 23, 2019

Member

i see, so that's a problem, yes.
consumers should have an easy way to manage the streams.

This comment has been minimized.

@errordeveloper

errordeveloper Feb 13, 2019

Author Member

Yes, so I'm think of addressing this in a follow-up PR, what do you think?

This comment has been minimized.

@neolit123

neolit123 Feb 18, 2019

Member

sounds good!

@errordeveloper

This comment has been minimized.

Copy link
Member Author

errordeveloper commented Jan 11, 2019

/test pull-kubernetes-integration

@errordeveloper errordeveloper changed the title Refactor most of `kubectl drain` as a library WIP: Refactor most of `kubectl drain` as a library Jan 11, 2019

@wking

This comment has been minimized.

Copy link
Contributor

wking commented Jan 12, 2019

It's going to take me a bit to work through the diff of this vs. openshift/kubernetes-drain, but just comparing the APIs, it looks like you've decided to not include (un)cordon functions? Are you expecting to do that in follow-up work? It seems like they'd be important for folks trying to use a drain library.

@errordeveloper

This comment has been minimized.

Copy link
Member Author

errordeveloper commented Jan 12, 2019

APIs, it looks like you've decided to not include (un)cordon functions? Are you expecting to do that in follow-up work?

Yes, indeed I am intending to do that. I also need those myself, of course.

@errordeveloper

This comment has been minimized.

Copy link
Member Author

errordeveloper commented Jan 12, 2019

It's going to take me a bit to work through the diff of this vs. openshift/kubernetes-drain

Have you tried to compare it to what's in master here in k/k now?

@wking

This comment has been minimized.

Copy link
Contributor

wking commented Jan 12, 2019

It's going to take me a bit to work through the diff of this vs. openshift/kubernetes-drain

Have you tried to compare it to what's in master here in k/k now?

From openshift/kubernetes-drain@2b21795, you can see that I split off from the then-current d43e1b3. Changes since then:

$ git describe --always origin/master
v1.14.0-alpha.0-1656-gdc6f3d6
$ git log --oneline --decorate d43e1b3..dc6f3d6 -- pkg/kubectl/cmd/drain.go
33adf36 Move each kubectl command to a separate directory
97b2992 Update gofmt for go1.11
fecb5ed Merge pull request #66266 from wking/kubectl-drain-drop-backOff
686f29f (origin/pr/69438) Merge pull request #66301 from wking/kubectl-drain-drop-typer
f008365 Merge pull request #68806 from seans3/legacy-scheme-update
ab993e3 (origin/pr/66266) kubectl: Drop backOff from DrainOptions
7a2a987 Move legacyscheme (internal version) to kubectl scheme (external version)
452615c (origin/pr/68767) Fix drain for evicting terminal DS pods and pods with local storage
967280b (origin/pr/68069) Add --server-dry-run flag to `kubectl apply`
5b55e1f (origin/pr/67658) Create cli-runtime staging repository
0d81e83 (origin/pr/66301) kubectl: Drop typer from DrainOptions

#66266 and #66301 ported some of my downstream changes back upstream here. I'll take a close look at #68767, #68806, and the others when I get a moment.

@errordeveloper errordeveloper force-pushed the errordeveloper:drain-pkg branch 4 times, most recently from 6c1298a to 3a1dbf4 Jan 12, 2019

@errordeveloper errordeveloper force-pushed the errordeveloper:drain-pkg branch from 3a1dbf4 to 5a40f5a Jan 12, 2019

@errordeveloper

This comment was marked as resolved.

Copy link
Member Author

errordeveloper commented Jan 14, 2019

I am really not sure why this error appears in tests:

            error: unable to cordon node "node": no kind "Node" is registered for the internal version of group "" in scheme "pkg/kubectl/scheme/scheme.go:28"

I assume it breaks tests as output has changed, but I worry that the above error was there before this change.

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Jan 14, 2019

/priority important-longterm

@errordeveloper

This comment has been minimized.

Copy link
Member Author

errordeveloper commented Feb 19, 2019

Hi @soltysh! I certainly had no intention of destroying any kind of progress. Let's find a better approach to this, I'm happy to connect here or on Slack. Or, if you insist, in a SIG meeting.

@errordeveloper errordeveloper force-pushed the errordeveloper:drain-pkg branch from 37abbf0 to d7faf9d Feb 19, 2019

@errordeveloper

This comment has been minimized.

Copy link
Member Author

errordeveloper commented Feb 19, 2019

/assign @soltysh

As we discussed on Slack, I've moved the new package under pkg/kubectl/drain (so that when kubectl gets moved later, this doesn't get in the way).
If there is anything else that I should do, please let me know. The PR still only moves parts of the code, and it seems plausible to me to finish it all up in a follow-up PR (possibly even more than one). With regards to tests, there aren't any tests yet for the new package, it's only covered by existing unit tests in pkg/kubectl/cmd/drain and whatever e2e tests there're that cover kubectl drain functionality.

@errordeveloper

This comment was marked as outdated.

Copy link
Member Author

errordeveloper commented Feb 19, 2019

@errordeveloper errordeveloper force-pushed the errordeveloper:drain-pkg branch 3 times, most recently from aa6765f to 87ccb28 Feb 20, 2019

@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented Feb 20, 2019

/hold cancel

@errordeveloper errordeveloper force-pushed the errordeveloper:drain-pkg branch 2 times, most recently from e8efd6d to 5f2db94 Feb 20, 2019

@soltysh
Copy link
Contributor

soltysh left a comment

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 22, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: errordeveloper, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented Feb 22, 2019

Please squash your changes and I'll push that forward.

Refactor core functionality of `kubectl drain` as a library
- structured pod filter functions
- naming improvements
  - consistent use of daemonSets and DaemonSets
  - rename field to reflect its usage
- new cordon/uncordon helper
  - use Core API client direcly instead of generic CLI runtime

@errordeveloper errordeveloper force-pushed the errordeveloper:drain-pkg branch from 5f2db94 to 8c09a71 Feb 25, 2019

@errordeveloper

This comment has been minimized.

Copy link
Member Author

errordeveloper commented Feb 25, 2019

@soltysh thanks a lot! I've squashed it into one commit.

@awh

This comment has been minimized.

Copy link
Contributor

awh commented Feb 25, 2019

Nice work @errordeveloper - once I can use this in kured I no longer have to include kubectl in the image, which will reduce its size significantly.

@soltysh
Copy link
Contributor

soltysh left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 25, 2019

@k8s-ci-robot k8s-ci-robot merged commit 3b11f95 into kubernetes:master Feb 25, 2019

16 checks passed

cla/linuxfoundation errordeveloper authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@errordeveloper errordeveloper deleted the errordeveloper:drain-pkg branch Feb 25, 2019

if podOk {
pods = append(pods, pod)
if newErrs != nil {
fmt.Fprintf(o.ErrOut, "following errors also occurred:\n%s", utilerrors.NewAggregate(newErrs))

This comment has been minimized.

@tedyu

tedyu Feb 25, 2019

Contributor

Should newErrs be aggregated into err ?

This comment has been minimized.

@errordeveloper

errordeveloper Feb 25, 2019

Author Member

You mean from the library call? That can be discussed. There are still quite a few improvements to be made.

This comment has been minimized.

@tedyu

tedyu Feb 25, 2019

Contributor

What do you think of the following ?

diff --git a/pkg/kubectl/cmd/drain/drain.go b/pkg/kubectl/cmd/drain/drain.go
index 8e98e84ee6..76beaef2cf 100644
--- a/pkg/kubectl/cmd/drain/drain.go
+++ b/pkg/kubectl/cmd/drain/drain.go
@@ -322,6 +322,8 @@ func (o *DrainCmdOptions) deleteOrEvictPodsSimple(nodeInfo *resource.Info) error
                }
                if newErrs != nil {
                        fmt.Fprintf(o.ErrOut, "following errors also occurred:\n%s", utilerrors.NewAggregate(newErrs))
+                       newErrs = append(newErrs, err)
+                       err = utilerrors.NewAggregate(newErrs)
                }
                return err
        }

This comment has been minimized.

@errordeveloper

errordeveloper Feb 25, 2019

Author Member

It could be good, but I am not entirely sure, as the intention here was to avoid drastic changes of kubectl output, or general behaviour. This code originally returned the first error and printed new errors, so I kept it the same way. Not say it has to be that way, please open another PR if you strongly believe this must change. Small PR would be easy to discuss.

@errordeveloper errordeveloper referenced this pull request Feb 28, 2019

Merged

Add copy of `k8s.io/pkg/kubectl/drain` #589

2 of 2 tasks complete

errordeveloper added a commit to weaveworks/eksctl that referenced this pull request Feb 28, 2019

Copy drain package from errordeveloper/kubernetes@230da79
We have to copy the package, as there are conflicting dependencies
due to the fact that the package is in k/k@master.

This is based on kubernetes/kubernetes#72827 with a few additional
improvements (new PR pending).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.