Rewrite the mount resource to fix at least one existing issue#744
Rewrite the mount resource to fix at least one existing issue#744ffrank wants to merge 2 commits intopurpleidea:masterfrom
Conversation
The mount approach was flawed. It relied on editing fstab, then restarting the systemd service that is responsible for mounting all defined filesystems. Among other issues, this approach made it impossible for mgmt to unmount managed filesystems, because after removing them from fstab, they would just be ignored by systemd. The new approach is based on creating a systemd unit for each managed mount, through a unit file in /etc/systemd/system. These files are managed by wrapped File resources now. Then a wrapped Svc resource manages the lifecycle of the mount. This works even after removing the unit definition from systemd, because as long as the filesystem is still mounted, systemd will be able to derive a transient unit that can be managed. This also simplifies the logic of the MountRes a lot, because we don't have to be so careful anymore about matching existing fstab entries to what is expected by a given Mount resource. We just overwrite the unit definition with whatever is specified, and rely on systemd to handle the details. Systemd will even create mount points if they don't yet exist. Currently this approach is not yet equipped to handle the case that a mount could be defined via fstab already, or through a unit file that is in a different location than /etc/systemd/system. As such, this second iteration should be considered as a partial (if more rounded than the first one) solution. Fixes purpleidea#525
As the --converged-timeout does not work anymore, we cannot use this current approach to test the mount resource. Simple resources like file can sync in time before the main loop terminates, but MountRes cannot. Therefor this test script is skipping itself for now. We should replace all of these tests with a more sophisticated approach. Still, while we have them, we can reduce some copy/paste work by adding another util function for checking sudo.
|
Fixes #525 |
|
The wrapping of resources is based on how NspawnRes does it. But I wonder: It would feel much better to just run Watch of both the wrapped file and svc, and in the mount Watch, only wait for the svc to finish, and then also finish. |
|
Hey great to see this! I had a quick look first. Lots of nice work there. Here are the open issues to discuss:
Thanks Felix! |
|
|
|
||
| obj.init.Running() // when started, notify engine that we're running | ||
|
|
||
| // // watch the unit file |
There was a problem hiding this comment.
This should probably be uncommented, but I want better end-to-end testing facilities first. These wrapped resources make life a little bit complicated.
| // restarting `local-fs.target` and `remote-fs.target` units. | ||
| if err := mountReload(ctx); err != nil { | ||
| return false, errwrap.Wrapf(err, "error reloading /etc/fstab") | ||
| if !checkOK { |
There was a problem hiding this comment.
ah shoot this needs an && apply
|
FYI as per Felix good idea we're in #745 for now until we agree on how to proceed. |
purpleidea
force-pushed
the
master
branch
3 times, most recently
from
b8072b2 to
380004b
Compare
purpleidea
force-pushed
the
master
branch
2 times, most recently
from
37fdda9 to
5f4ae05
Compare
purpleidea
force-pushed
the
master
branch
2 times, most recently
from
3221a93 to
4ad2b9a
Compare
2 participants
Let's go. It's still not perfect, but simpler and more correct.