mirrord container docker check run (#2933)

* Fix?

* Changelog

* Tiny

* Small

* Ops

* Fix progress bar

* Use const bool for READONLY in add_volume

* Docs + rename

* Ops

changelog.d/2927.fixed.md

@@ -0,0 +1 @@
+Manually call `docker start <sidecar_id>` if after our sidecar `run` command the container hasn't started yet and is in "created" status.

mirrord/cli/src/container.rs

@@ -1,6 +1,5 @@
 use std::{io::Write, net::SocketAddr, path::Path, process::Stdio, time::Duration};
 
-use exec::execvp;
 use local_ip_address::local_ip;
 use mirrord_analytics::{
     AnalyticsError, AnalyticsReporter, CollectAnalytics, ExecutionKind, Reporter,
@@ -22,7 +21,7 @@ use tokio::{
 use tracing::Level;
 
 use crate::{
-    config::{ContainerCommand, ExecParams, RuntimeArgs},
+    config::{ContainerCommand, ContainerRuntime, ExecParams, RuntimeArgs},
     connection::AGENT_CONNECT_INFO_ENV_KEY,
     container::command_builder::RuntimeCommandBuilder,
     error::{CliResult, ContainerError},
@@ -193,6 +192,46 @@ async fn create_sidecar_intproxy(
             )
         })?;
 
+    // For Docker runtime sometimes the sidecar doesn't start so we double check.
+    // See [#2927](https://github.com/metalbear-co/mirrord/issues/2927)
+    if matches!(base_command.runtime(), ContainerRuntime::Docker) {
+        let mut container_inspect_command = Command::new(&runtime_binary);
+        container_inspect_command
+            .args(["inspect", &sidecar_container_id])
+            .stdout(Stdio::piped());
+
+        let container_inspect_output = container_inspect_command.output().await.map_err(|err| {
+            ContainerError::UnsuccesfulCommandOutput(
+                format_command(&container_inspect_command),
+                err.to_string(),
+            )
+        })?;
+
+        let (container_inspection,) =
+            serde_json::from_slice::<(serde_json::Value,)>(&container_inspect_output.stdout)
+                .unwrap_or_default();
+
+        let container_status = container_inspection
+            .get("State")
+            .and_then(|inspect| inspect.get("Status"));
+
+        if container_status
+            .map(|status| status == "created")
+            .unwrap_or(false)
+        {
+            let mut container_start_command = Command::new(&runtime_binary);
+
+            container_start_command.args(["start", &sidecar_container_id]);
+
+            let _ = container_start_command.status().await.map_err(|err| {
+                ContainerError::UnsuccesfulCommandOutput(
+                    format_command(&container_start_command),
+                    err.to_string(),
+                )
+            })?;
+        }
+    }
+
     // After spawning sidecar with -d flag it prints container_id, now we need the address of
     // intproxy running in sidecar to be used by mirrord-layer in execution container
     let intproxy_address: SocketAddr = {
@@ -221,8 +260,8 @@ pub(crate) async fn container_command(
     runtime_args: RuntimeArgs,
     exec_params: ExecParams,
     watch: drain::Watch,
-) -> CliResult<()> {
-    let progress = ProgressTracker::from_env("mirrord container");
+) -> CliResult<i32> {
+    let mut progress = ProgressTracker::from_env("mirrord container");
 
     if runtime_args.command.has_publish() {
         progress.warning("mirrord container may have problems with \"-p\" directly container in command, please add to \"contanier.cli_extra_args\" in config if you are planning to publish ports");
@@ -342,13 +381,14 @@ pub(crate) async fn container_command(
     );
 
     runtime_command.add_env(MIRRORD_CONFIG_FILE_ENV, "/tmp/mirrord-config.json");
-    runtime_command.add_volume(composed_config_file.path(), "/tmp/mirrord-config.json");
+    runtime_command
+        .add_volume::<true, _, _>(composed_config_file.path(), "/tmp/mirrord-config.json");
 
     let mut load_env_and_mount_pem = |env: &str, path: &Path| {
         let container_path = format!("/tmp/{}.pem", env.to_lowercase());
 
         runtime_command.add_env(env, &container_path);
-        runtime_command.add_volume(path, container_path);
+        runtime_command.add_volume::<true, _, _>(path, container_path);
     };
 
     if let Some(path) = config.internal_proxy.client_tls_certificate.as_ref() {
@@ -381,14 +421,28 @@ pub(crate) async fn container_command(
         sidecar_intproxy_address.to_string(),
     );
 
+    progress.success(None);
+
     let (binary, binary_args) = runtime_command
         .with_command(runtime_args.command)
-        .into_execvp_args();
+        .into_command_args();
+
+    let runtime_command_result = Command::new(binary)
+        .args(binary_args)
+        .stdin(Stdio::inherit())
+        .stdout(Stdio::inherit())
+        .stderr(Stdio::inherit())
+        .status()
+        .await;
 
-    let err = execvp(binary, binary_args);
-    tracing::error!("Couldn't execute {:?}", err);
+    let _ = composed_config_file.close();
 
-    analytics.set_error(AnalyticsError::BinaryExecuteFailed);
+    match runtime_command_result {
+        Err(err) => {
+            analytics.set_error(AnalyticsError::BinaryExecuteFailed);
 
-    Ok(())
+            Err(ContainerError::UnableToExecuteCommand(err).into())
+        }
+        Ok(status) => Ok(status.code().unwrap_or_default()),
+    }
 }

mirrord/cli/src/container/command_builder.rs

@@ -17,6 +17,10 @@ pub struct RuntimeCommandBuilder<T = Empty> {
 }
 
 impl<T> RuntimeCommandBuilder<T> {
+    pub(super) fn runtime(&self) -> &ContainerRuntime {
+        &self.runtime
+    }
+
     fn push_arg<V>(&mut self, value: V)
     where
         V: Into<String>,
@@ -34,7 +38,7 @@ impl RuntimeCommandBuilder {
         }
     }
 
-    pub fn add_env<K, V>(&mut self, key: K, value: V)
+    pub(super) fn add_env<K, V>(&mut self, key: K, value: V)
     where
         K: AsRef<OsStr>,
         V: AsRef<OsStr>,
@@ -46,7 +50,7 @@ impl RuntimeCommandBuilder {
         self.push_arg(format!("{key}={value}"))
     }
 
-    pub fn add_envs<I, K, V>(&mut self, iter: I)
+    pub(super) fn add_envs<I, K, V>(&mut self, iter: I)
     where
         I: IntoIterator<Item = (K, V)>,
         K: AsRef<OsStr>,
@@ -57,24 +61,33 @@ impl RuntimeCommandBuilder {
         }
     }
 
-    pub fn add_volume<H, C>(&mut self, host_path: H, container_path: C)
+    pub(super) fn add_volume<const READONLY: bool, H, C>(&mut self, host_path: H, container_path: C)
     where
         H: AsRef<Path>,
         C: AsRef<Path>,
     {
         match self.runtime {
             ContainerRuntime::Podman | ContainerRuntime::Docker | ContainerRuntime::Nerdctl => {
                 self.push_arg("-v");
-                self.push_arg(format!(
-                    "{}:{}",
-                    host_path.as_ref().display(),
-                    container_path.as_ref().display()
-                ));
+
+                if READONLY {
+                    self.push_arg(format!(
+                        "{}:{}:ro",
+                        host_path.as_ref().display(),
+                        container_path.as_ref().display()
+                    ));
+                } else {
+                    self.push_arg(format!(
+                        "{}:{}",
+                        host_path.as_ref().display(),
+                        container_path.as_ref().display()
+                    ));
+                }
             }
         }
     }
 
-    pub fn add_volumes_from<V>(&mut self, volumes_from: V)
+    pub(super) fn add_volumes_from<V>(&mut self, volumes_from: V)
     where
         V: Into<String>,
     {
@@ -86,7 +99,7 @@ impl RuntimeCommandBuilder {
         }
     }
 
-    pub fn add_network<N>(&mut self, network: N)
+    pub(super) fn add_network<N>(&mut self, network: N)
     where
         N: Into<String>,
     {
@@ -98,7 +111,10 @@ impl RuntimeCommandBuilder {
         }
     }
 
-    pub fn with_command(self, command: ContainerCommand) -> RuntimeCommandBuilder<WithCommand> {
+    pub(super) fn with_command(
+        self,
+        command: ContainerCommand,
+    ) -> RuntimeCommandBuilder<WithCommand> {
         let RuntimeCommandBuilder {
             runtime,
             extra_args,
@@ -115,7 +131,7 @@ impl RuntimeCommandBuilder {
 
 impl RuntimeCommandBuilder<WithCommand> {
     /// Return completed command command with updated arguments
-    pub fn into_command_args(self) -> (String, impl Iterator<Item = String>) {
+    pub(super) fn into_command_args(self) -> (String, impl Iterator<Item = String>) {
         let RuntimeCommandBuilder {
             runtime,
             extra_args,
@@ -133,12 +149,4 @@ impl RuntimeCommandBuilder<WithCommand> {
                 .chain(runtime_args),
         )
     }
-
-    /// Same as [`RuntimeCommandBuilder::<WithCommand>::into_command_args`] execvp expects first arg
-    /// to be binary so we pass the same value as runtime as first element
-    pub fn into_execvp_args(self) -> (String, impl Iterator<Item = String>) {
-        let (runtime, args) = self.into_command_args();
-
-        (runtime.clone(), std::iter::once(runtime).chain(args))
-    }
 }

mirrord/cli/src/main.rs

@@ -682,7 +682,11 @@ fn main() -> miette::Result<()> {
             Commands::Diagnose(args) => diagnose_command(*args).await?,
             Commands::Container(args) => {
                 let (runtime_args, exec_params) = args.into_parts();
-                container_command(runtime_args, exec_params, watch).await?
+                let exit_code = container_command(runtime_args, exec_params, watch).await?;
+
+                if exit_code != 0 {
+                    std::process::exit(exit_code);
+                }
             }
             Commands::ExternalProxy { port } => external_proxy::proxy(port, watch).await?,
             Commands::PortForward(args) => port_forward(&args, watch).await?,