Refactor docker deployment and user setup #863
Conversation
…: reimplemented the docker-compose user setup to enhance product setup experience and cover much more cases.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #863 +/- ##
===========================================
- Coverage 56.43% 24.94% -31.49%
===========================================
Files 196 96 -100
Lines 15601 4406 -11195
Branches 558 0 -558
===========================================
- Hits 8804 1099 -7705
+ Misses 6543 3053 -3490
Partials 254 254
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…mple.yml, as it is defined in the Dockerfile; optimized Dockerfile combining all RUN directives of PROD stage into 1, which will produce single layer and save some space; added Dockerfile-dev, docker-compose-dev.yml, and new "dev" and "dev-down" directives into Makefile, which allows to setup development env in Docker. Instructions of how to use it are in comments at the beginning of Dockerfile-dev and docker-compose-dev.yml files
|
As a response to issue #876 I added the development environment setup. It could be enhanced by mounting the code folders to the Photoview container and running the server using dev server tools, like nodeamon, but these improvements require a good understanding of the code and dev environment, so if you see some benefits from that, as a developer, implement (and test) that from your side. To get more details, please read the conversation in the #876. In case these dev server enhancements are implemented and you'd like to include them in this container setup, please provide me a detailed guide on how to replicate them inside containers and I'll add this to the PR. |
|
While optimizing the Dockerfile, I noticed that Debian is used as a base image. While it is stable, it is old-fashioned and outdated compared to some newer Debian-based images (like Ubuntu). From my point of view, it is not an optimal base image. Does anybody have some objections against switching to ubuntu:latest? Maybe you see some even better options, taking stability and security into account. |
… so that the image could be later run with non-root permissions and the app still be able to do needed operations in the FS
I think both Debian and Ubuntu may be bloated for photoview, I'll try to write a Dockerfile using Alpine. |
|
Your PR looks like a piece of nice work! I have one issue with it though, I think introducing a Makefile has some serious drawbacks:
For me Make shouldn't be presented as the go-to way to setup photoview with docker. That setup should require docker only. Using Make creates complexity (system complexity and complexity for users) and additional assumptions: about users' OS and users' setup. As an example, my homeserver is very thin besides being a docker host - thanks to docker I don't have a lot of packages, including make. I also combine all services related to similar areas inside one folder, so I have photoview together with other media services in one compose. My proposition is:
This is a small change, but I think important. Otherwise your PR looks like a very helpful change for new users! |
|
@c13mn14k, thanks for your review and detailed feedback!
|
…` target and enhanced comments in the Makefile; commented out the `docker system prune -f` with the comment about the command and why it is there; added optional and commented by default `7zz` commands to the `backup` section of the Makefile
|
Set it temporary to Draft state to make sure it is aligned with the current product workflow |
|
Action Items:
|
…docker-compose # Conflicts: # docker-compose.example.yml
…rfile to build with less layers and run as non-root; mapped only Photoview related services to Watchtower by default instead of updating all running images on a host; added template for Postgres to the .env; reverted compose executable definition, so the new compose is called when present; added a tip about `lnav` to help
…mize backup target in Makefile
@jordy2254, would you be able to help me with this action item from the dev perspective? However, to provide a truly useful docker file, we need to replace the statically built executable web server binary with a dynamically rebuilding web server for development (like nodeamon for frontend and backend or an alternative tool). For this type of change, I'd need a developer, who can configure the corresponding tools from our project's code perspective and provide the step-by-step guide for me together with all needed configs. Then I'll use them to create frontend and backend docker files for developers in our project. Alternatively, I'll exclude the dev-dockerfile functionality from the PR completely, so that later someone else can create a new PR for that. |
|
This is a big PR with a lot of changes, so here is the proposed code review path to make it as clear and straightforward, as possible: |
There was a problem hiding this comment.
I have added some comments in the code.
Generally I think that the example is fairly nice. I don't have strong feelings on the Makefile, I think it's fine to have it as it can provide convenience to the user and they can choose to use it if they want. As long as the Makefile is not required to perform some actions and only for convenience. I'm maybe thinking about backups here.
My major concern is that the example is very complicated (because it does a lot of things). And it is also very opinionated. Maybe not everyone wants to use Watchtower or they have another solution that they want to use, the same goes for backups.
I think the old docker-compose.example.yml which only had the bare minimum to get Photoview up and running is much easier to approach for someone who might be new to Docker or wants to try Photoview for the first time.
One solution might be to keep the old and simple docker-compose.example.yml, but provide a fully featured and opinionated setup along with it, maybe we could add a README file to the example directory to explain how it works? Then we could add all the nice features that we want, including backup etc. without worrying about complexity for users who want to have their own setup.
I don't think we need to introduce breaking changes in this pull request, as all the changes don't need it or could easily be implemented in a way that doesn't.
| RUN npm ci --omit=dev --ignore-scripts \ | ||
| # Build frontend | ||
| && npm run build -- --base=$UI_PUBLIC_URL |
There was a problem hiding this comment.
Why merge these into one command. Wouldn't caching be more efficient by installing dependencies first. Then copy the source files afterwards. That way only npm run build needs to run when depdencies don't change?
COPY ui/package*.json /app/
RUN npm ci --omit=dev --ignore-scripts
There was a problem hiding this comment.
No, we've discussed this topic in the Discord: https://discord.com/channels/794576839813234688/1223295288668717146/1229127778679390319
Here is my answer for transparency:
Regarding layers: building production image on CI won't benefit much (if at all) from build cache, as in our project merges to master happen not so often. Having a well-optimized image for users is much better, as it pulls faster and uses less space on the host. It is even more important for ARM-based systems, where usually system partition is limited in space and performance.
In the case of the PR testing CI job, I'm not sure how exactly the docker cache works in Github, but from my experience with other CI/CD services, the docker cache is related to the branch, so every new PR will start a new thread of cache and won't utilize the cache from other PRs or master, so here there are no benefits from having more layers too.
Dockerfile
Outdated
| COPY --from=api /app/photoview /app/photoview | ||
|
|
||
| ENV PHOTOVIEW_LISTEN_IP 127.0.0.1 | ||
| ENV PHOTOVIEW_LISTEN_PORT 80 | ||
| ENV PHOTOVIEW_LISTEN_PORT 8080 |
There was a problem hiding this comment.
it is required by the "run as non-root user" improvement. We discussed this here: #863 (comment)
There was a problem hiding this comment.
Are you sure that this is the case for Docker environments as well?
I just found this post: https://forums.docker.com/t/non-root-user-able-to-bind-to-port-80-why/140524
Maybe we should make an issue keeping track of the changes that we want to make for version 3.0?
There was a problem hiding this comment.
We discussed this as part of the @Omar007 port change proposal. While I can confirm that the recent version of Docker allows using 80 port by a non-root user inside a container, it is not clear, starting from which version of Docker this change is introduced.
I can assume that some Linux distros or devices with proprietary Linux distros might still provide an older Docker version, where this is not yet implemented.
So, making sure that Photoview works according to Linux best practices on all supported platforms, looks more beneficial than keeping port 80 with the hope that it will work correctly everywhere.
I don't see a negative impact of switching the port number. Please share your thoughts if there is something negative, related to this change.
There was a problem hiding this comment.
I see @kkovaletp covered the reasoning already but @viktorstrate is also technically correct in that it can be 'breaking'.
If a user had been running as root or granted the required capabilities (or had been on a Docker version greater than ...?) and has never explicitly set PHOTOVIEW_LISTEN_PORT themselves at any point (which it would've been if they used the compose file) and where to just swap out the image, it would be 'breaking' in the sense that their port-forwards/mapping would stop working if they don't explicitly set PHOTOVIEW_LISTEN_PORT=80 when they update.
Since this is a defaults change but also one that is backward compatible (it does not require these users to change assigned permissions or capabilites; they could start dropping these now but they don't need to) and non-breaking if they use(d) the compose file, I wouldn't say this would require a major version bump, postponing to a major release or anything like that, but I would at the very least make sure this is mentioned on w/e the tag ends up being for people running custom deployments.
There was a problem hiding this comment.
Thanks @Omar007 for the explanation!
If we review the case, where a user updates an image, but doesn't update the compose and .env files (which doesn't make much sense from fixes and improvements delivery, but can happen in reality) nothing is going to break, as the compose file from the current master contains the PHOTOVIEW_LISTEN_PORT=80:
photoview/docker-compose.example.yml
Line 27 in 4133694
Of course, if this line was removed or commented out, it wouldn't work, but that is not the recommended config.
BTW, while answering this comment, I noticed the hardcoded port in the EXPOSE and HEALTHCHECK, so I replaced it with the variable, then tested that it works fine with the 8080 and 80 values (setting 80 in the compose file for the same image)
| # devices: | ||
| ## Uncomment next devices mappings if they are available in your host system | ||
| ## Intel QSV | ||
| # - "/dev/dri:/dev/dri" | ||
| ## Nvidia CUDA | ||
| # - "/dev/nvidia0:/dev/nvidia0" | ||
| # - "/dev/nvidiactl:/dev/nvidiactl" | ||
| # - "/dev/nvidia-modeset:/dev/nvidia-modeset" | ||
| # - "/dev/nvidia-nvswitchctl:/dev/nvidia-nvswitchctl" | ||
| # - "/dev/nvidia-uvm:/dev/nvidia-uvm" | ||
| # - "/dev/nvidia-uvm-tools:/dev/nvidia-uvm-tools" | ||
| ## Video4Linux Video Encode Device (h264_v4l2m2m) | ||
| # - "/dev/video11:/dev/video11" |
There was a problem hiding this comment.
Should we add this to the documentation website?
There was a problem hiding this comment.
I can if you'd want it to be there, but I don't see a benefit: the compose file is read every time the configuration is done, it is self-descriptive with the comments (as to me), putting it in the documentation creates a duplicate of the info with no additional value
Yes, this PR is 1 year old and includes fixes for many reported (and some not reported) issues in the project. It is not possible to fix all of them with a simpler file. Taking comments out of it is not an option, as users might not understand the logic and will make many configuration mistakes without the in-line explanations.
Sure, that is why Watchtower is marked as optional. The reason, why it is enabled by default, is to encourage users to use it and have always the latest images running. This will decrease the potential # of issues, submitted for previous builds, while there is a new build with the fix. However, if you see more benefits in getting it disabled (commented out) by default, please share your thoughts. Backups are configured in the Makefile, which is optional. There is only a variable in the
Yes, it is. But also, it doesn't contain many optimizations and fixes, so keeping it in the repo would create a false impression that both are equivalent from a fixes perspective and only the difference in advanced configuration with more features provided in this file, while it is not the case.
Now this Readme is included into the config files in the form of in-line comments, which makes these configs self-descriptive and straightforward to configure, while following the configuration path from the project's Readme. Extracting the comments to the external Readme wouldn't simplify the configs, but add more complexity to users, as they would need to constantly switch between the config and the Readme to understand how to modify it according to their cases.
1st of all, please let me know, what do you mean by "breaking changes"? I communicated with engineers, who understand it a bit differently. |
|
And thanks a lot for your review and the feedback provided, @viktorstrate ) |
|
Based on our recent discussion I set the |
kkovaletp
changed the title
|
According to the discussion in the product's Discord about this PR and the future product development strategy in general, it looks like this PR is going to be merged in some undefined future (if it will be at all). |
|
I really like the changes proposed in this PR, and I'm really sad to hear that this is getting dragged out so much and you start losing motivation. For me this change would be awesome if two things was changed:
|
|
@viktorstrate, thanks for your feedback and the summary of your expectations. Here are my short comments on them (the long and detailed explanations are here in the thread or the Discord - I don't remember already):
|
|
Can you elaborate and help me understand what things would cause misunderstandings for users? I like to have a bare minimum unopinionated example for users that is quick and easy to understand, and the let the users chose. |
|
@viktorstrate, instead of starting another iteration of the same discussion, I've created the |
|
I've found some minor bugs in config and will push a fixing commit, so please don't merge yet |
…ssions for storage folder; fix healthcheck command for postgres
|
I committed the fixes for the bugs, found y-day. Feel free to merge the PR. |
|
Thank you! I will merge this one just to move forward. A lot of changes has been made but nothing breaking so if we find issues later on, let's just handle and fix it then. If someone still have concerns with the changes here, feel free to open a new issue or start a discussion on Discord. |
Fix #862, address #826, and maybe some other tickets: reimplemented the docker-compose user setup to enhance the product setup experience and cover much more cases:
Please feel free to give your feedback and ask questions if you have any.