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 ./volumes at cloning #555

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ukkopahis
Copy link

@ukkopahis ukkopahis commented Apr 29, 2022

DRAFT - Do NOT merge

Letting this ruminate for a bit while trying to understand if this may cause problems down the line.


Make ~/IOTstack/volumes/ explicitly part of the project structure.

It's clearer that ./volumes exists right from the first checkout, rather than being created either by menu or Docker.

Even selecting some menu services and building the stack won't create the ./volumes -folder when. Hence Docker will create it when it creates service volume mountpoints. This results in volumes being owned by root. This will later cause problems for build hooks trying to prepare directories into ./volumes.

E.g. adding nextcloud from the menu when ./volumes is root-owned, will fail with:

Error running PreBuildHook on 'nextcloud'
[Errno 13] Permission denied: './volumes/nextcloud'

Creating volumes as part of the initial git-clone will ensure it's owned by the correct user.

@ukkopahis ukkopahis marked this pull request as draft April 29, 2022 12:59
@Paraphraser
Copy link

My view is that build-hooks and other IOTstack menu artifacts should adapt to the reality of what docker/docker-compose do by default, never the other way around.

In general, I'm not a fan of the menu or anything else futzing about in volumes. When building a stack, the menu should confine itself to the services directory and the compose file. We've already been down the "patch on-the-fly" path (with directoryfix.sh scripts) where things did/didn't work depending on whether and when the menu had been run and invoked the script. Those were forever causing problems, which is why they were steadily replaced with Dockerfile-based self-repair solutions which work independently of the menu - so they work whether the menu has run or not, whether the container has been brought up before or not, whether its persistent storage is as it was the last time the container ran, or has been reloaded from backup, or has just been nuked.

The acid test should be:

  1. Assume a docker-compose.yml. It doesn't matter whether it was produced by the menu or loaded from a restore or hand-crafted.
  2. Erase the volumes folder.
  3. Bring up the stack.

Whatever happens within that re-created volumes folder in terms of ownership and permissions should be the reference model. It should be treated as immutable fact. If a container needs something else within its own area, the container should make the change.

That all said, if there's a very very good case where a build-hook is to be preferred over a Dockerfile-based solution, and the build-hook needs more permissions than it has now, it should escalate with sudo. By "good case" I mean in a technical sense (eg a Dockerfile-based solution is basically impossible for some reason), rather than in a "convenience" sense. The solution also needs to show how it will still work if the container's persistent storage is nuked.

For all those reasons, I'm afraid that this PR isn't going to get a thumbs up from me. Not that anybody put me in charge, of course. I'm simply expressing a view based on what happened with directoryfix.sh, and seeing this PR as akin to "son of directoryFix" and, accordingly, a retrograde step. Just my 2¢.

👎

@Paraphraser
Copy link

I could also add that nextcloud (which, unless I've misunderstood what I was reading on Discord, was the trigger for this PR) works perfectly well under the "acid test" mentioned above.

In other words, whatever the nextcloud menu / build-hook is doing is unnecessary. For nextcloud, the only thing that needs to happen in the menu is to set the passwords for nextcloud and its database. No changes are needed in the volumes area.

I'm not saying this will necessarily be true for every container where an existing hook futzes about inside volumes (I haven't run every container where IOTstack has a template). But it's definitely true for nextcloud.

@ukkopahis
Copy link
Author

ukkopahis commented Apr 29, 2022

We've already been down the "patch on-the-fly" path (with directoryfix.sh scripts) ... were forever causing problems, which is why they were steadily replaced with Dockerfile-based self-repair solutions which work independently of the menu

Yes, the menu shouldn't go touching permissions of files mounted into containers. Lets label the different levels:

  1. ~/IOTstack/
  2. ~/IOTstack/volumes
  3. ~/IOTstack/volumes/wireguard

Only the third one is something that is touched and used by the container. It's mounted as ./volumes/wireguard:/config. Changing its permissions or anything inside is proper only from a Dockerfile/entrypoint-script.

The first two, on the other hand, are just parts of the project structure. What owner or permissions they have make no difference to the service. It's only about convenience.

The acid test should be:
...
2. Erase the volumes folder.

Change this to:
Erase all contents of volumes, but not the volumes-folder itself.

No need to cut off the hand in order to drop everything. There is no container that can control the ~/IOTstack/volumes-folder, only its subfolders may be mounted.

I could also add that nextcloud (which, unless I've misunderstood what I was reading on Discord, was the trigger for this PR) works perfectly well under the "acid test" mentioned above.

I'll add a fourth point to the ACID test:
4. All possible menu choices should produce a working docker-compose.yml

This PR is created because these steps cause menu to fail this test:

  1. clean clone
  2. add service without build.py (e.g. wireguard) using menu
  3. docker-compose up -d (resulting in volumes being owned by root)
  4. add nextcloud in menu and build: [Errno 13] Permission denied: './volumes/nextcloud'
    -> nextcloud is not added to docker-compose.yml

In other words, whatever the nextcloud menu / build-hook is doing is unnecessary.

Yes, nextcloud's build.py also has creation of folders that are mounted, and such things should be removed. I may create another PR to cleanup it.

But the error in question is not about a mounted Docker volume folder. Unlike wireshark, nextcloud mounts volumes from as subfolders to ./volumes/nextcloud. As such ./volumes/nextcloud is part of the project infrastructure and may be pre-created using a convenient filesystem owner. But using the acid test would create it owned by root. Such ambiguity is bad. The problem is that the build.py does lots of things without which the nextcloud menu-selection wouldn't work. And I feel like that is properly fixable only after #505.

Menu usually won't explicitly create the ./volumes -folder, hence Docker
will create it as owned by root. This will later cause problems for
build hooks trying to prepare directories into a now root-owned folder.

E.g. adding nextcloud later will fail with:

    Error running PreBuildHook on 'nextcloud'
    [Errno 13] Permission denied: './volumes/nextcloud'

Creating volumes as part of the initial git-clone will ensure it's owned
by the correct user.

For existing users, also add command to fix its owner when the menu is
started.
@ukkopahis
Copy link
Author

ukkopahis commented May 1, 2022

Is this change a good idea boils down to:

Will changing the owner of ~/IOTstack/volumes (but not touching its subfolders) matter to the functionality of any containers? Nothing said points to it having any effect.

The benefits are:

  • a user owned ~/IOTstack/volumes makes things easier for beginners, by requiring less sudo-usage.
  • fixes menu scripts failing due to permissions

@ukkopahis ukkopahis changed the title menu: create volumes with current user Create ./volumes at cloning May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants