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

Clarify some issues regarding bind-dirs #299

Merged
merged 1 commit into from
Mar 21, 2017
Merged

Conversation

unman
Copy link
Member

@unman unman commented Mar 3, 2017

@andrewdavidwong andrewdavidwong requested a review from marmarek March 3, 2017 23:34
Copy link
Member

@marmarek marmarek left a comment

Choose a reason for hiding this comment

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

The only thing really needed to change is typo at the beginning. Others - let me know what do you think about it.

With [bind-dirs.sh](https://github.com/QubesOS/qubes-core-agent-linux/blob/master/vm-systemd/bind-dirs.sh)
you can make arbitrary files or folders persistent in TemplateBasedVMs.
With [bind-dirs](https://github.com/QubesOS/qubes-core-agent-linux/blob/master/vm-systemd/bind-dirs.sh)
any arbitrary files or folderscan be made persistent in TemplateBasedVMs.
Copy link
Member

Choose a reason for hiding this comment

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

missing space


Creation of the file and folders in /rw/bind-dirs should be automatic the first time the TemplateBasedVM is restarted after configuration.

If you want to circumvent this process, you can create the relevant filestructure under /rw/bind-dirs and make any changes at the same time that you perform the configuration, before reboot.
Copy link
Member

Choose a reason for hiding this comment

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

I think much easier option is to edit configuration and then launch bind-dirs script manually to create appropriate directories structure. It's easier to do and harder to make a typo. But see limitations below.
Do you think it worth mentioning this simpler method, maybe with a reference to a limitation below + a ticket (AFAIK there is none for it yet)?

Copy link
Member

Choose a reason for hiding this comment

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

Would QubesOS/qubes-issues#2673 suffice as the ticket you'd reference there or did you have something different in mind?

Copy link
Contributor

@jpouellet jpouellet Mar 15, 2017

Choose a reason for hiding this comment

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

Just here to say that noting that the persisted data lives in /rw/bind-dirs is a useful thing to document, regardless of whether bind-dirs.sh is idempotent. +1 for this PR

* Re-running `sudo /usr/lib/qubes/bind-dirs.sh` without previous `sudo /usr/lib/qubes/bind-dirs.sh umount` does not work.
* Files that exist in the TemplateVM root image cannot be deleted in the TemplateBasedVMs root image using bind-dirs.sh.
* The file / folder in question must already exist in the root image. I.e. a file that does not exist in the root image cannot be bind mounted in the TemplateBasedVM.
* Re-running `sudo /usr/lib/qubes/bind-dirs.sh` without a previous `sudo /usr/lib/qubes/bind-dirs.sh umount` does not work.
Copy link
Member

Choose a reason for hiding this comment

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

Actually, if you created bind-dirs configuration for the first time (it was empty during VM startup), it works fine. But in other cases, it will indeed result in some weird situation (multiple mounts in the same place etc).

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps bind-dirs should figure out the state etc.. If already mounted, avoid duplicate mount etc.

(And if it is not possible to figure out the state - keep a state file with the state.)

Copy link
Member

Choose a reason for hiding this comment

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

There is mountpoint tool which can check if a directory or file is a mountpoint. That would be a first step. The second one (less important here): checking if mounted thing is a file/directory from /rw/bind-dirs.

Copy link
Member

Choose a reason for hiding this comment

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

Created QubesOS/qubes-issues#2673 for it. (Feel free to edit any of my tickets in case I misquoted.)

* Running 'sudo /usr/lib/qubes/bind-dirs.sh umount' after boot (before shutdown) is probably not sane and nothing can be done about that.
* Many editors create a temporary file and copy it over the original file. If you have bind mounted an individual file this will break the mount.
Copy link
Member

Choose a reason for hiding this comment

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

In many cases it's easy to workaround: configure the whole directory for bind-dirs. Of course it isn't feasible if said directory would be the whole /etc or such...

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, bind-dirs would be as robust as supporting even /. :)

Copy link
Contributor

@ypid ypid Mar 16, 2017

Choose a reason for hiding this comment

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

copy it over the original file. If you have bind mounted an individual file this will break the mount.

This is not the actual limitation as I observed it (with /etc/default/dnsmasq being a bind mounted file by bind-dirs):

root@s-cm:/etc/default# cp dnsmasq dnsmasq-copy
root@s-cm:/etc/default# cp dnsmasq-copy dnsmasq
root@s-cm:/etc/default# strace -e trace=file mv dnsmasq-copy dnsmasq
[...]
rename("dnsmasq-copy", "dnsmasq")       = -1 EBUSY (Device or resource busy)
mv: cannot move ‘dnsmasq-copy’ to ‘dnsmasq’
open("/usr/share/locale/en_US.UTF-8/LC_MESSAGES/libc.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
: Device or resource busy
+++ exited with 1 +++

root@s-cm:/etc/default# strace -e trace=file rm dnsmasq
[...]
unlinkat(AT_FDCWD, "dnsmasq", 0)        = -1 EBUSY (Device or resource busy)
rm: cannot remove ‘dnsmasq’
open("/usr/share/locale/en_US.UTF-8/LC_MESSAGES/libc.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
: Device or resource busy
+++ exited with 1 +++

Moving/recreating is also a thing that config management systems like to do after templating a file (and validating it) which is also effected by this:

ansible-playbook -l s-cm ansible/ypid-playbooks/service/dnsmasq-qubes_os.yml
[...]
TASK [debops.dnsmasq : Configure defaults for dnsmasq service] *****************
fatal: [s-cm]: FAILED! => {"changed": true, "failed": true, "msg": "Could not replace file: /home/user/.ansible/tmp/ansible-tmp-1489657252.52-223355023089868/source to /etc/default/dnsmasq:
[Errno 16] Device or resource busy"}

Maybe generalize the point to: Deleting/renaming of bind mounted files/directories does not work. Programs might do this internally when updating a file by first creating a new file and then doing a rename or unlinkat. Examples include editors and configuration management systems. A workaround might be to bind mount a parent directory instead or do the file operations in the rw location (/rw/bind-dirs/) (the last option will require a unmount/remount for the changes to become visible).

Copy link
Member

@adrelanos adrelanos left a comment

Choose a reason for hiding this comment

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

I am okay with these changes. Mergeable.

@adrelanos
Copy link
Member

If you want to circumvent this process, you can create the relevant filestructure under /rw/bind-dirs

With any examples this is still difficult to figure out.

Anyhow. These changes are okay and I'd appreciate if merged. I am a fan of many small iterations with improvements over time, wiki style. We could still add this on top later.

@andrewdavidwong
Copy link
Member

@unman or @adrelanos: Please resolve the nontrivial conflicts.

@andrewdavidwong andrewdavidwong self-requested a review March 6, 2017 01:24
Copy link
Member

@andrewdavidwong andrewdavidwong left a comment

Choose a reason for hiding this comment

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

Please resolve any nontrivial conflicts.

* Running 'sudo /usr/lib/qubes/bind-dirs.sh umount' after boot (before shutdown) is probably not sane and nothing can be done about that.
* Many editors create a temporary file and copy it over the original file. If you have bind mounted an individual file this will break the mount.
Copy link
Contributor

@ypid ypid Mar 16, 2017

Choose a reason for hiding this comment

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

copy it over the original file. If you have bind mounted an individual file this will break the mount.

This is not the actual limitation as I observed it (with /etc/default/dnsmasq being a bind mounted file by bind-dirs):

root@s-cm:/etc/default# cp dnsmasq dnsmasq-copy
root@s-cm:/etc/default# cp dnsmasq-copy dnsmasq
root@s-cm:/etc/default# strace -e trace=file mv dnsmasq-copy dnsmasq
[...]
rename("dnsmasq-copy", "dnsmasq")       = -1 EBUSY (Device or resource busy)
mv: cannot move ‘dnsmasq-copy’ to ‘dnsmasq’
open("/usr/share/locale/en_US.UTF-8/LC_MESSAGES/libc.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
: Device or resource busy
+++ exited with 1 +++

root@s-cm:/etc/default# strace -e trace=file rm dnsmasq
[...]
unlinkat(AT_FDCWD, "dnsmasq", 0)        = -1 EBUSY (Device or resource busy)
rm: cannot remove ‘dnsmasq’
open("/usr/share/locale/en_US.UTF-8/LC_MESSAGES/libc.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
: Device or resource busy
+++ exited with 1 +++

Moving/recreating is also a thing that config management systems like to do after templating a file (and validating it) which is also effected by this:

ansible-playbook -l s-cm ansible/ypid-playbooks/service/dnsmasq-qubes_os.yml
[...]
TASK [debops.dnsmasq : Configure defaults for dnsmasq service] *****************
fatal: [s-cm]: FAILED! => {"changed": true, "failed": true, "msg": "Could not replace file: /home/user/.ansible/tmp/ansible-tmp-1489657252.52-223355023089868/source to /etc/default/dnsmasq:
[Errno 16] Device or resource busy"}

Maybe generalize the point to: Deleting/renaming of bind mounted files/directories does not work. Programs might do this internally when updating a file by first creating a new file and then doing a rename or unlinkat. Examples include editors and configuration management systems. A workaround might be to bind mount a parent directory instead or do the file operations in the rw location (/rw/bind-dirs/) (the last option will require a unmount/remount for the changes to become visible).

unman added a commit to unman/qubes-doc that referenced this pull request Mar 18, 2017
* Running 'sudo /usr/lib/qubes/bind-dirs.sh umount' after boot (before shutdown) is probably not sane and nothing can be done about that.
* Many editors create a temporary file and copy it over the original file. If you have bind mounted an individual file this will break the mount.
Any changes you make will not survive a reboot. If you think it likely you will want to edit a file, then either include the parent directory in bind-dirs.rather than the file, or perform the file operation on the file in /rw/bind-dirs.
* Some files are altered when a qube boots - e.g. /etc/hosts. If you try to use bind-dirs on such files you may break your qube in unpredictable ways.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change "bind-dirs.rather than the file, or perform the file operation on the file in /rw/bind-dirs." to "bind-dirs rather than the file, or perform the file operation on the file in /rw/bind-dirs which will require a unmount/remount for the changes to become visible)."

Copy link
Member Author

Choose a reason for hiding this comment

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

@ypid I take your point, but since we have highlighted the fact that rerunning the script without umount doesnt work, and umount isn't sane, it's not appropriate at the moment. Many users will be able to use this without knowing how to umount/remount an individual file - that's what this script is for. (Note that the situation may change.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining.

@unman
Copy link
Member Author

unman commented Mar 19, 2017

@andrewdavidwong I think you should merge it now. (Patrick is happy as you can see).
we can deal with any further issues or changes as they arise.

@andrewdavidwong
Copy link
Member

@unman: It looks like there are still nontrivial conflicts that I need you to resolve first.

@unman unman reopened this Mar 20, 2017
@andrewdavidwong andrewdavidwong merged commit 56863c2 into QubesOS:master Mar 21, 2017
@andrewdavidwong
Copy link
Member

Thanks, @unman and all!

@unman unman deleted the 2661 branch February 7, 2024 13:31
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.

6 participants