From 13ac0a0af405936bb63da234669adb993fe73867 Mon Sep 17 00:00:00 2001 From: Dmitry Dodzin Date: Wed, 27 Nov 2024 13:50:07 +0200 Subject: [PATCH 1/4] Fix failing nodejs integration test on macos (#2938) * Fix test? * only resolv? --- changelog.d/2935.fixed.md | 1 + mirrord/layer/tests/issue2283.rs | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) create mode 100644 changelog.d/2935.fixed.md diff --git a/changelog.d/2935.fixed.md b/changelog.d/2935.fixed.md new file mode 100644 index 00000000000..346a46d873e --- /dev/null +++ b/changelog.d/2935.fixed.md @@ -0,0 +1 @@ +Add `expect_file_open_for_reading` for /etc/resolv.conf path in `test_issue2283` test. diff --git a/mirrord/layer/tests/issue2283.rs b/mirrord/layer/tests/issue2283.rs index ad94c57fcdb..f3d7a2a9b2b 100644 --- a/mirrord/layer/tests/issue2283.rs +++ b/mirrord/layer/tests/issue2283.rs @@ -38,13 +38,13 @@ async fn test_issue2283( if cfg!(target_os = "macos") { intproxy - .expect_file_open_for_reading("/etc/hostname", 2137) + .expect_file_open_for_reading("/etc/resolv.conf", 2136) .await; - intproxy.expect_only_file_read(2137).await; + intproxy.expect_only_file_read(2136).await; intproxy - .answer_file_read("metalbear-hostname".as_bytes().to_vec()) + .answer_file_read("search home\nnameserver 10.0.0.138\n".as_bytes().to_vec()) .await; - intproxy.expect_file_close(2137).await; + intproxy.expect_file_close(2136).await; } let message = intproxy.recv().await; From a73fb72c0c434ca7f57e1805f6b7cd7e4630e7c4 Mon Sep 17 00:00:00 2001 From: meowjesty <43983236+meowjesty@users.noreply.github.com> Date: Wed, 27 Nov 2024 09:54:45 -0300 Subject: [PATCH 2/4] Retry http request (intproxy) on hyper IncompleteMessage error. (#2934) * Retry http request (intproxy) on hyper IncompleteMessage error. * also applies for streamed * changelog * apply great extra dot suggestion * another neat suggestion involving dots --------- Co-authored-by: Aviram Hassan --- ...+retry-on-http-incomplete-message.fixed.md | 1 + mirrord/intproxy/src/proxies/incoming.rs | 3 ++- .../src/proxies/incoming/interceptor.rs | 23 +++++++++++++++++-- 3 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 changelog.d/+retry-on-http-incomplete-message.fixed.md diff --git a/changelog.d/+retry-on-http-incomplete-message.fixed.md b/changelog.d/+retry-on-http-incomplete-message.fixed.md new file mode 100644 index 00000000000..592904039de --- /dev/null +++ b/changelog.d/+retry-on-http-incomplete-message.fixed.md @@ -0,0 +1 @@ +Retry http request (intproxy) on hyper IncompleteMessage error. diff --git a/mirrord/intproxy/src/proxies/incoming.rs b/mirrord/intproxy/src/proxies/incoming.rs index 6d1e223ef03..966d1175acb 100644 --- a/mirrord/intproxy/src/proxies/incoming.rs +++ b/mirrord/intproxy/src/proxies/incoming.rs @@ -620,7 +620,8 @@ impl IncomingProxy { } // Retry on known errors. Err(error @ InterceptorError::Reset) - | Err(error @ InterceptorError::ConnectionClosedTooSoon(..)) => { + | Err(error @ InterceptorError::ConnectionClosedTooSoon(..)) + | Err(error @ InterceptorError::IncompleteMessage(..)) => { tracing::warn!(%error, ?request, "Failed to read first frames of streaming HTTP response"); let interceptor = self diff --git a/mirrord/intproxy/src/proxies/incoming/interceptor.rs b/mirrord/intproxy/src/proxies/incoming/interceptor.rs index 5bf5301a6c5..2d6486d709f 100644 --- a/mirrord/intproxy/src/proxies/incoming/interceptor.rs +++ b/mirrord/intproxy/src/proxies/incoming/interceptor.rs @@ -71,6 +71,10 @@ pub enum InterceptorError { /// The layer closed connection too soon to send a request. #[error("connection closed too soon")] ConnectionClosedTooSoon(HttpRequestFallback), + + #[error("incomplete message")] + IncompleteMessage(HttpRequestFallback), + /// Received a request with an unsupported HTTP version. #[error("{0:?} is not supported")] UnsupportedHttpVersion(Version), @@ -282,6 +286,19 @@ impl HttpConnection { None, )) } + Err(InterceptorError::Hyper(e)) if e.is_incomplete_message() => { + tracing::warn!( + "Sending request to local application failed with: {e:?}. \ + Connection closed before the message could complete!" + ); + tracing::trace!( + ?request, + "Retrying the request, see \ + [https://github.com/hyperium/hyper/issues/2136] for more info." + ); + + Err(InterceptorError::IncompleteMessage(request)) + } Err(fail) => { tracing::warn!(?fail, "Request to local application failed!"); @@ -385,11 +402,13 @@ impl HttpConnection { Ok(response) => return Ok(response), Err(error @ InterceptorError::Reset) - | Err(error @ InterceptorError::ConnectionClosedTooSoon(_)) => { + | Err(error @ InterceptorError::ConnectionClosedTooSoon(_)) + | Err(error @ InterceptorError::IncompleteMessage(_)) => { tracing::warn!( ?request, %error, - "Either the connection closed or we got a reset, retrying!" + "Either the connection closed, the message is incomplete, \ + or we got a reset, retrying!" ); let Some(backoff) = backoffs.next() else { From 172e92e8722a325a1a41be75c0cf8fcc0317aa98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Smolarek?= <34063647+Razz4780@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:59:55 +0100 Subject: [PATCH 3/4] 3.125.1 (#2941) --- CHANGELOG.md | 15 +++++ Cargo.lock | 56 +++++++++---------- Cargo.toml | 2 +- ...+retry-on-http-incomplete-message.fixed.md | 1 - changelog.d/+scaledown-prep.internal.md | 1 - changelog.d/2935.fixed.md | 1 - 6 files changed, 44 insertions(+), 32 deletions(-) delete mode 100644 changelog.d/+retry-on-http-incomplete-message.fixed.md delete mode 100644 changelog.d/+scaledown-prep.internal.md delete mode 100644 changelog.d/2935.fixed.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 420c84d27d8..299e19d495b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,21 @@ This project uses [*towncrier*](https://towncrier.readthedocs.io/) and the chang +## [3.125.1](https://github.com/metalbear-co/mirrord/tree/3.125.1) - 2024-11-27 + + +### Fixed + +- Added retry of HTTP requests (intproxy) on hyper's `IncompleteMessage` error. + + +### Internal + +- Updated `RolloutSpec` and operator setup. +- Added `expect_file_open_for_reading` for `/etc/resolv.conf` path in + `test_issue2283` test. + [#2935](https://github.com/metalbear-co/mirrord/issues/2935) + ## [3.125.0](https://github.com/metalbear-co/mirrord/tree/3.125.0) - 2024-11-21 diff --git a/Cargo.lock b/Cargo.lock index 2e57c619bd9..4a399a605db 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2372,7 +2372,7 @@ dependencies = [ [[package]] name = "fileops" -version = "3.125.0" +version = "3.125.1" dependencies = [ "libc", ] @@ -3511,7 +3511,7 @@ checksum = "7943c866cc5cd64cbc25b2e01621d07fa8eb2a1a23160ee81ce38704e97b8ecf" [[package]] name = "issue1317" -version = "3.125.0" +version = "3.125.1" dependencies = [ "actix-web", "env_logger 0.11.5", @@ -3521,7 +3521,7 @@ dependencies = [ [[package]] name = "issue1776" -version = "3.125.0" +version = "3.125.1" dependencies = [ "errno 0.3.9", "libc", @@ -3530,7 +3530,7 @@ dependencies = [ [[package]] name = "issue1776portnot53" -version = "3.125.0" +version = "3.125.1" dependencies = [ "libc", "socket2", @@ -3538,14 +3538,14 @@ dependencies = [ [[package]] name = "issue1899" -version = "3.125.0" +version = "3.125.1" dependencies = [ "libc", ] [[package]] name = "issue2001" -version = "3.125.0" +version = "3.125.1" dependencies = [ "libc", ] @@ -3871,7 +3871,7 @@ checksum = "78b3ae25bc7c8c38cec158d1f2757ee79e9b3740fbc7ccf0e59e4b08d793fa89" [[package]] name = "listen_ports" -version = "3.125.0" +version = "3.125.1" [[package]] name = "litemap" @@ -4115,7 +4115,7 @@ checksum = "c9be0862c1b3f26a88803c4a49de6889c10e608b3ee9344e6ef5b45fb37ad3d1" [[package]] name = "mirrord" -version = "3.125.0" +version = "3.125.1" dependencies = [ "actix-codec", "clap", @@ -4175,7 +4175,7 @@ dependencies = [ [[package]] name = "mirrord-agent" -version = "3.125.0" +version = "3.125.1" dependencies = [ "actix-codec", "async-trait", @@ -4232,7 +4232,7 @@ dependencies = [ [[package]] name = "mirrord-analytics" -version = "3.125.0" +version = "3.125.1" dependencies = [ "assert-json-diff", "base64 0.22.1", @@ -4246,7 +4246,7 @@ dependencies = [ [[package]] name = "mirrord-auth" -version = "3.125.0" +version = "3.125.1" dependencies = [ "bcder", "chrono", @@ -4267,7 +4267,7 @@ dependencies = [ [[package]] name = "mirrord-config" -version = "3.125.0" +version = "3.125.1" dependencies = [ "bimap", "bitflags 2.6.0", @@ -4290,7 +4290,7 @@ dependencies = [ [[package]] name = "mirrord-config-derive" -version = "3.125.0" +version = "3.125.1" dependencies = [ "proc-macro2", "proc-macro2-diagnostics", @@ -4300,7 +4300,7 @@ dependencies = [ [[package]] name = "mirrord-console" -version = "3.125.0" +version = "3.125.1" dependencies = [ "bincode", "drain", @@ -4316,7 +4316,7 @@ dependencies = [ [[package]] name = "mirrord-intproxy" -version = "3.125.0" +version = "3.125.1" dependencies = [ "bytes", "exponential-backoff", @@ -4346,7 +4346,7 @@ dependencies = [ [[package]] name = "mirrord-intproxy-protocol" -version = "3.125.0" +version = "3.125.1" dependencies = [ "bincode", "mirrord-protocol", @@ -4356,7 +4356,7 @@ dependencies = [ [[package]] name = "mirrord-kube" -version = "3.125.0" +version = "3.125.1" dependencies = [ "actix-codec", "async-stream", @@ -4386,7 +4386,7 @@ dependencies = [ [[package]] name = "mirrord-layer" -version = "3.125.0" +version = "3.125.1" dependencies = [ "actix-codec", "base64 0.22.1", @@ -4436,7 +4436,7 @@ dependencies = [ [[package]] name = "mirrord-layer-macro" -version = "3.125.0" +version = "3.125.1" dependencies = [ "proc-macro2", "quote", @@ -4445,7 +4445,7 @@ dependencies = [ [[package]] name = "mirrord-macros" -version = "3.125.0" +version = "3.125.1" dependencies = [ "proc-macro2", "proc-macro2-diagnostics", @@ -4455,7 +4455,7 @@ dependencies = [ [[package]] name = "mirrord-operator" -version = "3.125.0" +version = "3.125.1" dependencies = [ "base64 0.22.1", "bincode", @@ -4489,7 +4489,7 @@ dependencies = [ [[package]] name = "mirrord-progress" -version = "3.125.0" +version = "3.125.1" dependencies = [ "enum_dispatch", "indicatif", @@ -4523,7 +4523,7 @@ dependencies = [ [[package]] name = "mirrord-sip" -version = "3.125.0" +version = "3.125.1" dependencies = [ "apple-codesign", "object 0.36.5", @@ -4536,7 +4536,7 @@ dependencies = [ [[package]] name = "mirrord-vpn" -version = "3.125.0" +version = "3.125.1" dependencies = [ "futures", "ipnet", @@ -4866,7 +4866,7 @@ dependencies = [ [[package]] name = "outgoing" -version = "3.125.0" +version = "3.125.1" [[package]] name = "outref" @@ -5924,14 +5924,14 @@ dependencies = [ [[package]] name = "rust-bypassed-unix-socket" -version = "3.125.0" +version = "3.125.1" dependencies = [ "tokio", ] [[package]] name = "rust-e2e-fileops" -version = "3.125.0" +version = "3.125.1" dependencies = [ "libc", ] @@ -5947,7 +5947,7 @@ dependencies = [ [[package]] name = "rust-unix-socket-client" -version = "3.125.0" +version = "3.125.1" dependencies = [ "tokio", ] diff --git a/Cargo.toml b/Cargo.toml index 387457b5abd..70d302d5026 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,7 +27,7 @@ resolver = "2" # latest commits on rustls suppress certificate verification [workspace.package] -version = "3.125.0" +version = "3.125.1" edition = "2021" license = "MIT" readme = "README.md" diff --git a/changelog.d/+retry-on-http-incomplete-message.fixed.md b/changelog.d/+retry-on-http-incomplete-message.fixed.md deleted file mode 100644 index 592904039de..00000000000 --- a/changelog.d/+retry-on-http-incomplete-message.fixed.md +++ /dev/null @@ -1 +0,0 @@ -Retry http request (intproxy) on hyper IncompleteMessage error. diff --git a/changelog.d/+scaledown-prep.internal.md b/changelog.d/+scaledown-prep.internal.md deleted file mode 100644 index 32843a86a5d..00000000000 --- a/changelog.d/+scaledown-prep.internal.md +++ /dev/null @@ -1 +0,0 @@ -Updated `RolloutSpec` and operator setup. \ No newline at end of file diff --git a/changelog.d/2935.fixed.md b/changelog.d/2935.fixed.md deleted file mode 100644 index 346a46d873e..00000000000 --- a/changelog.d/2935.fixed.md +++ /dev/null @@ -1 +0,0 @@ -Add `expect_file_open_for_reading` for /etc/resolv.conf path in `test_issue2283` test. From 04800870aced385ef48edfa3f93bf15a5036e6d1 Mon Sep 17 00:00:00 2001 From: Dmitry Dodzin Date: Wed, 27 Nov 2024 19:14:01 +0200 Subject: [PATCH 4/4] 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 | 1 + mirrord/cli/src/container.rs | 76 +++++++++++++++++--- mirrord/cli/src/container/command_builder.rs | 48 +++++++------ mirrord/cli/src/main.rs | 6 +- 4 files changed, 99 insertions(+), 32 deletions(-) create mode 100644 changelog.d/2927.fixed.md diff --git a/changelog.d/2927.fixed.md b/changelog.d/2927.fixed.md new file mode 100644 index 00000000000..66f98075187 --- /dev/null +++ b/changelog.d/2927.fixed.md @@ -0,0 +1 @@ +Manually call `docker start ` if after our sidecar `run` command the container hasn't started yet and is in "created" status. diff --git a/mirrord/cli/src/container.rs b/mirrord/cli/src/container.rs index 1a3af5eb01c..1a2afa27a76 100644 --- a/mirrord/cli/src/container.rs +++ b/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 { + 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::(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::(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()), + } } diff --git a/mirrord/cli/src/container/command_builder.rs b/mirrord/cli/src/container/command_builder.rs index 37282650104..e4cd6d843af 100644 --- a/mirrord/cli/src/container/command_builder.rs +++ b/mirrord/cli/src/container/command_builder.rs @@ -17,6 +17,10 @@ pub struct RuntimeCommandBuilder { } impl RuntimeCommandBuilder { + pub(super) fn runtime(&self) -> &ContainerRuntime { + &self.runtime + } + fn push_arg(&mut self, value: V) where V: Into, @@ -34,7 +38,7 @@ impl RuntimeCommandBuilder { } } - pub fn add_env(&mut self, key: K, value: V) + pub(super) fn add_env(&mut self, key: K, value: V) where K: AsRef, V: AsRef, @@ -46,7 +50,7 @@ impl RuntimeCommandBuilder { self.push_arg(format!("{key}={value}")) } - pub fn add_envs(&mut self, iter: I) + pub(super) fn add_envs(&mut self, iter: I) where I: IntoIterator, K: AsRef, @@ -57,7 +61,7 @@ impl RuntimeCommandBuilder { } } - pub fn add_volume(&mut self, host_path: H, container_path: C) + pub(super) fn add_volume(&mut self, host_path: H, container_path: C) where H: AsRef, C: AsRef, @@ -65,16 +69,25 @@ impl RuntimeCommandBuilder { 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(&mut self, volumes_from: V) + pub(super) fn add_volumes_from(&mut self, volumes_from: V) where V: Into, { @@ -86,7 +99,7 @@ impl RuntimeCommandBuilder { } } - pub fn add_network(&mut self, network: N) + pub(super) fn add_network(&mut self, network: N) where N: Into, { @@ -98,7 +111,10 @@ impl RuntimeCommandBuilder { } } - pub fn with_command(self, command: ContainerCommand) -> RuntimeCommandBuilder { + pub(super) fn with_command( + self, + command: ContainerCommand, + ) -> RuntimeCommandBuilder { let RuntimeCommandBuilder { runtime, extra_args, @@ -115,7 +131,7 @@ impl RuntimeCommandBuilder { impl RuntimeCommandBuilder { /// Return completed command command with updated arguments - pub fn into_command_args(self) -> (String, impl Iterator) { + pub(super) fn into_command_args(self) -> (String, impl Iterator) { let RuntimeCommandBuilder { runtime, extra_args, @@ -133,12 +149,4 @@ impl RuntimeCommandBuilder { .chain(runtime_args), ) } - - /// Same as [`RuntimeCommandBuilder::::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) { - let (runtime, args) = self.into_command_args(); - - (runtime.clone(), std::iter::once(runtime).chain(args)) - } } diff --git a/mirrord/cli/src/main.rs b/mirrord/cli/src/main.rs index 04048229b6e..e02b713ee64 100644 --- a/mirrord/cli/src/main.rs +++ b/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?,