vmm: imporve observability to vmm-task & vmm-sandboxer #146

Ziy1-Tan · 2024-08-04T09:28:54Z

closes #5

Line 342 in 08cfdc8

    
           async fn append_container(&mut self, id: &str, options: ContainerOption) -> Result<()> {

Visualize the results:

Main Changes

: Replace env_logger with crate tracing;
: Add #[instrument] for Sandbox and Container API;
: Consume tracing data with the opentelemetry backend (e.g. Jaeger).

Steps

Enable tracing feature:

[sandbox]
enable_tracing = true
[hypervisor]
[hypervisor.task]
enable_tracing = true
[hypervisor.virtiofsd]

Run the jaeger server:

docker run --rm --name jaeger \
  -p 6831:6831/udp \
  -p 6832:6832/udp \
  -p 5778:5778 \
  -p 16686:16686 \
  -p 4317:4317 \
  -p 4318:4318 \
  -p 14250:14250 \
  -p 14268:14268 \
  -p 14269:14269 \
  -p 9411:9411 \
  jaegertracing/all-in-one

Run the Kuasar: https://github.com/kuasar-io/kuasar#quick-start
Check the result: visit the web page http://localhost:16686/

Instrumentation Scope

The following API will be instrumented:

Sandbox API

pub trait Sandboxer {
    type Sandbox: Sandbox + Send + Sync;
    async fn create(&self, id: &str, s: SandboxOption) -> Result<()>;
    async fn start(&self, id: &str) -> Result<()>;
    async fn update(&self, id: &str, s: SandboxData) -> Result<()>;
    async fn sandbox(&self, id: &str) -> Result<Arc<Mutex<Self::Sandbox>>>;
    async fn stop(&self, id: &str, force: bool) -> Result<()>;
    async fn delete(&self, id: &str) -> Result<()>;
}

#[async_trait]
pub trait Sandbox: Sync + Send {
    type Container: Container + Send + Sync;
    fn status(&self) -> Result<SandboxStatus>;
    async fn ping(&self) -> Result<()>;
    async fn container(&self, id: &str) -> Result<&Self::Container>;
    async fn append_container(&mut self, id: &str, option: ContainerOption) -> Result<()>;
    async fn update_container(&mut self, id: &str, option: ContainerOption) -> Result<()>;
    async fn remove_container(&mut self, id: &str) -> Result<()>;
    async fn exit_signal(&self) -> Result<Arc<ExitSignal>>;
    fn get_data(&self) -> Result<SandboxData>;
}

Task API

#[async_trait]
pub trait ContainerFactory<C> {
    async fn create(&self, ns: &str, req: &CreateTaskRequest) -> Result<C>;
    async fn cleanup(&self, ns: &str, c: &C) -> Result<()>;
}

#[async_trait]
pub trait ProcessFactory<E> {
    async fn create(&self, req: &ExecProcessRequest) -> Result<E>;
}

#[async_trait]
pub trait ProcessLifecycle<P: Process> {
    async fn start(&self, p: &mut P) -> crate::Result<()>;
    async fn kill(&self, p: &mut P, signal: u32, all: bool) -> crate::Result<()>;
    async fn delete(&self, p: &mut P) -> crate::Result<()>;
    async fn update(&self, p: &mut P, resources: &LinuxResources) -> crate::Result<()>;
    async fn stats(&self, p: &P) -> crate::Result<Metrics>;
    async fn ps(&self, p: &P) -> crate::Result<Vec<ProcessInfo>>;
}

Ziy1-Tan · 2024-09-16T07:11:31Z

cc @Burning1020

Burning1020

Good job! /cc @abel-von @flyflypeng
Could you add a markdown to give more detail usages and results?
Another question, after switching from log to tracing, what changes occurred in the format and amount of log output? Was there a comparison?

Burning1020 · 2024-09-20T06:53:39Z

vmm/sandbox/src/bin/cloud_hypervisor/main.rs

+    let env_filter = init_logger_filter(&args.log_level.unwrap_or(config.sandbox.log_level()))
+        .expect("failed to init logger filter");
+
+    let mut layers = vec![tracing_subscriber::fmt::layer().boxed()];
+    if config.sandbox.enable_tracing {
+        let tracer = init_otlp_tracer("kuasar-vmm-sandboxer-clh-tracing-service")
+            .expect("failed to init otlp tracer");
+        layers.push(tracing_opentelemetry::layer().with_tracer(tracer).boxed());
+    }
+
+    let subscriber = Registry::default().with(env_filter).with(layers);
+    tracing::subscriber::set_global_default(subscriber).expect("unable to set global subscriber");
+
+    let root_span = info_span!("kuasar-vmm-sandboxer-clh-root").entered();


Could you extra L44-54 in a common function and give sandboxers different service name?

Fixed. Maybe service name could be also configured by toml else use it default?

I think it’s better if the name isn’t configurable.

vmm/sandbox/src/bin/cloud_hypervisor/main.rs

vmm/task/Cargo.toml

vmm/task/src/main.rs

Ziy1-Tan · 2024-09-21T04:02:49Z

Good job! /cc @abel-von @flyflypeng Could you add a markdown to give more detail usages and results? Another question, after switching from log to tracing, what changes occurred in the format and amount of log output? Was there a comparison?

There are slight differences in the log format (brackets & colons):

env_logger::Builder::from_default_env()
	.format_timestamp_micros()
	.init();
log::error!("hello world from env_logger");  // [2024-09-21T04:02:26.007041Z ERROR log_test] Hello from env_logger
tracing::error!("hello world from tracing"); // 2024-09-21T04:02:05.022320Z ERROR log_test: Hello from tracing

Burning1020 · 2024-09-26T04:02:15Z

vmm/sandbox/src/bin/cloud_hypervisor/main.rs

+    tracer::setup_tracing(
+        &args.log_level.unwrap_or(config.sandbox.log_level()),
+        enable_tracing,
+        "kuasar-vmm-sandboxer-clh-otlp-service",


Please use kuasar-vmm-sandboxer-clh, as "oltp" is not the key info

Burning1020 · 2024-09-26T06:45:00Z

vmm/sandbox/src/stratovirt/hooks.rs

@@ -47,4 +52,11 @@ impl Hooks<StratoVirtVM> for StratoVirtHooks {
        sandbox.sync_clock().await;
        Ok(())
    }
+
+    async fn post_stop(&self, _sandbox: &mut KuasarSandbox<StratoVirtVM>) -> Result<()> {


post_stop was called every time before the sandbox stopped, so it's not appropriate call it here.

To make it easy, how about not positive stopping tracing, stopping it with the whole process exits?

Burning1020 · 2024-09-26T06:46:17Z

vmm/task/src/main.rs

+    if config.enable_tracing {
+        tracer::shutdown_tracing();
+    }


The task process also stopped tracing when it exited.

Burning1020 · 2024-10-23T06:44:12Z

vmm/common/src/tracer.rs

+    ENABLED.load(Ordering::Relaxed)
+}
+
+// 设置 ENABLED 的值


Please translate it to English

Burning1020 · 2024-10-23T06:47:02Z

vmm/common/src/tracer.rs

+    if enable_tracing {
+        let tracer = init_otlp_tracer(otlp_service_name)
+            .map_err(|e| anyhow!("failed to init otlp tracer: {}", e))?;
+        layers.push(tracing_opentelemetry::layer().with_tracer(tracer).boxed());
+    } else {
+        layers.push(
+            tracing_opentelemetry::layer()
+                .with_tracer(NoopTracer::new())
+                .boxed(),
+        );
+    }


let tracer = if enable_tracing { init_otlp_tracer(otlp_service_name) .map_err(|e| anyhow!("failed to init otlp tracer: {}", e))? } else { NoopTracer::new() }; layers.push(tracing_opentelemetry::layer().with_tracer(tracer).boxed());

tracing_opentelemetry::layer() will be added only when enable_tracing = true now.

Burning1020 · 2024-10-23T06:52:03Z

vmm/common/src/tracer.rs

+            SIGINT | SIGTERM => {
+                info!("Received signal {}, stopping tracing and exiting...", sig);
+                shutdown_tracing();
+                std::process::exit(0);
+            }
+            _ => {
+                if let Ok(sig) = nix::sys::signal::Signal::try_from(sig) {
+                    debug!("received {}", sig);
+                } else {
+                    warn!("received invalid signal {}", sig);
+                }
+            }


I think only signal SIGUSR1 should be handled, and the others should be kept same as before.

Burning1020 · 2024-10-23T07:02:45Z

vmm/sandbox/src/bin/cloud_hypervisor/main.rs

+    let log_level = args.log_level.unwrap_or(config.sandbox.log_level());
+    let service_name = "kuasar-vmm-sandboxer-clh-service";
+    tracer::setup_tracing(&log_level, enable_tracing, service_name).unwrap();
+    tracer::set_enabled(enable_tracing);


The ENABLED: AtomicBool static variable is the only one to detiminate, so it's better to:

... tracer::set_enabled(config.sandbox.enable_tracing); tracer::setup_tracing(&log_level, service_name).unwrap();

and let setup_tracing() read ENABLED

Burning1020 · 2024-10-23T07:10:33Z

vmm/common/src/tracer.rs

+    global::shutdown_tracer_provider();
+}
+
+pub async fn handle_signals(log_level: &str, otlp_service_name: &str) {


handle_signal() is used to handle all signals of the current process, so this function should not in the tracer.rs file(e.g. signal.rs), it is a more common funtion.

vmm/task/src/main.rs

Burning1020

LGTM. Thanks

Burning1020

LGTM. Thanks

Burning1020 · 2024-10-30T01:27:37Z

Please squash your commits. @Ziy1-Tan

Signed-off-by: Ziy1-Tan <[email protected]>

Ziy1-Tan requested a review from a team as a code owner August 4, 2024 09:28

Ziy1-Tan marked this pull request as draft August 4, 2024 09:29

Ziy1-Tan changed the title ~~vmm: Add observability to vmm-task & vmm-sandboxer~~ vmm: imporve observability to vmm-task & vmm-sandboxer Aug 4, 2024

Ziy1-Tan force-pushed the feat-trace branch from 4229fec to ca1da2d Compare August 11, 2024 08:15

Ziy1-Tan force-pushed the feat-trace branch from 452927a to 91da3f9 Compare September 8, 2024 13:27

Ziy1-Tan marked this pull request as ready for review September 8, 2024 13:35

Ziy1-Tan force-pushed the feat-trace branch 2 times, most recently from c7bb961 to 7171a71 Compare September 16, 2024 07:02

Ziy1-Tan requested a review from a team as a code owner September 16, 2024 07:02

Ziy1-Tan force-pushed the feat-trace branch 5 times, most recently from 5a8ef7b to ac3d462 Compare September 19, 2024 15:52

Burning1020 reviewed Sep 20, 2024

View reviewed changes

Ziy1-Tan force-pushed the feat-trace branch from 7466936 to 85664b5 Compare September 22, 2024 05:21

Burning1020 reviewed Sep 26, 2024

View reviewed changes

Ziy1-Tan force-pushed the feat-trace branch 2 times, most recently from 1014996 to 78276ac Compare October 19, 2024 16:41

Ziy1-Tan requested a review from Burning1020 October 19, 2024 16:47

Burning1020 reviewed Oct 23, 2024

View reviewed changes

Ziy1-Tan force-pushed the feat-trace branch from a372d7a to 4f11fb0 Compare October 25, 2024 13:56

Ziy1-Tan requested a review from Burning1020 October 26, 2024 06:42

Burning1020 approved these changes Oct 26, 2024

View reviewed changes

Ziy1-Tan force-pushed the feat-trace branch from a579ef1 to ecdca7c Compare October 29, 2024 12:00

vmm: support trace for sandbox and task api

1d3b361

Signed-off-by: Ziy1-Tan <[email protected]>

Ziy1-Tan force-pushed the feat-trace branch from ecdca7c to 1d3b361 Compare October 30, 2024 11:26

Burning1020 approved these changes Oct 30, 2024

View reviewed changes

Burning1020 merged commit 6504897 into kuasar-io:main Oct 30, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vmm: imporve observability to vmm-task & vmm-sandboxer #146

vmm: imporve observability to vmm-task & vmm-sandboxer #146

Ziy1-Tan commented Aug 4, 2024 •

edited

Loading

Ziy1-Tan commented Sep 16, 2024

Burning1020 left a comment

Burning1020 Sep 20, 2024

Ziy1-Tan Sep 22, 2024

Burning1020 Sep 25, 2024

Ziy1-Tan commented Sep 21, 2024 •

edited

Loading

Burning1020 Sep 26, 2024

Burning1020 Sep 26, 2024

Burning1020 Sep 26, 2024

Burning1020 Oct 23, 2024

Ziy1-Tan Oct 25, 2024

Burning1020 Oct 23, 2024

Ziy1-Tan Oct 25, 2024

Burning1020 Oct 23, 2024

Ziy1-Tan Oct 25, 2024

Burning1020 Oct 23, 2024

Ziy1-Tan Oct 25, 2024

Burning1020 Oct 23, 2024

Ziy1-Tan Oct 25, 2024

Burning1020 left a comment

Burning1020 left a comment

Burning1020 commented Oct 30, 2024

vmm: imporve observability to vmm-task & vmm-sandboxer #146

vmm: imporve observability to vmm-task & vmm-sandboxer #146

Conversation

Ziy1-Tan commented Aug 4, 2024 • edited Loading

Main Changes

Steps

Instrumentation Scope

Ziy1-Tan commented Sep 16, 2024

Burning1020 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ziy1-Tan commented Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Burning1020 left a comment

Choose a reason for hiding this comment

Burning1020 left a comment

Choose a reason for hiding this comment

Burning1020 commented Oct 30, 2024

Ziy1-Tan commented Aug 4, 2024 •

edited

Loading

Ziy1-Tan commented Sep 21, 2024 •

edited

Loading