Rewrite clean-nginx-conf.sh in Go to speed up admission webhook #7076
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @cgorbit! |
|
Hi @cgorbit. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
|
ping @bowei ? |
|
/assign @bowei |
|
Sorry if I misunderstood instruction and make early assign |
|
/ok-to-test Thanks for the PR. Will review during this week/end :) |
k8s-ci-robot
added
ok-to-test
|
@andersosthus compiled a custom build, for internal testing, with this fix on top of 0.46. In a K8s 1.18 cluster with approx 900 complex ingress objects (several hundred hosts, snippets, regex, certs etc) validation using the bash based code did not succeed within the 30 seconds limit. Now with this Go based implementation, validation takes around 4-7 seconds. So I would say this is a huge improvement. The previous implementation did not work for us due to the timeout. |
|
Any news on when this fix will be merged? We have been running with this in production for two weeks as of now, it really is a game changes for clusters with many ingress objects and complex configurations. |
|
Hi there, I've been a bit busy with #7156 and this is why I'm taking some time to answer. As soon as I finish that one I'm really willing to look at this PR, sounds really promising. Thanks |
|
/priority important-soon |
k8s-ci-robot
added
priority/important-soon
There was a problem hiding this comment.
@cgorbit Thanks for your PR!
I must confess I'm a bit confuse by the logic inside the byte reader, so I would like to ask:
- Not ignoring or panic'ng errors, but instead returning them as part of the function and dealing with them in the caller.
- Add some comments with examples of what each switch case is doing and when it may happen
- As we are moving to Go, some unit test as well with known cases.
Thanks!
| if err == io.EOF { | ||
| return | ||
| } | ||
| panic(err) |
There was a problem hiding this comment.
Instead of panic, let's return the error here. Maybe the function should be cleanConf(in, out *bytes.Buffer) error
There was a problem hiding this comment.
As I understand from documentation of bytes package, It's impossible for ReadByte() to return something other than io.EOF. That's why I wrote panic there. This allows me to leave cleanConf signature without error. If I will handle errors in cleanConf they will always be nil actually according to documentation.
There was a problem hiding this comment.
I got it, but still I don't like the idea of something internally panic'ing. If this behavior changes in a future, we might end up with this panic doing a world stop into ingress nginx :)
It would be better to return this error and check if this is not nil, even if we know that now it will always be nil, if this changes in a future Go compilation we won't get some strange magic error :)
| @@ -86,6 +92,77 @@ func NewTemplate(file string) (*Template, error) { | |||
| }, nil | |||
| } | |||
|
|
|||
| func cleanConf(in *bytes.Buffer, out *bytes.Buffer) { | |||
There was a problem hiding this comment.
| func cleanConf(in *bytes.Buffer, out *bytes.Buffer) { | |
| func cleanConf(in, out *bytes.Buffer) error { |
| if err == io.EOF { | ||
| return | ||
| } | ||
| panic(err) |
There was a problem hiding this comment.
As I understand from documentation of bytes package, It's impossible for ReadByte() to return something other than io.EOF. That's why I wrote panic there. This allows me to leave cleanConf signature without error. If I will handle errors in cleanConf they will always be nil actually according to documentation.
|
Hey @cgorbit yes I would like that we deal with the error instead of panic (as my previous comment) just to make sure that some change into future go version wont break things or cause panic in a future (even if today we know it always returns nil). Also, this is a nit but if possible please document the behavior of the package/what exactly cleanConf is doing (tabulating, removing characters, etc). You can do this on a followup/later PR as well, but would be great to have this. As soon as you think this is ready, ping me on Slack (my email is pretty messy...) and I will have a new look into this and hopefully merge. So we can also cherry pick this to the branch release-v1beta1 to be available in past ingress version. |
k8s-ci-robot
added
size/XL
|
/label tide/merge-method-squash As this is a pretty important change, after this gets merged, would you mind opening a PR/cherrypick to branch release-v1beta1 to be also inside the old v0.X releases? Thanks |
k8s-ci-robot
added
the
tide/merge-method-squash
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgorbit, rikatz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
…rnetes#7076) * Rewrite clean-nginx-conf.sh to speed up admission webhook * Less diff with original clean-nginx-conf.sh * Add error handling, add documentation, add unit test * indent code * Don't ignore Getwd() error
… (#7322) * Rewrite clean-nginx-conf.sh to speed up admission webhook * Less diff with original clean-nginx-conf.sh * Add error handling, add documentation, add unit test * indent code * Don't ignore Getwd() error
…rnetes#7076) * Rewrite clean-nginx-conf.sh to speed up admission webhook * Less diff with original clean-nginx-conf.sh * Add error handling, add documentation, add unit test * indent code * Don't ignore Getwd() error
What this PR does / why we need it:
Current implementation of validating web hook may be too slow in some
circumstances because it use Shell-script to tidy generated full nginx config.
On my dev-cluster with lots of namespaces/ingresses this sometimes lead too (in apiserver):
web hook:
ingress-nginx/internal/admission/controller/main.go
Line 57 in f5cfd57
call to shell-script:
ingress-nginx/internal/ingress/controller/template/template.go
Line 113 in f5cfd57
shell-script: https://github.com/kubernetes/ingress-nginx/blob/f5cfd5730c4b296c87fbc531c83a6e4f33483b75/rootfs/ingress-controller/clean-nginx-conf.sh
I rewrite tidy-code in Golang in this PR to speed it up.
My kubernetes in AWS:
On localhost (not so impressive):
Types of changes
This is performance improvement.
Which issue/s this PR fixes
No related issues
How Has This Been Tested?
The only diff is: https://gist.github.com/cgorbit/c0c9d6442dc18d44113d685e2e6bce27
Which is improvement, I think.
https://gist.github.com/cgorbit/b6f2db83275c8c996f5bb5b4de668820
Checklist: