Skip to content

Conversation

@rcdailey
Copy link
Contributor

@rcdailey rcdailey commented Dec 22, 2018

If is_writable() fails, fall back to logic that attempts to create a file
and then checks if it exists. If this check fails, an error occurs as it
did before.

Discussion on this solution was found here:
https://help.nextcloud.com/t/write-errors-for-nfs-mount/23328

Fixes #7124

@rcdailey
Copy link
Contributor Author

I'm very doubtful the developers want this logic to be completely removed. I'd love some feedback from the developers on what the appropriate solution should be. Additional configuration to disable the checks? Replace the checks with something less ambiguous? Let me know.

@rullzer
Copy link
Member

rullzer commented Jan 12, 2019

After some checking this seems to indeed be bug in the is_writable (or more precisely in the access function called). This might not work correctly on NFS mounts.

@rcdailey can I ask you to modify this PR. And instead of removing the faling tests. if the test fail try to write a file to the data directory (generate a long unqiue fiename, try to write it?).

Then I'm fine with getting this in.

@rullzer rullzer added bug enhancement 1. to develop Accepted and waiting to be taken care of labels Jan 12, 2019
@rullzer rullzer added this to the Nextcloud 16 milestone Jan 12, 2019
@rcdailey
Copy link
Contributor Author

@rullzer Could we please get this into stable15? I'm trying to keep the scope of change minimal so that it can be applied as a hotfix for current production release. I personally will not upgrade to v16 until it releases, and the next hotfix for 15 will come out faster. Not sure what your process is but can this be done?

Also, I run Nextcloud in a docker container. Using VS Code to debug PHP from Windows to a Docker Container seems sophisticated, so I'm not sure the best way to test this. Advice is appreciated. I am not normally a PHP developer so this technology is new to me.

@rcdailey rcdailey changed the title Disable data directory write checks Improve data directory write checking for NFS mounts Jan 12, 2019
@rcdailey
Copy link
Contributor Author

So I just replaced the PHP code in my config directory on my live instance, booted, and it worked. For a moment when I refreshed the page, I saw the text of the filename in the browser view, but after I did a 2nd refresh it went back to Nextcloud. So I'm not sure what that was about.

@rullzer
Copy link
Member

rullzer commented Jan 12, 2019

It gets into master first and then it gets backported.

@rcdailey
Copy link
Contributor Author

Ok I will let you test on master. I don't know how to do that without updrading my server to v16 which I don't want to do.

@rcdailey
Copy link
Contributor Author

@rullzer I've worked through a lot of the issues I had, although I can't consistently get Nextcloud to execute the checkServer() code. Seems to happen non-deterministically. Advice on how to properly reproduce the server check logic would be very helpful.

In the meantime, I think I am settled on the implementation. Would love your feedback as soon as you have time.

@rullzer
Copy link
Member

rullzer commented Jan 17, 2019

Looks sane to me. Let me get the others in here.

@rullzer rullzer changed the base branch from stable15 to master January 17, 2019 09:38
@rullzer rullzer changed the base branch from master to stable15 January 17, 2019 09:38
@rullzer rullzer added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Jan 17, 2019
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

code looks good, didn't test

@rcdailey
Copy link
Contributor Author

Do you prefer the target branch be master?

On an unrelated note, I find the way your developers do backports of fixes to stable branches confusing & unintuitive. Normally, hotfixes are applied to your stable releases and merged to your development branches (e.g. stable15 merges to master). Not doing this makes your git history look crazy, which it does, and you probably can't merge branches due to the fact that this also doesn't establish commit ancestry since you do cherry picks.

In any case, take it as general feedback, but we won't solve process issues here. I had to keep it on stable15 for my own personal testing, but I can rebase to master if that is required. Just please bear in mind I do not plan to do any testing on master since I do not have a staging environment. Please let me know where to go from here. I'm sure you're busy, but I was really hoping to get this merged in quicker since it's blocking me, and I don't like making docker image builds off my fork.

@rcdailey
Copy link
Contributor Author

@rullzer Sorry to keep pinging you. Could I get an update on this? When can we merge it? I want it off my plate. Thanks !

@kesselb
Copy link
Contributor

kesselb commented Jan 21, 2019

Do you prefer the target branch be master?

Yes

I can't consistently get Nextcloud to execute the checkServer() code. Seems to happen non-deterministically. Advice on how to properly reproduce the server check logic would be very helpful.

if (\OC::$server->getSession()->exists('checkServer_succeeded') && \OC::$server->getSession()->get('checkServer_succeeded')) {
return $errors;
}

Try to comment the block above. It seems that checkServer is cached per session.

If `is_writable()` fails, fall back to logic that attempts to create a file
and then checks if it exists. If this check fails, an error occurs as it
did before.

Discussion on this solution was found here:
https://help.nextcloud.com/t/write-errors-for-nfs-mount/23328

Fixes nextcloud#7124

Signed-off-by: Robert Dailey <rcdailey@gmail.com>
@rcdailey rcdailey changed the base branch from stable15 to master January 25, 2019 02:40
@rcdailey
Copy link
Contributor Author

@danielkesselberg I have rebased my branch onto master and made some of your suggested corrections. Please let me know if there is anything else required before it can be merged.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Lets do this 🚀

@rullzer rullzer merged commit ecc8ccc into nextcloud:master Jan 30, 2019
@welcome
Copy link

welcome bot commented Jan 30, 2019

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!

@rullzer
Copy link
Member

rullzer commented Jan 30, 2019

/backport to stable15

@backportbot-nextcloud
Copy link

backport to stable15 in #13906

@cgmastertecnology
Copy link

cgmastertecnology commented Apr 3, 2022

still not solved…getting crazy with NFS /data mount because of no write/read permission on folder
i can't install NC

I # following files:

/usr/src/nextcloud/lib/private/legacy/OC_Util.php
/var/www/html/lib/private/legacy/OC_Util.php

lines:
“Check if there is a writable install folder” and "Check if config folder is writable"

i can write on data folder from inside the container
Any suggestion?

My dockerfile:

version: "2"
services:
nextcloud:
image: nextcloud
container_name: nextcloud
environment:

  • PUID=${PUID}
  • PGID=${PGID}
  • TZ=${TZ}
  • NEXTCLOUD_DATA_DIR=/data
    volumes:
  • /home/**************/Docker/Nextcloud/Config:/var/www/html
  • nfs-data:/data
    ports:
  • 444:443
  • 4488:80
    restart: unless-stopped
    depends_on:
  • nextcloud_db

nextcloud_db:
image: linuxserver/mariadb
container_name: nextcloud_db
environment:

  • PUID=${PUID}
  • PGID=${PGID}
  • MYSQL_ROOT_PASSWORD=************
  • TZ=${TZ}
  • MYSQL_DATABASE=nextcloud_db
  • MYSQL_USER=nextcloud
  • MYSQL_PASSWORD=**************
    volumes:
  • /home/*********/Docker/Nextcloud/mariadb:/config
    restart: unless-stopped

volumes:
nfs-data:
driver_opts:
type: nfs4
o: addr=192.168.111.50,nfsvers=4,rw
device: :/volume1/Nextcloud/

@smarwei
Copy link

smarwei commented Oct 16, 2023

In case anyone else still has problems with NFS and finds this ticket: I'm mounting the data directory with samba now and it works fine.

st3iny added a commit that referenced this pull request Mar 4, 2024
The built-in method `is_writable()` is not reliable on NFS mounts so we
should fall back to creating a random file to check for delete
permissions.

Ref https://stackoverflow.com/a/50801358
Ref #13237

Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Php v7 filesystem check bug with nfs v3 mount from synology

7 participants