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.

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

Unable to mount datadir without /data suffix #531

Open
Deaddy opened this issue Feb 6, 2024 · 6 comments · May be fixed by #538
Open

Unable to mount datadir without /data suffix #531

Deaddy opened this issue Feb 6, 2024 · 6 comments · May be fixed by #538

Comments

@Deaddy
Copy link
Contributor

Deaddy commented Feb 6, 2024

Describe your Issue

At the moment it is impossible to mount a data directory which is not $somepath/data.

The offending code is in lines 218-226 of deployment.yaml:

            {{- if and .Values.persistence.nextcloudData.enabled .Values.persistence.enabled }}
            - name: nextcloud-data
              mountPath: {{ .Values.nextcloud.datadir }}
              subPath: {{ ternary "data" (printf "%s/data" .Values.persistence.nextcloudData.subPath) (empty .Values.persistence.nextcloudData.subPath) }}
            {{- else }}
            - name: nextcloud-main
              mountPath: {{ .Values.nextcloud.datadir }}
              subPath: {{ ternary "data" (printf "%s/data" .Values.persistence.subPath) (empty .Values.persistence.subPath) }}
            {{- end }}

We see that /data is being appended irrespective of the content or existence of subPath.

Another issue is that the data dir is only used if also .Values.persistence.enabled is enabled and it would be convenient to remove this unnecessary and-condition in the same PR, but I am not yet familiar with how atomic we like our PRs here. :-) (context would be #532)

Approaches

I think there are basically two ways to deal with this.

  1. The cleaner version would be to set /data as the subPath-default Value, so it would only break for people who have set a subPath and they would need to update their values accordingly.

  2. The not so nice but non-breaking change would be to add another boolean variable with a glorious name like postFixData which is by default set to true and would then use the old logic, and if it is not set to true it would drop the subPath if empty and not add /data if it is set.

So, which approach do we prefer or did I possibly even miss a trick to make this work without code change?

@provokateurin
Copy link
Member

I'm not sure I understand the problem. Why do you need a to have the data dir at a different location? I'm not even sure if Nextcloud itself allows that.

@Deaddy
Copy link
Contributor Author

Deaddy commented Feb 6, 2024

It does.

Assume you have a non-k8s deployment of nextcloud which stores its data on some shared Storage like /nfs/nextcloud/ and want to move it over to k8s.

Now you do not want to move the data over to /nfs/nextcloud/data but keep it in place, lest you need to update all the 2 TB of oc_filecache and all the shares etc.

@provokateurin
Copy link
Member

Ah ok, I see 👍

@Deaddy
Copy link
Contributor Author

Deaddy commented Feb 6, 2024

Excellent!

Which approach do you favor? :-)

@provokateurin
Copy link
Member

The first is definitely cleaner, even though it is a breaking change.

@Deaddy
Copy link
Contributor Author

Deaddy commented Feb 19, 2024

To give an update to the PR (or lack thereof):

  • I think the subPath variable is appropriate in the scenario where we use one persistent volume for everything
  • at the moment I am unable to install a fresh nextcloud with the change and the official image because some of the script magic there still expects the data subdirectory and due to permission issues (listening on port 80/running as root) I am unable to manually try it out with occ, so I have to take a look at that
    Edit: I just overlooked the _helpers.tpl

Deaddy added a commit to Deaddy/nc-helm that referenced this issue Feb 22, 2024
- subPath now does not necessarily end on /data, in particular empty
  subPath possible for datadir on specific PVC
- datadir persistence now does not depend on overall persistence anymore
- bump .minor because old installs will need to add `/data` back to the
  subPath in the values file

Signed-off-by: Marcel Wunderlich <[email protected]>
@Deaddy Deaddy linked a pull request Feb 22, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants