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

Create default Storage folders #228

Open
azukaar opened this issue Apr 28, 2024 · 39 comments
Open

Create default Storage folders #228

azukaar opened this issue Apr 28, 2024 · 39 comments
Labels
feature A new feature to be added.

Comments

@azukaar
Copy link

azukaar commented Apr 28, 2024

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

@simjanos-dev
Copy link
Owner

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

@simjanos-dev simjanos-dev added the feature A new feature to be added. label Apr 28, 2024
@azukaar
Copy link
Author

azukaar commented Apr 28, 2024

I'm busted :p

@simjanos-dev
Copy link
Owner

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.

@azukaar
Copy link
Author

azukaar commented Apr 28, 2024

NP i was just investigating :)

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

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.

@sergiolaverde0
Copy link
Contributor

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).

@azukaar
Copy link
Author

azukaar commented Apr 28, 2024

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?

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

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.

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

simjanos-dev commented May 2, 2024

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?

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

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?

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

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.

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

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.

@sergiolaverde0
Copy link
Contributor

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

@simjanos-dev
Copy link
Owner

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?

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

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.

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

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.

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

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.

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

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.

@sergiolaverde0
Copy link
Contributor

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.

@simjanos-dev
Copy link
Owner

simjanos-dev commented May 9, 2024

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.)

@sergiolaverde0
Copy link
Contributor

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/

Well now those are created at startup from the entrypoint before the Apache server is launched so you can skip that if you want.

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

Those are created too ;)

I closed the PR to the deploy branch since it is now pointless, and pushed a new commit to the other PR that creates the folders if they are missing. I was able to persist both uploaded fonts and book covers and copy dictionaries to the folder without running chmod at any moment, so the lack of default book cover and the fake error message when installing languages are the only issues for now. Dictionary upload can be added later if you don't want to delay the next release too much.

The storage folder still needs to be present on the location before starting the containers. It seems I can do a workaround for that with shell witchery but I might not do it on time for the release.

@simjanos-dev
Copy link
Owner

I was able to persist both uploaded fonts and book covers and copy dictionaries to the folder without running chmod at any moment

So the sudo chmod is not needed at all, or is it still required to update from v0.11 to v0.12?

fake error message when installing languages are the only issues for now

I forgot about that!

Dictionary upload can be added later if you don't want to delay the next release too much.

In that case, I fix the fake error message, move the default files, and everything will be finished for v0.12 today on my part. I will keep using and testing it until that. But again, we can release whenever, there are no deadlines.

The windows bat install file will also have to be modified, but since the deploy branch won't change anymore, it can stay the same for a while.

The storage folder still needs to be present on the location before starting the containers. It seems I can do a workaround for that with shell witchery but I might not do it on time for the release.

We can just do a smaller v0.13 update when it gets ready. I probably will be working on code staff that does not affect users, and small UI changes.

Thank you so much for this! This will be a huge step towards a standard docker process.

@simjanos-dev
Copy link
Owner

I've updated my dev environment again, because it threw an error. I copy-pasted the production one, and removed 2 lines from it. I found no errors.

I created a dev image, pulled, started the server, used sudo chmod 777 -R on the storage folder, and found no error in that so far either.

I think everything is ready for v0.12, the only thing missing is user manual and readme.

@simjanos-dev
Copy link
Owner

I updated the readme file with the new process. Also removed parts that were duplicated in the user manual. Can you please take a look at it, and check if everything is correct in the installation and update sections?

Is the windows script still usable? It still pulls the deploy branch. I would prefer to update that in v0.13. I'm also not sure if we will need that anymore, because the whole process became literally 1 or 2 commands.

I assume windows won't need the sudo chmod -R 777 ./ command (or windows version of it) for v0.12 to update, because we had no permission issues on windows in the first place.

Thank you again for working on this, this will be a huge update! I plan on releasing on Friday morning if there won't be any new issues. I've been using it, and found no problems.

@sergiolaverde0
Copy link
Contributor

I think if we are going to point them to download the compose file we might as well place it into the dev/main branches too. We removed it from there because people tried to use it when they couldn't but nowadays we actually want them to do that.

Also remember we only need to affect the storage folder so the command should be sudo chmod -R 777 ./storage, since I didn't touch the MySQL container and we don't want side effects on the database.

Is the windows script still usable? It still pulls the deploy branch. I would prefer to update that in v0.13.

If that is your intention then yes, it doesn't need any changes. For next release we could update it to merely check if docker is installed and running and to create the storage folder if that is still needed.

I assume windows won't need the sudo chmod -R 777 ./**storage** command

I don't really know, I'm not familiar with running docker in Windows. If we never encountered issues then we probably won't this time either, and if we do we at least know the solution.

@simjanos-dev
Copy link
Owner

I think if we are going to point them to download the compose file we might as well place it into the dev/main branches too. We removed it from there because people tried to use it when they couldn't but nowadays we actually want them to do that.

I changed that based on your suggestion. I also noticed version: "3" while copying it to the dev/main branch, and remembered you removed that in a closed PR, so I removed it as well.

Also remember we only need to affect the storage folder so the command should be sudo chmod -R 777 ./storage, since I didn't touch the MySQL container and we don't want side effects on the database.

Fixed that.

@simjanos-dev
Copy link
Owner

simjanos-dev commented May 17, 2024

@sergiolaverde0

On windows Laravel threw errors about not having permission for the log file. The user deleted the storage/logs and storage/framework folders, and it worked afterwards. Surprisingly it did not have any problem with the storage/app directory. We tested book cover images, fonts, dictionaries and ebook imports, it had no problem with any of those.

Edit:
Same thing for installing languages. I updated the readme file with a fix.

@simjanos-dev
Copy link
Owner

I am going to replace the dictionary file copy method with file upload in the next few days.

@simjanos-dev
Copy link
Owner

I changed the dictionary import process. Users now can upload the dictionary files, instead of having to copy them into the storage folder. I think this was the last problem in the code that blocked linguacafe from being added to Cosmos.

I will test it again on my personal production linguacafe server before I release v0.13. I'll also create a dev image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature to be added.
Projects
None yet
Development

No branches or pull requests

3 participants