Re: [pbs-devel] [RFC proxmox 00/23] upgrade to hyper/http 1.0

["Max Carrara" <m.carrara@proxmox.com>]

On Wed Apr 2, 2025 at 3:53 PM CEST, Max Carrara wrote:
> On Wed Mar 26, 2025 at 4:23 PM CET, Fabian Grünbichler wrote:
> > this RFC series adapts proxmox and proxmox-backup to hyper/http 1.0. I
> > also have similar patches for PDM, but those require an update of gloo
> > and proxmox-yew-comp and the basic approach is the same as with the
> > patches here, and since I expect some feedback to incorporate anyway I
> > saved those for the first "proper" version.
> >
> > hyper 1.0 came with a lot of changes, the most notable ones:
> >
> > Body is now a trait, not a struct
> > - there's a new Incoming impl for incoming requests on the server side,
> > and incoming responses on the client side
> > - http-body-util has some more impls
> > - proxmox-http has a new impl covering our two common use cases, see
> > the patch there for details
> >
> > hyper now doesn't expose tower's Service or tokio's
> > AsyncRead/AsyncWrite, but has its own variants for both with
> > corresponding wrappers/adapters.
> >
> > the previous Accept trait for translation from a listening socket to
> > connections is gone, an accept loop should be used instead.
> >
> > the pooling client is moved from hyper to hyper-util. despite its
> > "legacy" label we still use it, as we'd need to either implement a ton
> > of code ourself or switch to reqwest otherwise.
> >
> > graceful shutdown of connections is handled differently, so are
> > connection ugprades.
> >
> > I did some rough testing of the usual things without noticing any
> > breakage, but I am sure I missed some parts. there's also room for
> > improvement for sure, in particular surrounding the rest-server and
> > connection accepting part - suggestions welcome!
>
> All the changes seem pretty solid to me; switching to hyper/1.0 was
> never really going to be pretty, but at least hyper-util provides a lot
> of the compat wrappers for that.
>
> (Note: The proxmox-backup patches only apply with `git am -3` for me.)
>
> I will give this a spin on my PBS dev VM and let it run on there for a
> while to see if I notice anything off. I haven't spotted anything sus in
> the code, so I don't expect anything to come up, but still.
>
> Also, I haven't really had any concrete ideas yet for the return value
> trait of `accept_tls_optional()` and family, but I'll let you know once
> (or if) I do. Previously, all of the individual parts could just be
> nicely composed together (incoming connection receiver, connection
> handler, etc.), so perhaps we could cook up something similar. The new
> pattern with the explicitly defined loop makes that a little hard,
> though.
>
> Apart from that, there are a few comments inline which can IMO be
> addressed in a follow-up (if at all). Nothing major. This otherwise
> looks pretty good to me, nice work! Especially the solution with our
> "own" `Body` implementation is pretty nice and a good middle ground. 
>
> Should nothing else come up and this be merged without a refresh,
> consider:
>
> Reviewed-by: Max Carrara <m.carrara@proxmox.com>
>
>
> Feel free to ping me for a re-review / more testing, should you refresh
> this series.

Gave this a more thorough test run today and didn't encounter anything.

- Ran some backups
- Restored a VM
- Restored an individual file
- Tried out various things in the UI, such as forgetting backup groups,
  viewing logs, running various things (console, verify job, GC job, ..)
- Set up ACME using my personal domain on OVH + LetsEncrypt Staging

Everything seems to work just fine. I'll keep the binaries built with
this series on my VM for a couple more days in case anything pops up,
but everything I have tried so far seems to work, even ACME.

So, unless something comes up, consider:

Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>

Just to note again, feel free to ping me for a re-review / more testing,
should you refresh this series.

Nice work! 🫡

>
> >
> > proxmox workspace:
> >
> > Fabian Grünbichler (17):
> >   http: order feature values
> >   http: rate-limited-stream: update to hyper/http 1.0
> >   http: adapt MaybeTlsStream to hyper 1.x
> >   http: adapt connector to hyper 1.x
> >   http: add Body implementation
> >   http: adapt simple client to hyper 1.x
> >   http: websocket: update to http/hyper 1
> >   openid: use http 0.2 to avoid openidconnect update
> >   proxmox-login: switch to http 1.x
> >   client: switch to hyper/http 1.0
> >   metrics: update to hyper/http 1.0
> >   acme: switch to http/hyper 1.0
> >   proxmox-router: update to hyper 1.0
> >   proxmox-rest-server: update to hyper 1.0
> >   proxmox-rest-server: fix and extend example
> >   proxmox-auth-api: update to hyper 1.0
> >   proxmox-acme-api: update to hyper 1.0
> >
> >  Cargo.toml                                    |   8 +-
> >  proxmox-acme-api/Cargo.toml                   |   4 +
> >  proxmox-acme-api/src/acme_plugin.rs           |  63 +++++--
> >  proxmox-acme/Cargo.toml                       |   3 +-
> >  proxmox-acme/src/async_client.rs              |  11 +-
> >  proxmox-auth-api/Cargo.toml                   |   2 +
> >  proxmox-auth-api/src/api/access.rs            |   4 +-
> >  proxmox-client/Cargo.toml                     |   1 +
> >  proxmox-client/src/client.rs                  |  22 +--
> >  proxmox-http/Cargo.toml                       |  45 +++--
> >  proxmox-http/src/body.rs                      | 133 ++++++++++++++
> >  proxmox-http/src/client/connector.rs          |  44 +++--
> >  proxmox-http/src/client/simple.rs             |  93 +++++++---
> >  proxmox-http/src/client/tls.rs                |   2 +-
> >  proxmox-http/src/lib.rs                       |   5 +
> >  proxmox-http/src/rate_limited_stream.rs       |   2 +-
> >  proxmox-http/src/websocket/mod.rs             |   6 +-
> >  proxmox-login/Cargo.toml                      |   2 +-
> >  proxmox-metrics/src/influxdb/http.rs          |   5 +-
> >  proxmox-openid/Cargo.toml                     |   3 +-
> >  proxmox-rest-server/Cargo.toml                |   9 +-
> >  .../examples/minimal-rest-server.rs           |  48 ++++-
> >  proxmox-rest-server/src/api_config.rs         |  44 ++---
> >  proxmox-rest-server/src/connection.rs         |  14 +-
> >  proxmox-rest-server/src/formatter.rs          |   8 +-
> >  proxmox-rest-server/src/h2service.rs          |  15 +-
> >  proxmox-rest-server/src/lib.rs                |   2 +-
> >  proxmox-rest-server/src/rest.rs               | 164 +++++++++++-------
> >  proxmox-router/Cargo.toml                     |   6 +-
> >  proxmox-router/src/router.rs                  |  19 +-
> >  proxmox-router/src/stream/parsing.rs          |  16 +-
> >  31 files changed, 567 insertions(+), 236 deletions(-)
> >  create mode 100644 proxmox-http/src/body.rs
> >
> > proxmox-backup:
> >
> > Fabian Grünbichler (6):
> >   Revert "h2: switch to legacy feature"
> >   pbs-client: adapt http client to hyper/http 1.0
> >   pbs-client: vsock: adapt to hyper/http 1.0
> >   restore daemon: adapt to hyper/http 1.0
> >   adapt to hyper/http 1.0
> >   adapt examples to hyper/http 1.0
> >
> >  Cargo.toml                                    | 10 ++-
> >  examples/h2client.rs                          |  6 +-
> >  examples/h2s-client.rs                        |  6 +-
> >  examples/h2s-server.rs                        | 28 +++-----
> >  examples/h2server.rs                          | 28 +++-----
> >  pbs-client/Cargo.toml                         |  4 +-
> >  pbs-client/src/backup_writer.rs               |  8 +--
> >  pbs-client/src/http_client.rs                 | 38 +++++-----
> >  pbs-client/src/pipe_to_stream.rs              |  2 +-
> >  pbs-client/src/vsock_client.rs                | 27 +++----
> >  proxmox-backup-client/Cargo.toml              |  1 +
> >  proxmox-backup-client/src/snapshot.rs         |  2 +-
> >  proxmox-restore-daemon/Cargo.toml             |  2 +
> >  proxmox-restore-daemon/src/main.rs            | 24 +++++--
> >  .../src/proxmox_restore_daemon/api.rs         |  6 +-
> >  .../src/proxmox_restore_daemon/auth.rs        |  5 +-
> >  src/acme/client.rs                            |  6 +-
> >  src/acme/plugin.rs                            | 62 +++++++++++-----
> >  src/api2/admin/datastore.rs                   | 20 +++---
> >  src/api2/backup/environment.rs                |  3 +-
> >  src/api2/backup/mod.rs                        | 10 +--
> >  src/api2/backup/upload_chunk.rs               | 47 +++++++------
> >  src/api2/helpers.rs                           |  3 +-
> >  src/api2/node/mod.rs                          |  7 +-
> >  src/api2/node/tasks.rs                        |  7 +-
> >  src/api2/reader/mod.rs                        | 17 +++--
> >  src/bin/proxmox-backup-api.rs                 | 40 +++++++----
> >  src/bin/proxmox-backup-proxy.rs               | 70 +++++++++++++++----
> >  28 files changed, 297 insertions(+), 192 deletions(-)
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel