v1.10 backports 2022-07-21 #20620
Merged
v1.10 backports 2022-07-21 #20620
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ upstream commit 9242f05 ] This will come in handy for upcoming commits that will try to improve the performance of these functions. Signed-off-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 189d4d3 ] Use strings.Builder instead of fmt.Sprintf() which is known to be not performant. This optimization was identified by inspecting a pprof (memory profile) of a cluster with a wildcard L7 DNS rule policy (matchPattern: '*') and a workload selected by the policy making a large amount of unique DNS requests. ``` $ go test -v -run '^$' -bench 'Benchmark_maskedIPNetToLabelString' -benchtime 50000x -benchmem ./pkg/labels/cidr > old.txt $ go test -v -run '^$' -bench 'Benchmark_maskedIPNetToLabelString' -benchtime 50000x -benchmem ./pkg/labels/cidr > new.txt $ benchcmp old.txt new.txt benchcmp is deprecated in favor of benchstat: https://pkg.go.dev/golang.org/x/perf/cmd/benchstat benchmark old ns/op new ns/op delta Benchmark_maskedIPNetToLabelString-8 8122 4450 -45.21% benchmark old allocs new allocs delta Benchmark_maskedIPNetToLabelString-8 39 32 -17.95% benchmark old bytes new bytes delta Benchmark_maskedIPNetToLabelString-8 488 368 -24.59% ``` Signed-off-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 46e3d07 ] Avoid using fmt.Sprintf() when a simple string concatentation does the same job, and preallocate slices where possible. These optimizations were identified by inspecting a pprof (memory profile) of a cluster with a wildcard L7 DNS rule policy (matchPattern: '*') and a workload selected by the policy making a large amount of unique DNS requests. Without the previous commit that optimizes the maskedIPToLabelString(): ``` $ go test -v -run '^$' -bench 'Benchmark_GetCIDRLabels' -benchtime 5000x -benchmem ./pkg/labels/cidr > old.txt $ go test -v -run '^$' -bench 'Benchmark_GetCIDRLabels' -benchtime 5000x -benchmem ./pkg/labels/cidr > new.txt $ benchcmp old.txt new.txt benchcmp is deprecated in favor of benchstat: https://pkg.go.dev/golang.org/x/perf/cmd/benchstat benchmark old ns/op new ns/op delta Benchmark_GetCIDRLabels-8 391590 390163 -0.36% benchmark old allocs new allocs delta Benchmark_GetCIDRLabels-8 1455 1424 -2.13% benchmark old bytes new bytes delta Benchmark_GetCIDRLabels-8 64143 63189 -1.49% ``` With previous commit: ``` benchmark old ns/op new ns/op delta Benchmark_GetCIDRLabels-8 390163 295259 -24.32% benchmark old allocs new allocs delta Benchmark_GetCIDRLabels-8 1424 1131 -20.58% benchmark old bytes new bytes delta Benchmark_GetCIDRLabels-8 63189 58707 -7.09% ``` Signed-off-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 372407f ] The DNS proxy has a configurable semaphore, which rejects requests if there are more than the configured number of requests in flight. There is a metric to measure how long a DNS proxy waited on the semaphore, but no metric to track if requests were dropped because of the semaphore. The existing monitoring isn't quite sufficient. The metric proxy_upstream_time{"timeout", "dns", "semaphoreTime"} will measure requests rejected due to timeouts, but also any other error which implements neterror.Timeout (e.g. an actual timeout communicating with upstream DNS). Signed-off-by: Rahul Joshi <rkjoshi@google.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro
added
backport/1.10
kind/backports
gandro
requested review from
christarazi,
aanm,
joamaki,
michi-covalent,
raphink and
tklauser
tklauser
approved these changes
Jul 21, 2022
There was a problem hiding this comment.
- Add metric on number of requests rejected by DNS Proxy semaphore #20491 -- Add metric on number of requests rejected by DNS Proxy semaphore (@rahulkjoshi)
- fqdn/dnsproxy: fix test build #20537 -- fqdn/dnsproxy: fix test build (@tklauser)
These LGTM. The latter was a test build fix for the former which was sent by an external contributor, so I've reviewed that PR's backport as well.
thanks sebastian, but let me open a separate pull request to backport this. helm chart changed quite a bit since v1.10, and it's complaining about |
|
opened #20628 |
Thanks! I'll remove the commit from this one then. |
[ upstream commit a0c1ad6 ] updateVersion only set EndpointSliceV1, but not EndpointSlice. Fix this, so that tests can set the capabilities correctly via Force() for older k8s versions. Fixes: 7a1039f ("k8s: Consolidate check for EndpointSlice support") Signed-off-by: Jussi Maki <jussi@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 11b678b ] Commit 372407f exported errFailedAcquireSemaphore and errTimedOutAcquireSemaphore but didn't update the tests to use the new types. This wasn't caught because privileged tests weren't run on the corresponding PR cilium#20491. Fixes: 372407f ("Add metric on number of requests rejected by DNS Proxy semaphore") Signed-off-by: Tobias Klauser <tobias@cilium.io> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 908316c ] Signed-off-by: Raphaël Pinson <raphael@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 522da61 ] Signed-off-by: Raphaël Pinson <raphael@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 25e35f1 ] When stopping the EndpointSlice Kubernetes watchers we should also cancel the waiting to sync this group resource. In failing doing it so, Cilium will timeout on waiting for these resources on Kubernetes versions that should have EndpointSlice v1beta1 available but it's not enabled. Fixes: a0c1ad6 ("pkg/k8s/version: Set EndpointSlice cap when version >=1.17") Signed-off-by: André Martins <andre@cilium.io> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro
force-pushed
the
pr/v1.10-backport-2022-07-21
branch
from
149aa2c
to
1a1001a
Compare
|
/test-backport-1.10 |
christarazi
approved these changes
Jul 27, 2022
|
|
/ci-aks-1.10 |
|
/ci-eks-1.10 |
|
/ci-gke-1.10 |
aanm
approved these changes
Jul 28, 2022
|
I'm marking this ready to merge. The two outstanding reviews are for rather trivial PRs and one of the authors (Jussi) is on PTO. As mentioned before, l4lb is known broken at the moment, and so is CI AKS (it was also failing on the last PR #20509) |
gandro
added
the
ready-to-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ℹ️ Minor conflicts in copyright header
Once this PR is merged, you can update the PR labels via: