Move ratelimiter to its own crate #580

acatangiu · 2018-11-12T10:44:11Z

Refactored vmm-config/drive.rs to eliminate the need to clone any BlockDeviceConfig.
Moved the rate limiter implementation from fc_util to its own crate.
Eliminated TokenBucketDescription and unified uses to TokenBucket.
Eliminated RateLimiterDescription and implemented custom deserialization for RateLimiter so that a valid RateLimiter object is built at deserialization and can be used thereafter.

- Improved interface for `BlockDeviceConfigs` struct. - `BlockDeviceConfigs` internal list is now on top of `VecDeque`. - Refactored code so that `BlockDeviceConfig` structs are no longer cloned. - Fixed an issue where updating a block device to be root could result in an invalid configuration for `BlockDeviceConfigs` because the updated root device was not moved to the front of the queue. - Both Block and Net configs have test-only clone implementations that ignore the rate limiter. Signed-off-by: Adrian Catangiu <acatan@amazon.com>

alexandruag

Some general comments (or related to things which are hard to reach via GitHub comments):

There may be some cargo fmt issues. rustup update stable first might also be a good idea.
There are some publicly facing things which don't have doc comments. You can add #![warn(missing_docs)] at the top of rate_limiter/src/lib.rs, and you will see them listed when building.
Why isn't the TokenBucket API (new(), reduce(), replenish()) also public?
It looks like RateLimiter::get_token_bucket() can be moved to mod tests.
If, for some reason, you really want to have TokenBucket implement Serialize, we have to make sure
the processed_* members are recomputed during deserialization, because they are not serialized, and only new() currently updates them.
Nice to have (should have am associated issue otherwise as it's a pretty nice first PR opportunity for someone): the rate_limiter crate has everything bunched up in lib.rs. This is actually a very good example of situation where splitting things into modules, and then pub use-ing stuff in lib.rs to give the appearance of a flat namespace, makes a lot of sense.

alexandruag · 2018-11-12T13:46:33Z

rate_limiter/src/lib.rs

+    bandwidth: Option<TokenBucket>,
+    ops: Option<TokenBucket>,
+
+    #[serde(skip_deserializing, skip_serializing)]


U can replace this with the shorthand #[serde(skip)].

Was already done in a later commit.

alexandruag · 2018-11-12T13:52:28Z

rate_limiter/src/lib.rs

@@ -237,14 +238,35 @@ pub enum TokenType {
 /// RateLimiters will generate events on the FDs provided by their `AsRawFd` trait
 /// implementation. These events are meant to be consumed by the user of this struct.
 /// On each such event, the user must call the `event_handler()` method.
+#[derive(Deserialize, Serialize)]


I think we should either not derive Serialize here, or implement Clone for RateLimiter as the equivalent of serialize() followed by deserialize(), because semantically it's the same thing. This also applies to TokenBucket.

Removed Serialize altogether.

alexandruag · 2018-11-12T14:04:41Z

rate_limiter/src/lib.rs

+    }
+}
+
+use std::fmt;


This is ugly in the middle of the file like that :-s

alexandruag · 2018-11-12T14:11:26Z

rate_limiter/src/lib.rs

+                        }
+                    }
+                }
+                let bandwidth = bandwidth.unwrap_or(TokenBucket::default());


I think there's already an unwrap_or_default() method.

alexandruag · 2018-11-12T14:36:30Z

rate_limiter/Cargo.toml

 serde = ">=1.0.27"
 serde_derive = "=1.0.27"

-fc_util = { path = "../fc_util" }
+logger = { path = "../logger" }


Is this still needed here?

Yes, we're logging an error! when trying to consume more tokens that full capacity.

This looks like a problem, because we have a private dependency in this crate, which is supposed to also be a stand-alone library at some point. Moreover, also related to the library angle, we're pretty much forcing an operation on the user by having that error! invocation there. One potential solution involves this small piece of logic being invoked by the caller of reduce() based on some sort of return value.

If we don't get rid of this right now, we should at least create an issue, because I think the crate is not really usable as a generic library otherwise :-s

No need to address this now. I wouldn't even create an issue for it. When we decide to publish this crate, this will just be one of the things we naturally have to address.
It is besides the point of this PR.

Well, the thing is moving logic to its own crate is mainly about making it only depend on crates which are (going to be) publicly available and individually usable from crates.io. Otherwise, in this case for example, we could've just left the rate limiter in fc_util, and never bother with separating it. It's fine to have this as a multi-step process, the first part of which is this PR, but the work is not complete to that end. Having an issue makes it easier to plan things before actually starting them, and makes it obvious for other people this still needs to be done, just in case they might want to pick it up.

Also, not everything about this is naturally evident. For example, we could replace the internal dependency to logger with one using the public log crate. At this point, things look fine, but the point about getting the error! macro invocation out of the method still stands, and it's not necessarily super obvious (even less so to someone not following this PR).

alexandruag · 2018-11-12T14:43:46Z

rate_limiter/src/lib.rs

+
+impl TokenBucket {
+    /// Creates a TokenBucket
+    ///  @total_capacity: the total capacity of the token bucket


This is using a different doc comment style than other functions, namely it's missing the Arguments subsection, and it's using @param_name: description... instead of param_name - description`. It's nice to use the same style if the output actually looks different between the two.

alxiord · 2018-11-13T11:51:49Z

1c07b95 has a fmt error.

alxiord · 2018-11-13T13:02:47Z

rate_limiter/src/lib.rs

+use timerfd::{ClockId, SetTimeFlags, TimerFd, TimerState};
+
+#[derive(Debug)]
+pub enum Error {


Undocumented pub enum

alxiord · 2018-11-13T13:06:16Z

rate_limiter/src/lib.rs

-#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)]
+/// Enum that describes the type of token used.
+///
+/// `TokenType::Bytes` tokens are used for bandwidth limiting, while


Undocumented pub enum fields - you could move the comments above the enum definition just before each enum variant.

Suggested change

/// `TokenType::Bytes` tokens are used for bandwidth limiting, while

pub enum TokenType {

/// Tokens used for bandwidth limiting.

Bytes,

/// Tokens used for operations/second limiting.

Ops,

}

Signed-off-by: Adrian Catangiu <acatan@amazon.com>

Make TokenBucket look the same as TokenBucketDescription. Use and (de)serialize on TokenBucket instead of using extra struct TokenBucketDescription. Signed-off-by: Adrian Catangiu <acatan@amazon.com>

Incremental step to (de)serialize on RateLimiter instead of using extra struct RateLimiterDescription. Signed-off-by: Adrian Catangiu <acatan@amazon.com>

Added custom implementation for the Deserialize trait to allow fully building a RateLimiter on deserialization. Signed-off-by: Adrian Catangiu <acatan@amazon.com>

Switch to using only RateLimiter and do (de)serialization directly on that. RateLimiter is completely built on Deserialization and ownership is passed around all the way to the consumer. Signed-off-by: Adrian Catangiu <acatan@amazon.com>

Signed-off-by: Adrian Catangiu <acatan@amazon.com>

BlockDeviceConfig and NetworkInterfaceConfig structures contain unified data: description, state and low-level resources. Low-level resources cannot be serialized and just ignoring them is not the correct approach. Serializing then deserializing an object should result in identical objects which in this case cannot happen. Since it's not actually used, let's just disable serialization. Signed-off-by: Adrian Catangiu <acatan@amazon.com>

alexandruag · 2018-11-14T10:26:40Z

Created #589.

acatangiu self-assigned this Nov 12, 2018

alexandruag suggested changes Nov 12, 2018

View reviewed changes

alxiord self-requested a review November 13, 2018 11:02

alxiord reviewed Nov 13, 2018

View reviewed changes

acatangiu added 7 commits November 13, 2018 16:24

rate-limiter: move from fc_util to its own crate

e2dc845

Signed-off-by: Adrian Catangiu <acatan@amazon.com>

rate-limiter: remove TokenBucketDescription

5c74b1a

Make TokenBucket look the same as TokenBucketDescription. Use and (de)serialize on TokenBucket instead of using extra struct TokenBucketDescription. Signed-off-by: Adrian Catangiu <acatan@amazon.com>

rate-limiter: refactor to allow (de)serialization

f6f1dd5

Incremental step to (de)serialize on RateLimiter instead of using extra struct RateLimiterDescription. Signed-off-by: Adrian Catangiu <acatan@amazon.com>

rate-limiter: implement custom Deserialize

47625b7

Added custom implementation for the Deserialize trait to allow fully building a RateLimiter on deserialization. Signed-off-by: Adrian Catangiu <acatan@amazon.com>

rate-limiter: remove RateLimiterDescription

c6a4df8

Switch to using only RateLimiter and do (de)serialization directly on that. RateLimiter is completely built on Deserialization and ownership is passed around all the way to the consumer. Signed-off-by: Adrian Catangiu <acatan@amazon.com>

rate-limiter: reiterate on documentation

63ab328

Signed-off-by: Adrian Catangiu <acatan@amazon.com>

alxiord approved these changes Nov 13, 2018

View reviewed changes

alexandruag approved these changes Nov 14, 2018

View reviewed changes

acatangiu merged commit e065c03 into firecracker-microvm:master Nov 14, 2018

andreeaflorescu mentioned this pull request Nov 20, 2018

Rate Limiter - refactoring #545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ratelimiter to its own crate #580

Move ratelimiter to its own crate #580

acatangiu commented Nov 12, 2018

alexandruag left a comment

alexandruag Nov 12, 2018

acatangiu Nov 13, 2018

alexandruag Nov 12, 2018

acatangiu Nov 13, 2018

alexandruag Nov 12, 2018

acatangiu Nov 13, 2018

alexandruag Nov 12, 2018

acatangiu Nov 13, 2018

alexandruag Nov 12, 2018

acatangiu Nov 13, 2018

alexandruag Nov 13, 2018

acatangiu Nov 13, 2018

alexandruag Nov 13, 2018

alexandruag Nov 12, 2018

acatangiu Nov 13, 2018

alxiord commented Nov 13, 2018

alxiord Nov 13, 2018

acatangiu Nov 13, 2018

alxiord Nov 13, 2018

acatangiu Nov 13, 2018

alexandruag commented Nov 14, 2018

Move ratelimiter to its own crate #580

Move ratelimiter to its own crate #580

Conversation

acatangiu commented Nov 12, 2018

alexandruag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxiord commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandruag commented Nov 14, 2018