From 49f840d934c000a2799f475984d18587086a62c8 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 1 Apr 2021 22:50:59 -0400 Subject: [PATCH] kubectl: send policy/v1 evictions to servers that support it Kubernetes-commit: f07fc213b07568442b22757b2220b80a5041cced --- pkg/cmd/drain/drain_test.go | 11 ++--- pkg/cmd/testing/fake.go | 1 + pkg/drain/drain.go | 80 ++++++++++++++++++------------------- pkg/drain/drain_test.go | 66 ++++++++++++++++++++---------- 4 files changed, 90 insertions(+), 68 deletions(-) diff --git a/pkg/cmd/drain/drain_test.go b/pkg/cmd/drain/drain_test.go index 34568342..eacef7fd 100644 --- a/pkg/cmd/drain/drain_test.go +++ b/pkg/cmd/drain/drain_test.go @@ -34,7 +34,6 @@ import ( appsv1 "k8s.io/api/apps/v1" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" - policyv1beta1 "k8s.io/api/policy/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -767,7 +766,7 @@ func TestDrain(t *testing.T) { { Name: "policy", PreferredVersion: metav1.GroupVersionForDiscovery{ - GroupVersion: "policy/v1beta1", + GroupVersion: "policy/v1", }, }, }, @@ -780,8 +779,10 @@ func TestDrain(t *testing.T) { if testEviction { resourceList.APIResources = []metav1.APIResource{ { - Name: drain.EvictionSubresource, - Kind: drain.EvictionKind, + Name: drain.EvictionSubresource, + Kind: drain.EvictionKind, + Group: "policy", + Version: "v1", }, } } @@ -849,7 +850,7 @@ func TestDrain(t *testing.T) { if test.failUponEvictionOrDeletion { return nil, errors.New("request failed") } - return &http.Response{StatusCode: http.StatusCreated, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &policyv1beta1.Eviction{})}, nil + return &http.Response{StatusCode: http.StatusCreated, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &metav1.Status{})}, nil default: t.Fatalf("%s: unexpected request: %v %#v\n%#v", test.description, req.Method, req.URL, req) return nil, nil diff --git a/pkg/cmd/testing/fake.go b/pkg/cmd/testing/fake.go index db0ac18b..0fc3c86f 100644 --- a/pkg/cmd/testing/fake.go +++ b/pkg/cmd/testing/fake.go @@ -565,6 +565,7 @@ func (f *TestFactory) KubernetesClientSet() (*kubernetes.Clientset, error) { clientset.AppsV1beta2().RESTClient().(*restclient.RESTClient).Client = fakeClient.Client clientset.AppsV1().RESTClient().(*restclient.RESTClient).Client = fakeClient.Client clientset.PolicyV1beta1().RESTClient().(*restclient.RESTClient).Client = fakeClient.Client + clientset.PolicyV1().RESTClient().(*restclient.RESTClient).Client = fakeClient.Client clientset.DiscoveryClient.RESTClient().(*restclient.RESTClient).Client = fakeClient.Client return clientset, nil diff --git a/pkg/drain/drain.go b/pkg/drain/drain.go index 26585814..84b9eb2b 100644 --- a/pkg/drain/drain.go +++ b/pkg/drain/drain.go @@ -24,12 +24,14 @@ import ( "time" corev1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" policyv1beta1 "k8s.io/api/policy/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/cli-runtime/pkg/resource" @@ -103,35 +105,21 @@ type waitForDeleteParams struct { // CheckEvictionSupport uses Discovery API to find out if the server support // eviction subresource If support, it will return its groupVersion; Otherwise, -// it will return an empty string -func CheckEvictionSupport(clientset kubernetes.Interface) (string, error) { +// it will return an empty GroupVersion +func CheckEvictionSupport(clientset kubernetes.Interface) (schema.GroupVersion, error) { discoveryClient := clientset.Discovery() - groupList, err := discoveryClient.ServerGroups() - if err != nil { - return "", err - } - foundPolicyGroup := false - var policyGroupVersion string - for _, group := range groupList.Groups { - if group.Name == "policy" { - foundPolicyGroup = true - policyGroupVersion = group.PreferredVersion.GroupVersion - break - } - } - if !foundPolicyGroup { - return "", nil - } + + // version info available in subresources since v1.8.0 in https://github.com/kubernetes/kubernetes/pull/49971 resourceList, err := discoveryClient.ServerResourcesForGroupVersion("v1") if err != nil { - return "", err + return schema.GroupVersion{}, err } for _, resource := range resourceList.APIResources { - if resource.Name == EvictionSubresource && resource.Kind == EvictionKind { - return policyGroupVersion, nil + if resource.Name == EvictionSubresource && resource.Kind == EvictionKind && len(resource.Group) > 0 && len(resource.Version) > 0 { + return schema.GroupVersion{Group: resource.Group, Version: resource.Version}, nil } } - return "", nil + return schema.GroupVersion{}, nil } func (d *Helper) makeDeleteOptions() metav1.DeleteOptions { @@ -157,7 +145,7 @@ func (d *Helper) DeletePod(pod corev1.Pod) error { } // EvictPod will evict the give pod, or return an error if it couldn't -func (d *Helper) EvictPod(pod corev1.Pod, policyGroupVersion string) error { +func (d *Helper) EvictPod(pod corev1.Pod, evictionGroupVersion schema.GroupVersion) error { if d.DryRunStrategy == cmdutil.DryRunServer { if err := d.DryRunVerifier.HasSupport(pod.GroupVersionKind()); err != nil { return err @@ -165,20 +153,30 @@ func (d *Helper) EvictPod(pod corev1.Pod, policyGroupVersion string) error { } delOpts := d.makeDeleteOptions() - eviction := &policyv1beta1.Eviction{ - TypeMeta: metav1.TypeMeta{ - APIVersion: policyGroupVersion, - Kind: EvictionKind, - }, - ObjectMeta: metav1.ObjectMeta{ - Name: pod.Name, - Namespace: pod.Namespace, - }, - DeleteOptions: &delOpts, - } - // Remember to change change the URL manipulation func when Eviction's version change - return d.Client.PolicyV1beta1().Evictions(eviction.Namespace).Evict(d.getContext(), eviction) + switch evictionGroupVersion { + case policyv1.SchemeGroupVersion: + // send policy/v1 if the server supports it + eviction := &policyv1.Eviction{ + ObjectMeta: metav1.ObjectMeta{ + Name: pod.Name, + Namespace: pod.Namespace, + }, + DeleteOptions: &delOpts, + } + return d.Client.PolicyV1().Evictions(eviction.Namespace).Evict(context.TODO(), eviction) + + default: + // otherwise, fall back to policy/v1beta1, supported by all servers that support the eviction subresource + eviction := &policyv1beta1.Eviction{ + ObjectMeta: metav1.ObjectMeta{ + Name: pod.Name, + Namespace: pod.Namespace, + }, + DeleteOptions: &delOpts, + } + return d.Client.PolicyV1beta1().Evictions(eviction.Namespace).Evict(context.TODO(), eviction) + } } // GetPodsForDeletion receives resource info for a node, and returns those pods as PodDeleteList, @@ -259,20 +257,20 @@ func (d *Helper) DeleteOrEvictPods(pods []corev1.Pod) error { } if !d.DisableEviction { - policyGroupVersion, err := CheckEvictionSupport(d.Client) + evictionGroupVersion, err := CheckEvictionSupport(d.Client) if err != nil { return err } - if len(policyGroupVersion) > 0 { - return d.evictPods(pods, policyGroupVersion, getPodFn) + if !evictionGroupVersion.Empty() { + return d.evictPods(pods, evictionGroupVersion, getPodFn) } } return d.deletePods(pods, getPodFn) } -func (d *Helper) evictPods(pods []corev1.Pod, policyGroupVersion string, getPodFn func(namespace, name string) (*corev1.Pod, error)) error { +func (d *Helper) evictPods(pods []corev1.Pod, evictionGroupVersion schema.GroupVersion, getPodFn func(namespace, name string) (*corev1.Pod, error)) error { returnCh := make(chan error, 1) // 0 timeout means infinite, we use MaxInt64 to represent it. var globalTimeout time.Duration @@ -313,7 +311,7 @@ func (d *Helper) evictPods(pods []corev1.Pod, policyGroupVersion string, getPodF refreshPod = false } - err := d.EvictPod(activePod, policyGroupVersion) + err := d.EvictPod(activePod, evictionGroupVersion) if err == nil { break } else if apierrors.IsNotFound(err) { diff --git a/pkg/drain/drain_test.go b/pkg/drain/drain_test.go index dc95e081..78efab1f 100644 --- a/pkg/drain/drain_test.go +++ b/pkg/drain/drain_test.go @@ -29,6 +29,7 @@ import ( "time" corev1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" policyv1beta1 "k8s.io/api/policy/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -214,13 +215,20 @@ func createPods(ifCreateNewPods bool) (map[string]corev1.Pod, []corev1.Pod) { return podMap, podSlice } +func addCoreNonEvictionSupport(t *testing.T, k *fake.Clientset) { + coreResources := &metav1.APIResourceList{ + GroupVersion: "v1", + } + k.Resources = append(k.Resources, coreResources) +} + // addEvictionSupport implements simple fake eviction support on the fake.Clientset -func addEvictionSupport(t *testing.T, k *fake.Clientset) { +func addEvictionSupport(t *testing.T, k *fake.Clientset, version string) { podsEviction := metav1.APIResource{ Name: "pods/eviction", Kind: "Eviction", - Group: "", - Version: "v1", + Group: "policy", + Version: version, } coreResources := &metav1.APIResourceList{ GroupVersion: "v1", @@ -238,13 +246,26 @@ func addEvictionSupport(t *testing.T, k *fake.Clientset) { return false, nil, nil } - eviction := *action.(ktest.CreateAction).GetObject().(*policyv1beta1.Eviction) + namespace := "" + name := "" + switch version { + case "v1": + eviction := *action.(ktest.CreateAction).GetObject().(*policyv1.Eviction) + namespace = eviction.Namespace + name = eviction.Name + case "v1beta1": + eviction := *action.(ktest.CreateAction).GetObject().(*policyv1beta1.Eviction) + namespace = eviction.Namespace + name = eviction.Name + default: + t.Errorf("unknown version %s", version) + } // Avoid the lock go func() { - err := k.CoreV1().Pods(eviction.Namespace).Delete(context.TODO(), eviction.Name, metav1.DeleteOptions{}) + err := k.CoreV1().Pods(namespace).Delete(context.TODO(), name, metav1.DeleteOptions{}) if err != nil { // Errorf because we can't call Fatalf from another goroutine - t.Errorf("failed to delete pod: %s/%s", eviction.Namespace, eviction.Name) + t.Errorf("failed to delete pod: %s/%s", namespace, name) } }() @@ -253,22 +274,23 @@ func addEvictionSupport(t *testing.T, k *fake.Clientset) { } func TestCheckEvictionSupport(t *testing.T) { - for _, evictionSupported := range []bool{true, false} { - evictionSupported := evictionSupported - t.Run(fmt.Sprintf("evictionSupported=%v", evictionSupported), + for _, evictionVersion := range []string{"", "v1", "v1beta1"} { + t.Run(fmt.Sprintf("evictionVersion=%v", evictionVersion), func(t *testing.T) { k := fake.NewSimpleClientset() - if evictionSupported { - addEvictionSupport(t, k) + if len(evictionVersion) > 0 { + addEvictionSupport(t, k, evictionVersion) + } else { + addCoreNonEvictionSupport(t, k) } apiGroup, err := CheckEvictionSupport(k) if err != nil { t.Fatalf("unexpected error: %v", err) } - expectedAPIGroup := "" - if evictionSupported { - expectedAPIGroup = "policy/v1" + expectedAPIGroup := schema.GroupVersion{} + if len(evictionVersion) > 0 { + expectedAPIGroup = schema.GroupVersion{Group: "policy", Version: evictionVersion} } if apiGroup != expectedAPIGroup { t.Fatalf("expected apigroup %q, actual=%q", expectedAPIGroup, apiGroup) @@ -312,7 +334,7 @@ func TestDeleteOrEvict(t *testing.T) { } // Create 4 pods, and try to remove the first 2 - var expectedEvictions []policyv1beta1.Eviction + var expectedEvictions []policyv1.Eviction var create []runtime.Object deletePods := []corev1.Pod{} for i := 1; i <= 4; i++ { @@ -325,9 +347,7 @@ func TestDeleteOrEvict(t *testing.T) { deletePods = append(deletePods, *pod) if tc.evictionSupported && !tc.disableEviction { - eviction := policyv1beta1.Eviction{} - eviction.Kind = "Eviction" - eviction.APIVersion = "policy/v1" + eviction := policyv1.Eviction{} eviction.Namespace = pod.Namespace eviction.Name = pod.Name @@ -344,7 +364,9 @@ func TestDeleteOrEvict(t *testing.T) { // Build the fake client k := fake.NewSimpleClientset(create...) if tc.evictionSupported { - addEvictionSupport(t, k) + addEvictionSupport(t, k, "v1") + } else { + addCoreNonEvictionSupport(t, k) } h.Client = k h.DisableEviction = tc.disableEviction @@ -372,19 +394,19 @@ func TestDeleteOrEvict(t *testing.T) { } // Test that pods were evicted as expected - var actualEvictions []policyv1beta1.Eviction + var actualEvictions []policyv1.Eviction for _, action := range k.Actions() { if action.GetVerb() != "create" || action.GetResource().Resource != "pods" || action.GetSubresource() != "eviction" { continue } - eviction := *action.(ktest.CreateAction).GetObject().(*policyv1beta1.Eviction) + eviction := *action.(ktest.CreateAction).GetObject().(*policyv1.Eviction) actualEvictions = append(actualEvictions, eviction) } sort.Slice(actualEvictions, func(i, j int) bool { return actualEvictions[i].Name < actualEvictions[j].Name }) if !reflect.DeepEqual(actualEvictions, expectedEvictions) { - t.Errorf("%s: unexpected evictions; actual %v; expected %v", tc.description, actualEvictions, expectedEvictions) + t.Errorf("%s: unexpected evictions; actual\n\t%v\nexpected\n\t%v", tc.description, actualEvictions, expectedEvictions) } }) }