feat(logger): allowed network policy logs retention customization #393
Conversation
|
@Pavani-Panakanti may I ask for a review? |
m00lecule
changed the title
m00lecule
force-pushed
the
feat-allow-network-policy-logs-custom-retention
branch
5 times, most recently
from
de857ee to
b01d275
Compare
|
@m00lecule Can you add more details in the PR description ? Thanks |
Description has been updated with more details. |
m00lecule
force-pushed
the
feat-allow-network-policy-logs-custom-retention
branch
from
97f2981 to
e0133cd
Compare
m00lecule
force-pushed
the
feat-allow-network-policy-logs-custom-retention
branch
from
e0133cd to
5e99c1c
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enables customization of network policy log retention by exposing new parameters for log file rotation to the logger.
- Adds new configuration fields and flag options for log file max size, max backups, max age, and compression.
- Updates logger implementation to pass these new parameters to the lumberjack logger.
- Modifies the main application initialization and controller configuration to incorporate the new logging options.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/logger/zaplogger.go | Passes new log rotation parameters to lumberjack.Logger and updates helper methods. |
| pkg/logger/logger.go | Introduces new default constants and updates logger constructors with additional parameters. |
| pkg/config/controller_config.go | Adds flag bindings and default values for the new log rotation parameters. |
| main.go | Updates logger initialization calls to include the log rotation parameters. |
main.go
Outdated
| @@ -53,15 +53,16 @@ func init() { | |||
| } | |||
|
|
|||
| func main() { | |||
| initLogger := logger.New("info", "") | |||
| initLogger := logger.New("info", "", 0, 0, 0, false) | |||
There was a problem hiding this comment.
[nitpick] Consider using the defined default constants instead of literal zero values when initializing the logger to ensure consistency with later default logger instantiation.
| initLogger := logger.New("info", "", 0, 0, 0, false) | |
| initLogger := logger.New("info", "", logger.DefaultLogFileMaxSize, logger.DefaultLogFileMaxBackups, logger.DefaultLogFileMaxAge, false) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
@m00lecule Can we also address this one ? Thanks
There was a problem hiding this comment.
@Pavani-Panakanti requested change has been implemented
pkg/config/controller_config.go
Outdated
| flagLogFileMaxSize = "log-file-max-size" | ||
| flagLogFileMaxBackups = "log-file-max-backups" | ||
| flagLogFileMaxAge = "log-file-max-age" | ||
| flagLogFileCompress = "log-file-compress" |
There was a problem hiding this comment.
Thanks for the contribution and these enhancements!
While supporting log-file-max-size and log-file-max-backups is helpful — especially for high-load environments with frequent pod churn, I don't think other two flags provide much benefit here
log-file-max-age: We could safely hardcode this to 30 days, as it's a reasonable retention window for local logs.
log-file-compress: Disabling compression seems rarely useful in practice. We can remove this flag and always default to allow compression
Let us know if you have any use case for overriding these two flags
There was a problem hiding this comment.
Hi, I just wanted to not leave those values hardcoded as somebody in future might want to tweat those also.
There was a problem hiding this comment.
Makes sense. We would prefer to keep the config to minimal unless there’s a strong use case today. If there's use case in the future, we can always add these back. We can remove log-file-max-age and log-file-compress for now.
There was a problem hiding this comment.
@Pavani-Panakanti requested change has been implemented
pkg/config/controller_config.go
Outdated
| defaultLogFileMaxSize = 200 | ||
| defaultLogFileMaxBackups = 8 | ||
| defaultLogFileMaxAge = 30 | ||
| defaultLogFileCompress = true |
There was a problem hiding this comment.
Default logger values are added in two places here and in logger.go file. We can keep them in one place. We can move all logger defaults to logger.go and use them here
defaultLogFileMaxSize = logger.DEFAULT_LOG_FILE_MAX_SIZE
There was a problem hiding this comment.
@Pavani-Panakanti requested change has been implemented
#392
Description of changes:
lumberjack.Logger, the default values are preserved (more informations can be found here: https://github.com/natefinch/lumberjack?tab=readme-ov-file#type-logger)Why we need this changes
Currently
/var/log/aws-routed-eni/network-policy-agent.logare rotated every 100 mb and only 5 rotated files are kept, resulting in ~500 mb networkpolicy logs kept locally. And what is worse, those parameters are hardcoded.During a long debugging sessions or while working on large EKS clusters we would benefit from keeping more data locally, especially given the fact they will be included in eks-log-collector archive - https://github.com/awslabs/amazon-eks-ami/blob/main/log-collector-script/linux/README.md.
Therefore the goal is to allow logs rotation configuration by adding arguments, which will override the defaults:
--log-file-max-sizeand--log-file-max-backups.Testing
Pass either
--log-file-max-sizeorlog-file-max-backupsand observe if networkpolicy logs (/var/log/aws-routed-eni/network-policy-agent.log) are rotated accordingly.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.