Migrate the cron container from sidecar to K8s CronJob

[MartinKirchner]

Description of the change

With this change we no longer deploy a sidecar container to execute the cronjob, instead we deploy a Kubernetes CronJob object.

Benefits

• For me this has the major advantage that the CronJob can run as user www-data (uid 33) and execute the php command directly whereas crond in the sidecar has to run as root since otherwise the impersonation of user www-data fails (crond: can't set groups: Operation not permitted).
• History of failed and successful executions
• Apart from that I think that is also more robust if the container fails for some reason.

Possible drawbacks

None obvious to me.

Applicable issues

• fixes Feature: Use CronJob resources instead of a container with crond #577

Additional information

Checklist

• I have read the CONTRIBUTING.md doc.
• DCO has been signed off on the commit.
• Chart version bumped in Chart.yaml according to semver.
• (optional) Parameters are documented in the README.md

[pschichtel]

One concern that I had while originally thinking about the implementation: This would allow the cronjob to run even while the rest of nextcloud is not running (e.g. during upgrades). I'm not entirely sure if this is actually a problem, but worth checking what the implications are. I'm also not entirely sure if the current setup properly handles this in all cases.

[MartinKirchner]

One concern that I had while originally thinking about the implementation: This would allow the cronjob to run even while the rest of nextcloud is not running (e.g. during upgrades). I'm not entirely sure if this is actually a problem, but worth checking what the implications are. I'm also not entirely sure if the current setup properly handles this in all cases.

Thanks for this thought. But I think using the CronJob is not worse (maybe better) than the current state. Yes, it might happen that a cronJob is executed while a new, updated pod is deployed and potentially migrate the data model. But I have no idea how to avoid this one way or the other.

[pschichtel]

I did come across a few apps that ask their users to setup additional cronjobs, I wonder if that could be supported.

[MartinKirchner]

I did come across a few apps that ask their users to setup additional cronjobs, I wonder if that could be supported.

With the configurable command you can do basically whatever you want to, e.g. chain several commands, mount a script and execute this, ...

[wrenix]

The following is more common in helm-charts.
Where you have to add app.kubernetes.io/component: app manuelle.

{{/*
Common labels
*/}}
{{- define "nextcloud.labels" -}}
helm.sh/chart: {{ include "nextcloud.chart" . }}
{{ include "nextcloud.selectorLabels" . }}
{{- if .Chart.AppVersion }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}

{{/*
Selector labels
*/}}
{{- define "nextcloud.selectorLabels" -}}
app.kubernetes.io/name: {{ include "headscale.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}

PS: Maybe you should move such changes into smaller PRs for easiert review (and merge)

[MartinKirchner]

@wrenix
Thanks for your comment.

To make the review easier I used a commit for every step of the refactoring over smaller PRs as it makes only sense to me if get everything merged in. In commit a874bc4 you can see that I simply moved the labels as they were from the deployment.yamltohelpers.tpl`. So I guess someone had good reasoning to define the labels like this.

[pschichtel]

I think you must differentiate the labels used by the deployment to select its pod vs the labels on the cronjob pods, otherwise cronjob pods would show up as pods of a deployment and might get controlled by the replicaset controller, right? So app in app.kubernetes.io/component: app should probably be cronjob for cronjob pods or so.

[MartinKirchner]

@pschichtel Good that you noticed this! Thank you.

[wrenix]

@MartinKirchner as a Co-Maintainer i could easier merge such single changes / small PRs.
For the change from sidecar to cronjob itself, i believe we need a longer discussion, see #683

PS: i like to have an _helper for the labels and that could be in this week if i got a easy to review.