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.

Sign up for GitHub

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

Jump to bottom

Allow to configure bpf-nat-global-max using Helm #10511

Merged
merged 2 commits into from Mar 9, 2020
Merged

Allow to configure bpf-nat-global-max using Helm #10511

merged 2 commits into from Mar 9, 2020

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Mar 9, 2020 •
edited by tgraf

Allow to configure the bpf-nat-global-max option using Helm. Set the value to the current value of option.NATMapEntriesGlobalDefault. This will allow to reduce it in PR #10289 in accordance with bpf-ct-global-tcp-max.

Also, as a preparatory commit re-generate quick-install.yaml after PR #9970 for the newly added policy-audit-node option.

To be merged before PR #10289

This change is Reviewable

@tklauser
helm: re-generate quick-install.yaml after PR #9970
85a8084 
quick-install.yaml wasn't re-generated after commit 6cd69ed
("Implement policy audit mode for the daemon") introducing the
policy-audit-node option. Do so now.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser
helm: allow to configure bpf-nat-global-max using Helm
ddee467 
Set the value to the current value of option.NATMapEntriesGlobalDefault

A successive PR will reduce it for #10056

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser added pending-review release-note/misc This PR makes changes that have no direct user impact. labels Mar 9, 2020
@tklauser tklauser requested review from borkmann, joestringer and a team March 9, 2020 12:40
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Mar 9, 2020
@tklauser tklauser mentioned this pull request Mar 9, 2020
Merged
@tklauser
Copy link
Member Author

tklauser commented Mar 9, 2020

test-me-please

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.632% when pulling ddee467 on pr/tklauser/helm-configure-bpf-nat-size into 0d7d76c on master.

borkmann
borkmann reviewed Mar 9, 2020
View reviewed changes
Copy link
Member

@borkmann borkmann left a comment

Choose a reason for hiding this comment

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

We should reduce it by default to 2/3 of TCP CT map size, and add some notes to the doc wrt customizing.

@tklauser
Copy link
Member Author

tklauser commented Mar 9, 2020 •
edited

We should reduce it by default to 2/3 of TCP CT map size, and add some notes to the doc wrt customizing.

Agree, will do that as part of #10289 which reduces the default for --bpf-nat-global-max due to the fact that it's defined as:

cilium/pkg/option/config.go

Lines 505 to 507 in ed74481

// NATMapEntriesGlobalDefault holds the default size of the NAT map
// and is 2/3 of the full CT size as a heuristic
NATMapEntriesGlobalDefault = int((CTMapEntriesGlobalTCPDefault + CTMapEntriesGlobalAnyDefault) * 2 / 3)

will also add a separate section to the upgrade docs as agreed with @joestringer

@joestringer joestringer merged commit 20f6083 into master Mar 9, 2020
1.8.0 automation moved this from In progress to Merged Mar 9, 2020
@joestringer joestringer deleted the pr/tklauser/helm-configure-bpf-nat-size branch March 9, 2020 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borkmann borkmann borkmann approved these changes

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tklauser @coveralls @borkmann @joestringer