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

endpoint: skip Envoy incremental updates if no Envoy redirects #31454

Merged
merged 1 commit into from
Apr 3, 2024
Merged

endpoint: skip Envoy incremental updates if no Envoy redirects #31454

merged 1 commit into from
Apr 3, 2024

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Mar 18, 2024 •
edited

This should significantly reduce the amount of time it takes to roll our incremental policy updates.

Edit: This PR was reverted and re-factored, see #31775

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 18, 2024
@squeed
Copy link
Contributor Author

squeed commented Mar 18, 2024

@jrajahalme / @sayboras Does this seem reasonable?

@squeed squeed added the release-note/misc This PR makes changes that have no direct user impact. label Mar 18, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 18, 2024
@squeed
Copy link
Contributor Author

squeed commented Mar 18, 2024

/test

@christarazi christarazi added kind/enhancement This would improve or streamline existing functionality. kind/performance There is a performance impact of this. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Mar 18, 2024
jrajahalme
jrajahalme approved these changes Mar 18, 2024
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

This seems safe to do, as the altered function (ApplyPolicyMapChanges()) is only called from endpointmanager's UpdatePolicyMaps(), which is called from the FQDN and IPCache code paths.

jrajahalme
jrajahalme requested changes Mar 18, 2024
View reviewed changes
pkg/endpoint/bpf.go Outdated Show resolved Hide resolved
@squeed squeed marked this pull request as ready for review March 26, 2024 10:00
@squeed squeed requested a review from a team as a code owner March 26, 2024 10:00
sayboras
sayboras approved these changes Mar 26, 2024
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks ✔️

@squeed
Copy link
Contributor Author

squeed commented Mar 26, 2024

/test

@squeed
Copy link
Contributor Author

squeed commented Apr 2, 2024

@sayboras @jrajahalme I updated this slightly; we now skip Envoy if there are no redirects or network policy did not change.

sayboras
sayboras approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Still lgtm ✔️

jrajahalme
jrajahalme approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Looks good, may fix typo in comment if need to respin for something else.

pkg/endpoint/bpf.go Outdated Show resolved Hide resolved
@squeed
endpoint: skip Envoy incremental updates if no Envoy redirects
b3a998e 
This should significantly reduce the amount of time it takes to roll our
incremental policy updates.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
@squeed
Copy link
Contributor Author

squeed commented Apr 2, 2024

/test

@squeed squeed added this pull request to the merge queue Apr 3, 2024
@squeed squeed added affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch affects/v1.15 This issue affects v1.15 branch labels Apr 3, 2024
Merged via the queue into cilium:main with commit 6888c64 Apr 3, 2024
62 checks passed
@squeed squeed deleted the skip-envoy-incremental branch April 3, 2024 08:45
This was referenced Apr 4, 2024
Merged
Open
@squeed squeed removed the affects/v1.13 This issue affects v1.13 branch label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrajahalme jrajahalme jrajahalme approved these changes

@sayboras sayboras sayboras approved these changes

@tommyp1ckles tommyp1ckles Awaiting requested review from tommyp1ckles tommyp1ckles is a code owner automatically assigned from cilium/endpoint

Assignees
No one assigned
Labels
affects/v1.14 This issue affects v1.14 branch affects/v1.15 This issue affects v1.15 branch kind/enhancement This would improve or streamline existing functionality. kind/performance There is a performance impact of this. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@squeed @jrajahalme @sayboras @christarazi