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

chore(deps): replace backoff with backon #1653

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flavio
Copy link
Contributor

@flavio flavio commented Nov 29, 2024

Motivation

The backoff crate is no longer maintained and it's pulling the instant which is also marked as unmaintained by RUSTSEC.

This PR fixes #1635

Solution

Replace the backoff dependency with backon. The former one is no longer maintained and is also pulling the instant crate, which has been marked as unmaintained by RUSTSEC.

Prior to this commit the public API of kube-rs exposed a trait defined by the backoff crate. This commits introduces a new trait defined by kube-rs, which wraps the backon trait.

I also had to introduce this new trait because the backon::Backoff trait doesn't have a reset method. kubers makes use of this method, hence the "trick" of defining our own trait solves this challenge. If you don't like this approach, I can try to go upstream to backon and propose the extension of their trait to include the reset method.

@flavio flavio force-pushed the replace-instant-with-backon branch from 53cc42e to a7d6e70 Compare November 29, 2024 11:46
@flavio
Copy link
Contributor Author

flavio commented Nov 29, 2024

About the rustfmt linter... I've updated my rust nightly and ran just fmt, everything is fine locally 🤔

@flavio
Copy link
Contributor Author

flavio commented Nov 29, 2024

Also, the tarpaulin failure doesn't seem related with this PR

@nightkr
Copy link
Member

nightkr commented Nov 29, 2024

Yeah.. ran into tarpaulin issues on my end too. Need to go figure out what's happening there. FWIW I took a stab at this as well (#1652) since I didn't realize you had picked it up too.

@flavio
Copy link
Contributor Author

flavio commented Dec 2, 2024

@nightkr: ouch, I didn't notice you had just submitted a similar PR 😓

@nightkr
Copy link
Member

nightkr commented Dec 2, 2024

Didn't realize you had actually picked up the issue 😅

@flavio
Copy link
Contributor Author

flavio commented Jan 14, 2025

A lot of work has already gone into #1652 . I'm closing this PR

@flavio flavio closed this Jan 14, 2025
@flavio flavio reopened this Jan 17, 2025
@flavio
Copy link
Contributor Author

flavio commented Jan 17, 2025

I've re-opened it based on what @clux said here.

@clux: let me know if there's anything I need to change inside of this PR

@flavio flavio force-pushed the replace-instant-with-backon branch from a7d6e70 to f085689 Compare January 17, 2025 09:23
@flavio
Copy link
Contributor Author

flavio commented Jan 17, 2025

I've rebased the initial code against the current main branch

@flavio flavio force-pushed the replace-instant-with-backon branch from f085689 to 102dd43 Compare January 17, 2025 09:26
@flavio
Copy link
Contributor Author

flavio commented Jan 17, 2025

Fixed rustfmt warnings with nightly

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

This one is fine with me, thank you. I'll let @nightkr chime in before merging. unfortunatete that you both made implementations at the same time!

Don't worry too much about the unrelated clippy warnings from the other PR, they can always be fixed after.

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 55.31915% with 21 lines in your changes missing coverage. Please review.

Project coverage is 75.8%. Comparing base (6a980c6) to head (5694214).

Files with missing lines Patch % Lines
kube-runtime/src/watcher.rs 0.0% 16 Missing ⚠️
kube-runtime/src/utils/backoff_reset_timer.rs 69.3% 4 Missing ⚠️
kube-runtime/src/controller/mod.rs 0.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1653     +/-   ##
=======================================
- Coverage   75.9%   75.8%   -0.0%     
=======================================
  Files         84      84             
  Lines       7611    7630     +19     
=======================================
+ Hits        5771    5779      +8     
- Misses      1840    1851     +11     
Files with missing lines Coverage Δ
kube-runtime/src/events.rs 98.1% <100.0%> (ø)
kube-runtime/src/utils/mod.rs 62.3% <ø> (ø)
kube-runtime/src/utils/stream_backoff.rs 88.2% <100.0%> (+1.5%) ⬆️
kube-runtime/src/utils/watch_ext.rs 22.3% <ø> (ø)
kube-runtime/src/controller/mod.rs 30.2% <0.0%> (ø)
kube-runtime/src/utils/backoff_reset_timer.rs 75.0% <69.3%> (-7.1%) ⬇️
kube-runtime/src/watcher.rs 42.7% <0.0%> (-1.7%) ⬇️

Replace the `backoff` dependency with `backon`. The former one is no
longer maintained and is also pulling the `instant` crate, which has
been marked as unmaintained by RUSTSEC.

Prior to this commit the public API of kube-rs exposed a trait defined
by the `backoff` crate. This commits introduces a new trait defined by
kube-rs, which wraps the `backon` trait.

Fixes kube-rs#1635

Signed-off-by: Flavio Castelli <[email protected]>
@flavio flavio force-pushed the replace-instant-with-backon branch from 102dd43 to 5694214 Compare January 17, 2025 09:43
@flavio
Copy link
Contributor Author

flavio commented Jan 17, 2025

I've fixed the clippy warnings

@nightkr
Copy link
Member

nightkr commented Jan 17, 2025

I'm not going to hard-block this, but I don't feel super comfortable with how we end up baking the boundary details into our API here. It also feels like we're not really gaining much from reusing backon if our API ends up being incompatible with them anyway.

@Xuanwo is this sort of adaptive reset policy something you'd be interested in tackling upstream at some point? I agree with Xuanwo/backon#52 (comment) that a "manual" reset() is kind of redundant, but it can end up helpful when trying to manage the state over time.

@Xuanwo
Copy link

Xuanwo commented Jan 17, 2025

@Xuanwo is this sort of adaptive reset policy something you'd be interested in tackling upstream at some point? I agree with Xuanwo/backon#52 (comment) that a "manual" reset() is kind of redundant, but it can end up helpful when trying to manage the state over time.

Hi, I'm interested in starting a discussion on this to learn more about your use case. I don't understand the difference between starting a new retry and resetting the API currently. More input would be greatly appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RUSTSEC-2024-0384: instant is unmaintained
4 participants