search

purpleidea / mgmt

Next generation distributed, event-driven, parallel config management!
https://purpleidea.com/tags/mgmtconfig/
GNU General Public License v3.0
3.6k stars 312 forks source link

Rewrite the mount resource to fix at least one existing issue #744

Open ffrank opened 7 months ago

ffrank commented 7 months ago

Let's go. It's still not perfect, but simpler and more correct.

ffrank commented 7 months ago

Fixes #525

ffrank commented 7 months ago

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.

purpleidea commented 7 months ago

Hey great to see this! I had a quick look first. Lots of nice work there. Here are the open issues to discuss:

  1. This removes fstab support-- I actually think that is still valuable to have, at least for now-- so how do we handle that? Do we have a separate resource? Do we have it be an optional for this resource? Do we have mount:systemd and mount:fstab ? Also see 2...

  2. Is the systemd variant of this resource (as shown here) similar enough to the svc resource that they should just be combined there? I think architecturally they're quite similar. I would need to investigate more.

  3. I'm not convinced I want shell tests to stay around. So we should consider a better approach if possible. This won't block your patch, but I want to mention it in case you have ideas.

Thanks Felix!

ffrank commented 7 months ago
  1. It's a big hassle in terms of required code. I will say, what was there seemed to do a good enough job. The unit tests were nicely elaborate, BUT most of them never ran (they were written as methods of MountRes...). Then again, this feature opens us up to many additional edge cases I believe.
  2. The whole point here is to generate the systemd unit first, which svc does not (and should not?) do. I was considering to move the systemd stuff to some common utility code, so that mount and svc could both rely on that. But with a very small patch to svc, it was embeddable as you had proposed originally. This seems like the right approach to me atm.
  3. That shell test is basically a placeholder. It does not run. I might go ahead and propose and prototype a better approach some time, and when I do, this test should not be forgotten. So having the placeholder is valuable from my point of view.
purpleidea commented 7 months ago

FYI as per Felix good idea we're in https://github.com/purpleidea/mgmt/issues/745 for now until we agree on how to proceed.