add container service for local development #4238

anonrig · 2025-06-03T20:50:22Z

Work in progress. Not ready to review yet.

windsurf-bot

Other comments (3)

src/workerd/server/server.h (105-105) The `enableDocker` field has been added to the `Durable` struct, but there's no corresponding field in the `Ephemeral` struct. If Docker functionality should be available for both durable and ephemeral actors, consider adding it to the `Ephemeral` struct as well for consistency.
src/workerd/io/docker-service.h (16-16) The constructor has an unnecessary semicolon after the function body.
```
  DockerService(kj::String socketPath_): socketPath(kj::mv(socketPath_)) {}
```
src/workerd/io/docker-service.c++ (1-1) The copyright year range (2017-2022) should be updated to include the current year since this is a new file.

1 file skipped due to size limits:

src/workerd/server/server.c++

💡 To request another review, post a new comment with "/windsurf-review".

src/workerd/server/workerd.capnp

src/workerd/io/docker-service.c++

src/workerd/server/workerd.capnp

src/rust/container/lib.rs

src/workerd/server/workerd.capnp

src/rust/container/lib.rs

src/workerd/server/workerd.capnp

src/rust/container/lib.rs

deps/rust/cargo.bzl

src/rust/container/lib.rs

github-actions · 2025-06-04T14:21:47Z

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

jasnell · 2025-06-05T02:07:26Z

src/workerd/server/workerd.capnp

+    docker @5 :DockerOptions;
+    # Optional configuration options for local docker container interaction.
+
+    struct DockerOptions {
+      containerName @0 :Text;
+      # Name of the docker container associated with the current DO namespace.
+    }


If this is only going to be used here, then this can probably be simplified into a Group rather than struct. But, I'm not the expert on capnp schemas so I'll defer to @kentonv on what's best for the structure here.

I see a few problems with a group:

you would have to add an explicit "enabled"

the field numbers in groups are from the parent struct

also I wonder if the docker options really belongs here or to a separate map of DO namespaces to Options - I don't think it's required now but it would be easier to migrate.

Maybe for the last point I would move the struct at top level instead of nested in DO, close to DockerConnectionOptions.

jasnell · 2025-06-05T02:10:04Z

src/workerd/io/container-client.h

+#include <kj/debug.h>
+
+namespace workerd::io {
+struct RustAsyncStream final: public kj::AsyncIoStream {


heh... what's that you always ~~nag me about~~ remind me to do ;-) ... some code comments in here would be ideal ;-)

danlapid · 2025-06-05T18:46:43Z

src/workerd/io/container-client.c++

+
+kj::Own<ContainerAsyncStream> createContainerStream(
+    kj::StringPtr address, kj::StringPtr containerName) {
+  auto sharedState = kj::heap<ContainerStreamSharedState>();


why not refcounted?
kj::Rc<T> + kj::rc<T>(...)

src/workerd/io/container-client.h

kentonv · 2025-06-05T18:53:32Z

src/workerd/server/workerd.capnp

+
+    struct ContainerOptions {
+      name @0 :Text;
+      # Name of the container associated with the current DO namespace.


How dose this work? Each DO instance gets its own container. Is this the name of the container image? This really needs a lot more commentary.

danlapid · 2025-06-05T18:53:58Z

src/rust/container/lib.rs

+        _params: container::GetTcpPortParams,
+        _results: container::GetTcpPortResults,
+    ) -> Promise<(), capnp::Error> {
+        Promise::from_future(async move {


You need to implement a rpc::Container::Port::Server and return an object of that kind.
That object implements connect which pumps bytes back and forth between the container tcp port and the incoming bytestream.

See example in fake-container-service.c++ internally.

kentonv · 2025-06-05T18:55:50Z

I think we need to back up and write out a document explaining what's going on here. The design here doesn't appear to match any design we've discussed so far and I have no idea what's going on.

mikenomitch · 2025-06-05T18:57:16Z

src/rust/container/lib.rs

+}
+
+impl container::Server for Server {
+    fn destroy(


I believe what happens here is that we send a SIGTERM, wait for 5 min for the container to stop, and if it is not stopped then we send a SIGKILL.

So I think if this were changed to sending a SIGTERM first, then we waited and checked for status and then called remove_container conditionally it'd be the right logic.

I'm not 100% positive that this happens on the destroy RPC command though (versus just when we update)...

@gabivlj - I tracked things to here at least: https://gitlab.cfdata.org/cloudflare/cc/cloudchamber/-/blob/master/go/cmd/cloudchamberd/internal/services/durableobject/binding.go#L362

Does the container.sidecar.Stop command do the two signals, or will this just immediately SIGKILL in this case?

Oh... maybe stop_container has the behavior we want? (https://docs.rs/bollard/latest/bollard/struct.Docker.html#method.stop_container) and it includes a timeout to wait before it sigkills (https://docs.rs/bollard/latest/bollard/container/struct.StopContainerOptions.html).

So I think:

If destroy is meant to immediately destory, then we call kill_container

If destory does in fact do the timeout behavior, we use stop_container and set the timeout

When signal() is called, we just relay that signal to the runtime. When destroy() is called, we send a SIGKILL.

anonrig · 2025-06-05T19:27:04Z

Why does this PR contain a whole lot of Rust code and Rust build file updates that seem unrelated? Can this be cleaned up to something reviewable?

@kentonv Adding a rust crate as a dependency is causing these changes. I don't know if it's possible but anybody knows a way happy to do it. cc @danlapid @mikea @fhanau

kentonv · 2025-06-05T19:42:06Z

Can you please move all the Rust dependency management into a separate commit? As is my UI is slowing to a crawl and github doesn't seem to want to display most of the files because there's too much stuff in the diff.

anonrig · 2025-06-05T19:56:30Z

Can you please move all the Rust dependency management into a separate commit? As is my UI is slowing to a crawl and github doesn't seem to want to display most of the files because there's too much stuff in the diff.

@kentonv Yes, I'll do it. As soon as possible.

kentonv · 2025-06-05T19:57:31Z

src/workerd/io/container-client.h

+// The stream uses shared state to coordinate between the MessageCallback
+// (which receives messages from the container asynchronously) and the
+// tryRead() method (which provides those messages to the C++ side).
+struct ContainerAsyncStream final: public kj::AsyncIoStream {


I read the comment but I don't understand the purpose of this class. Why do containers need a special kind of stream?

jasnell · 2025-06-05T22:58:00Z

src/workerd/io/container-client.c++

+  auto lockedQueue = messageQueue.lockExclusive();
+  for (auto& byte : message) {
+    lockedQueue->push(byte);
+  }
+
+  auto lockedWaiter = readWaiter.lockExclusive();
+  KJ_IF_SOME(fulfiller, *lockedWaiter) {
+    fulfiller->fulfill();
+    *lockedWaiter = kj::none;
+  }


Should both locks actually be held simultaneously? Or should each of these be in their own blocks?

jasnell · 2025-06-05T22:58:31Z

src/workerd/io/container-client.c++

+  }
+
+  size_t min = kj::min(lockedQueue->size(), maxBytes);
+  KJ_REQUIRE(min > 0, "Should never happen");


For these, use KJ_ASSERT rather than KJ_REQUIRE

jasnell · 2025-06-05T22:58:59Z

src/workerd/io/container-client.c++

+
+  size_t min = kj::min(lockedQueue->size(), maxBytes);
+  KJ_REQUIRE(min > 0, "Should never happen");
+  memcpy(buffer, &lockedQueue->front(), min);


Use kj::ArrayPtr copyFrom instead for more safety

With code from https://github.com/capnproto/capnproto-rust/tree/master/async-byte-channel

emily-shen · 2025-06-06T14:04:27Z

src/rust/container/lib.rs

+                .collect::<Vec<String>>();
+
+            let config = ContainerCreateBody {
+                image: Some(container_name.clone()),


it looks like the image ref is the same as container_name? i don't think this is right, because we will have multiple instances running from the same image.

miniflare will give workerd an image name:tag, which should be used here to create the container from the right image. then the container name can be whatever you want. Something readable for the end user preferably, idk if you have access here to the string the DO instance id is created from, but that would be ideal.

then all the other methods here should use that container name, not the image name that miniflare passed in.

maybe we can rename the config option in workerd.capnp to image_tag for clarity?

mikea · 2025-06-06T18:10:18Z

src/rust/container/io.rs

+use futures::AsyncWrite;
+use tokio::sync::mpsc;
+
+unsafe extern "C" {


@anonrig let restore the original switch. this wouldn't work on win?

anonrig requested review from a team as code owners June 3, 2025 20:50

windsurf-bot bot reviewed Jun 3, 2025

View reviewed changes

src/workerd/server/workerd.capnp Outdated Show resolved Hide resolved

src/workerd/io/docker-service.c++ Outdated Show resolved Hide resolved

src/workerd/server/workerd.capnp Outdated Show resolved Hide resolved

anonrig force-pushed the yagiz/add-docker-service branch 4 times, most recently from 610cd20 to 46b746b Compare June 3, 2025 21:53

anonrig requested a review from mikea June 3, 2025 22:04

anonrig marked this pull request as draft June 3, 2025 22:04

vicb reviewed Jun 4, 2025

View reviewed changes

src/rust/container/lib.rs Outdated Show resolved Hide resolved

vicb reviewed Jun 4, 2025

View reviewed changes

src/rust/container/lib.rs Outdated Show resolved Hide resolved

vicb reviewed Jun 4, 2025

View reviewed changes

src/workerd/server/workerd.capnp Outdated Show resolved Hide resolved

vicb reviewed Jun 4, 2025

View reviewed changes

src/rust/container/lib.rs Outdated Show resolved Hide resolved

vicb reviewed Jun 4, 2025

View reviewed changes

src/workerd/server/workerd.capnp Outdated Show resolved Hide resolved

emily-shen reviewed Jun 4, 2025

View reviewed changes

src/rust/container/lib.rs Outdated Show resolved Hide resolved

danlapid reviewed Jun 4, 2025

View reviewed changes

deps/rust/cargo.bzl Show resolved Hide resolved

emily-shen reviewed Jun 4, 2025

View reviewed changes

src/rust/container/lib.rs Show resolved Hide resolved

anonrig force-pushed the yagiz/add-docker-service branch from 46b746b to ab04db2 Compare June 4, 2025 14:17

anonrig force-pushed the yagiz/add-docker-service branch 9 times, most recently from 811ce9a to 89a9e98 Compare June 4, 2025 21:50

jasnell reviewed Jun 5, 2025

View reviewed changes

danlapid reviewed Jun 5, 2025

View reviewed changes

src/workerd/io/container-client.h Outdated Show resolved Hide resolved

kentonv reviewed Jun 5, 2025

View reviewed changes

danlapid reviewed Jun 5, 2025

View reviewed changes

mikenomitch reviewed Jun 5, 2025

View reviewed changes

anonrig force-pushed the yagiz/add-docker-service branch from 995afc6 to 06e1ed1 Compare June 5, 2025 19:23

kentonv reviewed Jun 5, 2025

View reviewed changes

anonrig added 2 commits June 5, 2025 16:41

implement bi-directional async stream

9d4db22

try to find the bug

95276c5

anonrig force-pushed the yagiz/add-docker-service branch from 06e1ed1 to 95276c5 Compare June 5, 2025 22:05

jasnell reviewed Jun 5, 2025

View reviewed changes

Simplify rust channel implementation

cdc59de

With code from https://github.com/capnproto/capnproto-rust/tree/master/async-byte-channel

danlapid force-pushed the yagiz/add-docker-service branch from feb9a8e to cdc59de Compare June 5, 2025 23:26

danlapid added 2 commits June 6, 2025 01:18

Cleanups

d6f5f4d

fix loop blocking

b28f340

danlapid force-pushed the yagiz/add-docker-service branch from 811aaf8 to b28f340 Compare June 6, 2025 00:58

emily-shen reviewed Jun 6, 2025

View reviewed changes

mikea added 2 commits June 6, 2025 09:33

fixing message communication

15889b6

fixes fixes fixes

a1de2f8

mikea reviewed Jun 6, 2025

View reviewed changes

anonrig mentioned this pull request Jun 6, 2025

[cxx] add container service for local development #4272

Open

4 tasks

mikea added 2 commits June 6, 2025 11:52

Merge remote-tracking branch 'origin/main' into yagiz/add-docker-service

4f29886

fix memory leak

3b1d3e1

mikea force-pushed the yagiz/add-docker-service branch from 9ef5aa2 to 3b1d3e1 Compare June 6, 2025 19:59

add container service for local development #4238

Are you sure you want to change the base?

add container service for local development #4238

Uh oh!

Conversation

anonrig commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

windsurf-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danlapid Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentonv commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabivlj Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig commented Jun 5, 2025

Uh oh!

kentonv commented Jun 5, 2025

Uh oh!

anonrig commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anonrig commented Jun 3, 2025 •

edited

Loading

github-actions bot commented Jun 4, 2025 •

edited

Loading

danlapid Jun 5, 2025 •

edited

Loading

gabivlj Jun 5, 2025 •

edited

Loading