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

Upgrade to all dependencies. #123

Closed
wants to merge 2 commits into from
Closed

Conversation

markmandel
Copy link
Member

This upgrades all dependencies, which includes some breaking changes to Tokio, with adjustments to our code base to handle the changes.

For example: UdpSocket can no longer be split. The Tokio advice was to wrap the UdpSocket in an Arc. Also, the API surface for Tokio watch channels has also changed.

@markmandel markmandel added the kind/cleanup Refactoring code, fixing up documentation, etc label Oct 30, 2020
@google-cla google-cla bot added the cla: yes label Oct 30, 2020
@markmandel
Copy link
Member Author

Still working through some compilation issues, would love some help @iffyio - any ideas?

➜  quilkin git:(cleanup/upgrade-dependencies) ✗ cargo test --tests
   Compiling quilkin v0.1.0 (/home/markmandel/workspace/quilkin)
error[E0277]: the trait bound `bytes::Bytes: warp::Buf` is not satisfied
   --> src/xds/cluster.rs:102:43
    |
102 |             let cluster = Cluster::decode(Bytes::from(resource.value))
    |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `warp::Buf` is not implemented for `bytes::Bytes`
    |
    = note: required by `prost::Message::decode`

error[E0277]: the trait bound `bytes::Bytes: warp::Buf` is not satisfied
   --> src/xds/cluster.rs:194:60
    |
194 |             let assignment = ClusterLoadAssignment::decode(bytes).map_err(|err| {
    |                                                            ^^^^^ the trait `warp::Buf` is not implemented for `bytes::Bytes`
    |
    = note: required by `prost::Message::decode`

error[E0277]: the trait bound `bytes::Bytes: warp::Buf` is not satisfied
   --> src/xds/cluster.rs:102:43
    |
102 |             let cluster = Cluster::decode(Bytes::from(resource.value))
    |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `warp::Buf` is not implemented for `bytes::Bytes`
    |
    = note: required by `prost::Message::decode`

error[E0277]: the trait bound `bytes::Bytes: warp::Buf` is not satisfied
   --> src/xds/cluster.rs:194:60
    |
194 |             let assignment = ClusterLoadAssignment::decode(bytes).map_err(|err| {
    |                                                            ^^^^^ the trait `warp::Buf` is not implemented for `bytes::Bytes`
    |
    = note: required by `prost::Message::decode`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0277`.
error: could not compile `quilkin`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0277`.
error: build failed

@markmandel
Copy link
Member Author

Interesting - the CI system (which is Rust 1.145) gives a slightly different error:

error[E0277]: the trait bound `bytes::bytes::Bytes: bytes::buf::buf_impl::Buf` is not satisfied
    --> src/xds/cluster.rs:102:43
     |
102  |             let cluster = Cluster::decode(Bytes::from(resource.value))
     |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `bytes::buf::buf_impl::Buf` is not implemented for `bytes::bytes::Bytes`
     |
help: trait impl with same name found
    --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/bytes-0.6.0/src/buf/buf_impl.rs:1010:1
     |
1010 | / impl<T: Buf + ?Sized> Buf for &mut T {
1011 | |     deref_forward_buf!();
1012 | | }
     | |_^
     = note: perhaps two different versions of crate `bytes` are being used?
     = note: required by `prost::message::Message::decode`

error[E0277]: the trait bound `bytes::bytes::Bytes: bytes::buf::buf_impl::Buf` is not satisfied
    --> src/xds/cluster.rs:194:47
     |
194  |                 ClusterLoadAssignment::decode(Bytes::from(resource.value)).map_err(|err| {
     |                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `bytes::buf::buf_impl::Buf` is not implemented for `bytes::bytes::Bytes`
     |
help: trait impl with same name found
    --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/bytes-0.6.0/src/buf/buf_impl.rs:1010:1
     |
1010 | / impl<T: Buf + ?Sized> Buf for &mut T {
1011 | |     deref_forward_buf!();
1012 | | }
     | |_^
     = note: perhaps two different versions of crate `bytes` are being used?
     = note: required by `prost::message::Message::decode`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0277`.
error: could not compile `quilkin`.

(I'm running 1.47 locally)

@markmandel
Copy link
Member Author

Nvm, worked it out! bytes was too far into the future.

@markmandel markmandel force-pushed the cleanup/upgrade-dependencies branch 2 times, most recently from 0213309 to 79e3790 Compare October 31, 2020 00:33
@markmandel
Copy link
Member Author

We are now compiling. But I have some hanging tests. Working that part out now.

@markmandel
Copy link
Member Author

markmandel commented Oct 31, 2020

Okay, narrowed something down. it's the integration test metrics_server that is failing - the round trip packet never gets received, and I'm not sure why.

Weirdly, if I replace t.run_server_with_metrics(server_config, server_metrics) with t.run_server(server_config) the packet successfully round trips, but then metric tests then fail.

@markmandel
Copy link
Member Author

Narrowed it down further.

Inside start_metrics_server, the warp function bind_with_graceful_shutdown looks to be blocked on the bind! function, and it never resolves itself.

@markmandel
Copy link
Member Author

Not sure if red herring or not, but I think warp is complaining there's not a Tokio engine started, which seems weird.

failures:

---- tests::metrics_server stdout ----
thread 'tests::metrics_server' panicked at 'there is no reactor running, must be called from the context of Tokio runtime', /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.22/src/io/driver/mod.rs:202:14
stack backtrace:
   0: rust_begin_unwind
             at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39/library/std/src/panicking.rs:475
   1: core::panicking::panic_fmt
             at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39/library/core/src/panicking.rs:85
   2: core::option::expect_failed
             at /rustc/18bf6b4f01a6feaf7259ba7cdae58031af1b7b39/library/core/src/option.rs:1213
   3: core::option::Option<T>::expect
             at /home/markmandel/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:333
   4: tokio::io::driver::Handle::current
             at /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.22/src/io/driver/mod.rs:201
   5: tokio::io::registration::Registration::new_with_ready
             at /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.22/src/io/registration.rs:106
   6: tokio::io::poll_evented::PollEvented<E>::new_with_ready
             at /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.22/src/io/poll_evented.rs:206
   7: tokio::io::poll_evented::PollEvented<E>::new
             at /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.22/src/io/poll_evented.rs:178
   8: tokio::net::tcp::listener::TcpListener::from_std
             at /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.22/src/net/tcp/listener.rs:268
   9: hyper::server::tcp::AddrIncoming::from_std
             at /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.13.8/src/server/tcp.rs:33
  10: hyper::server::tcp::AddrIncoming::new
             at /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.13.8/src/server/tcp.rs:29
  11: hyper::server::tcp::AddrIncoming::bind
             at /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.13.8/src/server/tcp.rs:47
  12: warp::server::Server<F>::bind_with_graceful_shutdown::{{closure}}
             at /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/warp-0.2.5/src/server.rs:99
  13: warp::server::Server<F>::bind_with_graceful_shutdown
             at /home/markmandel/.cargo/registry/src/github.com-1ecc6299db9ec823/warp-0.2.5/src/server.rs:283
  14: quilkin::proxy::metrics::start_metrics_server
             at ./src/proxy/metrics.rs:46
  15: quilkin::proxy::server::Server::run::{{closure}}
             at ./src/proxy/server.rs:58

@markmandel
Copy link
Member Author

...and I found the issue. Warp doesn't support a Tokio of version 0.3 yet
seanmonstar/warp#725

@iffyio
Copy link
Collaborator

iffyio commented Oct 31, 2020

...and I found the issue. Warp doesn't support a Tokio of version 0.3 yet
seanmonstar/warp#725

Ah 😞 we likely would need to wait for warp/hyper to update first? It sounded like they're already on it though hopefully not too long

@markmandel
Copy link
Member Author

awslabs/aws-lambda-rust-runtime#266 (comment)

Might be a potential workaround in the meantime.

@iffyio
Copy link
Collaborator

iffyio commented Nov 2, 2020

awslabs/aws-lambda-rust-runtime#266 (comment)

Might be a potential workaround in the meantime.

That looks promising if its not much work, from the docs it sounds like it should be enough with .compat().await?; on the main function but maybe there's something more. Otherwise we could wait for hyper to upgrade, there's a PR in the works so likely that'll land pretty soon hyperium/hyper#2302 and warp can also upgrade

@markmandel
Copy link
Member Author

Looks like it's worth taking for a spin anyway. I'm behind on some Agones reviews, so I should probably head back over there for a bit, then come back and see if I can make this work.

Oh yeah, and finish my other PR's here and review your work too 🤣 😭

This upgrades all dependencies, which includes some breaking changes to
Tokio, with adjustments to our code base to handle the changes.

For example: UdpSocket can no longer be split. The Tokio advice was to
wrap the UdpSocket in an Arc. Also, the API surface for Tokio watch
channels has also changed.
@markmandel
Copy link
Member Author

Tried a bunch of stuff with tokio-compat-02 - couldn't manage to get it to work. Asked on discord, and also filed:
LucioFranco/tokio-compat-02#9

@markmandel
Copy link
Member Author

I got REALLY close with help from Tokio team on Discord.

The only issue is, the function now blocks here:

operation.compat().await;

And I can't work out how to make it run in the background with tokio::spawn()

@markmandel
Copy link
Member Author

I'm going to close this for now, and we can return it it again when Hyper supports Tokio 0.3

@markmandel markmandel closed this Dec 2, 2020
@markmandel markmandel deleted the cleanup/upgrade-dependencies branch January 26, 2021 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes kind/cleanup Refactoring code, fixing up documentation, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants