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

helm: add bpf-policy-map-max option #11478

Merged
merged 1 commit into from
May 12, 2020
Merged

helm: add bpf-policy-map-max option #11478

merged 1 commit into from
May 12, 2020

Conversation

alex1989hu
Copy link
Contributor

The source of the default value origin is: v1.7/cmdref/cilium-agent

I faced an issue today which solution and problem statement is described here #9117

This PR adds an option to let users override the default value 16384.

Special notes for your reviewer:
Please backport to v1.7 release

Signed-off-by: Alex Szakaly alex.szakaly@gmail.com

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Fixes:
N/A

@alex1989hu alex1989hu requested a review from a team as a code owner May 11, 2020 19:09
@alex1989hu alex1989hu requested a review from a team May 11, 2020 19:09
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Thanks for the submisson!

The helm lint check failed:
https://github.com/cilium/cilium/pull/11478/checks?check_run_id=664504076

I believe you will need to run make -C install/kubernetes/ and commit the results.

The source of the default value origin is: v1.7/cmdref/cilium-agent

Signed-off-by: Alex Szakaly <alex.szakaly@gmail.com>
@coveralls
Copy link

coveralls commented May 11, 2020

Coverage Status

Coverage decreased (-0.01%) to 37.851% when pulling 0d4def8 on alex1989hu:helm/add-bpf-policy-map-max into f518e88 on cilium:master.

@joestringer
Copy link
Member

test-me-please

@gandro
Copy link
Member

gandro commented May 12, 2020

test-me-please

@joestringer
Copy link
Member

joestringer commented May 12, 2020

K8s-1.11-Kernel-netnext CI run hit known flake #11442 .
Runtime-4.9 CI run failed in sudo make privileged-tests and I see the same failure in the master pipeline so this PR is not changing anything here. There are no other failures in that test job. No artifacts were gathered due to #11503 so it's hard to debug further here.

I'm happy to override and mark this as ready-to-merge based upon the above findings.

@joestringer joestringer added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 12, 2020
@gandro gandro merged commit 6817b28 into cilium:master May 12, 2020
1.8.0 automation moved this from In progress to Merged May 12, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.4 May 13, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.4 May 14, 2020
tklauser added a commit that referenced this pull request May 15, 2020
The policyMapMax helm option was introduced after the
mapDynamicSizeRatio option in #11478. Thus, the fact that explicitly
specifying the maximum disables the dynamic sizing of the policy map was
not mentioned in the upgrade guide. Document this now.

While at it, also fix the name of the helm option for the dynamic sizing
in the upgrade guide. It was using the command line option name, not the
helm option name.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
joestringer pushed a commit that referenced this pull request May 15, 2020
The policyMapMax helm option was introduced after the
mapDynamicSizeRatio option in #11478. Thus, the fact that explicitly
specifying the maximum disables the dynamic sizing of the policy map was
not mentioned in the upgrade guide. Document this now.

While at it, also fix the name of the helm option for the dynamic sizing
in the upgrade guide. It was using the command line option name, not the
helm option name.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
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/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.7.4
Backport done to v1.7
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants