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

Introduce resiliency package #27614

Merged
merged 1 commit into from Aug 25, 2023
Merged

Introduce resiliency package #27614

merged 1 commit into from Aug 25, 2023

Conversation

derailed
Copy link
Contributor

@derailed derailed commented Aug 21, 2023 •
edited

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

This package introduces the concept of a retryable error.
This provides call sites with a hint to potentially retry a failed call if need be.

Signed-off-by: Fernand Galiana fernand.galiana@gmail.com

@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 Aug 21, 2023
@derailed
Copy link
Contributor Author

/test

@derailed derailed marked this pull request as ready for review August 22, 2023 14:32
@derailed derailed requested a review from a team as a code owner August 22, 2023 14:32
joestringer
joestringer reviewed Aug 22, 2023
View reviewed changes
pkg/resiliency/wrap.go Outdated Show resolved Hide resolved
pkg/resiliency/types.go Outdated Show resolved Hide resolved
@joestringer joestringer requested review from a team and thorn3r and removed request for a team August 22, 2023 21:20
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Aug 22, 2023
@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 Aug 22, 2023
@derailed derailed force-pushed the resiliency_pkg branch 2 times, most recently from f71198d to 8db5f58 Compare August 23, 2023 17:40
@christarazi
Copy link
Member

Just a heads up, there's no need to rebase PRs unless there are conflicting files. GH will warn on the latter. Otherwise, when a PR is merged, it is automatically rebased on top of main. So no need to explicitly rebase PRs periodically unless absolutely necessary.

@derailed derailed requested a review from a team as a code owner August 23, 2023 19:45
@derailed derailed requested a review from bimmlerd August 23, 2023 19:45
@derailed
Copy link
Contributor Author

/test

@christarazi christarazi added kind/enhancement This would improve or streamline existing functionality. area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. sig/agent Cilium agent related. labels Aug 23, 2023
@derailed
Copy link
Contributor Author

/test

christarazi
christarazi approved these changes Aug 24, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

The previous revision had a few unit tests which would still be helpful. I think a very simple sanity check unit test is still good to have in the tree. It doesn't need to be complicated because we've removed the need for the Unwrap.

@derailed
Copy link
Contributor Author

@christarazi Right I've axe them since I don't think we need to test out the golang apis?

@derailed
Copy link
Contributor Author

/test

@derailed derailed mentioned this pull request Aug 25, 2023
Merged
8 tasks
@joestringer
Copy link
Member

Looks like this needs a rebase + re-run CI, then it should be good to merge.

@derailed
Add resiliency package
bf493fb 
This package introduces the concept of a retryable error. This provides
call sites with a hint to potentially retry a failed call if need be.

Signed-off-by: Fernand Galiana <fernand.galiana@gmail.com>
@joestringer
Copy link
Member

/test

@derailed derailed added the resiliency Tracks resiliency issues label Aug 25, 2023
@derailed derailed mentioned this pull request Aug 25, 2023
Merged
8 tasks
@joestringer joestringer merged commit 8365bb0 into cilium:main Aug 25, 2023
60 checks passed
@christarazi christarazi added affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch labels Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@christarazi christarazi christarazi approved these changes

@thorn3r thorn3r Awaiting requested review from thorn3r thorn3r is a code owner automatically assigned from cilium/sig-agent

@bimmlerd bimmlerd Awaiting requested review from bimmlerd bimmlerd is a code owner automatically assigned from cilium/contributing

Assignees
No one assigned
Labels
affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. resiliency Tracks resiliency issues sig/agent Cilium agent related.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@derailed @christarazi @joestringer