Merge pull request #1070 from rust-lang/unification

Unify the one-off and websocket Coordinator factories

Author: shepmaster

compiler/base/Dockerfile

@@ -56,7 +56,7 @@ COPY --chown=playground entrypoint.sh /playground/tools/
 
 FROM toolchain as wasm-tools
 
-RUN cargo install wasm-tools
+RUN cargo install --locked wasm-tools
 
 # Fetch all the crate source files
 
@@ -68,18 +68,11 @@ COPY --chown=playground Cargo.toml /playground/Cargo.toml
 COPY --chown=playground crate-information.json /playground/crate-information.json
 RUN cargo fetch
 
-# Build our tool for modifying Cargo.toml at runtime
-
-FROM toolchain as modify-cargo-toml
-
-COPY --chown=playground modify-cargo-toml /playground/modify-cargo-toml
-RUN cargo build --release --manifest-path=/playground/modify-cargo-toml/Cargo.toml
-
 # Set up cargo-chef for faster builds
 
 FROM toolchain as chef-available
 
-RUN cargo install cargo-chef
+RUN cargo install --locked cargo-chef
 
 WORKDIR /orchestrator
 
@@ -99,10 +92,10 @@ FROM chef-available as build-orchestrator
 COPY --chown=playground asm-cleanup /asm-cleanup
 COPY --chown=playground modify-cargo-toml /modify-cargo-toml
 COPY --chown=playground --from=prepare-orchestrator /orchestrator/recipe.json /orchestrator/recipe.json
-RUN cargo chef cook --release
+RUN cargo chef cook --locked --release
 
 COPY --chown=playground orchestrator /orchestrator
-RUN cargo install --path .
+RUN cargo install --locked --path .
 
 # Compiler and pre-compiled crates
 
@@ -116,7 +109,6 @@ RUN cargo clippy
 RUN if [ "${channel}" = 'nightly' ]; then cargo miri setup; cargo miri run; fi
 RUN rm src/*.rs
 
-COPY --from=modify-cargo-toml /playground/modify-cargo-toml/target/release/modify-cargo-toml /playground/.cargo/bin
 COPY --from=build-orchestrator /playground/.cargo/bin/worker /playground/.cargo/bin/worker
 COPY --from=wasm-tools /playground/.cargo/bin/wasm-tools /playground/.cargo/bin
 COPY --chown=playground cargo-wasm /playground/.cargo/bin

compiler/base/entrypoint.sh

@@ -2,14 +2,6 @@
 
 set -eu
 
-if [[ -z "${PLAYGROUND_ORCHESTRATOR:-}" ]]; then
-    timeout=${PLAYGROUND_TIMEOUT:-10}
-
-    modify-cargo-toml
-
-    # Don't use `exec` here. The shell is what prints out the useful
-    # "Killed" message
-    timeout --signal=KILL ${timeout} "$@"
-else
-    exec "$@"
-fi
+# This entrypoint is a no-op but I'm leaving it to be easier to
+# re-create at some future point.
+exec "$@"

compiler/base/modify-cargo-toml/src/main.rs

@@ -820,25 +820,33 @@ enum DemultiplexCommand {
     ListenOnce(JobId, oneshot::Sender<WorkerMessage>),
 }
 
-#[derive(Debug, Copy, Clone)]
-pub struct CoordinatorId {
-    start: u64,
-    id: u64,
+/// The [`Coordinator`][] usually represents a mostly-global resource,
+/// such as a Docker container. To avoid conflicts, each container
+/// must have a unique name, but that uniqueness can only be
+/// guaranteed by whoever is creating [`Coordinator`][]s via the
+/// [`CoordinatorFactory`][].
+pub trait IdProvider: Send + Sync + fmt::Debug + 'static {
+    fn next(&self) -> String;
 }
 
-/// Enforces a limited number of concurrent `Coordinator`s.
+/// A reasonable choice when there's a single [`IdProvider`][] in the
+/// entire process.
+///
+/// This represents uniqueness via a combination of
+///
+/// 1. **process start time** — this helps avoid conflicts from other
+///    processes, assuming they were started at least one second apart.
+///
+/// 2. **instance counter** — this avoids conflicts from other
+///    [`Coordinator`][]s started inside this process.
 #[derive(Debug)]
-pub struct CoordinatorFactory {
-    semaphore: Arc<Semaphore>,
-
+pub struct GlobalIdProvider {
     start: u64,
     id: AtomicU64,
 }
 
-impl CoordinatorFactory {
-    pub fn new(maximum: usize) -> Self {
-        let semaphore = Arc::new(Semaphore::new(maximum));
-
+impl GlobalIdProvider {
+    pub fn new() -> Self {
         let now = std::time::SystemTime::now();
         let start = now
             .duration_since(std::time::UNIX_EPOCH)
@@ -847,28 +855,40 @@ impl CoordinatorFactory {
 
         let id = AtomicU64::new(0);
 
-        Self {
-            semaphore,
-            start,
-            id,
-        }
+        Self { start, id }
     }
+}
 
-    fn next_id(&self) -> CoordinatorId {
+impl IdProvider for GlobalIdProvider {
+    fn next(&self) -> String {
         let start = self.start;
         let id = self.id.fetch_add(1, Ordering::SeqCst);
 
-        CoordinatorId { start, id }
+        format!("{start}-{id}")
+    }
+}
+
+/// Enforces a limited number of concurrent `Coordinator`s.
+#[derive(Debug)]
+pub struct CoordinatorFactory {
+    semaphore: Arc<Semaphore>,
+    ids: Arc<dyn IdProvider>,
+}
+
+impl CoordinatorFactory {
+    pub fn new(ids: Arc<dyn IdProvider>, maximum: usize) -> Self {
+        let semaphore = Arc::new(Semaphore::new(maximum));
+
+        Self { semaphore, ids }
     }
 
     pub fn build<B>(&self) -> Coordinator<B>
     where
-        B: Backend + From<CoordinatorId>,
+        B: Backend + From<Arc<dyn IdProvider>>,
     {
         let semaphore = self.semaphore.clone();
 
-        let id = self.next_id();
-        let backend = B::from(id);
+        let backend = B::from(self.ids.clone());
 
         Coordinator::new(semaphore, backend)
     }
@@ -2586,25 +2606,20 @@ fn basic_secure_docker_command() -> Command {
 }
 
 pub struct DockerBackend {
-    id: CoordinatorId,
-    instance: AtomicU64,
+    ids: Arc<dyn IdProvider>,
 }
 
-impl From<CoordinatorId> for DockerBackend {
-    fn from(id: CoordinatorId) -> Self {
-        Self {
-            id,
-            instance: Default::default(),
-        }
+impl From<Arc<dyn IdProvider>> for DockerBackend {
+    fn from(ids: Arc<dyn IdProvider>) -> Self {
+        Self { ids }
     }
 }
 
 impl DockerBackend {
     fn next_name(&self) -> String {
-        let CoordinatorId { start, id } = self.id;
-        let instance = self.instance.fetch_add(1, Ordering::SeqCst);
+        let id = self.ids.next();
 
-        format!("playground-{start}-{id}-{instance}")
+        format!("playground-{id}")
     }
 }
 
@@ -2617,6 +2632,9 @@ impl Backend for DockerBackend {
             .args(["--name", &name])
             .arg("-i")
             .args(["-a", "stdin", "-a", "stdout", "-a", "stderr"])
+            // PLAYGROUND_ORCHESTRATOR is vestigial; I'm leaving it
+            // for a bit to allow new containers to get built and
+            // distributed.
             .args(["-e", "PLAYGROUND_ORCHESTRATOR=1"])
             .arg("--rm")
             .arg(channel.to_container_name())
@@ -2791,8 +2809,8 @@ mod tests {
         project_dir: TempDir,
     }
 
-    impl From<CoordinatorId> for TestBackend {
-        fn from(_id: CoordinatorId) -> Self {
+    impl From<Arc<dyn IdProvider>> for TestBackend {
+        fn from(_ids: Arc<dyn IdProvider>) -> Self {
             static COMPILE_WORKER_ONCE: Once = Once::new();
 
             COMPILE_WORKER_ONCE.call_once(|| {
@@ -2846,8 +2864,12 @@ mod tests {
             .unwrap_or(5)
     });
 
-    static TEST_COORDINATOR_FACTORY: Lazy<CoordinatorFactory> =
-        Lazy::new(|| CoordinatorFactory::new(*MAX_CONCURRENT_TESTS));
+    static TEST_COORDINATOR_ID_PROVIDER: Lazy<Arc<GlobalIdProvider>> =
+        Lazy::new(|| Arc::new(GlobalIdProvider::new()));
+
+    static TEST_COORDINATOR_FACTORY: Lazy<CoordinatorFactory> = Lazy::new(|| {
+        CoordinatorFactory::new(TEST_COORDINATOR_ID_PROVIDER.clone(), *MAX_CONCURRENT_TESTS)
+    });
 
     fn new_coordinator_test() -> Coordinator<TestBackend> {
         TEST_COORDINATOR_FACTORY.build()

compiler/base/orchestrator/src/coordinator.rs

@@ -1,8 +1,8 @@
 GEM
   remote: https://rubygems.org/
   specs:
-    addressable (2.8.6)
-      public_suffix (>= 2.0.2, < 6.0)
+    addressable (2.8.7)
+      public_suffix (>= 2.0.2, < 7.0)
     base64 (0.2.0)
     capybara (3.40.0)
       addressable
@@ -21,36 +21,38 @@ GEM
     launchy (3.0.1)
       addressable (~> 2.8)
       childprocess (~> 5.0)
+    logger (1.6.0)
     matrix (0.4.2)
     mini_mime (1.1.5)
     mini_portile2 (2.8.7)
-    nokogiri (1.16.5)
+    nokogiri (1.16.6)
       mini_portile2 (~> 2.8.2)
       racc (~> 1.4)
-    public_suffix (5.0.5)
+    public_suffix (6.0.0)
     racc (1.8.0)
-    rack (3.0.11)
+    rack (3.1.4)
     rack-test (2.1.0)
       rack (>= 1.3)
     regexp_parser (2.9.2)
-    rexml (3.2.8)
-      strscan (>= 3.0.9)
+    rexml (3.3.1)
+      strscan
     rspec (3.13.0)
       rspec-core (~> 3.13.0)
       rspec-expectations (~> 3.13.0)
       rspec-mocks (~> 3.13.0)
     rspec-core (3.13.0)
       rspec-support (~> 3.13.0)
-    rspec-expectations (3.13.0)
+    rspec-expectations (3.13.1)
       diff-lcs (>= 1.2.0, < 2.0)
       rspec-support (~> 3.13.0)
     rspec-mocks (3.13.1)
       diff-lcs (>= 1.2.0, < 2.0)
       rspec-support (~> 3.13.0)
     rspec-support (3.13.1)
     rubyzip (2.3.2)
-    selenium-webdriver (4.21.1)
+    selenium-webdriver (4.22.0)
       base64 (~> 0.2)
+      logger (~> 1.4)
       rexml (~> 3.2, >= 3.2.5)
       rubyzip (>= 1.2.2, < 3.0)
       websocket (~> 1.0)