Skip to content
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

pkg/fqdn: use LRU in FQDN policy calculation #17224

Merged
merged 1 commit into from Sep 2, 2021

Conversation

aanm
Copy link
Member

@aanm aanm commented Aug 23, 2021

Using an LRU for the memory-intensive operations such as regex.Compile
bring some benefits as presented in the following benchmarks [1]. These
benchmarks assumed there were 2 CNPs that shared 100 FQDN
matchPattern on a node with 20 endpoints.

[1]

name                             old time/op    new time/op    delta
_perEPAllow_setPortRulesForID-8    13.9ms ± 6%     1.2ms ±63%  -91.10%  (p=0.008 n=5+5)

name                             old alloc/op   new alloc/op   delta
_perEPAllow_setPortRulesForID-8    17.4MB ± 0%     0.6MB ± 0%  -96.56%  (p=0.008 n=5+5)

name                             old allocs/op  new allocs/op  delta
_perEPAllow_setPortRulesForID-8     42.8k ± 0%      8.1k ± 0%  -81.13%  (p=0.008 n=5+5)

Signed-off-by: André Martins andre@cilium.io

Optimize memory consumption for clusters with high number of repeated FQDN matchPattern or matchNames

@aanm aanm added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.10 labels Aug 23, 2021
@aanm aanm requested a review from a team August 23, 2021 21:44
@aanm aanm requested a review from a team as a code owner August 23, 2021 21:44
@aanm aanm requested review from joamaki and rolinh August 23, 2021 21:44
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.4 Aug 23, 2021
@aanm
Copy link
Member Author

aanm commented Aug 23, 2021

test-me-please

Job 'Cilium-PR-K8s-1.19-kernel-5.4' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig AutoDirectNodeRoutes Check connectivity with sockops and direct routing

Failure Output

FAIL: Error creating resource /home/jenkins/workspace/Cilium-PR-K8s-1.19-kernel-5.4/src/github.com/cilium/cilium/test/k8sT/manifests/l3-policy-demo.yaml: Cannot retrieve cilium pod cilium-2fq7c policy revision: cannot get revision from json output '': could not parse JSON from command "kubectl exec -n kube-system cilium-2fq7c -- cilium policy get -o json"

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.19-kernel-5.4 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDemosTest Tests Star Wars Demo

Failure Output

FAIL: unable to access deathstar.default.svc.cluster.local/v1/exhaust-port when policy allows it; 000

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.16-net-next' has 1 failure but they might be new flake since it also hit 1 known flake: #17176 (91.52)

Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor docs rewording suggested

pkg/fqdn/dnsproxy/proxy.go Outdated Show resolved Hide resolved
@aanm
Copy link
Member Author

aanm commented Aug 24, 2021

test-1.16-netnext

@aanm
Copy link
Member Author

aanm commented Aug 24, 2021

test-1.19-5.4

@aanm
Copy link
Member Author

aanm commented Aug 24, 2021

test-gke

@aanm
Copy link
Member Author

aanm commented Aug 24, 2021

ci-eks

Using an LRU for the memory-intensive operations such as regex.Compile
bring some benefits as presented in the following benchmarks [1]. These
benchmarks assumed there were 2 CNPs that shared 100 FQDN
`matchPattern` on a node with 20 endpoints.

[1]
```
name                             old time/op    new time/op    delta
_perEPAllow_setPortRulesForID-8    13.9ms ± 6%     1.2ms ±63%  -91.10%  (p=0.008 n=5+5)

name                             old alloc/op   new alloc/op   delta
_perEPAllow_setPortRulesForID-8    17.4MB ± 0%     0.6MB ± 0%  -96.56%  (p=0.008 n=5+5)

name                             old allocs/op  new allocs/op  delta
_perEPAllow_setPortRulesForID-8     42.8k ± 0%      8.1k ± 0%  -81.13%  (p=0.008 n=5+5)
```

Signed-off-by: André Martins <andre@cilium.io>
@pchaigno
Copy link
Member

pchaigno commented Aug 30, 2021

test-me-please

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sIstioTest Istio Bookinfo Demo Tests bookinfo inter-service connectivity

Failure Output

FAIL: Pods are not ready after timeout

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

@pchaigno
Copy link
Member

pchaigno commented Aug 31, 2021

The test was passing before I made the comment change. It also doesn't load any FQDN policies, so probably unrelated to this pull request.

/mlh new-flake Cilium-PR-K8s-GKE

👍 created #17270

@pchaigno
Copy link
Member

The GKE flake is unrelated (see above) and AKS has been failing for a while. All tests were passing before I changed the comment and rebased. All team review requests are covered. Marking ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 31, 2021
@joestringer joestringer added this to Needs backport from master in 1.10.5 Sep 1, 2021
@joestringer joestringer removed this from Needs backport from master in 1.10.4 Sep 1, 2021
@aditighag aditighag merged commit 42417c7 into cilium:master Sep 2, 2021
@aanm aanm deleted the pr/add-lru-cache branch September 15, 2021 14:50
@joestringer joestringer moved this from Needs backport from master to Backport done to v1.10 in 1.10.5 Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.10.5
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

7 participants