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

feat: add async path strategy service class #118

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Conversation

jpcsmith
Copy link
Contributor

@jpcsmith jpcsmith commented Jan 15, 2024

This adds path lookup strategies.

Pending tasks:

  • Remove the PathRefresher strategy into from this PR.
    • Add tests for the PathRefresher strategy.
    • Add integration tests for the PathRefresher strategy.
  • From the latency strategy from this PR.
  • Implement the AsyncPathService trait for the AsyncPathStrategy
  • Fix brittle tests that rely on timing and so fail in CI.
  • Cleanup various TODOs.

@jpcsmith jpcsmith self-assigned this Jan 15, 2024
@jpcsmith
Copy link
Contributor Author

Hey @mlegner , could you please have a look at this draft? Specifically, at the AsyncPathStrategy implementation and the AsyncPathService and PathStrategy traits which will be the basis for this PR.

If you want an idea of the intended implementation of a PathStrategy, there is the PathRefresher strategy as a reference.

@mlegner
Copy link
Contributor

mlegner commented Jan 15, 2024

Thanks a lot for adding this important piece, @jpcsmith. I will review the code today.

Copy link
Contributor

@mlegner mlegner left a comment

Choose a reason for hiding this comment

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

Thanks again for this addition!

I haven't fully wrapped my head around tho whole design; so let me summarize my current understanding to make sure it is more or less correct:

  • We have a path service (AsyncPathService), which provides a way to request paths, a path strategy (PathStrategy), which determines when to refresh paths and how to cache them, and a controller (AsyncPathStrategy and AsyncPathStrategyInner), which combines the two and provides a convenient async interface.
  • Client code would instantiate an AsyncPathStrategy with an appropriate PathStrategy and PathService, which results in a new tokio task that performs the query and refresh operations.

Question: What exactly does the client do when it needs a path?

Question: How do we determine the best path of a set of paths? Would this be an additional component PathSelector or PathRater? There is the PathOrdering in the PathRefresher, but I don't really see what role that plays.

I'm not sure I fully understand the PathRefresher implementation. Maybe I need to think about it some more...

crates/scion-proto/src/path.rs Show resolved Hide resolved
crates/scion/src/pan/path_service.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategies.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategies.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategies/async_strategy.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategies/path_refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategies/path_refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategies/path_refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategies/path_refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategies.rs Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jan 15, 2024

Code Coverage

Package Line Rate Health
crates/scion-proto/src/packet 81%
crates/scion-proto/src/path/standard 92%
crates/scion/src 77%
crates/scion-proto/src/path/metadata 100%
crates/scion/src/pan 63%
crates/scion-proto/src/packet/headers 85%
crates/scion-proto/src/path 83%
crates/scion-proto/src/reliable 95%
crates/scion-proto/src/scmp 77%
crates/scion-proto/src 79%
crates/scion/src/daemon 95%
crates/scion-proto/src/address 70%
crates/scion/src/pan/path_strategy 72%
Summary 79% (1672 / 2121)

@jpcsmith
Copy link
Contributor Author

Hey Markus, thanks for the feedback. Please see my responses inline below.

Thanks again for this addition!

I haven't fully wrapped my head around tho whole design; so let me summarize my current understanding to make sure it is more or less correct:

  • We have a path service (AsyncPathService), which provides a way to request paths, a path strategy (PathStrategy), which determines when to refresh paths and how to cache them, and a controller (AsyncPathStrategy and AsyncPathStrategyInner), which combines the two and provides a convenient async interface.
  • Client code would instantiate an AsyncPathStrategy with an appropriate PathStrategy and PathService, which results in a new tokio task that performs the query and refresh operations.

Your understanding is correct. AsyncPathService would, for example, be implemented by the DaemonClient that we already have.

Question: What exactly does the client do when it needs a path?

AsyncPathStrategy also implements AsyncPathService itself (with a WIP implementation in the most recent commit). That was a pending todo item. My apologies that that was not clearer.

Question: How do we determine the best path of a set of paths? Would this be an additional component PathSelector or PathRater? There is the PathOrdering in the PathRefresher, but I don't really see what role that plays.

The idea here was that the "best" path would be PathStrategy specific. There is no other component. In the case of PathRefresher, this would be the path with the lowest number of hops, as described below:

/// When queried for a path, it returns a path with the lowest number of interface
/// hops that is not nearly expired. If no such paths are found, it returns a path
/// with the lowest number of hops that is nearly expired; or otherwise None.
pub struct PathRefresher {

That being said, this could be achieved by wrapping an existing path strategy (such as a path refresher) with a strategy that selects from among the available paths, or PathRefresher could be further generalised.

I'm not sure I fully understand the PathRefresher implementation. Maybe I need to think about it some more...

Okay, it's not part of this PR and likely has several logic errors as there are currently no tests. It's now commented out as it breaks with my most recent changes.

@jpcsmith jpcsmith force-pushed the feat/path-refresher branch 2 times, most recently from 1bad7dc to 56ad3cc Compare January 16, 2024 15:11
@jpcsmith jpcsmith marked this pull request as ready for review January 16, 2024 15:14
@jpcsmith jpcsmith requested a review from mlegner January 16, 2024 15:17
Copy link
Contributor

@mlegner mlegner left a comment

Choose a reason for hiding this comment

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

I went through all the code except the tests and everything looks great. As usual, just some minor points.

I will look at the tests after lunch but don't expect anything major there. 🙂

crates/scion/src/pan/path_service.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_service.rs Show resolved Hide resolved
crates/scion/src/pan/path_strategy.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy.rs Show resolved Hide resolved
crates/scion/src/pan/path_strategy.rs Show resolved Hide resolved
crates/scion/src/pan/path_strategy/async_strategy.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/async_strategy.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@mlegner mlegner left a comment

Choose a reason for hiding this comment

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

Nice test setup. I haven't done any mocking in Rust before, so I cannot really provide any deep feedback. I can just say that from my reading of the mockall documentation, this all seems reasonable. 😅

crates/scion/src/pan/path_strategy/async_strategy.rs Outdated Show resolved Hide resolved
@mlegner mlegner merged commit e719110 into main Jan 17, 2024
11 checks passed
@mlegner mlegner deleted the feat/path-refresher branch January 17, 2024 17:35
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.

2 participants