Skip to content

Commit

Permalink
[newrelic-logging] Adjust order of env list in daemonset to fix depen…
Browse files Browse the repository at this point in the history
…dency reference (#1441)

<!--
Thank you for contributing to New Relic's Helm charts. Before you submit
this PR we'd like to
make sure you are aware of our technical requirements:

*
https://github.com/newrelic-experimental/helm-charts/blob/master/CONTRIBUTING.md#technical-requirements

For a quick overview across what we will look at reviewing your PR,
please read
our review guidelines:

*
https://github.com/newrelic-experimental/helm-charts/blob/master/REVIEW_GUIDELINES.md

Following our best practices right from the start will accelerate the
review process and
help get your PR merged quicker.

When updates to your PR are requested, please add new commits and do not
squash the
history. This will make it easier to identify new changes. The PR will
be squashed
anyways when it is merged. Thanks.

For fast feedback, please @-mention maintainers that are listed in the
Chart.yaml file.

Please make sure you test your changes before you push them. Once
pushed, a Github Action
will run across your changes and do some initial checks and linting.
These checks run
very quickly. Please check the results. We would like these checks to
pass before we
even continue reviewing your changes.
-->
#### Is this a new chart
No

#### What this PR does / why we need it:
When using `persistentVolume` mode, the env variable `FB_DB` references
`NODE_NAME`. This reference is currently not resolved, because
`NODE_NAME` is defined after `FB_DB`. Therefore, only a single database
file named `'$(NODE_NAME)-fb.db'` is created in the persistent volume,
instead of one file for each node. #1408 traced the problem down to this
[change](f6c7161#diff-dba7a505c5df68aac95df70ba65d1a43ac0349a7692e6a1ecd2899854ba90894L108-L128),
which moved the definition of `NODE_NAME` after the definition of
`FB_DB`.

This PR fixes this, by moving the definition of `NODE_NAME` before the
definition of `FB_DB` again.

The behavior of kubernetes for dependent environment variables is
documented
[here](https://kubernetes.io/docs/tasks/inject-data-application/define-interdependent-environment-variables/).

#### Which issue this PR fixes
  - fixes #1408

#### Special notes for your reviewer:
I was thinking about a test that checks if `NODE_NAME` is defined before
`FB_DB`, but I wasn't able to come up with a way to do this using
`helm-unittest`.

#### Checklist
- [x] Chart Version bumped
- [ ] Variables are documented in the README.md
- [x] Title of the PR starts with chart name (e.g. `[mychartname]`)

Co-authored-by: nr-rkallempudi <[email protected]>
  • Loading branch information
nluedema and nr-rkallempudi authored Oct 28, 2024
1 parent dcb19ec commit a02a800
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 5 deletions.
2 changes: 1 addition & 1 deletion charts/newrelic-logging/Chart.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
apiVersion: v2
description: A Helm chart to deploy New Relic Kubernetes Logging as a DaemonSet, supporting both Linux and Windows nodes and containers
name: newrelic-logging
version: 1.23.0
version: 1.23.1
appVersion: 2.0.2
home: https://github.com/newrelic/kubernetes-logging
icon: https://newrelic.com/assets/newrelic/source/NewRelic-logo-square.svg
Expand Down
9 changes: 5 additions & 4 deletions charts/newrelic-logging/templates/daemonset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ spec:
{{- else }}
value: "docker,cri"
{{- end }}
# NODE_NAME needs to be defined before FB_DB, because FB_DB references NODE_NAME in its value when using persistentVolume
- name: NODE_NAME
valueFrom:
fieldRef:
fieldPath: spec.nodeName
{{- if or (not .Values.fluentBit.persistence) (eq .Values.fluentBit.persistence.mode "hostPath") }}
- name: FB_DB
value: {{ .Values.fluentBit.db | quote }}
Expand All @@ -116,10 +121,6 @@ spec:
value: {{ include "newrelic-logging.lowDataMode" . | default "false" | quote }}
- name: RETRY_LIMIT
value: {{ .Values.fluentBit.retryLimit | quote }}
- name: NODE_NAME
valueFrom:
fieldRef:
fieldPath: spec.nodeName
- name: HOSTNAME
valueFrom:
fieldRef:
Expand Down

0 comments on commit a02a800

Please sign in to comment.