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

Unison > option to map uid from host #21

Closed
wants to merge 4 commits into from
Closed

Unison > option to map uid from host #21

wants to merge 4 commits into from

Conversation

mickaelperrin
Copy link
Contributor

This pull request adds a special value from_host for sync_userid and sync_groupid options.

When used with unison, it enables syncing of host uid gid alongside the files.

Additionally it sets the adequate ownership for the mounted folder.

@EugenMayer
Copy link
Owner

This will only work for the first sync, since your docker exec is not executed when docker-sync sync is executed, after starting the sync, if you create a new file, you will see it having the wrong ownershipt in the container. This needs a different solution.

Also you are using Process.uid - this uid might simply not exist on the unison container and will change - you do create this user pre container start.

I also do not want to run arbitrary docker exec commands which actually should have been things to be part of the entrypoint shell wrapper. We should

a) offer user/uid group/guid as ENV vars to the unison image
b) the unison image should be customised in terms of the entrypoint and create the users the same way we do for rsync
c) we need to ensure that permissions are applied for every new synced file, so this being part either of unison or the "sync" command as a custom docker exec (other solutions preferred).

I am sorry to not offer you a quick-shot solution here, but this needs tender care before we introduce this.

Thanks for your thoughs and the contribution

@@ -82,6 +85,7 @@ def start_container
if exists == ''
say_status 'ok', "creating #{container_name} container", :white if @options['verbose']
cmd = "docker run -p '#{@options['sync_host_port']}:#{UNISON_CONTAINER_PORT}' -v #{volume_name}:#{@options['dest']} -e UNISON_VERSION=#{UNISON_VERSION} -e UNISON_WORKING_DIR=#{@options['dest']} --name #{container_name} -d #{UNISON_IMAGE}"
cmd = cmd + " && docker exec #{container_name} chown #{Process.uid} #{@options['dest']}" if @options['sync_userid'] == 'from_host'
Copy link
Owner

Choose a reason for hiding this comment

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

should not happen here, otherwise its on container start only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't understand why do we need it elsewhere than docker start ? unison manage uid/gid syncing for other folders, those that will be updated.

Copy link
Owner

Choose a reason for hiding this comment

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

when the dest folder on the unison container has uid 10 and you start the container, sync and then run the chown, the files are properly owned.

When you now create a file on the host FS, a new one, which then triggers fswatch, which triggers docker-sync sync - this will just run unison on the host, without the docker exec. This means, the files have the wrong permissions on the host - or are they handled "magical" stickiness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You maybe didn't have remarked that the chown is not recursive. The only permission issue that we need to manage manually is the 'base' folder. unison handles the permission of all files / directories created inside the 'base' folder with the use of user, group and numericids options.

Copy link
Owner

Choose a reason for hiding this comment

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

ah, those are very intersting points - i simply was not aware of this, thank you!

@mickaelperrin
Copy link
Contributor Author

Indeed, I intend to work on a new docker image for unison, but was mainly to manage fsevents on the container side.

I don't understand why we have to create users in the volume container. I thought that what's important is uid/gid not users. Depending on where the folder should be mounted you could use the same uid but mapped to a different user.

For example:

  • src is mounted in nginx you could want to map the uid to the nginx user.
  • src is mounted in php-fpm, you could want to map the uid to the www-data user.

In my opinion, user creation / update should be done in the app / stack side not in the volume container.

@EugenMayer
Copy link
Owner

EugenMayer commented Jul 10, 2016

Sure, the app-container does need this user and needs to create it - not doubt.

But since the unison as the rsync container do work as a kind of "filesystem proxy", AFAIU you need the target user there, since you then mount the volume of the sync-container and that exact uid will be mapped on the target app container.

You cannot "remap" the uid on the app-container during "mounting that volume"

For the fsevent part, did you find https://github.com/codekitchen/fsevents_to_vm ?

@mickaelperrin
Copy link
Contributor Author

For what I know, fsevents_to_vm was done to forward fsevents on NFS mounted folder in the boot2docker vm.

Before your project, I was doing a kind of two-way sync with two rsync and two fsevents watcher, one on the host, the other on the container. Worked, but really heavy CPU when syncing vendor / core Drupal after bootstrapping a new project. I hope unison will do it more elegantly and with less CPU usage.

@EugenMayer
Copy link
Owner

CPU usage comes from fswatch rather then from the sync itself. I think you will gain a lot of pain with this kind of two way sync with 2 sided fswatch.

Let me state this balsy statement: While i understand every use case is different, i find that code-sharing is always the same. The requirement of 2 way sync is the extra 2% you might want to have, but it comes with a huge minus and is not worth the gain.

As describe here : #20 (comment) you can get it running with rsync very well. We are doing this for a 12k source file project ( 20k in total ) for us, also drupal.

We yet never missed any two-way sync behavior, much more, it would have been an issue with drupal:

  • sites ( user data ) are created in the middle of the codebase ( sites/mysite ) which is a code-base change synced back, while being plain user-data on the container. You simply do not want that on your host ( and work with arbitrary .gitignore entries )
  • You end up syncing back batched assets, uploaded files and all those things, in the middle of your codebase
  • your codebase can get polluted by a container script without of you wanting that at all - synced back and maybe even committed

With rsync, you use symlinks for sites and store those site-data on a name volume ( or dynamic volume ) to split it from the container.

@EugenMayer
Copy link
Owner

@mickaelperrin i removed your question from the other topic since we will create a huge unreadable thread there.. i did my own mistaked doing that, sorry.

rsync does not use --delete for now, but i worked with that already. I am yet not sure if we should add it to the defaults and make it possible to disable it ( there are some cases were you do not want this, i have such ) or keep the default as such.

I created #37 and happy to hear your input on this

@EugenMayer
Copy link
Owner

@mickaelperrin are you aware of https://github.com/EugenMayer/docker-unison ? it would be great if you could build uppon that and have image-based pull requests there, this way also https://github.com/onnimonni/docker-unison can profit from that and we join efforts on the unison approach.

Really tensed to see what you come up with, thank you for your time that you put into this!

@mickaelperrin
Copy link
Contributor Author

mickaelperrin commented Jul 13, 2016

Yes, github was down for me when I started working on this and couldn't do a proper fork, but I started my work by using what onnimonni has done.

So far, by making simple files creation / deletion by hand on both sides it works... now it's time for the composer test :p And... it pass, with most CPU usage that I would have loved to see, but far far less than what I did before with rsync. Regarding the use case, that's surely worth it.

@EugenMayer
Copy link
Owner

@mickaelperrin a pretty interesting POV here https://forums.docker.com/t/file-access-in-mounted-volumes-extremely-slow-cpu-bound/8076/130?u=eugenmayer

I though about it, created a concept: #42

This is probably the better solution, what do you think, in terms of both, performance and robustnes. Share your thoughts on this if you like.

Do not get me wrong, i do not want to defocus you, just avoiding to probably creating work which might again be replaced ( reduction of iterations ). Hope you get this as a contribution

@mickaelperrin
Copy link
Contributor Author

mickaelperrin commented Jul 13, 2016

I don't care... what I would love is that someone find a working solution and we could focus on doing things with tools not bulding tools... I would love that your idea will provide a better result than what I am currently experiencing, that would say that we should be near the good solution...

Currently I just want to provide a PR and have your opinion / tests on this approach, then depending on the result, I will think about the new approach you proposed.

@mickaelperrin
Copy link
Contributor Author

I have opened a new PR #47 that supersedes this one. Maybe we should close this one and continue the discussion in the new one ?

@EugenMayer
Copy link
Owner

Looking at the other PR, it looks far more superior, i would also tend to close this one. If we want to go back, we can still reopen this one.

@EugenMayer EugenMayer closed this Jul 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants