LinguaCafe icon indicating copy to clipboard operation
LinguaCafe copied to clipboard

Create default Storage folders

Open azukaar opened this issue 1 year ago • 39 comments

The app should create the storage folder itself upon first start rather than having to mount a bunch of empty folders manually this way you can let people manage their folders easily via customizing the mount. Note that without that it's impossible to use a volume for the mount without crashing the app

azukaar avatar Apr 28 '24 17:04 azukaar

Will do in the future. I know this is one of the issues blocking us from adding it to cosmos.

simjanos-dev avatar Apr 28 '24 19:04 simjanos-dev

I'm busted :p

azukaar avatar Apr 28 '24 20:04 azukaar

Sorry, there are just so many things to do, it will take a lot of time to address all the requests.

@sergiolaverde0

What other issues are there that prevent linguacafe to be added to Cosmos? I remember copying into the dictionaries folder being one.

simjanos-dev avatar Apr 28 '24 21:04 simjanos-dev

NP i was just investigating :)

azukaar avatar Apr 28 '24 21:04 azukaar

The app should create the storage folder itself upon first start rather than having to mount a bunch of empty folders manually

Docker permissions have been problematic for us when using this approach, most notably you wouldn't be able to put dictionaries in folders created by the docker process unless you do it as root, hence why simjanos mentioned the dictionaries. Also Laravel logs, I should go back to that detail this week.

What other issues are there that prevent linguacafe to be added to Cosmos?

Ideally the only interaction outside the app would be to create/download a docker-compose.yml and then do a docker compose up -d. Anything more than that would make people raise their eyebrows at us. This means we should stop relying in people cloning the deploy branch and not rely on it to ship updates to our deployment config; this will be less of an issue once we stabilize the project.

sergiolaverde0 avatar Apr 28 '24 21:04 sergiolaverde0

Also Laravel logs, I should go back to that detail this week.

What is the problem with laravel logs? It's not a directory users copy into.

2 people also mentioned the 777 permissions, which is there because of the shared temp folder between the two containers.

v0.12 will probably also have a fonts folder inside storage, I will use file upload for it.

I should go back to that detail this week

Please take your time with it. I only asked so I can understand it, it's not urgent.

simjanos-dev avatar Apr 28 '24 21:04 simjanos-dev

What is the problem with laravel logs? It's not a directory users copy into

No but it is a folder Laravel writes into, which runs as its own user instead of the root user. So unless you chmod it, it can't and that creates a breaking error (not even a warning).

sergiolaverde0 avatar Apr 28 '24 21:04 sergiolaverde0

Docker permissions have been problematic for us when using this approach, most notably you wouldn't be able to put dictionaries in folders created by the docker process unless you do it as root, hence why simjanos mentioned the dictionaries

Are users expected to copy those files manually? Or is it a script doing it? When I did the setup I just had dictionnaries pre-installed I think

I am assuming your main process is running as root in the container?

azukaar avatar Apr 28 '24 22:04 azukaar

Are users expected to copy those files manually?

Yes, as per setup instructions in the wiki.

I am assuming your main process is running as root in the container?

The webserver (Laravel) runs as its own user, the python service does run as root.

sergiolaverde0 avatar Apr 28 '24 23:04 sergiolaverde0

Not sure if it's useful, but Laravel has its own docker tools. At the time I wrote the docker file myself to learn about docker.

simjanos-dev avatar Apr 29 '24 11:04 simjanos-dev

I have managed to "skip" the chmod on a fresh install without Laravel complaining by having the process run as its own, non root user, but:

  • We still need the empty folders, without them present docker creates them as root and then the webserver lacks permissions to write to them.
  • It is not really backwards compatible unless the users manually delete (with sudo) the old log file since it exists with root ownership.

If I create the storage folder before mounting it I still have problems, specifically:

file_put_contents(/var/www/html/storage/framework/sessions/tPuB0jJWpUfvTpQs9u28Jr2PCJIalISSNGcAcG1o): Failed to open stream: No such file or directory

Which takes us back to OP's request to create the folders if they are not present.

I also don't know what would happen if you try to run this as a root like some run their home servers.

sergiolaverde0 avatar May 01 '24 21:05 sergiolaverde0

We still need the empty folders, without them present docker creates them as root and then the webserver lacks permissions to write to them.

Can we create them in the entrypoint, and set the permissions for them?

I also don't know what would happen if you try to run this as a root like some run their home servers.

Would this be a custom docker setup?

Can I do something to help with this issue, other than converting the dictionary system?

simjanos-dev avatar May 02 '24 01:05 simjanos-dev

We still need the empty folders, without them present docker creates them as root and then the webserver lacks permissions to write to them.

Can we create them in the entrypoint, and set the permissions for them?

Not a solution for that bit since I meant the storage folder on the host that gets mounted as a volume, not the one inside the container. However, this might work for the subfolders inside, will tinker with this idea a bit.

I also don't know what would happen if you try to run this as a root like some run their home servers.

Would this be a custom docker setup?

Not really, I just meant logging in a server as the root user and running the docker commands to start the service. People shouldn't do that but that means someone out there is doing it.

Can I do something to help with this issue, other than converting the dictionary system?

I am not sure, I think I can handle the rest and you also have your own backlog of things to do. I will keep you informed if I make progress.

sergiolaverde0 avatar May 02 '24 11:05 sergiolaverde0

I think this change from copying the files to uploading them will be confusing for some users, because they won't check the update notes. Should I use a new directory for the dictionaries, like dictionaries_uploaded, so it won't cause a problem? And should we delete the old one in the entrypoint?

simjanos-dev avatar May 02 '24 18:05 simjanos-dev

I think the Ideal solution would be to keep supporting copying the files for a couple of versions but mark it as deprecated in the wiki and encourage the file upload instead, using a single folder for everything.

New users will use the new method and old users won't be affected . By the time the old method gets removed it would have been several weeks or a few months so asking them to check the wiki won't be as annoying.

sergiolaverde0 avatar May 02 '24 18:05 sergiolaverde0

It is not really backwards compatible unless the users manually delete (with sudo) the old log file since it exists with root ownership.

I can change the name of the logfile in laravel config, so it will create one with the correct permissions.

simjanos-dev avatar May 03 '24 20:05 simjanos-dev

I think I have been able to make it work by both having the process inside the container run as non root and by using a named volume in the compose file.

The good:

  • If the steps are followed backwards compatibility should work perfectly fine.
  • New installs won't have to run chmodever again.
  • The process inside the container no longer runs as root for even more security.
  • The named volumes is the recommended way to share persistent data between containers.
  • Users only need the compose file and an empty folder, greatly simplifying deployment.
  • Adding folders to the images no longer needs to be reflected in the deploybranch.
  • The location of the storage folder can be programmatically defined in the .env file and can be anywhere with whatever name.
  • I also removed the version since it stopped doing anything 6 years ago and the warning was getting on my nerves.

The bad:

  • Users need to update both the image and and the compose file at the same time, and will have to run sudo chmod -R 777 ./ once from the root folder where things are mounted. The sudo is not optional.
  • The empty folder for storage still needs to be present in the location define, ./storage by default.
  • Deleting the folder does not delete the volume and viceversa. I haven't tested what happens if you change the path in the .env file but don't delete the old volume with docker volume rm linguacafe_xxx nor any of the other possible ways things might go wrong.

The ugly:

  • When trying to install a new language I'm getting an error message telling me that it failed. However if I go out of the screen and back in to force a refresh of the installed languages I can see it appears as installed. I tested Japanese and it worked perfectly. The logs of the Python container say it is returning status 200 so it is the PHP side that is complaining.
  • Having to delete both the folders and the volume was a bit annoying while testing. Named volumes are global and I didn't feel like trying what would happen if I create volumes with the same name bound to different locations.

See the drafts at #238 and #239 and let me know if they are solid, and I am building a test image from my fork if you want an easier time testing it.

I also want to take this chance to hear what does @azukaar think of this solution, if it meets his expectations, and if there are other blocking issues from this project being published to the Cosmos store.

sergiolaverde0 avatar May 05 '24 00:05 sergiolaverde0

Thank you so much! I've read it all, I will take a deeper look at it later today.

I am working on font type management now, will finish it in a few days. After that I will only change small things, and will release v0.12 sometime in May. I'll merge the 2 PR-s in afterwards if that works for you. Probably will make it its own update.

simjanos-dev avatar May 05 '24 07:05 simjanos-dev

Yes, that is fine by me, I will probably keep testing all the edge cases to make sure they are properly documented.

sergiolaverde0 avatar May 05 '24 12:05 sergiolaverde0

I will finish the font type management system today. What should I do with the 2 default font files for v0.12? Should I just upload them to the deploy branch?

simjanos-dev avatar May 07 '24 05:05 simjanos-dev

I haven't actually followed that feature, but if it makes sense for them to be mounted, because the location must be accessible for the other container or because user content will be persisted there, go ahead.

sergiolaverde0 avatar May 07 '24 12:05 sergiolaverde0

because user content will be persisted there, go ahead

That is the reason.

I was asking because we will copy these files from the image to the mounted folder automatically in the future if they are missing, and wanted to know if I should implement that now (and if yes, where/how should I do it), or should I just do it with the old deploy branch method.

simjanos-dev avatar May 07 '24 12:05 simjanos-dev

If you don't mind delaying the release a bit I can test if placing the files in the storage folder of the repo and building the image works on already existing installs, since that is one of the cases I want to test.

I know it will work with new installs just like the default book cover gets placed in its own path just fine.

sergiolaverde0 avatar May 07 '24 18:05 sergiolaverde0

~~Sorry, I do not have very deep knowledge about docker. Does using volumes means that docker will copy automatically the missing default files from inside the image to the volume/mounted directory?~~

I Read The FM:

If you start a container which creates a new volume, and the container has files or directories in the directory to be mounted such as /app/, Docker copies the directory's contents into the volume. The container then mounts and uses the volume, and other containers which use the volume also have access to the pre-populated content.

But I also have no idea how it would work with already existing files for existing users which we want to keep.

If you don't mind delaying the release a bit

If there were no further things to add, I wanted to release it next friday (17). I'm okay with pushing it back as much as it needed. Does this mean that you will want me to merge the the 2 PR-s for v0.12?

I want to test

Can I leave the testing of the docker process changes to you, and is it okay if I only test it once? My laptop would freeze a lot during it.

simjanos-dev avatar May 07 '24 20:05 simjanos-dev

But I also have no idea how it would work with already existing files for existing users which we want to keep.

Yes, this is specifically what I want to test. Where would the font files go? I don't see them in storage/ on the dev branch.

If there were no further things to add, I wanted to release it next friday (17)

This time frame sounds reasonable, weekend should be plenty for me to test. Maybe not to fix, in which case I will be notifying you to go with the original plan.

Does this mean that you will want me to merge the the 2 PR-s for v0.12?

If all goes well, yes. However there is still the error that pops up when installing new languages even when the Python service returns code 200 that should be addressed before this goes live; it only happens after my changes to the image. I have a test image at ghcr.io/sergiolaverde0/linguacafe-webserver:chmod.

Can I leave the testing of the docker process changes to you, and is it okay if I only test it once?

I think that's fine, I have gotten more through on my testing as I get used to all the corner cases I have to be careful about.

sergiolaverde0 avatar May 07 '24 22:05 sergiolaverde0

Yes, this is specifically what I want to test. Where would the font files go? I don't see them in storage/ on the dev branch.

"linguacafe/storage/app/fonts/". The 2 noto font files would be the default font files from "LinguaCafeDev/public/fonts", but without any subdirectory.

If all goes well, yes. However there is still the error that pops up when installing new languages even when the Python service returns code 200 that should be addressed before this goes live; it only happens after my changes to the image. I have a test image at ghcr.io/sergiolaverde0/linguacafe-webserver:chmod.

I forgot about that. Since you said it is on the PHP side, I will fix this one.

Does this mean that you will want me to merge the the 2 PR-s for v0.12? If all goes well, yes

In that case I will have to rewrite the dictionary upload part. I think v0.12 might will be ready around the end of the month instead.

I think that's fine, I have gotten more through on my testing as I get used to all the corner cases I have to be careful about.

Thank you so much! I really appreciate this. This whole docker task would have taken me months to do by myself.

@azukaar I think after these changes there will be nothing blocking us from adding linguacafe to Cosmos.

simjanos-dev avatar May 08 '24 05:05 simjanos-dev

Well I have tested it and the fonts folder wasn't added to the already existing volume when switching the image so there goes my plan. I works with new installs but old ones would have to add the folder manually and that's not a good user experience.

I was also reflecting: if the fonts already exist public what is the point in duplicating them to a volume? I have also been wondering about the default book cover being there instead of a folder right next to the flags at publictoo.

So I think we are back to the drawing board. OP's initial proposal of having the service check if the folder exists on startup and create it with fitting permissions if it doesn't is probably the best for the user since it gets rid of the git step and allows us to simplify to a single docker-compose.yml (yes I saw the reddit comment on r/selfhosted), then get rid of the empty folders.

I'm sorry if it sounds like I'm evading this task and handing it back to you, but I think that's the long term solution that keeps compatibility and does not bother the users too much when updating.

sergiolaverde0 avatar May 09 '24 00:05 sergiolaverde0

I was also reflecting: if the fonts already exist public what is the point in duplicating them to a volume?

They won't be duplicated, they will be moved.

I have also been wondering about the default book cover being there instead of a folder right next to the flags at public too.

Flag images are not being extended by users, there is just a set number of them, so I used the public folder. Fonts and cover images are uploaded, so they must go to the storage folder. Since every user uploaded font and dictionary goes to the storage folder, I saw no reason to separate the default ones to the public. I would also have to add a few lines of code so if the default file is requested, the program would have to retrieve it from a different place. In the case of fonts, I would also have to rename default files so I can identify them by filename, and handle uploaded files that contains the "default" in their filename. Which is not a big deal, I just saw no reason to do it differently, because this was the simplest way.

Would it be better to use the public folder?

I'm sorry if it sounds like I'm evading this task and handing it back to you, but I think that's the long term solution that keeps compatibility and does not bother the users too much when updating.

Not at all, I'm happy to help if I can. But honestly, I do not understand how copying from the image to the volume should work. Where should I create the empty folders and copy the empty files in the startup process? I think we could do it in entrypoint.sh.

The part that I don't really understand: where should I copy the files from to where? If I understand it correctly, we will store the default empty folders and files in the storage directory, which will be mounted as a volume.

I'll try to replicate the docker changes to the dev environment.

simjanos-dev avatar May 09 '24 06:05 simjanos-dev

Would it be better to use the public folder?

I think so. It is good practice to have your own assets separate from the ones provided by the user. Currently the docker images are technically not self contained since they need the default book cover to be mounted on a volume, and users can modify or remove it which beats the purpose. Defaults should be read only to ensure they can fulfill their role of being a something you can fall back to if everything else fails.

If I understand it correctly, we will store the default empty folders and files in the storage directory, which will be mounted as a volume.

Oh no, this is what I tried to do with the named volume and while it worked on a fresh install it fails to add new folders when updating.

My proposal was to have PHP check if the subfolders on the storage directory exists before trying to write to them, and if they don't then create them to prevent any errors. If new folders are needed they would be created by the container without user interaction.

However your idea of adding it to the entry point makes even more sense since it would run on startup, I will give it a go once I'm back from work.

sergiolaverde0 avatar May 09 '24 10:05 sergiolaverde0

I think so. It is good practice to have your own assets separate from the ones provided by the user. Currently the docker images are technically not self contained since they need the default book cover to be mounted on a volume, and users can modify or remove it which beats the purpose. Defaults should be read only to ensure they can fulfill their role of being a something you can fall back to if everything else fails.

Those are very good points, I'll make these changes for v0.12. I'll make sure that linguacafe will work without these folders:

  • /storage/app/images/book_images/
  • /storage/app/dictionaries/
  • /storage/app/fonts/

My proposal was to have PHP check if the subfolders on the storage directory exists before trying to write to them, and if they don't then create them to prevent any errors. If new folders are needed they would be created by the container without user interaction.

I think /storage/framework and /storage/logs folders and their subfolders will have to be created without Laravel, before anything Laravel related code runs.

However your idea of adding it to the entry point makes even more sense since it would run on startup, I will give it a go once I'm back from work.

Thank you so much! (But please only work on it if you feel like it. The discussed dates for the next updates are arbitrary, I'm okay with however long things take.)

simjanos-dev avatar May 09 '24 12:05 simjanos-dev