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

endpoint: skip Envoy incremental updates if no Envoy redirects (try 2) #31775

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Apr 4, 2024

This commit fixes a bug introduced by #31454.

It removes the check that skips updating Envoy when there are no pending PolicyMap changes. This is because there are cases where endpoint regeneration will flush incremental changes, but not apply them to Envoy.

The basic flow is:

  1. thread 1: e.regenerate() -> e.updateNetworkPolicy() pushes policy to Envoy
  2. thread 2: Generates an incremental update
  3. thread 1: e.regenerate() -> e.applyPolicyMapChanges() consumes incremental update but does not apply it to envoy
  4. thread 1: unlocks endpoint, returns
  5. thread 2: e.ApplyPolicyMapChanges() consumes incremental update, applies to envoy

However, in this case, there is no pending incremental update for the BPF maps. The previous version of this code also skipped updating Envoy, which was incorrect.

This reverts commit 6888c64.

This commit was not quite right, and should not have merged. There are
cases where there are no pending policy-map changes, but there are
pending Envoy ones.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
This should significantly reduce the amount of time it takes to roll out
incremental policy updates by avoiding Envoy updates for endpoints which
do not have any Envoy redirects.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
@squeed squeed added the release-note/misc This PR makes changes that have no direct user impact. label Apr 4, 2024
@squeed squeed requested a review from a team as a code owner April 4, 2024 12:36
@squeed squeed requested a review from tommyp1ckles April 4, 2024 12:36
@squeed
Copy link
Contributor Author

squeed commented Apr 4, 2024

/test

@squeed squeed requested a review from jrajahalme April 4, 2024 12:39
@jrajahalme jrajahalme added this pull request to the merge queue Apr 4, 2024
Merged via the queue into cilium:main with commit 5b1ff58 Apr 4, 2024
63 checks passed
@christarazi christarazi added area/daemon Impacts operation of the Cilium daemon. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants