Skip to content

Conversation

pippolo84
Copy link
Member

Provide an API to let callers wait for the endpoint restoration to be completed. To avoid import cycles, the interface wrapping the API is defined outside of the daemon/cmd package where it is implemented. This allows cells to require an endpointstate.Restorer through Hive dependency injection without pulling in the daemon/cmd package.

This is a required step to migrate the k8s CEP watchers to Resource[T]. Refer to this comment for more information.

Related: #28159

@pippolo84 pippolo84 added release-note/misc This PR makes changes that have no direct user impact. area/agent Cilium agent related. area/modularization Relates to code modularization and maintenance. labels Nov 17, 2023
@pippolo84
Copy link
Member Author

/test

@lmb
Copy link
Contributor

lmb commented Nov 21, 2023

@pippolo84 can you add a codeowner for endpointstate please?

@pippolo84 pippolo84 force-pushed the pr/pippolo84/expose-daemon-endpoint-restore branch from f854d91 to 8008794 Compare November 21, 2023 14:26
@pippolo84 pippolo84 requested a review from a team as a code owner November 21, 2023 14:26
@pippolo84 pippolo84 requested a review from qmonnet November 21, 2023 14:26
@pippolo84
Copy link
Member Author

@pippolo84 can you add a codeowner for endpointstate please?

Done. I've assigned cilium/endpoint

Copy link
Member

@qmonnet qmonnet 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. A few comments on the commit description:

  • There's a typo (to requires)
  • It would be nice to mention the use case that motivates the change (you mention it in the PR description, not in the commit log)

Not blocking, though.

Provide an API to let callers wait for the endpoint restoration to be
completed.  To avoid import cycles, the interface wrapping the API is
defined outside of the daemon/cmd package where it is implemented. This
allows cells to require an endpointstate.Restorer through Hive
dependency injection without pulling in the daemon/cmd package.

This is a required step to migrate the k8s CEP watchers to Resource[T].

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 force-pushed the pr/pippolo84/expose-daemon-endpoint-restore branch from 8008794 to 818a7be Compare November 22, 2023 10:56
@pippolo84
Copy link
Member Author

Looks good. A few comments on the commit description:

  • There's a typo (to requires)
  • It would be nice to mention the use case that motivates the change (you mention it in the PR description, not in the commit log)

Not blocking, though.

Thanks for the pointers, I've fixed the commit message. 👍

@pippolo84
Copy link
Member Author

/test

Copy link
Contributor

@thorn3r thorn3r left a comment

Choose a reason for hiding this comment

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

excellent work

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Nice work! 🚀

@tklauser tklauser removed the request for review from jrajahalme November 28, 2023 15:58
@tklauser tklauser enabled auto-merge November 28, 2023 15:59
@tklauser tklauser requested a review from jrajahalme November 28, 2023 16:11
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.

I do not see the new interface being used yet in this PR, but this is likely as planned.

@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 Nov 30, 2023
@pippolo84
Copy link
Member Author

pippolo84 commented Nov 30, 2023

I do not see the new interface being used yet in this PR, but this is likely as planned.

Yep, it'll be used in the modularized endpoint GC: #29246

@aanm aanm disabled auto-merge December 1, 2023 09:54
@aanm aanm merged commit f234b72 into cilium:main Dec 1, 2023
@jrajahalme jrajahalme mentioned this pull request Nov 4, 2024
@jrajahalme jrajahalme added the backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. label Nov 4, 2024
@julianwiedmann julianwiedmann removed the backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. label Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agent Cilium agent related. area/modularization Relates to code modularization and maintenance. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants