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

Migrate from backoff to backon #1652

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Nov 28, 2024

Motivation

backoff has been unmaintained for a while now, which is starting to cause rustsec warnings (#1635). backon is a maintained alternative.

Fixes #1635.

Solution

This PR migrates all uses of backoff to backon. This is a pretty breaking change for anyone who interacts with our backoff system beyond letting Controller use its default.

backon::Backoff doesn't have an equivalent to backoff::Backoff::reset, so this PR adds a new ResettableBackoff wrapper system for those use cases.

Fixes kube-rs#1635

Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
@nightkr
Copy link
Member Author

nightkr commented Nov 28, 2024

Hm, locally all tests pass except for test::pod_can_exec_and_write_to_stdin_from_node_proxy, since that assumes that it is executed from a machine with a running kubelet. nvm it works after I enable port forwarding for it.

Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
@nightkr nightkr requested a review from a team November 28, 2024 16:21
@nightkr
Copy link
Member Author

nightkr commented Nov 28, 2024

/cc @Xuanwo

This seems to have been added in Rust 1.83 that was just released, and is only triggered by tarpaulin?

Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
@nightkr
Copy link
Member Author

nightkr commented Nov 28, 2024

Hm, really not sure about what's still making tarpaulin unhappy.. :/

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.

some minor nits and naming questions, but looks sensible.
did not expect to have two backon PRs today 😅

kube-runtime/src/controller/mod.rs Show resolved Hide resolved
//! Delays and deduplicates [`Stream`](futures::stream::Stream) items
//! Delays and deduplicates [`Stream`] items
Copy link
Member

Choose a reason for hiding this comment

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

has rustdoc gotten smarter lately? this stuff used to give broken links when running just doc

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, damn. You're right.. clippy lead me astray. Will revert.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, this actually seems to have come from the --no-deps flag given by just doc. Removing that generates the links correctly.

Copy link
Member

Choose a reason for hiding this comment

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

hopefully this is how docs.rs builds docs. i tried to find some info on this, but not much on their docs. it is possible to inject rustdoc args though, so should probably be fine

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the code that controls the arguments is https://github.com/rust-lang/docs.rs/blob/5ec4326126b2f244442f55f0183ca7523611f0df/crates/metadata/lib.rs#L253-L300. Building it as RUSTC_BOOTSTRAP=1 cargo rustdoc --lib -Zrustdoc-map --all-features -- --cfg docsrs does seem to work, and links to the corresponding docs.rs entries.

kube-runtime/src/utils/backoff_resettable.rs Show resolved Hide resolved
Comment on lines +9 to +11
/// Builder for [`ResetTimerBackoff`].
#[derive(Debug, Clone)]
pub struct ResetTimerBackoffBuilder<B> {
Copy link
Member

Choose a reason for hiding this comment

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

I do see the other PR is sidestepping this by adding a trait instead. https://github.com/kube-rs/kube/pull/1653/files#diff-32fcc57da3d0b322998a85b05a1a82dae3f76df733b8a878efa1c97695af386eL3-R13

not sure if this is better or not, but it's possibly less abstraction overhead?

Copy link
Member Author

Choose a reason for hiding this comment

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

That PR requires you to implement its Backoff trait manually for each use case, which adds a lot of (code) overhead, especially for downstream users that would be subject to the orphaning rules.

Copy link
Member

Choose a reason for hiding this comment

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

True, but are there realistically that many people that would need to tweak backoff behavior? Do we need to optimise for those people? We don't have examples/ to show people how to do it (which to me is indicative that this is not super high value). I can only think of people trying to use it as a half-baked solution to extremely tight consistency guarantees (by hammering the api-server more during network stress) - and that feels not like something we want to encourage. Maybe I am missing something?

The downside to this is complexity of our side; 4 types that we expose from utils/mod (with relations that are hard to decipher), a generic type that sneaks its way into type signatures (in an already complicated controller/mod).

Copy link
Member Author

@nightkr nightkr Dec 2, 2024

Choose a reason for hiding this comment

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

True, but are there realistically that many people that would need to tweak backoff behavior?

It just feels weird that it becomes.. almost composable, but not quite. Especially since there ends up being a lot of stutter in the implementation (looking at their PR, they have to hard-code the same constants in both ::new() and ::reset()).

a generic type that sneaks its way into type signatures (in an already complicated controller/mod).

We should be able to hide that by integrating it into the implementation of StreamBackoff.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should be able to hide that by integrating it into the implementation of StreamBackoff.

Ah right no, since BackoffBuilder doesn't make sense as a trait object, so that complicates the type of Controller.

@clux
Copy link
Member

clux commented Nov 29, 2024

not sure about tarpaulin either. ci is has tarpaulin pinned to 0.28.0 (so should not be a new version thing), and it should otherwise run equivalently locally with just tarpaulin. last stuff was a test failure in crd_derive_schema so maybe that had a stray failure? re-running.

@nightkr
Copy link
Member Author

nightkr commented Nov 29, 2024

Might be related to this error in the middle of the compile log..

/home/n/Documents/kube-rs/target/debug/deps/kube_derive-81b5765bf8276125: error while loading shared libraries: libstd-ca74a2d9c5166d9f.so: cannot open shared object file: No such file or directory

That doesn't happen if I use the Rust 1.82.0 toolchain instead (and tarpaulin completes successfully).

@nightkr
Copy link
Member Author

nightkr commented Dec 9, 2024

#1658 seems to have sorted out the CI issues at least.

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
2 participants