From 76c38048594e453531a7f2b35e4bf13962fd1128 Mon Sep 17 00:00:00 2001 From: Benjamin Bauer Date: Thu, 20 Oct 2016 13:40:40 -0700 Subject: [PATCH] Made changes to DELETE API to let v1.DeleteOptions be passed in as a QueryParameter --- pkg/apiserver/api_installer.go | 13 ++- pkg/apiserver/apiserver_test.go | 78 +++++++++++++ pkg/apiserver/resthandler.go | 11 +- test/e2e/BUILD | 1 + test/e2e/pods.go | 189 ++++++++++++++++++++++++++++++++ test/test_owners.csv | 1 + 6 files changed, 289 insertions(+), 4 deletions(-) create mode 100644 test/e2e/pods.go diff --git a/pkg/apiserver/api_installer.go b/pkg/apiserver/api_installer.go index d90beb8c332..f0bc9417cdb 100644 --- a/pkg/apiserver/api_installer.go +++ b/pkg/apiserver/api_installer.go @@ -250,14 +250,15 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag return nil, err } + var versionedDeleteOptions runtime.Object var versionedDeleterObject interface{} switch { case isGracefulDeleter: - objectPtr, err := a.group.Creater.New(optionsExternalVersion.WithKind("DeleteOptions")) + versionedDeleteOptions, err = a.group.Creater.New(optionsExternalVersion.WithKind("DeleteOptions")) if err != nil { return nil, err } - versionedDeleterObject = indirectArbitraryPointer(objectPtr) + versionedDeleterObject = indirectArbitraryPointer(versionedDeleteOptions) isDeleter = true case isDeleter: gracefulDeleter = rest.GracefulDeleteAdapter{Deleter: deleter} @@ -642,6 +643,9 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag Returns(http.StatusOK, "OK", versionedStatus) if isGracefulDeleter { route.Reads(versionedDeleterObject) + if err := addObjectParams(ws, route, versionedDeleteOptions); err != nil { + return nil, err + } } addParams(route, action.Params) ws.Route(route) @@ -944,6 +948,11 @@ func addObjectParams(ws *restful.WebService, route *restful.RouteBuilder, obj in } switch sf.Type.Kind() { case reflect.Interface, reflect.Struct: + case reflect.Ptr: + if sf.Type.Elem().Kind() == reflect.Interface || sf.Type.Elem().Kind() == reflect.Struct { + continue + } + fallthrough default: jsonTag := sf.Tag.Get("json") if len(jsonTag) == 0 { diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go index e16987c85cb..23ccd3ae860 100644 --- a/pkg/apiserver/apiserver_test.go +++ b/pkg/apiserver/apiserver_test.go @@ -27,6 +27,7 @@ import ( "net/http/httptest" "net/url" "reflect" + "strconv" "strings" "sync" "testing" @@ -1988,6 +1989,83 @@ func TestDeleteWithOptions(t *testing.T) { } } +func TestDeleteWithOptionsQuery(t *testing.T) { + storage := map[string]rest.Storage{} + simpleStorage := SimpleRESTStorage{} + ID := "id" + storage["simple"] = &simpleStorage + handler := handle(storage) + server := httptest.NewServer(handler) + defer server.Close() + + grace := int64(300) + item := &api.DeleteOptions{ + GracePeriodSeconds: &grace, + } + + client := http.Client{} + request, err := http.NewRequest("DELETE", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/"+ID+"?gracePeriodSeconds="+strconv.FormatInt(grace, 10), nil) + res, err := client.Do(request) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if res.StatusCode != http.StatusOK { + t.Errorf("unexpected response: %s %#v", request.URL, res) + s, err := ioutil.ReadAll(res.Body) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + t.Logf(string(s)) + } + if simpleStorage.deleted != ID { + t.Errorf("Unexpected delete: %s, expected %s", simpleStorage.deleted, ID) + } + simpleStorage.deleteOptions.GetObjectKind().SetGroupVersionKind(unversioned.GroupVersionKind{}) + if !api.Semantic.DeepEqual(simpleStorage.deleteOptions, item) { + t.Errorf("unexpected delete options: %s", diff.ObjectDiff(simpleStorage.deleteOptions, item)) + } +} + +func TestDeleteWithOptionsQueryAndBody(t *testing.T) { + storage := map[string]rest.Storage{} + simpleStorage := SimpleRESTStorage{} + ID := "id" + storage["simple"] = &simpleStorage + handler := handle(storage) + server := httptest.NewServer(handler) + defer server.Close() + + grace := int64(300) + item := &api.DeleteOptions{ + GracePeriodSeconds: &grace, + } + body, err := runtime.Encode(codec, item) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + client := http.Client{} + request, err := http.NewRequest("DELETE", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/"+ID+"?gracePeriodSeconds="+strconv.FormatInt(grace+10, 10), bytes.NewReader(body)) + res, err := client.Do(request) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if res.StatusCode != http.StatusOK { + t.Errorf("unexpected response: %s %#v", request.URL, res) + s, err := ioutil.ReadAll(res.Body) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + t.Logf(string(s)) + } + if simpleStorage.deleted != ID { + t.Errorf("Unexpected delete: %s, expected %s", simpleStorage.deleted, ID) + } + simpleStorage.deleteOptions.GetObjectKind().SetGroupVersionKind(unversioned.GroupVersionKind{}) + if !api.Semantic.DeepEqual(simpleStorage.deleteOptions, item) { + t.Errorf("unexpected delete options: %s", diff.ObjectDiff(simpleStorage.deleteOptions, item)) + } +} + func TestLegacyDelete(t *testing.T) { storage := map[string]rest.Storage{} simpleStorage := SimpleRESTStorage{} diff --git a/pkg/apiserver/resthandler.go b/pkg/apiserver/resthandler.go index 4182547cef6..f59292861d7 100644 --- a/pkg/apiserver/resthandler.go +++ b/pkg/apiserver/resthandler.go @@ -739,7 +739,7 @@ func UpdateResource(r rest.Updater, scope RequestScope, typer runtime.ObjectType } // DeleteResource returns a function that will handle a resource deletion -func DeleteResource(r rest.GracefulDeleter, checkBody bool, scope RequestScope, admit admission.Interface) restful.RouteFunction { +func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope RequestScope, admit admission.Interface) restful.RouteFunction { return func(req *restful.Request, res *restful.Response) { // For performance tracking purposes. trace := util.NewTrace("Delete " + req.Request.URL.Path) @@ -759,7 +759,7 @@ func DeleteResource(r rest.GracefulDeleter, checkBody bool, scope RequestScope, ctx = api.WithNamespace(ctx, namespace) options := &api.DeleteOptions{} - if checkBody { + if allowsOptions { body, err := readBody(req.Request) if err != nil { scope.err(err, res.ResponseWriter, req.Request) @@ -781,6 +781,13 @@ func DeleteResource(r rest.GracefulDeleter, checkBody bool, scope RequestScope, scope.err(fmt.Errorf("decoded object cannot be converted to DeleteOptions"), res.ResponseWriter, req.Request) return } + } else { + if values := req.Request.URL.Query(); len(values) > 0 { + if err := scope.ParameterCodec.DecodeParameters(values, scope.Kind.GroupVersion(), options); err != nil { + scope.err(err, res.ResponseWriter, req.Request) + return + } + } } } diff --git a/test/e2e/BUILD b/test/e2e/BUILD index de447eef05f..ef42cf2136f 100644 --- a/test/e2e/BUILD +++ b/test/e2e/BUILD @@ -77,6 +77,7 @@ go_library( "persistent_volumes.go", "petset.go", "pod_gc.go", + "pods.go", "portforward.go", "pre_stop.go", "proxy.go", diff --git a/test/e2e/pods.go b/test/e2e/pods.go new file mode 100644 index 00000000000..fee5a4c9b47 --- /dev/null +++ b/test/e2e/pods.go @@ -0,0 +1,189 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package e2e + +import ( + "crypto/tls" + "fmt" + "net/http" + "regexp" + "strconv" + "time" + + "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/labels" + "k8s.io/kubernetes/pkg/util/uuid" + "k8s.io/kubernetes/pkg/util/wait" + "k8s.io/kubernetes/pkg/watch" + "k8s.io/kubernetes/test/e2e/framework" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = framework.KubeDescribe("Pods Delete Grace Period", func() { + f := framework.NewDefaultFramework("pods") + var podClient *framework.PodClient + BeforeEach(func() { + podClient = f.PodClient() + }) + It("should be submitted and removed [Conformance]", func() { + By("creating the pod") + name := "pod-submit-remove-" + string(uuid.NewUUID()) + value := strconv.Itoa(time.Now().Nanosecond()) + pod := &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: name, + Labels: map[string]string{ + "name": "foo", + "time": value, + }, + }, + Spec: api.PodSpec{ + Containers: []api.Container{ + { + Name: "nginx", + Image: "gcr.io/google_containers/nginx-slim:0.7", + }, + }, + }, + } + + By("setting up watch") + selector := labels.SelectorFromSet(labels.Set(map[string]string{"time": value})) + options := api.ListOptions{LabelSelector: selector} + pods, err := podClient.List(options) + Expect(err).NotTo(HaveOccurred(), "failed to query for pod") + Expect(len(pods.Items)).To(Equal(0)) + options = api.ListOptions{ + LabelSelector: selector, + ResourceVersion: pods.ListMeta.ResourceVersion, + } + w, err := podClient.Watch(options) + Expect(err).NotTo(HaveOccurred(), "failed to set up watch") + + By("submitting the pod to kubernetes") + podClient.Create(pod) + + By("verifying the pod is in kubernetes") + selector = labels.SelectorFromSet(labels.Set(map[string]string{"time": value})) + options = api.ListOptions{LabelSelector: selector} + pods, err = podClient.List(options) + Expect(err).NotTo(HaveOccurred(), "failed to query for pod") + Expect(len(pods.Items)).To(Equal(1)) + + By("verifying pod creation was observed") + select { + case event, _ := <-w.ResultChan(): + if event.Type != watch.Added { + framework.Failf("Failed to observe pod creation: %v", event) + } + case <-time.After(framework.PodStartTimeout): + Fail("Timeout while waiting for pod creation") + } + + // We need to wait for the pod to be running, otherwise the deletion + // may be carried out immediately rather than gracefully. + framework.ExpectNoError(f.WaitForPodRunning(pod.Name)) + // save the running pod + pod, err = podClient.Get(pod.Name) + Expect(err).NotTo(HaveOccurred(), "failed to GET scheduled pod") + + // start local proxy, so we can send graceful deletion over query string, rather than body parameter + cmd := framework.KubectlCmd("proxy", "-p", "0") + stdout, stderr, err := framework.StartCmdAndStreamOutput(cmd) + Expect(err).NotTo(HaveOccurred(), "failed to start up proxy") + defer stdout.Close() + defer stderr.Close() + defer framework.TryKill(cmd) + buf := make([]byte, 128) + var n int + n, err = stdout.Read(buf) + Expect(err).NotTo(HaveOccurred(), "failed to read from kubectl proxy stdout") + output := string(buf[:n]) + proxyRegexp := regexp.MustCompile("Starting to serve on 127.0.0.1:([0-9]+)") + match := proxyRegexp.FindStringSubmatch(output) + Expect(len(match)).To(Equal(2)) + port, err := strconv.Atoi(match[1]) + Expect(err).NotTo(HaveOccurred(), "failed to convert port into string") + + endpoint := fmt.Sprintf("http://localhost:%d/api/v1/namespaces/%s/pods/%s?gracePeriodSeconds=30", port, pod.Namespace, pod.Name) + tr := &http.Transport{ + TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + } + client := &http.Client{Transport: tr} + req, err := http.NewRequest("DELETE", endpoint, nil) + Expect(err).NotTo(HaveOccurred(), "failed to create http request") + + By("deleting the pod gracefully") + rsp, err := client.Do(req) + Expect(err).NotTo(HaveOccurred(), "failed to use http client to send delete") + + defer rsp.Body.Close() + + By("verifying the kubelet observed the termination notice") + Expect(wait.Poll(time.Second*5, time.Second*30, func() (bool, error) { + podList, err := framework.GetKubeletPods(f.ClientSet, pod.Spec.NodeName) + if err != nil { + framework.Logf("Unable to retrieve kubelet pods for node %v: %v", pod.Spec.NodeName, err) + return false, nil + } + for _, kubeletPod := range podList.Items { + if pod.Name != kubeletPod.Name { + continue + } + if kubeletPod.ObjectMeta.DeletionTimestamp == nil { + framework.Logf("deletion has not yet been observed") + return false, nil + } + return true, nil + } + framework.Logf("no pod exists with the name we were looking for, assuming the termination request was observed and completed") + return true, nil + })).NotTo(HaveOccurred(), "kubelet never observed the termination notice") + + By("verifying pod deletion was observed") + deleted := false + timeout := false + var lastPod *api.Pod + timer := time.After(30 * time.Second) + for !deleted && !timeout { + select { + case event, _ := <-w.ResultChan(): + if event.Type == watch.Deleted { + lastPod = event.Object.(*api.Pod) + deleted = true + } + case <-timer: + timeout = true + } + } + if !deleted { + Fail("Failed to observe pod deletion") + } + + Expect(lastPod.DeletionTimestamp).ToNot(BeNil()) + Expect(lastPod.Spec.TerminationGracePeriodSeconds).ToNot(BeZero()) + + selector = labels.SelectorFromSet(labels.Set(map[string]string{"time": value})) + options = api.ListOptions{LabelSelector: selector} + pods, err = podClient.List(options) + Expect(err).NotTo(HaveOccurred(), "failed to query for pods") + Expect(len(pods.Items)).To(Equal(0)) + + }) +}) diff --git a/test/test_owners.csv b/test/test_owners.csv index 11a376fc0d7..5737ed4bd33 100644 --- a/test/test_owners.csv +++ b/test/test_owners.csv @@ -303,6 +303,7 @@ Pet set recreate should recreate evicted statefulset,roberthbailey,1 "Pod Disks should schedule a pod w/ a readonly PD on two hosts, then remove both ungracefully.",saad-ali,1 "Pod Disks should schedule a pod w/two RW PDs both mounted to one container, write to PD, verify contents, delete pod, recreate pod, verify contents, and repeat in rapid succession",saad-ali,0 Pod garbage collector should handle the creation of 1000 pods,wojtek-t,1 +Pods Delete Grace Period should be submitted and removed,bdbauer,0 Pods should allow activeDeadlineSeconds to be updated,derekwaynecarr,0 Pods should be submitted and removed,davidopp,1 Pods should be updated,derekwaynecarr,1