diff --git a/CHANGELOG.md b/CHANGELOG.md index 4aa53837293..68576891f29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to ### Changed +- [#4028](https://github.com/firecracker-microvm/firecracker/pull/4028): + Firecracker now creates the log and metrics files if they do not exist, + simplifying the launch of Firecracker by removing a manual step. + ### Deprecated ### Removed diff --git a/docs/getting-started.md b/docs/getting-started.md index 485d223a8ab..63e70d007f3 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -239,9 +239,6 @@ sudo iptables -t nat -A POSTROUTING -o "$HOST_IFACE" -j MASQUERADE API_SOCKET="/tmp/firecracker.socket" LOGFILE="./firecracker.log" -# Create log file -touch $LOGFILE - # Set log file sudo curl -X PUT --unix-socket "${API_SOCKET}" \ --data "{ diff --git a/src/vmm/src/device_manager/mod.rs b/src/vmm/src/device_manager/mod.rs index c045e17f0bf..ad30da5b5db 100644 --- a/src/vmm/src/device_manager/mod.rs +++ b/src/vmm/src/device_manager/mod.rs @@ -7,7 +7,6 @@ use std::convert::Infallible; use std::fmt::Debug; -use std::os::unix::prelude::OpenOptionsExt; use std::path::PathBuf; use std::sync::{Arc, Mutex}; @@ -36,6 +35,7 @@ use crate::devices::virtio::device::VirtioDevice; use crate::devices::virtio::transport::mmio::{IrqTrigger, MmioTransport}; use crate::resources::VmResources; use crate::snapshot::Persist; +use crate::utils::open_file_write_nonblock; use crate::vstate::bus::BusError; use crate::vstate::memory::GuestMemoryMmap; use crate::{EmulateSerialInitError, EventManager, Vm}; @@ -127,14 +127,7 @@ impl DeviceManager { output: Option<&PathBuf>, ) -> Result>, std::io::Error> { let (serial_in, serial_out) = match output { - Some(ref path) => ( - None, - std::fs::OpenOptions::new() - .custom_flags(libc::O_NONBLOCK) - .write(true) - .open(path) - .map(SerialOut::File)?, - ), + Some(path) => (None, open_file_write_nonblock(path).map(SerialOut::File)?), None => { Self::set_stdout_nonblocking(); diff --git a/src/vmm/src/logger/logging.rs b/src/vmm/src/logger/logging.rs index 21ede373991..8afdf976ffb 100644 --- a/src/vmm/src/logger/logging.rs +++ b/src/vmm/src/logger/logging.rs @@ -3,7 +3,6 @@ use std::fmt::Debug; use std::io::Write; -use std::os::unix::fs::OpenOptionsExt; use std::path::PathBuf; use std::str::FromStr; use std::sync::{Mutex, OnceLock}; @@ -14,6 +13,7 @@ use serde::{Deserialize, Deserializer, Serialize}; use utils::time::LocalTime; use super::metrics::{IncMetric, METRICS}; +use crate::utils::open_file_write_nonblock; /// Default level filter for logger matching the swagger specification /// (`src/firecracker/swagger/firecracker.yaml`). @@ -62,11 +62,7 @@ impl Logger { ); if let Some(log_path) = config.log_path { - let file = std::fs::OpenOptions::new() - .custom_flags(libc::O_NONBLOCK) - .write(true) - .open(log_path) - .map_err(LoggerUpdateError)?; + let file = open_file_write_nonblock(&log_path).map_err(LoggerUpdateError)?; guard.target = Some(file); }; diff --git a/src/vmm/src/utils/mod.rs b/src/vmm/src/utils/mod.rs index 8577bdcfd08..b5d5f94c7ff 100644 --- a/src/vmm/src/utils/mod.rs +++ b/src/vmm/src/utils/mod.rs @@ -10,7 +10,13 @@ pub mod signal; /// Module with state machine pub mod sm; +use std::fs::{File, OpenOptions}; use std::num::Wrapping; +use std::os::unix::fs::OpenOptionsExt; +use std::path::Path; +use std::result::Result; + +use libc::O_NONBLOCK; /// How many bits to left-shift by to convert MiB to bytes const MIB_TO_BYTES_SHIFT: usize = 20; @@ -64,3 +70,15 @@ pub const fn align_down(addr: u64, align: u64) -> u64 { debug_assert!(align != 0); addr & !(align - 1) } + +/// Create and open a File for writing to it. +/// In case we open a FIFO, in order to not block the instance if nobody is consuming the message +/// that is flushed to it, we are opening it with `O_NONBLOCK` flag. +/// In this case, writing to a pipe will start failing when reaching 64K of unconsumed content. +pub fn open_file_write_nonblock(path: &Path) -> Result { + OpenOptions::new() + .custom_flags(O_NONBLOCK) + .create(true) + .write(true) + .open(path) +} diff --git a/src/vmm/src/vmm_config/metrics.rs b/src/vmm/src/vmm_config/metrics.rs index 604c30e07b3..9d44c35f6a3 100644 --- a/src/vmm/src/vmm_config/metrics.rs +++ b/src/vmm/src/vmm_config/metrics.rs @@ -6,8 +6,8 @@ use std::path::PathBuf; use serde::{Deserialize, Serialize}; -use super::open_file_nonblock; use crate::logger::{FcLineWriter, METRICS}; +use crate::utils::open_file_write_nonblock; /// Strongly typed structure used to describe the metrics system. #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)] @@ -26,7 +26,7 @@ pub enum MetricsConfigError { /// Configures the metrics as described in `metrics_cfg`. pub fn init_metrics(metrics_cfg: MetricsConfig) -> Result<(), MetricsConfigError> { let writer = FcLineWriter::new( - open_file_nonblock(&metrics_cfg.metrics_path) + open_file_write_nonblock(&metrics_cfg.metrics_path) .map_err(|err| MetricsConfigError::InitializationFailure(err.to_string()))?, ); METRICS @@ -42,12 +42,6 @@ mod tests { #[test] fn test_init_metrics() { - // Error case: initializing metrics with invalid pipe returns error. - let desc = MetricsConfig { - metrics_path: PathBuf::from("not_found_file_metrics"), - }; - init_metrics(desc).unwrap_err(); - // Initializing metrics with valid pipe is ok. let metrics_file = TempFile::new().unwrap(); let desc = MetricsConfig { diff --git a/src/vmm/src/vmm_config/mod.rs b/src/vmm/src/vmm_config/mod.rs index d266328f3a8..3f544751142 100644 --- a/src/vmm/src/vmm_config/mod.rs +++ b/src/vmm/src/vmm_config/mod.rs @@ -2,12 +2,8 @@ // SPDX-License-Identifier: Apache-2.0 use std::convert::{From, TryInto}; -use std::fs::{File, OpenOptions}; use std::io; -use std::os::unix::fs::OpenOptionsExt; -use std::path::Path; -use libc::O_NONBLOCK; use serde::{Deserialize, Serialize}; use crate::rate_limiter::{BucketUpdate, RateLimiter, TokenBucket}; @@ -167,18 +163,6 @@ impl RateLimiterConfig { } } -/// Create and opens a File for writing to it. -/// In case we open a FIFO, in order to not block the instance if nobody is consuming the message -/// that is flushed to the two pipes, we are opening it with `O_NONBLOCK` flag. -/// In this case, writing to a pipe will start failing when reaching 64K of unconsumed content. -fn open_file_nonblock(path: &Path) -> Result { - OpenOptions::new() - .custom_flags(O_NONBLOCK) - .read(true) - .write(true) - .open(path) -} - #[cfg(test)] mod tests { use super::*; diff --git a/tests/integration_tests/functional/test_logging.py b/tests/integration_tests/functional/test_logging.py index 91d2944e556..68a15208b55 100644 --- a/tests/integration_tests/functional/test_logging.py +++ b/tests/integration_tests/functional/test_logging.py @@ -71,27 +71,6 @@ def check_log_message_format(log_str, instance_id, level, show_level, show_origi assert tag_level_no <= configured_level_no -def test_log_config_failure(uvm_plain): - """ - Check passing invalid FIFOs is detected and reported as an error. - """ - microvm = uvm_plain - microvm.spawn(log_file=None) - microvm.basic_config() - - # only works if log level is Debug - microvm.time_api_requests = False - - expected_msg = re.escape("No such file or directory (os error 2)") - with pytest.raises(RuntimeError, match=expected_msg): - microvm.api.logger.put( - log_path="invalid log file", - level="Info", - show_level=True, - show_log_origin=True, - ) - - def test_api_requests_logs(uvm_plain): """ Test that API requests are logged.