runtime: improvement/test struct for api mode watcher

[daemonfire300]

Motivation

Motivation is #1528
Instead of reviving the stale PR I attempted a new approach from scratch.

Solution

IMPORTANT NOTE: Contains unrelated files that are currently tracked in git because they have to be otherwise my devShell does not work (flake, nix, direnv). So feel free to ignore the flake.nix+flake.lock, if you want me to and like the nix devShell I can put that on a dedicated second PR. Also added a helper and git hooks for myself to have a Cargo.lock on hand, because I prefer having one when developing within a nix devShell, makes it more reproducible. I.e., I am going to remove those files from the PR later.

It's not much and the change is NOT complete, before continuing I wanted to discuss the "interface" and behaviour of test struct(s). For example does the Duration i.e., ability to sleep/wait between calls to watch make sense, are we missing something to model a certain test. (Also I am not 100% sure whether the location of where I put the tests is the desired location for using that test struct.)

Happy to receive feedback.

[codecov]

Codecov Report

❌ Patch coverage is 94.44444% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.1%. Comparing base (8e2c23e) to head (c7961bf).
⚠️ Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
kube-runtime/src/stub_watcher.rs	86.7%	8 Missing ⚠️
kube-runtime/src/watcher.rs	98.4%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2022     +/-   ##
=======================================
+ Coverage   77.3%   78.1%   +0.8%     
=======================================
  Files         89      94      +5     
  Lines       9016    9220    +204     
=======================================
+ Hits        6967    7198    +231     
+ Misses      2049    2022     -27     

Files with missing lines	Coverage Δ
kube-runtime/src/watcher.rs	76.5% <98.4%> (+17.7%)	⬆️
kube-runtime/src/stub_watcher.rs	86.7% <86.7%> (ø)

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[daemonfire300]

cc @mateiidavid since you have been on the old PR.

[daemonfire300]

I ll try to do a proper fix of the clippy issues tomorrow. Caught me by surprise because afaik just clippy does not run for --test and I get a bit of unrelated fallout from files I did not touch as well.

[doxxx93]

Don't worry about the msrv failure, it's unrelated to your PR. enum-ordinalize 4.4.0 (via educe) just raised its MSRV to 1.89, which breaks our 1.88 check on any lockfile-less resolve.

cc) @clux should we consider CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS ?

[daemonfire300]

Any input on this so far?

[daemonfire300]

switched to draft as it technically is a DRAFT.

[clux]

thanks a lot for trying this. some initial comments.

this is definitely on the complex side, but the watcher is also complex, so it's not unexpected. the streaming setup and bookeeping seems very sensible, my biggest source of confusion and hesitation is with the input structures (for fixtures and sequence), and curious if there are alternatives along those lines.

i've put down some comments. mostly thinking out loud for now. still need some time to think about this. don't feel like you have to reply to all of it.

[clux]

I like the recorder and the way you are storing them within the test mode.

Some documentation on how it works and and what the strings represents would be useful.

[clux]

afaikt TestStream is exclusively dedicated for streaming things for .watch?

What happens in the case of errors and we need to return to an initial list?

[daemonfire300]

Thanks for the remark: I think TestStream is insufficient. I re-read my implementation mutliple times now and TestStream only allows to model the outer happy path, not the outer Err case and internally it allows for both.

I.e., any call to watch will succeed if we would use it as is, which might not be desired/missing functionality. Will put that on my list of things to fix.

The part of "return an initial list" is, however, not clear to me. Either it is an error or we return a list, this should be this section https://github.com/daemonfire300/kube/blob/817092633873d7d84d41c3a8c10e64fd02acf07f/kube-runtime/src/watcher.rs#L674-L687 unless I am missing something

flowchart TD
    testMode["TestMode"]
    watch["watch"]
    sequences["inner Vec Sequence<...>"]
    error["Err(...)"]
    success["Ok(TestStream)"]

    testMode --- watch
    watch --> sequences
    watch -->|missing| error
    sequences -->|pop Vec Seq and put into Stream| success

    style error stroke:#ff3333,color:#ff3333,stroke-width:2px
    style success stroke:#20a83a,color:#20a83a,stroke-width:2px

Loading

[daemonfire300]

The behaviour of cosntructing the TestStream on a call to watch is broken. I have Vec<Seq<..>> and pop from it, and then I think mistakenly create a single element Vec<Seq<..>> off of that and put it into the stream which is (a) confusing and (b) not what I intended.

[daemonfire300]

I also need to fix the behaviour of poll_next, Poll::Ready(None) terminates the stream instead of returning an empty result (the case for Sequence::Empty), which is confusing and "wrong".

[clux]

maybe it helps to write some failing tests for the last cases for what you want first so you can hammer on the complex (sounding) behavior.

if the internals in the stream impl ends up being a bit ugly to accommodate the various test scenarios, that would still be a good tradeoff.

[daemonfire300]

thanks a lot for trying this. some initial comments.

this is definitely on the complex side, but the watcher is also complex, so it's not unexpected. the streaming setup and bookeeping seems very sensible, my biggest source of confusion and hesitation is with the input structures (for fixtures and sequence), and curious if there are alternatives along those lines.

i've put down some comments. mostly thinking out loud for now. still need some time to think about this. don't feel like you have to reply to all of it.

Thank you very(!) much for the thorough review. Yes the tests are the weakest part. Will go over your points and (a) try to address them and (b) make things easier to understand.

[daemonfire300]

### NOTE: This still needs to addressed, because it's not necessarily proper behavior.

[clux]

I think this looks a lot better now. The tests read better, and are mostly understandable from reading the tests alone without having to dig deep into the implementation. You've addressed a lot of my concerns.

I don't have many more bikesheds on the actual code bits themselves, but I do think it would be interesting to try to add 2 more test cases to see if the abstraction can handle reality. Particularly around error handling and initial lists;

1. does the test setup support a streaming list init?
2. can we inject an error in the watch stream, and have it recover and get the next element?

[clux]

tiny nits. mostly about documentation.

[clux]

if that's the reason you could maybe use a VecDeque slightly more ergonomically. given perf isn't really a concern here.

[daemonfire300]

It was VecDeque initially, then I thought technically we only need to remove (pop) elements and to not need the whole "baggage" VecDeque comes with. I have little experience w.r.t. to VecDeque vs Vec with small data but a lot of objects. I guess I am still paranoid because I got stung very much by HashMap in a private project. But you suggestion sounds sensible. 👍

[clux]

end result looks a little cleaner 👍

[clux]

doc comment outdated now though (no mut, we don't reverse anymore)

[daemonfire300]

I think this looks a lot better now. The tests read better, and are mostly understandable from reading the tests alone without having to dig deep into the implementation. You've addressed a lot of my concerns.

I don't have many more bikesheds on the actual code bits themselves, but I do think it would be interesting to try to add 2 more test cases to see if the abstraction can handle reality. Particularly around error handling and initial lists;

1. does the test setup support a streaming list init?

2. can we inject an error in the watch stream, and have it recover and get the next element?

Built a reference test, and I think this is broken by default by design, when I "re-use" my list case but port it to watch instead.
step infinity loops and the test hangs. TestMode is "depleted", but step continues so call it. Since I wrap TestMode using https://docs.rs/futures/latest/futures/stream/trait.StreamExt.html#method.fuse I am going to investigate this a bit more tomorrow. The primitive solution would probably to potential not use .fuse() to wrap the stream and have it's own state management and return some sort of error OR call step_trampolined manually instead, but I am not sure whether this would be very pleasant ergonomics. In either case, I think this is by design as I said earlier, because a k8s controller using watch is not expecting the stream to "ever end" regularly which conflicts with the semantics of https://docs.rs/futures/latest/futures/stream/trait.Stream.html#tymethod.poll_next

Poll::Ready(None) means that the stream has terminated, and poll_next should not be invoked again.

I must say, this is exciting 👍 (to be read unironically)

[daemonfire300]

There was a bug in TestMode watch, fixed it, I was returning only the first element of a SequenceStep::List and discarding the rest.

As for the infinite loop on stream end: I am going to choose an error and return that so that any test using this does not hang. The alternative is having a specific error/code path which is guarded by #[cfg(test)] but I would not like to put test specific code mixed into the heart of the watcher.rs.

[daemonfire300]

most likely some helper/marker/tombstone error like this

fn exhausted_watch_sequence() -> kube_client::Error {
    kube_client::Error::ReadEvents(std::io::Error::new(
        std::io::ErrorKind::UnexpectedEof,
        "TestMode watch sequence exhausted",
    ))
}

[clux]

this error matching to handle the stream is "over" could perhaps be encasulated a bit better so the tests are more readable (since it's a magic value in the error).

[clux]

I think we need one final test here that verifies the working -> error -> resync setup. ie.

#[tokio::test]
async fn streaming_list_resync() {

where (pseudocode)

        for expected in [
            "InitApply(a)",
            "InitApply(b)",
            "InitDone",
            "Apply(a)",
            "Error(WatchFailed)",
            "InitApply(a),
        ] 

so we can simulate the internal step machinery resuming properly in the case of errors.