Correctly Evict Pods For >= v1.16

[jameshopkins]

Issue #, if available

Fixes https://github.com/aws-samples/amazon-k8s-node-drainer/issues/27

Description of changes:

Allows for the eviction of pods for Kubernetes 1.16+. Functionally tested against a 1.16 cluster.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[svozza]

Thank you for this pull request. It looks good but I just have a question, would it not be simpler just to remove to the -eviction part of the name entirely regardless of version? It would save having check what version of k8s is running.

[svozza]

Ah I see that was the approach @thomasdupas took. Is there a downside I'm missing here (it's been a quite some time since I've worked with k8s)?

[jameshopkins]

... would it not be simpler just to remove to the -eviction part of the name entirely regardless of version?

If we were to do that, then this PR wouldn't be backwards compatible with =< 0.15, since those versions require the inclusion of -eviction in the eviction name itself.

[svozza]

Oh right, it's been so long since I wrote this code I thought I'd just randomly tacked that string on. If that's the case then I think we're good to merge.

Edit: Wait, are we sure? For example, look at this stack overflow question for EKS 1.10, there's no mention of this magic string:

https://stackoverflow.com/questions/53273108/kubernetes-python-eviction-api-complain-about-name-parameter-required

[jameshopkins]

Wait, are we sure? For example, look at this stack overflow question for EKS 1.10, there's no mention of this magic string.

The first argument in the function signature (create_namespaced_pod_eviction(name=podName...) referenced in the Stack Overflow answer, is the same as the 1.15 version in this project - i.e pod name.

[thomasdupas]

I was fairly convinced that it would work like that on 1.15, but now I'm doubting I'll spin up a 1.15 cluster quickly to check

[svozza]

Thank you very much Thomas!

[jameshopkins]

I've actually just pushed another commit that gets rid of one of the describe_cluster calls. Functionally tested on a 1.16 cluster without kubeconfig present

[thomasdupas]

verified on a 1.15 cluster (hooray for vagrant and a from-scratch kubernetes setup :-p), it doesn't care about the object name, only the content. So I don't think we have to care about the k8s version detection

root@master-1:~# kubectl version
Client Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.12", GitCommit:"e2a822d9f3c2fdb5c9bfbe64313cf9f657f0a725", GitTreeState:"clean", BuildDate:"2020-05-06T05:17:59Z", GoVersion:"go1.12.17", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.12", GitCommit:"e2a822d9f3c2fdb5c9bfbe64313cf9f657f0a725", GitTreeState:"clean", BuildDate:"2020-05-06T05:09:48Z", GoVersion:"go1.12.17", Compiler:"gc", Platform:"linux/amd64"}

root@master-1:~# cat eviction.json 
{
  "apiVersion": "policy/v1beta1",
  "kind": "Eviction",
  "metadata": {
    "name": "coredns-5d4dd4b4db-4f8qj",
    "namespace": "kube-system"
  }
}

root@master-1:~# curl -k -H 'Content-type: application/json' --cert certificate.crt --key certificate.key https://127.0.0.1:6443/api/v1/namespaces/kube-system/pods/corednsdadadas/eviction -d @eviction.json
{
  "kind": "Status",
  "apiVersion": "v1",
  "metadata": {

  },
  "status": "Success",
  "code": 201

[svozza]

Fantastic, I think this massively simplifies the PR.

[thomasdupas]

I can reopen the other PR (and alter the unit tests) if needed

[svozza]

Sounds good! Again thank you for your work on this @jameshopkins, it's greatly appreciated.

[thomasdupas]

I couldn't reopen #29 due to silly mistake (I rewrote the prior commit / force pushed, invalidating the PR), so it became #30

[jameshopkins]

Closed in favour of https://github.com/aws-samples/amazon-k8s-node-drainer/pull/30