-
Notifications
You must be signed in to change notification settings - Fork 45
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
Deploy Fluent Bit #2723
Deploy Fluent Bit #2723
Conversation
Hello alexandre-allard-scality,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
2209681
to
a7e07bb
Compare
31e4e2a
to
693fe20
Compare
a7e07bb
to
34cd3e2
Compare
693fe20
to
bf86469
Compare
34cd3e2
to
8ffd6e1
Compare
bf86469
to
a59134f
Compare
8ffd6e1
to
7d03763
Compare
a59134f
to
4612ca4
Compare
7d03763
to
e5fce98
Compare
e5fce98
to
784ae86
Compare
25bb1e4
to
6527baa
Compare
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch ( $ git fetch
$ git checkout origin/improvement/2701-deploy-fluent-bit-from-charts
$ git merge origin/development/2.6
$ # <intense conflict resolution>
$ git commit
$ git push origin HEAD:improvement/2701-deploy-fluent-bit-from-charts |
6527baa
to
6a13bbe
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
5433397
to
96c2445
Compare
96c2445
to
a0acb6b
Compare
[FILTER] | ||
Name modify | ||
Match host.* | ||
Rename HOSTNAME node |
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.
Not sure we should rename it to node because hostname may be different from node name :/
I would instead retrieve the node name of the current host here if it's doable (or add a hostname label on k8s logs but not sure it's easy to do as well)
If it's not possible or to much work, then ignore this comment and let's keep hostname as node for the moment
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.
PS look like hostname is not really good, on my test env I got some logs with localhost
and localhost.localdomain
as node (so likely HOSTNAME)
Looking on log it's seems these logs are not really relevant (maybe we should filter out logs with hostname localhost
and localhost.localdomain
)
Edit: Ok log with localhost
and localhost.localdomain
are only logs at the beginning before systemd-hostnamed
update the hostname
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.
It seems to be a little bit tricky to achieve that.
I can't find an easy way to retrieve the real hostname from the kubernetes logs or to retrieve the nodename from journal logs.
I've tried to use $HOSTNAME
env var, but it's set to the container hostname... not really useful.
Plus, the helm chart does not allow to set up env vars based on pod specs... otherwise we could have done:
[...]
spec:
containers:
- env:
- name: NODE_NAME
valueFrom:
fieldRef:
fieldPath: spec.nodeName
[...]
And use the env var in fluent bit configuration.
Maybe we should just have 2 different labels host
and node
... or we should not use this chart.
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.
Ok let's use hostname
and pod
for the moment, I updated the dashboard accordingly
helm repo add loki https://grafana.github.io/loki/charts helm repo update helm fetch -d charts --untar loki/fluent-bit Refs: #2701
this file is used to overwrite default values. Refs: #2701
there is a bug introduced by the latest change on how we handle CSC in this script, the script fails if no --service-config or --remove-manifest options are provided.
./charts/render.py fluent-bit --namespace metalk8s-logging \ --remove-manifest ConfigMap fluent-bit \ charts/fluent-bit.yaml charts/fluent-bit/ \ > salt/metalk8s/addons/logging/fluent-bit/deployed/chart.sls Refs: #2701
a0acb6b
to
0765628
Compare
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.
Just a comment about tests
16ea9b1
to
be626c7
Compare
spawn a container that logs a single line then try to retrieve it from Loki API Refs: #2701
This is needed for Fluent Bit to be able to send every logs to each Loki instance. Refs: #2682
We write all logs to each Loki replicas, so we need to define one Output block, in Fluent Bit configuration, per loki instance. Refs: #2701
be626c7
to
c0a0a46
Compare
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.
LGTM
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye alexandre-allard-scality. |
Component: salt, build, log
Context:
Summary:
Acceptance criteria:
Closes: #2701