Skip to content

Speed up CheckIngress in admission hook by eliminating the deep copy #7297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cgorbit opened this issue Jun 29, 2021 · 1 comment · Fixed by #7298
Closed

Speed up CheckIngress in admission hook by eliminating the deep copy #7297

cgorbit opened this issue Jun 29, 2021 · 1 comment · Fixed by #7298
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@cgorbit
Copy link
Contributor

cgorbit commented Jun 29, 2021

Hi,

In the company where I working we have several k8s clusters and one dedicated to developers personal stands.
k8s is working in aws ec2 nodes.

Developer's cluster's nginx ingress controller config contains 120 server sections, 5271 location sections and have 19MB size as text config.

On such big config amission hook for each ingress update becomes too slow, it lasts about 7 seconds now. And it was even slower before I speed it up by rewriting bash scripts (!) which were used inside CheckIngress (#7076).

But it's still very slow. So we disabled admission hooks, because when somebody creates new developer stand admission hooks must be evaluated many times and sometimes we got error from master that admission hook finished with time out (of 30s).

I found next place which take now about 3s on my config:

// copy location before any change
el, err := copystructure.Copy(location)
if err != nil {
klog.ErrorS(err, "copying location")
}

As far as I understand deep copy is redundant in this case. Shallow copy will be enough.

e2e test: https://gist.github.com/cgorbit/431086b0e78f1a8b75cd497235ae8d51

diff --git a/internal/ingress/controller/location.go b/internal/ingress/controller/location.go
index a6b3d2b14..d40e88960 100644
--- a/internal/ingress/controller/location.go
+++ b/internal/ingress/controller/location.go
@@ -20,10 +20,8 @@ import (
        "fmt"
        "strings"
 
-       "github.com/mitchellh/copystructure"
        networking "k8s.io/api/networking/v1"
        "k8s.io/ingress-nginx/internal/ingress"
-       "k8s.io/klog/v2"
 )
 
 var (
@@ -73,18 +71,14 @@ func updateServerLocations(locations []*ingress.Location) []*ingress.Location {
                        continue
                }
 
-               // copy location before any change
-               el, err := copystructure.Copy(location)
-               if err != nil {
-                       klog.ErrorS(err, "copying location")
-               }
+               var el ingress.Location = *location
 
                // normalize path. Must end in /
                location.Path = normalizePrefixPath(location.Path)
                newLocations = append(newLocations, location)
 
                // add exact location
-               exactLocation := el.(*ingress.Location)
+               exactLocation := &el
                exactLocation.PathType = &pathTypeExact
 
                newLocations = append(newLocations, exactLocation)

@rikatz
Copy link
Contributor

rikatz commented Jun 29, 2021

/triage accepted
/kind cleanup
/priority important-longterm

Maybe this seems to be a good performance improvement. Accepting so other folks can look

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants