-
Notifications
You must be signed in to change notification settings - Fork 308
helm-chart(v4): Use env var for setting CONTAINER_LOG_PATH, new default for containerRuntime #1774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
helm-chart(v4): Use env var for setting CONTAINER_LOG_PATH, new default for containerRuntime #1774
Conversation
|
@cw-Guo @marcofranssen This is the first v4 helm-chart feature. I think the CRD re-factor being discussed in #1765 is another candidate. The changes here are compatible with fluent-operator <=3.5 as well. |
marcofranssen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Some Nits regarding docs clarity.
- Suggestion on the values.
- question on one of the macro functions.
Furthermore we also have to run helm-docs to update the README with updated values.
| app.kubernetes.io/name: fluent-operator | ||
| data: | ||
| fluent-bit.env: | | ||
| CONTAINER_ROOT_DIR={{ include "fluent-operator.containerLogPath" . | trimSuffix "/containers" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trim suffix would only work if people don't customize the paths.
This makes me wonder if we should even allow people to configure the log paths. Why don't we just always default based on the containerRuntime? I guess it is very unlikely people will use customized log paths with any containerRuntime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowing custom log paths was mainly for backwards compatibility for those users who may still be running the docker runtime with non standard log paths.
Do you think we should remove the option to configure custom paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah since we are doing a major version bump I don't think we need to support previous/deprecate things.
In case we want to do a more smooth migration we probably even want to do a 3.x release that only deprecates and falls back to old settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 8239036
Signed-off-by: Josh Baird <jbaird@galileo.io>
Signed-off-by: Josh Baird <jbaird@galileo.io>
Signed-off-by: Josh Baird <jbaird@galileo.io>
Signed-off-by: Josh Baird <jbaird@galileo.io>
Signed-off-by: Josh Baird <jbaird@galileo.io>
Signed-off-by: Josh Baird <jbaird@galileo.io>
Co-authored-by: Marco Franssen <marco.franssen@gmail.com> Signed-off-by: Josh Baird <jbaird@galileo.io>
Co-authored-by: Marco Franssen <marco.franssen@gmail.com> Signed-off-by: Josh Baird <jbaird@galileo.io>
Co-authored-by: Marco Franssen <marco.franssen@gmail.com> Signed-off-by: Josh Baird <jbaird@galileo.io>
Co-authored-by: Marco Franssen <marco.franssen@gmail.com> Signed-off-by: Josh Baird <jbaird@galileo.io>
0ccf1be to
cb82377
Compare
Signed-off-by: Josh Baird <jbaird@galileo.io>
Signed-off-by: Josh Baird <jbaird@galileo.io>
Fixes #1670 (in combination with PR #1773).
Because this PR contains breaking changes, it bumps the helm chart to
v4.0.0-RC.1. There are additional changes we wish to make with v4.0 of the helm chart -- this is the first.What?
This PR modernizes and simplifies the chart's process for configuring
containerRuntimewhich influences how Fluentbit collects logs from nodes:containerRuntimefromdockertocontainerd-- support for thedockerruntime was removed in Kubernetes v1.24 (2022) and modern distributions usecontainerdCONTAINER_LOG_PATHenvironment variable to thefluent-operatorDeployment which is used by Support setting CONTAINER_LOG_PATH via an environment variable #1773dockercontainerRuntimefluent-operator-envConfigMap that exposesCONTAINER_ROOT_DIRas an environment variable which is mounted to the FluentBit container(s) as a fileoperator.logPath.containerdandoperator.logPath.criooperator.containerLogPathfor direct path specificationcontainerd→/var/log/containerscrio→/var/log/containersdocker→/var/lib/docker/containersMIGRATION-v4document which detailed migration instructionsWhy?
The initContainers used a legacy and outdated vendored image which was difficult to maintain. Using files to provide the environment variable, while functional, was overly complex. Most of the complexity was because we were supporting custom log paths for the
dockerruntime. This runtime is no longer in use as of Kubernetes.If users want to continue using the
dockerruntime and provide a custom log path, they can do so via theoperator.containerLogPathvalue -- which is documented in the migration guide.