feat: Option for no zero-len CIDs for client #2171
Conversation
They break the upcoming QNS migration tests.
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2171 +/- ##
=======================================
Coverage 95.39% 95.39%
=======================================
Files 112 112
Lines 36373 36373
=======================================
Hits 34697 34697
Misses 1676 1676 ☔ View full report in Codecov by Sentry. |
Failed Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
|
Benchmark resultsPerformance differences relative to 9b5ec71. coalesce_acked_from_zero 1+1 entries: Change within noise threshold. time: [98.792 ns 99.158 ns 99.509 ns]
change: [-1.0922% -0.6307% -0.1840%] (p = 0.01 < 0.05)
coalesce_acked_from_zero 3+1 entries: Change within noise threshold. time: [116.50 ns 116.84 ns 117.20 ns]
change: [-1.7612% -1.2663% -0.8127%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 10+1 entries: Change within noise threshold. time: [116.22 ns 116.69 ns 117.26 ns]
change: [-1.0836% -0.6186% -0.1831%] (p = 0.01 < 0.05)
coalesce_acked_from_zero 1000+1 entries: Change within noise threshold. time: [97.433 ns 97.602 ns 97.800 ns]
change: [-2.0630% -1.0329% -0.0286%] (p = 0.05 < 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [111.57 ms 111.62 ms 111.66 ms]
change: [-0.3211% -0.2526% -0.1854%] (p = 0.00 < 0.05)
transfer/pacing-false/varying-seeds: No change in performance detected. time: [25.902 ms 26.879 ms 27.872 ms]
change: [-6.7639% -1.6579% +3.4327%] (p = 0.53 > 0.05)
transfer/pacing-true/varying-seeds: No change in performance detected. time: [33.774 ms 35.458 ms 37.156 ms]
change: [-10.553% -4.7185% +1.7720%] (p = 0.15 > 0.05)
transfer/pacing-false/same-seed: No change in performance detected. time: [25.323 ms 26.032 ms 26.759 ms]
change: [-2.9228% +1.0946% +5.4403%] (p = 0.61 > 0.05)
transfer/pacing-true/same-seed: No change in performance detected. time: [38.558 ms 40.437 ms 42.362 ms]
change: [-9.9420% -4.2311% +1.9678%] (p = 0.19 > 0.05)
1-conn/1-100mb-resp (aka. Download)/client: No change in performance detected. time: [113.55 ms 114.00 ms 114.44 ms]
thrpt: [873.85 MiB/s 877.16 MiB/s 880.70 MiB/s]
change:
time: [-16.992% -6.2679% +0.4475%] (p = 0.41 > 0.05)
thrpt: [-0.4455% +6.6871% +20.470%]
1-conn/10_000-parallel-1b-resp (aka. RPS)/client: 💔 Performance has regressed. time: [317.06 ms 320.56 ms 324.00 ms]
thrpt: [30.864 Kelem/s 31.196 Kelem/s 31.540 Kelem/s]
change:
time: [+1.0641% +2.6348% +4.1806%] (p = 0.00 < 0.05)
thrpt: [-4.0129% -2.5672% -1.0529%]
1-conn/1-1b-resp (aka. HPS)/client: No change in performance detected. time: [33.693 ms 33.858 ms 34.034 ms]
thrpt: [29.382 elem/s 29.535 elem/s 29.680 elem/s]
change:
time: [-1.4548% -0.5765% +0.2634%] (p = 0.20 > 0.05)
thrpt: [-0.2627% +0.5799% +1.4763%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
|
Can this be conditional behaviour? That is, can we add a flag to enable/disable, defaulting to zero length, but turning it on for the migration test? |
|
I can make this a flag, good suggestion. |
larseggert
changed the title
| "rebind-port" | "rebind-addr" | "connectionmigration" => { | ||
| if self.cid_len == 0 { | ||
| qinfo!("Rebind/migration test won't work with len-0 CID; overwriting to 8"); | ||
| self.cid_len = 8; | ||
| } |
There was a problem hiding this comment.
We should be able to do client-side address rebinding if the client uses a zero-length CID. Which side is doing the rebinding again?
There was a problem hiding this comment.
I need to check tomorrow, but I think the issue is that we don't issue a new connection ID on the client when it is using a zero-length ID and when the server retires a path and the connection ID used for it.
There was a problem hiding this comment.
Yes, what's happening is that with a client using a len-0 CID, when the server has validated the new path after a rebinding event, it will retire the old path and hence send a RETIRE_CONNECTION_ID for CID number 0. After the client receives and processes that frame, it will no longer accept any packets from the server (Dropped received packet: Invalid DCID CID [0]: ;).
There was a problem hiding this comment.
I think this is a bug on the server; it should not retire the only CID it has for the client (and then keep using it anyway.)
They break the upcoming QNS migration tests.
Edit: We might not need this with #2176 and #2182. Verify.