Made changes to DELETE API to let v1.DeleteOptions be passed in as a QueryParameter

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 {

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{}

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
+					}
+				}
 			}
 		}
 

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",

test/e2e/pods.go

+/*
+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))
+
+	})
+})

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