From 4f363f54474bf363f0890c0c899dc331b7b34673 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 3 Nov 2016 23:08:56 -0400 Subject: [PATCH] SetSelfLink is inefficient Generating self links, especially for lists, is inefficient. Replace use of net.URL.String() with direct encoding that reduces number of allocations. Switch from calling meta.ExtractList|SetList to a function that iterates over each object in the list. In steady state for nodes performing frequently small get/list operations, and for larger LISTs significantly reduces CPU and allocations. --- pkg/api/meta/help.go | 51 ++++++ pkg/api/meta/help_test.go | 266 ++++++++++++++++++---------- pkg/apiserver/api_installer.go | 60 ++++--- pkg/apiserver/api_installer_test.go | 7 +- pkg/apiserver/resthandler.go | 46 ++--- pkg/apiserver/resthandler_test.go | 8 +- 6 files changed, 279 insertions(+), 159 deletions(-) diff --git a/pkg/api/meta/help.go b/pkg/api/meta/help.go index 0d733a58a09..9ca2871c308 100644 --- a/pkg/api/meta/help.go +++ b/pkg/api/meta/help.go @@ -57,6 +57,57 @@ func GetItemsPtr(list runtime.Object) (interface{}, error) { } } +// EachListItem invokes fn on each runtime.Object in the list. Any error immediately terminates +// the loop. +func EachListItem(obj runtime.Object, fn func(runtime.Object) error) error { + // TODO: Change to an interface call? + itemsPtr, err := GetItemsPtr(obj) + if err != nil { + return err + } + items, err := conversion.EnforcePtr(itemsPtr) + if err != nil { + return err + } + len := items.Len() + if len == 0 { + return nil + } + takeAddr := false + if elemType := items.Type().Elem(); elemType.Kind() != reflect.Ptr && elemType.Kind() != reflect.Interface { + if !items.Index(0).CanAddr() { + return fmt.Errorf("unable to take address of items in %T for EachListItem", obj) + } + takeAddr = true + } + + for i := 0; i < len; i++ { + raw := items.Index(i) + if takeAddr { + raw = raw.Addr() + } + switch item := raw.Interface().(type) { + case *runtime.RawExtension: + if err := fn(item.Object); err != nil { + return err + } + case runtime.Object: + if err := fn(item); err != nil { + return err + } + default: + obj, ok := item.(runtime.Object) + if !ok { + return fmt.Errorf("%v: item[%v]: Expected object, got %#v(%s)", obj, i, raw.Interface(), raw.Kind()) + } + if err := fn(obj); err != nil { + return err + } + } + } + return nil +} + // ExtractList returns obj's Items element as an array of runtime.Objects. // Returns an error if obj is not a List type (does not have an Items member). func ExtractList(obj runtime.Object) ([]runtime.Object, error) { diff --git a/pkg/api/meta/help_test.go b/pkg/api/meta/help_test.go index fe97d8b5a79..083556c44eb 100644 --- a/pkg/api/meta/help_test.go +++ b/pkg/api/meta/help_test.go @@ -46,94 +46,185 @@ func TestIsList(t *testing.T) { } func TestExtractList(t *testing.T) { - pl := &api.PodList{ - Items: []api.Pod{ - {ObjectMeta: api.ObjectMeta{Name: "1"}}, - {ObjectMeta: api.ObjectMeta{Name: "2"}}, - {ObjectMeta: api.ObjectMeta{Name: "3"}}, - }, + list1 := []runtime.Object{ + &api.Pod{ObjectMeta: api.ObjectMeta{Name: "1"}}, + &api.Service{ObjectMeta: api.ObjectMeta{Name: "2"}}, } - list, err := meta.ExtractList(pl) - if err != nil { - t.Fatalf("Unexpected error %v", err) - } - if e, a := len(list), len(pl.Items); e != a { - t.Fatalf("Expected %v, got %v", e, a) - } - for i := range list { - if e, a := list[i].(*api.Pod).Name, pl.Items[i].Name; e != a { - t.Fatalf("Expected %v, got %v", e, a) - } - } -} - -func TestExtractListV1(t *testing.T) { - pl := &v1.PodList{ - Items: []v1.Pod{ - {ObjectMeta: v1.ObjectMeta{Name: "1"}}, - {ObjectMeta: v1.ObjectMeta{Name: "2"}}, - {ObjectMeta: v1.ObjectMeta{Name: "3"}}, - }, - } - list, err := meta.ExtractList(pl) - if err != nil { - t.Fatalf("Unexpected error %v", err) - } - if e, a := len(list), len(pl.Items); e != a { - t.Fatalf("Expected %v, got %v", e, a) - } - for i := range list { - if e, a := list[i].(*v1.Pod).Name, pl.Items[i].Name; e != a { - t.Fatalf("Expected %v, got %v", e, a) - } - } -} - -func TestExtractListGeneric(t *testing.T) { - pl := &api.List{ - Items: []runtime.Object{ - &api.Pod{ObjectMeta: api.ObjectMeta{Name: "1"}}, - &api.Service{ObjectMeta: api.ObjectMeta{Name: "2"}}, - }, - } - list, err := meta.ExtractList(pl) - if err != nil { - t.Fatalf("Unexpected error %v", err) - } - if e, a := len(list), len(pl.Items); e != a { - t.Fatalf("Expected %v, got %v", e, a) - } - if obj, ok := list[0].(*api.Pod); !ok { - t.Fatalf("Expected list[0] to be *api.Pod, it is %#v", obj) - } - if obj, ok := list[1].(*api.Service); !ok { - t.Fatalf("Expected list[1] to be *api.Service, it is %#v", obj) - } -} - -func TestExtractListGenericV1(t *testing.T) { - pl := &v1.List{ + list2 := &v1.List{ Items: []runtime.RawExtension{ {Raw: []byte("foo")}, {Raw: []byte("bar")}, {Object: &v1.Pod{ObjectMeta: v1.ObjectMeta{Name: "other"}}}, }, } - list, err := meta.ExtractList(pl) - if err != nil { - t.Fatalf("Unexpected error %v", err) + list3 := &fakePtrValueList{ + Items: []*api.Pod{ + {ObjectMeta: api.ObjectMeta{Name: "1"}}, + {ObjectMeta: api.ObjectMeta{Name: "2"}}, + }, } - if e, a := len(list), len(pl.Items); e != a { - t.Fatalf("Expected %v, got %v", e, a) + list4 := &api.PodList{ + Items: []api.Pod{ + {ObjectMeta: api.ObjectMeta{Name: "1"}}, + {ObjectMeta: api.ObjectMeta{Name: "2"}}, + {ObjectMeta: api.ObjectMeta{Name: "3"}}, + }, } - if obj, ok := list[0].(*runtime.Unknown); !ok { - t.Fatalf("Expected list[0] to be *runtime.Unknown, it is %#v", obj) + list5 := &v1.PodList{ + Items: []v1.Pod{ + {ObjectMeta: v1.ObjectMeta{Name: "1"}}, + {ObjectMeta: v1.ObjectMeta{Name: "2"}}, + {ObjectMeta: v1.ObjectMeta{Name: "3"}}, + }, } - if obj, ok := list[1].(*runtime.Unknown); !ok { - t.Fatalf("Expected list[1] to be *runtime.Unknown, it is %#v", obj) + + testCases := []struct { + in runtime.Object + out []interface{} + equal bool + }{ + { + in: &api.List{}, + out: []interface{}{}, + }, + { + in: &v1.List{}, + out: []interface{}{}, + }, + { + in: &v1.PodList{}, + out: []interface{}{}, + }, + { + in: &api.List{Items: list1}, + out: []interface{}{list1[0], list1[1]}, + }, + { + in: list2, + out: []interface{}{&runtime.Unknown{Raw: list2.Items[0].Raw}, &runtime.Unknown{Raw: list2.Items[1].Raw}, list2.Items[2].Object}, + equal: true, + }, + { + in: list3, + out: []interface{}{list3.Items[0], list3.Items[1]}, + }, + { + in: list4, + out: []interface{}{&list4.Items[0], &list4.Items[1], &list4.Items[2]}, + }, + { + in: list5, + out: []interface{}{&list5.Items[0], &list5.Items[1], &list5.Items[2]}, + }, } - if obj, ok := list[2].(*v1.Pod); !ok { - t.Fatalf("Expected list[2] to be *runtime.Unknown, it is %#v", obj) + for i, test := range testCases { + list, err := meta.ExtractList(test.in) + if err != nil { + t.Fatalf("%d: extract: Unexpected error %v", i, err) + } + if e, a := len(test.out), len(list); e != a { + t.Fatalf("%d: extract: Expected %v, got %v", i, e, a) + } + for j, e := range test.out { + if e != list[j] { + if !test.equal { + t.Fatalf("%d: extract: Expected list[%d] to be %#v, but found %#v", i, j, e, list[j]) + } + if !reflect.DeepEqual(e, list[j]) { + t.Fatalf("%d: extract: Expected list[%d] to be %#v, but found %#v", i, j, e, list[j]) + } + } + } + } +} + +func TestEachListItem(t *testing.T) { + list1 := []runtime.Object{ + &api.Pod{ObjectMeta: api.ObjectMeta{Name: "1"}}, + &api.Service{ObjectMeta: api.ObjectMeta{Name: "2"}}, + } + list2 := &v1.List{ + Items: []runtime.RawExtension{ + {Raw: []byte("foo")}, + {Raw: []byte("bar")}, + {Object: &v1.Pod{ObjectMeta: v1.ObjectMeta{Name: "other"}}}, + }, + } + list3 := &fakePtrValueList{ + Items: []*api.Pod{ + {ObjectMeta: api.ObjectMeta{Name: "1"}}, + {ObjectMeta: api.ObjectMeta{Name: "2"}}, + }, + } + list4 := &api.PodList{ + Items: []api.Pod{ + {ObjectMeta: api.ObjectMeta{Name: "1"}}, + {ObjectMeta: api.ObjectMeta{Name: "2"}}, + {ObjectMeta: api.ObjectMeta{Name: "3"}}, + }, + } + list5 := &v1.PodList{ + Items: []v1.Pod{ + {ObjectMeta: v1.ObjectMeta{Name: "1"}}, + {ObjectMeta: v1.ObjectMeta{Name: "2"}}, + {ObjectMeta: v1.ObjectMeta{Name: "3"}}, + }, + } + + testCases := []struct { + in runtime.Object + out []interface{} + }{ + { + in: &api.List{}, + out: []interface{}{}, + }, + { + in: &v1.List{}, + out: []interface{}{}, + }, + { + in: &v1.PodList{}, + out: []interface{}{}, + }, + { + in: &api.List{Items: list1}, + out: []interface{}{list1[0], list1[1]}, + }, + { + in: list2, + out: []interface{}{nil, nil, list2.Items[2].Object}, + }, + { + in: list3, + out: []interface{}{list3.Items[0], list3.Items[1]}, + }, + { + in: list4, + out: []interface{}{&list4.Items[0], &list4.Items[1], &list4.Items[2]}, + }, + { + in: list5, + out: []interface{}{&list5.Items[0], &list5.Items[1], &list5.Items[2]}, + }, + } + for i, test := range testCases { + list := []runtime.Object{} + err := meta.EachListItem(test.in, func(obj runtime.Object) error { + list = append(list, obj) + return nil + }) + if err != nil { + t.Fatalf("%d: each: Unexpected error %v", i, err) + } + if e, a := len(test.out), len(list); e != a { + t.Fatalf("%d: each: Expected %v, got %v", i, e, a) + } + for j, e := range test.out { + if e != list[j] { + t.Fatalf("%d: each: Expected list[%d] to be %#v, but found %#v", i, j, e, list[j]) + } + } } } @@ -166,27 +257,6 @@ func (obj fakePtrValueList) GetObjectKind() unversioned.ObjectKind { return unversioned.EmptyObjectKind } -func TestExtractListOfValuePtrs(t *testing.T) { - pl := &fakePtrValueList{ - Items: []*api.Pod{ - {ObjectMeta: api.ObjectMeta{Name: "1"}}, - {ObjectMeta: api.ObjectMeta{Name: "2"}}, - }, - } - list, err := meta.ExtractList(pl) - if err != nil { - t.Fatalf("Unexpected error %v", err) - } - if e, a := len(list), len(pl.Items); e != a { - t.Fatalf("Expected %v, got %v", e, a) - } - for i := range list { - if obj, ok := list[i].(*api.Pod); !ok { - t.Fatalf("Expected list[%d] to be *api.Pod, it is %#v", i, obj) - } - } -} - func TestSetList(t *testing.T) { pl := &api.PodList{} list := []runtime.Object{ diff --git a/pkg/apiserver/api_installer.go b/pkg/apiserver/api_installer.go index d90beb8c332..02575b187dc 100644 --- a/pkg/apiserver/api_installer.go +++ b/pkg/apiserver/api_installer.go @@ -17,8 +17,10 @@ limitations under the License. package apiserver import ( + "bytes" "fmt" "net/http" + "net/url" gpath "path" "reflect" "sort" @@ -356,15 +358,17 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag itemPath := resourcePath + "/{name}" nameParams := append(params, nameParam) proxyParams := append(nameParams, pathParam) + suffix := "" if hasSubresource { - itemPath = itemPath + "/" + subresource + suffix = "/" + subresource + itemPath = itemPath + suffix resourcePath = itemPath resourceParams = nameParams } apiResource.Name = path apiResource.Namespaced = false apiResource.Kind = resourceKind - namer := rootScopeNaming{scope, a.group.Linker, gpath.Join(a.prefix, itemPath)} + namer := rootScopeNaming{scope, a.group.Linker, gpath.Join(a.prefix, resourcePath, "/"), suffix} // Handler for standard REST verbs (GET, PUT, POST and DELETE). // Add actions at the resource path: /api/apiVersion/resource @@ -416,8 +420,14 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag apiResource.Namespaced = true apiResource.Kind = resourceKind - itemPathFn := func(name, namespace string) string { - return itemPathPrefix + namespace + itemPathMiddle + name + itemPathSuffix + itemPathFn := func(name, namespace string) bytes.Buffer { + var buf bytes.Buffer + buf.WriteString(itemPathPrefix) + buf.WriteString(url.QueryEscape(namespace)) + buf.WriteString(itemPathMiddle) + buf.WriteString(url.QueryEscape(name)) + buf.WriteString(itemPathSuffix) + return buf } namer := scopeNaming{scope, a.group.Linker, itemPathFn, false} @@ -747,7 +757,8 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag type rootScopeNaming struct { scope meta.RESTScope runtime.SelfLinker - itemPath string + pathPrefix string + pathSuffix string } // rootScopeNaming implements ScopeNamer @@ -769,25 +780,26 @@ func (n rootScopeNaming) Name(req *restful.Request) (namespace, name string, err } // GenerateLink returns the appropriate path and query to locate an object by its canonical path. -func (n rootScopeNaming) GenerateLink(req *restful.Request, obj runtime.Object) (path, query string, err error) { +func (n rootScopeNaming) GenerateLink(req *restful.Request, obj runtime.Object) (uri string, err error) { _, name, err := n.ObjectName(obj) if err != nil { - return "", "", err + return "", err } if len(name) == 0 { _, name, err = n.Name(req) if err != nil { - return "", "", err + return "", err } } - path = strings.Replace(n.itemPath, "{name}", name, 1) - return path, "", nil + return n.pathPrefix + url.QueryEscape(name) + n.pathSuffix, nil } // GenerateListLink returns the appropriate path and query to locate a list by its canonical path. -func (n rootScopeNaming) GenerateListLink(req *restful.Request) (path, query string, err error) { - path = req.Request.URL.Path - return path, "", nil +func (n rootScopeNaming) GenerateListLink(req *restful.Request) (uri string, err error) { + if len(req.Request.URL.RawPath) > 0 { + return req.Request.URL.RawPath, nil + } + return req.Request.URL.EscapedPath(), nil } // ObjectName returns the name set on the object, or an error if the @@ -809,7 +821,7 @@ func (n rootScopeNaming) ObjectName(obj runtime.Object) (namespace, name string, type scopeNaming struct { scope meta.RESTScope runtime.SelfLinker - itemPathFn func(name, namespace string) string + itemPathFn func(name, namespace string) bytes.Buffer allNamespaces bool } @@ -842,28 +854,30 @@ func (n scopeNaming) Name(req *restful.Request) (namespace, name string, err err } // GenerateLink returns the appropriate path and query to locate an object by its canonical path. -func (n scopeNaming) GenerateLink(req *restful.Request, obj runtime.Object) (path, query string, err error) { +func (n scopeNaming) GenerateLink(req *restful.Request, obj runtime.Object) (uri string, err error) { namespace, name, err := n.ObjectName(obj) if err != nil { - return "", "", err + return "", err } if len(namespace) == 0 && len(name) == 0 { namespace, name, err = n.Name(req) if err != nil { - return "", "", err + return "", err } } if len(name) == 0 { - return "", "", errEmptyName + return "", errEmptyName } - - return n.itemPathFn(name, namespace), "", nil + result := n.itemPathFn(name, namespace) + return result.String(), nil } // GenerateListLink returns the appropriate path and query to locate a list by its canonical path. -func (n scopeNaming) GenerateListLink(req *restful.Request) (path, query string, err error) { - path = req.Request.URL.Path - return path, "", nil +func (n scopeNaming) GenerateListLink(req *restful.Request) (uri string, err error) { + if len(req.Request.URL.RawPath) > 0 { + return req.Request.URL.RawPath, nil + } + return req.Request.URL.EscapedPath(), nil } // ObjectName returns the name and namespace set on the object, or an error if the diff --git a/pkg/apiserver/api_installer_test.go b/pkg/apiserver/api_installer_test.go index 249cd342937..0d8028c7897 100644 --- a/pkg/apiserver/api_installer_test.go +++ b/pkg/apiserver/api_installer_test.go @@ -17,6 +17,7 @@ limitations under the License. package apiserver import ( + "bytes" "testing" "k8s.io/kubernetes/pkg/api" @@ -36,8 +37,8 @@ func TestScopeNamingGenerateLink(t *testing.T) { s := scopeNaming{ meta.RESTScopeNamespace, selfLinker, - func(name, namespace string) string { - return "/api/v1/namespaces/" + namespace + "/services/" + name + func(name, namespace string) bytes.Buffer { + return *bytes.NewBufferString("/api/v1/namespaces/" + namespace + "/services/" + name) }, true, } @@ -50,7 +51,7 @@ func TestScopeNamingGenerateLink(t *testing.T) { Kind: "Service", }, } - _, _, err := s.GenerateLink(&restful.Request{}, service) + _, err := s.GenerateLink(&restful.Request{}, service) if err != nil { t.Errorf("Unexpected error %v", err) } diff --git a/pkg/apiserver/resthandler.go b/pkg/apiserver/resthandler.go index 4182547cef6..f452ffcbf84 100644 --- a/pkg/apiserver/resthandler.go +++ b/pkg/apiserver/resthandler.go @@ -60,10 +60,11 @@ type ScopeNamer interface { // SetSelfLink sets the provided URL onto the object. The method should return nil if the object // does not support selfLinks. SetSelfLink(obj runtime.Object, url string) error - // GenerateLink creates a path and query for a given runtime object that represents the canonical path. - GenerateLink(req *restful.Request, obj runtime.Object) (path, query string, err error) - // GenerateLink creates a path and query for a list that represents the canonical path. - GenerateListLink(req *restful.Request) (path, query string, err error) + // GenerateLink creates an encoded URI for a given runtime object that represents the canonical path + // and query. + GenerateLink(req *restful.Request, obj runtime.Object) (uri string, err error) + // GenerateLink creates an encoded URI for a list that represents the canonical path and query. + GenerateListLink(req *restful.Request) (uri string, err error) } // RequestScope encapsulates common fields across all RESTful handler methods. @@ -990,18 +991,12 @@ func transformDecodeError(typer runtime.ObjectTyper, baseErr error, into runtime // plus the path and query generated by the provided linkFunc func setSelfLink(obj runtime.Object, req *restful.Request, namer ScopeNamer) error { // TODO: SelfLink generation should return a full URL? - path, query, err := namer.GenerateLink(req, obj) + uri, err := namer.GenerateLink(req, obj) if err != nil { return nil } - newURL := *req.Request.URL - // use only canonical paths - newURL.Path = path - newURL.RawQuery = query - newURL.Fragment = "" - - return namer.SetSelfLink(obj, newURL.String()) + return namer.SetSelfLink(obj, uri) } func hasUID(obj runtime.Object) (bool, error) { @@ -1045,31 +1040,20 @@ func setListSelfLink(obj runtime.Object, req *restful.Request, namer ScopeNamer) return 0, nil } - // TODO: List SelfLink generation should return a full URL? - path, query, err := namer.GenerateListLink(req) + uri, err := namer.GenerateListLink(req) if err != nil { return 0, err } - newURL := *req.Request.URL - newURL.Path = path - newURL.RawQuery = query - // use the path that got us here - newURL.Fragment = "" - if err := namer.SetSelfLink(obj, newURL.String()); err != nil { + if err := namer.SetSelfLink(obj, uri); err != nil { glog.V(4).Infof("Unable to set self link on object: %v", err) } - // Set self-link of objects in the list. - items, err := meta.ExtractList(obj) - if err != nil { - return 0, err - } - for i := range items { - if err := setSelfLink(items[i], req, namer); err != nil { - return len(items), err - } - } - return len(items), meta.SetList(obj, items) + count := 0 + err = meta.EachListItem(obj, func(obj runtime.Object) error { + count++ + return setSelfLink(obj, req, namer) + }) + return count, err } func getPatchedJS(patchType api.PatchType, originalJS, patchJS []byte, obj runtime.Object) ([]byte, error) { diff --git a/pkg/apiserver/resthandler_test.go b/pkg/apiserver/resthandler_test.go index 1fe870aa560..fe85840541c 100644 --- a/pkg/apiserver/resthandler_test.go +++ b/pkg/apiserver/resthandler_test.go @@ -134,13 +134,13 @@ func (p *testNamer) SetSelfLink(obj runtime.Object, url string) error { } // GenerateLink creates a path and query for a given runtime object that represents the canonical path. -func (p *testNamer) GenerateLink(req *restful.Request, obj runtime.Object) (path, query string, err error) { - return "", "", errors.New("not implemented") +func (p *testNamer) GenerateLink(req *restful.Request, obj runtime.Object) (uri string, err error) { + return "", errors.New("not implemented") } // GenerateLink creates a path and query for a list that represents the canonical path. -func (p *testNamer) GenerateListLink(req *restful.Request) (path, query string, err error) { - return "", "", errors.New("not implemented") +func (p *testNamer) GenerateListLink(req *restful.Request) (uri string, err error) { + return "", errors.New("not implemented") } type patchTestCase struct {