Add imquic relay and client docker images#57
Conversation
|
|
Hey Lorenzo! Thanks for putting this together - the overall structure is really solid. I tried building the images locally and worked through a few issues. Here's what I found: 1. Shell syntax error in both Dockerfiles (line 36-38) RUN apt-get update && apt-get install -y \
ca-certificates libglib2.0 libssl libjansson && \
&& rm -rf /var/lib/apt/lists/*The 2. Runtime apt package names After fixing the syntax error, the runtime stage fails with 3. Dockerfiles clone from GitHub instead of using the build context The current Dockerfiles do After fixing all three, I got the build through to the imquic compilation stage, where it hits the A few smaller things I noticed but didn't test (since the build didn't get to the runtime stage):
I'm happy to push fixes for the Dockerfile issues (#1-3 above plus the smaller items) to the branch if you'd like, so you can focus on the upstream picotls-fusion build issue. Or if you'd prefer to take another pass yourself, that works too. Let me know! |
|
Thanks for looking at it and the super helpful feedback! I think the picoquic-fusion is a different issue, since the picotls/picoquic building issue has been solved in the meanwhile. I think it may have to do with the fact that picotls-fusion is only built/needed on some systems, as fusion is not available everywhere, but we're always linking to it. I'll have to check if there's an easy way to check if it's been built, so that I only conditionally link to it in the library. If you could indeed apply the first fixes that would be really helpful, thanks! In the meanwhile, I'll work on updating the imquic repo to take care of the conditional fusion dependency. |
|
Hey Mike, I just added a check in the configure script, could you check if that helps? |
|
Nice, that worked! Your fusion check gets past the linker error on arm64. I can see the configure output detecting it correctly: With that fix plus the Dockerfile fixes from my earlier comment, both the relay and client images build and run successfully. The relay starts up, registers ALPN for draft-16 and draft-17, and listens on port 4443. One additional thing I found during runtime testing: the I'll push the Dockerfile fixes (including this one) to the branch shortly. |
|
Excellent, thanks for checking! And apologies for all the dumb copy/paste errors or typos on my side 😅 |
|
As a side note, considering that you may build images only once, and then run them multiple times, how does it work for when there are updates? Are they rebuilt on a regular basis, or is there some way I need to add to my repo to trigger that? |
…imquic to draft-16-17)
|
Pushed the Dockerfile fixes and also merged main to resolve the README conflict, so this should be ready to merge now if it looks good to you. To get the images building in CI, I'll need to add imquic to the GitHub Actions workflow and GHCR config on our side - happy to take care of that separately after this merges. On the rebuild question - right now I manually trigger image rebuilds when I know something's changed upstream. It's a known gap. I've been working on a staleness checker that would compare what's on GHCR against upstream HEAD so we at least have visibility into when images are out of date. Longer term I'd like to make the whole CI more data-driven so that adding Curious if you have thoughts on what would be most useful from your perspective - would some kind of signal from the imquic repo help (webhook, tag convention, etc.), or would periodic staleness checks be sufficient? |
I think that in general a periodic check is more than enough. That said, it may be helpful to also have ways to explicitly trigger a rebuild, though, e.g., in cases when we're doing more interactve and intensive interop sessions that may lead to frequent updates. That could be optional, and on how that would work in practice I don't have a specific suggestion. |
This is a first attempt to provide Docker images to use both the relay and the interop runner client from imquic as part of the CI. It's a draft at the moment for a few different reasons:
mainbranch, which is correct, butmaindoesn't contain the new v17 changes yet, so that would need to be merged upstream firstHope this is a good start anyway, feedback welcome!