endpoint: Only perform full synchronization periodically #27693
Merged
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
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
joamaki
added
the
release-note/minor
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
joamaki
force-pushed
the
pr/joamaki/policy-map-periodic-full-sync
branch
from
ec7872a
to
61a956e
Compare
|
/test |
joamaki
commented
Aug 30, 2023
jrajahalme
approved these changes
Aug 30, 2023
christarazi
reviewed
Aug 30, 2023
|
We know Cilium wasn’t relying on this because we were not seeing the warnings that would be logged if full sync did catch something. Furthermore, @jrajahalme who wrote this originally did not intend this as he expected UpdateController to be a no-op after the first call and not a trigger to run it immediately. |
joamaki
force-pushed
the
pr/joamaki/policy-map-periodic-full-sync
branch
2 times, most recently
from
aff193d
to
8046258
Compare
christarazi
approved these changes
Sep 1, 2023
christarazi
added
kind/enhancement
The UpdateController method performs either a creation, or updating of a controller. If an update is performed the controller is immediately triggered. In some cases we want to create the controller once without triggering or modifying it if it already has been created. Add a CreateController method for those use cases. Signed-off-by: Jussi Maki <jussi@isovalent.com>
Performing a full dump of the policy map on every policy map synchronization is expensive and is only necessary to catch rare cases where the agent's view of the policy map has diverged from the kernel's view which should only happen either due to kernel or other bugs or some other application modifying the endpoint policy map. To reduce the cost of synchronizing large endpoint policy maps perform the full synchronization only periodically. The full synchronization is defaults to 15 minutes. Configurable with the hidden option "--policy-map-full-reconciliation-interval". Fixes: 9dc1350 ("endpoint: Enhance policy map sync") Signed-off-by: Jussi Maki <jussi@isovalent.com>
joamaki
force-pushed
the
pr/joamaki/policy-map-periodic-full-sync
branch
from
8046258
to
f005f93
Compare
|
/test |
maintainer-s-little-helper
bot
added
the
ready-to-merge
joestringer
reviewed
Sep 6, 2023
| ctrlName := fmt.Sprintf("sync-policymap-%d", e.ID) | ||
| e.controllers.UpdateController(ctrlName, | ||
| e.controllers.CreateController(ctrlName, |
There was a problem hiding this comment.
Super minor nit but it's odd to return a boolean and ignore it. Looking at the line of code here it's not obvious that the result is being ignored. Ideally the underlying function wouldn't return a boolean if it's unused in production code (I see it's used in tests), but this could at least signal that the result is deliberately ignored with a _ = e.controllers.CreateController(...).
|
Huh, I was pondering why the policy trigger doesn't ratelimit these and then realized that's because the policy trigger is for triggering re-evaluation of the policy, not attempting a fresh synchronization of the synchronized policy to the BPF policymaps. So fair 👍 |
joamaki
added
needs-backport/1.12
needs-backport/1.13
gandro
added
backport-pending/1.14
|
I managed to resolve the conflicts for v1.14, but the conflicts for v1.13 and v1.12 are more complex (i.e. there is no managedController type). Marking as |
gandro
added
the
backport/author
gandro
added
backport-done/1.14
joamaki
added
backport-done/1.12
julianwiedmann
added
backport-done/1.13
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.
Performing a full dump of the policy map on every policy map synchronization
is expensive and is only necessary to catch rare cases where the agent's view
of the policy map has diverged from the kernel's view which should only happen
either due to kernel or other bugs or some other application modifying the
endpoint policy map.
To reduce the cost of synchronizing large endpoint policy maps perform the
full synchronization only periodically. The full synchronization is defaults
to 15 minutes. Configurable with the hidden option
"--policy-map-full-reconciliation-interval".
Fixes: 9dc1350 ("endpoint: Enhance policy map sync")