Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[bitnami/influxdb] Allow separate persistence cfg for backups #24586

Closed
wants to merge 2 commits into from
Closed

[bitnami/influxdb] Allow separate persistence cfg for backups #24586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b05670b
[bitnami/influxdb] Allow separate persistence cfg for backups
kjdelisle Dec 12, 2023
30f4505
Update README.md with readme-generator-for-helm
bitnami-bot May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bitnami/influxdb/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,4 @@ maintainers:
name: influxdb
sources:
- https://github.com/bitnami/charts/tree/main/bitnami/influxdb
version: 6.0.12
version: 6.1.0
10 changes: 9 additions & 1 deletion bitnami/influxdb/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ As an alternative, you can use of the preset configurations for pod affinity, po
The data is persisted by default using PVC(s). You can disable the persistence setting the `persistence.enabled` parameter to `false`.
A default `StorageClass` is needed in the Kubernetes cluster to dynamically provision the volumes. Specify another StorageClass in the `persistence.storageClass` or set `persistence.existingClaim` if you have already existing persistent volumes to use.

If you would like to define persistence settings for a backup volume that differ from the persistence settings for the database volume, you may do so under the `backup.persistence` section of the configuration. If this section is undefined, but `backup.enabled` is set to true, the backup volume will be defined using the `persistence` parameter section.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you would like to define persistence settings for a backup volume that differ from the persistence settings for the database volume, you may do so under the `backup.persistence` section of the configuration. If this section is undefined, but `backup.enabled` is set to true, the backup volume will be defined using the `persistence` parameter section.
If you would like to define persistence settings for a backup volume that differ from the persistence settings for the database volume, you may do so under the `backup.persistence` section of the configuration by setting `backup.persistence.ownConfig` to `true`. The backup volume will otherwise be defined using the `persistence` parameter section.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify: if someone doesn't set backup.persistence.ownConfig: true, and they have persistence.existingClaim set to some volume, should the effective value of backup.persistence.existingClaim be the same? That would lead to binding of the same target volume, which is the RWO problem I mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone has backup.persistence.ownConfig: false (which is the proposed default, at least on this first version) the chart will have the very same behavior than the current version and any backup.persistence.* parameter will be ignored.

Any bugs present in the current version will still be there unless you explicitly set backup.persistence.ownConfig: true. Then, you have all the flexibility you want, right?


### Adjust permissions of persistent volume mountpoint

As the images run as non-root by default, it is necessary to adjust the ownership of the persistent volumes so that the containers can write data into it.
Expand Down Expand Up @@ -377,6 +379,12 @@ There are K8s distribution, such as OpenShift, where you can dynamically define
| `backup.enabled` | Enable InfluxDB™ backup | `false` |
| `backup.directory` | Directory where backups are stored | `/backups` |
| `backup.retentionDays` | Retention time in days for backups (older backups are deleted) | `10` |
| `backup.persistence.enabled` | Enable data persistence for backup volume | `false` |
| `backup.persistence.existingClaim` | Use a existing PVC which must be created manually before bound | `""` |
| `backup.persistence.storageClass` | Specify the `storageClass` used to provision the volume | `""` |
| `backup.persistence.accessModes` | Access mode of data volume | `["ReadWriteOnce"]` |
| `backup.persistence.size` | Size of data volume | `8Gi` |
| `backup.persistence.annotations` | Persistent Volume Claim annotations | `{}` |
| `backup.cronjob.schedule` | Schedule in Cron format to save snapshots | `0 2 * * *` |
| `backup.cronjob.historyLimit` | Number of successful finished jobs to retain | `1` |
| `backup.cronjob.podAnnotations` | Pod annotations | `{}` |
Expand Down Expand Up @@ -604,4 +612,4 @@ Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
limitations under the License.
7 changes: 5 additions & 2 deletions bitnami/influxdb/templates/cronjob-backup.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,12 @@ spec:
{{- end }}
{{- end }}
- name: {{ include "common.names.fullname" . }}-backups
{{- if .Values.persistence.enabled }}
{{- if .Values.backup.persistence.enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{- if .Values.backup.persistence.enabled }}
{{- if ternary .Values.backup.persistence.enabled .Values.persistence.enabled .Values.backup.persistence.ownConfig }}

persistentVolumeClaim:
claimName: {{ include "common.names.fullname" . }}-backups
claimName: {{ default (printf "%s-%s" (include "common.names.fullname" . ) "backups") .Values.backup.persistence.existingClaim }}
{{- else if .Values.persistence.enabled }}
persistentVolumeClaim:
claimName: {{ default (printf "%s-%s" (include "common.names.fullname" . ) "backups") .Values.persistence.existingClaim }}
Comment on lines +55 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
claimName: {{ default (printf "%s-%s" (include "common.names.fullname" . ) "backups") .Values.backup.persistence.existingClaim }}
{{- else if .Values.persistence.enabled }}
persistentVolumeClaim:
claimName: {{ default (printf "%s-%s" (include "common.names.fullname" . ) "backups") .Values.persistence.existingClaim }}
claimName: {{ include "influxdb.backup.claimName" . }}

It may be worth a helper function here, for similarity with pvc.yaml:

{{/*
Return the InfluxDB™ backup PVC name.
*/}}
{{- define "influxdb.backup.claimName" -}}
{{- if and .Values.backup.persistence.ownConfig .Values.backup.persistence.existingClaim }}
    {{- printf "%s" (tpl .Values.backup.persistence.existingClaim $) -}}
{{- else -}}
    {{- printf "%s-backups" (include "common.names.fullname" .) -}}
{{- end -}}
{{- end -}}

Note that I'm also deliberately suggesting here to not take into account persistence.existingClaim for backwards compatibility.

{{- else }}
emptyDir: {}
{{- end }}
Expand Down
27 changes: 17 additions & 10 deletions bitnami/influxdb/templates/pvc-backup.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,33 @@
Copyright Broadcom, Inc. All Rights Reserved.
SPDX-License-Identifier: APACHE-2.0
*/}}

{{- if and .Values.backup.enabled .Values.persistence.enabled (not .Values.persistence.existingClaim) }}
{{-
/*
Prefer .Values.backup.persistence, but fall back to .Values.persistence if not present.
*/
-}}
{{- if .Values.backup.enabled }}
{{ $persistence := coalesce .Values.backup.persistence .Values.persistence }}
{{ if and $persistence.enabled (not $persistence.existingClaim) }}
Comment on lines +5 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{-
/*
Prefer .Values.backup.persistence, but fall back to .Values.persistence if not present.
*/
-}}
{{- if .Values.backup.enabled }}
{{ $persistence := coalesce .Values.backup.persistence .Values.persistence }}
{{ if and $persistence.enabled (not $persistence.existingClaim) }}
{{- $persistence := ternary .Values.backup.persistence .Values.persistence .Values.backup.persistence.ownConfig }}
{{- if and .Values.backup.enabled $persistence.enabled (not $persistence.existingClaim) }}

kind: PersistentVolumeClaim
apiVersion: v1
metadata:
name: {{ include "common.names.fullname" . }}-backups
namespace: {{ include "common.names.namespace" . | quote }}
labels: {{- include "common.labels.standard" ( dict "customLabels" .Values.commonLabels "context" $ ) | nindent 4 }}
name: {{ include "common.names.fullname" $ }}-backups
namespace: {{ include "common.names.namespace" $ | quote }}
labels: {{- include "common.labels.standard" ( dict "customLabels" $.Values.commonLabels "context" $ ) | nindent 4 }}
Comment on lines +16 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the original contexts

Suggested change
name: {{ include "common.names.fullname" $ }}-backups
namespace: {{ include "common.names.namespace" $ | quote }}
labels: {{- include "common.labels.standard" ( dict "customLabels" $.Values.commonLabels "context" $ ) | nindent 4 }}
name: {{ include "common.names.fullname" . }}-backups
namespace: {{ include "common.names.namespace" . | quote }}
labels: {{- include "common.labels.standard" ( dict "customLabels" .Values.commonLabels "context" $ ) | nindent 4 }}

app.kubernetes.io/component: influxdb
{{- if or .Values.persistence.annotations .Values.commonAnnotations }}
{{- $annotations := include "common.tplvalues.merge" ( dict "values" ( list .Values.persistence.annotations .Values.commonAnnotations ) "context" . ) }}
{{- if or $persistence.annotations $.Values.commonAnnotations }}
{{- $annotations := include "common.tplvalues.merge" ( dict "values" ( list $persistence.annotations $.Values.commonAnnotations ) "context" $ ) }}
Comment on lines +20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{- if or $persistence.annotations $.Values.commonAnnotations }}
{{- $annotations := include "common.tplvalues.merge" ( dict "values" ( list $persistence.annotations $.Values.commonAnnotations ) "context" $ ) }}
{{- if or $persistence.annotations .Values.commonAnnotations }}
{{- $annotations := include "common.tplvalues.merge" ( dict "values" ( list $persistence.annotations .Values.commonAnnotations ) "context" . ) }}

annotations: {{- include "common.tplvalues.render" ( dict "value" $annotations "context" $) | nindent 4 }}
{{- end }}
spec:
accessModes:
{{- range .Values.persistence.accessModes }}
{{- range $persistence.accessModes }}
- {{ . | quote }}
{{- end }}
resources:
requests:
storage: {{ .Values.persistence.size | quote }}
{{- include "common.storage.class" ( dict "persistence" .Values.persistence "global" $) | nindent 2 }}
storage: {{ $persistence.size | quote }}
{{- include "common.storage.class" ( dict "persistence" $persistence "global" $) | nindent 2 }}
{{- end }}
{{- end }}
30 changes: 30 additions & 0 deletions bitnami/influxdb/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,6 +924,36 @@ backup:
## @param backup.retentionDays Retention time in days for backups (older backups are deleted)
##
retentionDays: 10

## Persistence parameters
##
persistence:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
persistence:
persistence:
## @param backup.persistence.ownConfig Prefer independent own persistence parameters to configure the backup volume
## When set to `false` (for backwards compatibility), the rest of the persistence parameters below will be ignored.
## This parameter will be set to `true` and removed in a future release.
##
ownConfig: false

## @param backup.persistence.enabled Enable data persistence for backup volume
##
enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enabled: false
enabled: true

## @param backup.persistence.existingClaim Use a existing PVC which must be created manually before bound
## If defined, PVC must be created manually before volume will be bound
## The value is evaluated as a template
##
existingClaim: ""
## @param backup.persistence.storageClass Specify the `storageClass` used to provision the volume
## If defined, storageClassName: <storageClass>
## If set to "-", storageClassName: "", which disables dynamic provisioning
## If undefined (the default) or set to null, no storageClassName spec is
## set, choosing the default provisioner.
##
storageClass: ""
## @param backup.persistence.accessModes Access mode of data volume
##
accessModes:
- ReadWriteOnce
## @param backup.persistence.size Size of data volume
##
size: 8Gi
## @param backup.persistence.annotations Persistent Volume Claim annotations
##
annotations: {}

## Cronjob configuration
## This cronjob is used to create InfluxDB&trade; backups
##
Expand Down
Loading