add container ID and pod ID to RuntimeContext and logs #879
Conversation
github-actions
bot
added
C-wasmtime
C-containerd-shim-wasm
C-wamr
C-wasmedge
C-wasmer
labels
Mossaka
changed the title
…cess this commit initializes the containerd-shim::logger inside of the zygote global process (the container process) so that containers can send logs to contaienrd Signed-off-by: Jiaxiao (mossaka) Zhou <duibao55328@gmail.com>
…to it This commit adds the container id to the runtimeContext and uses it in the log macros. It does so by adding a new `log` macros that takes a `RuntimeContext` and uses it to log the container id. Signed-off-by: Jiaxiao (mossaka) Zhou <duibao55328@gmail.com>
Signed-off-by: Jiaxiao (mossaka) Zhou <duibao55328@gmail.com>
There was a problem hiding this comment.
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
crates/containerd-shim-wasm/src/sys/unix/container/executor.rs:33
- [nitpick] A new field 'id' is added to the Executor struct without accompanying inline documentation. Adding a comment that explains its purpose and how it is propagated to the container runtime context would improve code readability.
id: String,
crates/containerd-shim-wamr/src/instance.rs:83
- [nitpick] The error log message may not provide enough context about the failure. Consider including additional details (e.g. function or module name) so that the log message better aids in debugging.
containerd_shim_wasm::error!(ctx, "Error: {:?}", err);
| let os_args: Vec<_> = std::env::args_os().collect(); | ||
|
|
||
| let flags = parse(&os_args[1..]).unwrap(); | ||
| let argv0 = PathBuf::from(&os_args[0]); |
There was a problem hiding this comment.
Accessing os_args[0] assumes that the argument vector is non-empty. Consider adding a check to ensure os_args contains at least one element to prevent a potential panic.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
|
|
||
| // the container id for the running container | ||
| fn container_id(&self) -> &str; | ||
|
|
||
| // the pod id for the running container (if available) | ||
| // In Kubernetes environments, containers run within pods, and the pod ID is usually | ||
| // stored in the OCI spec annotations under "io.kubernetes.cri.sandbox-id" | ||
| fn pod_id(&self) -> Option<&str> { | ||
| None | ||
| } |
There was a problem hiding this comment.
I think this is fine. We might consider putting this behind a struct/function that hides this behind something like info() if we have more things we might want to log. But as of now I can't think of anything.
| @@ -54,7 +56,7 @@ impl Engine for WasmEdgeEngine { | |||
| .register_module(Some(&mod_name), module) | |||
| .context("registering module")?; | |||
|
|
|||
| log::debug!("running with method {func:?}"); | |||
| containerd_shim_wasm::debug!(ctx, "running with method {func:?}"); | |||
There was a problem hiding this comment.
is it possible to do something like:
| containerd_shim_wasm::debug!(ctx, "running with method {func:?}"); | |
| containerd_shim_wasm::debug!(ctx, func=func; "running method"); |
so down stream users could add there own structured logs?
There was a problem hiding this comment.
I don't think this is supported. do you think we need to support it?
There was a problem hiding this comment.
No not necessarily, I though it might just work with the marco as is or small tweak. It is a nice to have but not critical, a downstream consumer could bypass our convenience macro and pass their own structure if they wanted too
The purpose of adding
idto RuntimeContext is that containers can add the id to their log messages to differentiate from other containers in the same pod.The disadvantage of this method is that functions that do not import
ctx: RuntimeContextwill not be able to have the container id in their logs.This builds on top of #878
Example
The following is containerd log for a pod that has three wasm tasks.