Capture all logs #17
Capture all logs #17
Conversation
WalkthroughThe recent updates enhance logging and configuration capabilities for container management tools, focusing on capturing all logs to facilitate debugging. These changes span across configuration files and scripts, introducing new logging functions, environment variables, and adjustments to logging processes and permissions, ensuring comprehensive logging coverage for background services like Podman. Changes
Assessment against linked issues
The code changes effectively address the objective of capturing all logs for background services like Podman, as outlined in issue #15. By enhancing logging functions, introducing new environment variables for configuration, and making adjustments to the logging process and permissions, the updates ensure that logs from various services can be captured and routed effectively, aiding in debugging processes. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- base/root/etc/containers/containers.conf (1 hunks)
- lib/common.sh (1 hunks)
- runner/runner.sh (8 hunks)
Additional comments: 4
runner/runner.sh (2)
- 95-102: The introduction of
RUNNER_CONTAINERS_CONFDIRandRUNNER_LOGDIRas environment variables is a positive change for configurability and flexibility. However, consider the following:
- Best Practices: Ensure that the default values for these directories (
"/etc/containers"and"/var/log/runner") are appropriate for all deployment environments. It might be beneficial to document these defaults and their intended use cases.- Security: When creating directories or files, especially in global locations like
/var/log, ensure that permissions are set correctly to prevent unauthorized access. The script should explicitly set permissions forRUNNER_LOGDIRafter its creation.Overall, these changes are a step in the right direction for enhancing the script's configurability and logging capabilities.
- 284-298: The
runner_logfunction is a significant addition for managing logging more effectively. It redirects the output of a command to a log file and simultaneously uses thesublogfunction for formatted logging. Here are some considerations:
- Correctness: The function correctly handles background execution and PID tracking for both the main program and the
sublogprocess. This is crucial for clean termination and avoiding orphaned processes.- Performance: Consider the implications of logging every output of the main program. For verbose or high-output commands, this could lead to performance issues or disk space concerns. Implementing log rotation or size limits might be necessary.
- Maintainability: The function is well-structured and uses clear variable names. Adding comments to explain the purpose of each step and the expected format of the
SUBLOG_NAMEparameter would enhance maintainability.This function is a valuable addition to the script, improving the logging mechanism's flexibility and robustness.
base/root/etc/containers/containers.conf (2)
- 78-80: The addition of
default_sysctlswithnet.ipv4.ping_group_range=0 0is a useful configuration for allowing containers to adjust kernel parameters. However, it's important to ensure this change does not inadvertently introduce security vulnerabilities or conflict with other system settings.
- Please verify the impact of this sysctl setting on system security and stability.
- 504-504: Configuring
events_logfile_pathto"/var/log/podmand.log"and settingevents_loggerto"file"aligns with the PR's objective to enhance logging mechanisms. Ensure that appropriate log rotation and size limit configurations are in place to prevent issues with disk space usage.
- Please verify the log rotation and size limit configurations to ensure efficient disk space usage.
Also applies to: 518-518
| # `host` Share host Network Namespace with the container. | ||
| # `none` Containers do not use the network | ||
| # | ||
| netns = "host" |
There was a problem hiding this comment.
Setting netns to "host" means containers will share the host's network namespace by default. While this can simplify networking for certain use cases, it may introduce security risks by reducing isolation between containers and the host.
- Consider the security implications of this setting and evaluate its necessity based on the application's security requirements.
lib/common.sh
Outdated
| sublog() { | ||
| # Eagerly wait for the log file to exist | ||
| while ! [ -f "${1-0}" ]; do sleep 0.1; done | ||
| verbose "$1 now present on disk" | ||
|
|
||
| # Then reroute its content through our logging printf style | ||
| tail -n +0 -f "$1" | while IFS= read -r line; do | ||
| if [ -n "$line" ]; then | ||
| printf '[%s] [%s] %s\n' \ | ||
| "${2:-}@${KRUNVM_RUNNER_BIN:-$(basename "$0")}" \ | ||
| "$(date +'%Y%m%d-%H%M%S')" \ | ||
| "$line" \ | ||
| >&"$KRUNVM_RUNNER_LOG" | ||
| fi | ||
| done | ||
| } |
There was a problem hiding this comment.
The sublog() function introduces a mechanism to wait for a log file to exist and then continuously tail its contents, formatting each line before redirecting it to another log file. Here are some considerations and suggestions:
- Correctness: The loop
while ! [ -f "${1-0}" ]; do sleep 0.1; doneuses"${1-0}"which seems to be a typo. It should likely be"${1:-}"to correctly handle the case where$1is unset or null. The current default (0) does not make sense in the context of a file path. - Performance: Using
sleep 0.1in a loop can be CPU-intensive for a script that's expected to run as a background process. Consider increasing the sleep duration or implementing a more efficient waiting mechanism. - Maintainability: The function lacks comments explaining its parameters, expected behavior, and the format it uses for logging. Adding detailed comments would improve maintainability and understandability for future contributors.
- Best Practices: The use of
tail -n +0 -fis appropriate for following the log file from the beginning. However, consider handling the case where the log file grows very large, as this could potentially consume a lot of memory or processing power over time.
- while ! [ -f "${1-0}" ]; do sleep 0.1; done
+ while ! [ -f "${1:-}" ]; do sleep 0.1; doneConsider adding error handling for when the log file does not exist within a reasonable timeout and documenting the function's usage and parameters for clarity.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| sublog() { | |
| # Eagerly wait for the log file to exist | |
| while ! [ -f "${1-0}" ]; do sleep 0.1; done | |
| verbose "$1 now present on disk" | |
| # Then reroute its content through our logging printf style | |
| tail -n +0 -f "$1" | while IFS= read -r line; do | |
| if [ -n "$line" ]; then | |
| printf '[%s] [%s] %s\n' \ | |
| "${2:-}@${KRUNVM_RUNNER_BIN:-$(basename "$0")}" \ | |
| "$(date +'%Y%m%d-%H%M%S')" \ | |
| "$line" \ | |
| >&"$KRUNVM_RUNNER_LOG" | |
| fi | |
| done | |
| } | |
| sublog() { | |
| # Eagerly wait for the log file to exist | |
| while ! [ -f "${1:-}" ]; do sleep 0.1; done | |
| verbose "$1 now present on disk" | |
| # Then reroute its content through our logging printf style | |
| tail -n +0 -f "$1" | while IFS= read -r line; do | |
| if [ -n "$line" ]; then | |
| printf '[%s] [%s] %s\n' \ | |
| "${2:-}@${KRUNVM_RUNNER_BIN:-$(basename "$0")}" \ | |
| "$(date +'%Y%m%d-%H%M%S')" \ | |
| "$line" \ | |
| >&"$KRUNVM_RUNNER_LOG" | |
| fi | |
| done | |
| } |
runner/runner.sh
Outdated
| # Make directory for logging | ||
| mkdir -p "$RUNNER_LOGDIR" | ||
| chown "$RUNNER_USER" "$RUNNER_LOGDIR" | ||
| chmod g+rw "$RUNNER_LOGDIR" | ||
|
|
There was a problem hiding this comment.
Creating a dedicated directory for runner logs ($RUNNER_LOGDIR) and setting appropriate permissions is a good practice for managing application logs. However, ensure the following:
- Security: The script sets the owner of the log directory to
$RUNNER_USERand grants read/write permissions to the group. Verify that this does not inadvertently expose sensitive information to unauthorized users, especially in a shared environment. - Error Handling: Consider adding error handling for the
mkdir,chown, andchmodcommands to ensure the script can gracefully handle failures in directory creation or permission setting.
These precautions will help maintain the security and reliability of the logging mechanism.
Rewire the logs from the sub-processes of runner.sh through its logging mechanism. This provides more information about the underlying podman daemon and might help for debugging.
Close #15
Summary by CodeRabbit
sublogfunction for improved log file handling and content routing.runnerscript for enhanced logging, including Docker and Podman operations, and introduced a dedicated directory for runner logs.