Skip to content

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Jun 4, 2024

This tightens up the queued-prefix side of the ipcache asynchronous layer:

(see individual commit messages for more detail)

  • Fix a panic when allocation fails. This fixes a nil dereference when allocation fails
  • Split queued updates in to chunks. This can prevent identity spikes when a large number of prefixes are queued for updates. This is particularly relevant for prefix down-propagation.
  • Don't queue prefixes for updates when metadata is unchanged. When an upsert is a no-op, don't bother queuing prefixes.

@squeed squeed requested review from a team as code owners June 4, 2024 10:30
@squeed squeed requested a review from christarazi June 4, 2024 10:30
@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 Jun 4, 2024
@squeed squeed added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-note/misc This PR makes changes that have no direct user impact. labels Jun 4, 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 Jun 4, 2024
@squeed squeed requested a review from gandro June 4, 2024 10:31
@squeed
Copy link
Contributor Author

squeed commented Jun 4, 2024

/test

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

I like it!

Approving assuming the other nits are addressed

squeed added 4 commits June 5, 2024 15:23
It turns out that label injection panics if identity allocation fails.
This code-path is not practically reachable, as we would need to run out
of local identities. Still good to clean up, though.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
This is a private function that should never be called directly.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
If a large number of prefixes are queued for update, then we should not
update all at the same time. This is because outgoing identities are
only released at the end of doInjectLabels(), which means we could
double the set of in-use identities while application is happening.

The only exception is for the first run, in which case there is no point
in dividing, as no identities will be released.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
If a particular upsert operation is a no-op, then there's no point in
enqueuing a prefix update for it.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
@squeed squeed force-pushed the ipcache-perf-improvements branch from d58ab61 to 354a295 Compare June 5, 2024 13:29
@squeed
Copy link
Contributor Author

squeed commented Jun 5, 2024

/ci-runtime

@squeed
Copy link
Contributor Author

squeed commented Jun 5, 2024

/ci-integration

@squeed
Copy link
Contributor Author

squeed commented Jun 5, 2024

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 5, 2024
@christarazi christarazi added this pull request to the merge queue Jun 5, 2024
Merged via the queue into cilium:main with commit c77d1a3 Jun 5, 2024
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/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.

3 participants