neqo icon indicating copy to clipboard operation
neqo copied to clipboard

Published 20 hours ago • verified publisher icon mozilla

add initial RTT configurability

Open robamu opened this issue 1 year ago • 18 comments

This adds configurability for the initial round-trip time, which has a use-case for something like space-applications where a higher round-trip time might be expected. This is a first draft PR, an end-to-end test with other QUIC stacks still needs to be done.

robamu avatar Aug 05 '24 16:08 robamu

Failed Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest as server

All results

Succeeded Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest as server

Unsupported Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest as server

github-actions[bot] avatar Aug 05 '24 17:08 github-actions[bot]

Firefox builds for this PR

The following builds are available for testing. Crossed-out builds did not succeed.

github-actions[bot] avatar Aug 05 '24 18:08 github-actions[bot]

@robamu it would be helpful it you could compile the PR and run the test suite locally, just to make sure that CI doesn't immediately fail.

larseggert avatar Aug 29 '24 16:08 larseggert

Ah, I can do that.

robamu avatar Aug 29 '24 16:08 robamu

I have problems running the tests currently..

neqo on  initial-rtt-configurability is 📦 v0.8.2 via 🦀 v1.80.1
❯ cargo test
   Compiling neqo-bin v0.8.2 (/home/rmueller/Rust/neqo/neqo-bin)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 2.57s
     Running unittests src/lib.rs (target/debug/deps/neqo_bin-33111872ccada738)

running 1 test
0s  0ms ERROR Minimum NSS version of 3.103
 not supported
test tests::write_qlog_file ... FAILED

failures:

---- tests::write_qlog_file stdout ----
thread 'tests::write_qlog_file' panicked at neqo-bin/src/lib.rs:309:80:
called `Result::unwrap()` on an `Err` value: UnsupportedVersion
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::write_qlog_file

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass `-p neqo-bin --lib`

I can check and build the project though and I have installed NSS v3.103 according to https://www.linuxfromscratch.org/blfs/view/svn/postlfs/nss.html .. is there something special I need to take care of when installing NSS from the sources for the neqo tests? I can not use the Ubuntu package provides NSS tools/libraries because the version is too old.

robamu avatar Aug 29 '24 16:08 robamu

Are you sure you are linking against the LFS version of NSS you installed and not some other version? Check with ldd?

I suggest you follow what's in the readme, i.e., check out the sources and set the environment variables so that neqo will do the right thing.

larseggert avatar Aug 29 '24 18:08 larseggert

Running ldd yields the following:

neqo on  main [$] is 📦 v0.8.2 via 🦀 v1.80.1
❯ ldd /usr/lib/libnss3.so
/usr/lib/libnss3.so: /lib/x86_64-linux-gnu/libnssutil3.so: version `NSSUTIL_3.101' not found (required by /usr/lib/libnss3.so)
        linux-vdso.so.1 (0x00007ffce9f5d000)
        libnssutil3.so => /lib/x86_64-linux-gnu/libnssutil3.so (0x000072ffb3e2f000)
        libplc4.so => /lib/x86_64-linux-gnu/libplc4.so (0x000072ffb3e28000)
        libplds4.so => /lib/x86_64-linux-gnu/libplds4.so (0x000072ffb3e23000)
        libnspr4.so => /lib/x86_64-linux-gnu/libnspr4.so (0x000072ffb3de3000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x000072ffb3a00000)
        /lib64/ld-linux-x86-64.so.2 (0x000072ffb3fbd000)

so.. some problem with libnssutil3 oder some other missing library?

I also tried following the readme by setting NSS_DIR to the NSS sources path and then running a fresh build of neqo, but there seems to be some issue with the build.sh script of NSS.. I was only able to build NSS with make so far.

   Compiling neqo-crypto v0.8.2 (/home/rmueller/Rust/neqo/neqo-crypto)
error: failed to run custom build command for `neqo-crypto v0.8.2 (/home/rmueller/Rust/neqo/neqo-crypto)`

Caused by:
  process didn't exit successfully: `/home/rmueller/Rust/neqo/target/debug/build/neqo-crypto-5811bbeef5d53e68/build-script-build` (exit status: 101)
  --- stdout
  cargo:rustc-check-cfg=cfg(nss_nodb)
  cargo:rerun-if-env-changed=NSS_DIR
  NSPR [1/5] configure ...

  --- stderr
  ./build.sh: line 36: ../configure: No such file or directory
  thread 'main' panicked at neqo-crypto/build.rs:135:5:
  NSS build failed
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...

I have gyp and ninja installed, so I am not sure what is going on here

robamu avatar Aug 29 '24 18:08 robamu

Running ldd yields the following:

neqo on  main [$] is 📦 v0.8.2 via 🦀 v1.80.1
❯ ldd /usr/lib/libnss3.so
/usr/lib/libnss3.so: /lib/x86_64-linux-gnu/libnssutil3.so: version `NSSUTIL_3.101' not found (required by /usr/lib/libnss3.so)

...

so.. some problem with libnssutil3 oder some other missing library?

This looks like at least some of your installation is NSS 3.101 and not 3.103, which would explain the error when you try and run it. If you want to use the LFS-installed NSS, you need to make sure it's installed correctly in the system paths, and that no other (older) versions are lingering.

I also tried following the readme by setting NSS_DIR to the NSS sources path and then running a fresh build of neqo, but there seems to be some issue with the build.sh script of NSS.. I was only able to build NSS with make so far.

   Compiling neqo-crypto v0.8.2 (/home/rmueller/Rust/neqo/neqo-crypto)

...

./build.sh: line 36: ../configure: No such file or directory thread 'main' panicked at neqo-crypto/build.rs:135:5: NSS build failed

This looks like the environment is not set up correctly. You need to check out NSS and NSPR into a directory hierarchy like this:

/your/absolute/path
nss
├── nss
└── nspr

Your environment then should be:

export NSS_DIR=/your/absolute/path/nss/nss
export NSS_TARGET=Release
export LD_LIBRARY_PATH $NSS_DIR/../dist/$NSS_TARGET/lib

larseggert avatar Aug 30 '24 05:08 larseggert

I have managed to build and run the tests now with cargo nextest r , one test fails for me for both main and the PR branch:

--- STDOUT:              neqo-common hrtime::test::update_multi ---

running 1 test
sleep(1ms) → 1.061399ms Δ61.399µs
sleep(1ms) → 1.058584ms Δ58.584µs
sleep(2ms) → 2.048389ms Δ48.389µs
sleep(2ms) → 2.066653ms Δ66.653µs
sleep(3ms) → 3.074642ms Δ74.642µs
sleep(3ms) → 3.073069ms Δ73.069µs
sleep(5ms) → 5.066455ms Δ66.455µs
sleep(5ms) → 5.072656ms Δ72.656µs
sleep(8ms) → 8.064904ms Δ64.904µs
sleep(8ms) → 8.067559ms Δ67.559µs
sleep(10ms) → 10.076534ms Δ76.534µs
sleep(10ms) → 10.071084ms Δ71.084µs
sleep(12ms) → 12.064259ms Δ64.259µs
sleep(12ms) → 12.063176ms Δ63.176µs
sleep(15ms) → 15.066245ms Δ66.245µs
sleep(15ms) → 15.062899ms Δ62.899µs
sleep(20ms) → 20.067497ms Δ67.497µs
sleep(20ms) → 20.06357ms Δ63.57µs
sleep(25ms) → 25.062007ms Δ62.007µs
sleep(25ms) → 25.068479ms Δ68.479µs
sleep(30ms) → 30.326814ms Δ326.814µs
sleep(1ms) → 1.077138ms Δ77.138µs
sleep(30ms) → 31.640385ms Δ1.640385ms
sleep(1ms) → 1.551488ms Δ551.488µs
sleep(2ms) → 2.430786ms Δ430.786µs
sleep(2ms) → 3.138001ms Δ1.138001ms
sleep(3ms) → 3.478609ms Δ478.609µs
sleep(3ms) → 5.995656ms Δ2.995656ms
sleep(5ms) → 5.087443ms Δ87.443µs
sleep(8ms) → 8.070364ms Δ70.364µs
sleep(10ms) → 10.086934ms Δ86.934µs
sleep(12ms) → 12.115404ms Δ115.404µs
sleep(15ms) → 15.082845ms Δ82.845µs
sleep(1ms) → 1.072259ms Δ72.259µs
sleep(2ms) → 2.108822ms Δ108.822µs
sleep(3ms) → 3.101262ms Δ101.262µs
sleep(5ms) → 6.947169ms Δ1.947169ms
sleep(20ms) → 20.086693ms Δ86.693µs
sleep(25ms) → 25.072748ms Δ72.748µs
sleep(30ms) → 30.073519ms Δ73.519µs
test hrtime::test::update_multi ... FAILED

failures:

failures:
    hrtime::test::update_multi

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 64 filtered out; finished in 0.27s


--- STDERR:              neqo-common hrtime::test::update_multi ---
thread '<unnamed>' panicked at neqo-common/src/hrtime.rs:416:38:
called `Result::unwrap()` on an `Err` value: ()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'hrtime::test::update_multi' panicked at neqo-common/src/hrtime.rs:475:20:
called `Result::unwrap()` on an `Err` value: Any { .. }

I see that there is a client & server test and the interoperability tests, should I try to make those work locally as well?

robamu avatar Aug 30 '24 18:08 robamu

Are you running this in a VM or cloud instance? Because those have bad timer accuracy. IIRC there is either an environment variable or rust feature you can use to skip those timer tests.

larseggert avatar Aug 30 '24 19:08 larseggert

Also, if cargo nextest run --no-fail-fast otherwise passes, i.e., if the timer test failure is the only failure, please update the PR and I'll have CI run.

larseggert avatar Sep 02 '24 06:09 larseggert

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.39%. Comparing base (5d8651a) to head (1bc2aa6). Report is 2 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2041      +/-   ##
==========================================
- Coverage   95.39%   95.39%   -0.01%     
==========================================
  Files         112      112              
  Lines       36449    36457       +8     
==========================================
+ Hits        34770    34777       +7     
- Misses       1679     1680       +1

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 13 '24 18:09 codecov[bot]

I actually ran this on my (decent) machine.. I can try to have another look to continue this on the weekend.. Maybe some more unittests would be good as well?

robamu avatar Sep 13 '24 18:09 robamu

Yes, in order for us to land this, CI needs to pass and test coverage needs to be there. (And we need some review approvals.)

larseggert avatar Sep 14 '24 07:09 larseggert

@robamu are you still working on this PR?

larseggert avatar Nov 08 '24 17:11 larseggert

I'll have another look at it

robamu avatar Nov 11 '24 10:11 robamu

The local tests passed except the timer test. I've added a very basic test for the configuration of the connection parameters so far. I did not see any unit tests for the client except one test for a qlog file. Do you need/want some tests for the new clap arguments in some shape or form, and if yes, where is the best place to add them?

robamu avatar Nov 11 '24 13:11 robamu

Code under neqo-bin doesn't need tests, but all changes under neqo-transport do.

larseggert avatar Nov 11 '24 13:11 larseggert

@robamu are you still pursuing this PR or should we close it?

larseggert avatar Jul 04 '25 14:07 larseggert

I won't have the time to pursue this further. I think we can close it. If anyone is interested in picking this up and completing it, I will keep the branch.

robamu avatar Jul 04 '25 14:07 robamu