Handle retire before parameter in new connection ID #1150
Conversation
|
@WesleyRosenblum can you take a look at this PR? Does it do what you need? |
| if ((cnx->remote_parameters.migration_disabled != 0 && | ||
| path_x->p_remote_cnxid != NULL && | ||
| path_x->p_remote_cnxid->sequence >= cnx->retire_cnxid_before)|| | ||
| cnx->local_parameters.migration_disabled != 0) { |
There was a problem hiding this comment.
Why does this need to check remote_parameters.migration_disabled or local_parameters.migration_disabled at all? In my mind, connection migration is distinct from switching connection IDs. Connection migration may require using a new connection ID, but issuing a new connection ID/retiring all connection IDs doesn't mean that a connection is migrating.
There was a problem hiding this comment.
The spec has evolved over time. Early versions more or less required to keep the same CID throughout the connection if the migration was disabled. Of course, there are now different controls for "voluntary migration" and for "number of CID accepted from the peer". I could remove these checks, but I will need first clarifications inside the WG. Maybe make that a different issue?
There was a problem hiding this comment.
Sounds good. I'll let you open that issue if thats ok.
There was a problem hiding this comment.
Created issue #1151
picoquic/sender.c
Outdated
| cnx->local_parameters.migration_disabled == 0 && | ||
| cnx->nb_local_cnxid < (int)(cnx->remote_parameters.active_connection_id_limit) && | ||
| cnx->nb_local_cnxid <= PICOQUIC_NB_PATH_TARGET) { | ||
| while ((cnx->remote_parameters.migration_disabled == 0 || |
There was a problem hiding this comment.
Same here, why do you check if migration is disabled?
|
@WesleyRosenblum The new commits define a "time to live" for the locally generated connection ID, and automatically retire CID if the time expires (and the connection is still up). This seems much easier to use than the basic API that would just set a sequence number. The old API has been removed, and the test has been adapted. |
|
Can the "privateoctopus/picoquic:latest" docker image be updated with this? |
|
@cmiller-NA When you created the Dockerfile for Picoquic, how did you produce the updated docker image? Should we have a tool that produces that automatically? |
|
@huitema To produce a docker image you just need to run "docker build . -t <tag_name>" from the root directory of the workspace. Anyone who wants an updated picoquic docker image has to execute that command and picoquic is built from source every time a new docker image is built using the dockerfile. See "docker build" documentation I am not sure if storing the docker image in the tree would be a good idea. I'd recommend maybe having a docker_build.sh script which can be triggered by anyone which will generate a docker image. |
|
I meant to update the prebuilt docker image in Docker hub: https://hub.docker.com/r/privateoctopus/picoquic so that interop could pick it up |
|
Oh, that one! That's not a generic image. It needs all kinds of customization for participation in the interop. I build it in a separate project, picoquic-interop-runner. I run the Docker script manually in an Ubuntu VM. |
|
Got it. Appreciate updating that when you can to see any impact on the interop report |
|
Done. |
This PR adds two tests:
In both cases, the goal is to support scenarios in which server side load balancing is based on routing by connection ID, but the configuration changes sometimes require new sets of connection ID frames.