sous-chefs / nfs

Development repository for the nfs cookbook
https://supermarket.chef.io/cookbooks/nfs
Apache License 2.0
40 stars 88 forks source link

nfs_export LWRP not truly idempotent - creates duplicate entries if things change #48

Open StFS opened 9 years ago

StFS commented 9 years ago

If I run a recipe that creates an export for /mnt/foo everything works. However, if I then change something about that export (for instance, add or remove an option), re-running the recipe will create a second entry in /etc/exports and the run will fail since the NFS server will complain that it can't handle duplicate entries.

I would think that the LWRP should look for lines based on the exported directory and update the line if it exists already.

atomic-penguin commented 9 years ago

The LWRP is good about inserting things, but there is no mechanism for removing things. Its an append-only operation as it stands now.

By changing something about the composed line after its been inserted, then you are effectively creating a different share as far as the LWRP is concerned. As far as exportfs is concerned, its really not too picky about the uniqueness of a share. There is no magic primary key in an export tab to key off of for replacement.

I am not opposed to changing it to a replacement model. But, it has potential to be a breaking change.

StFS commented 9 years ago

Okay... well as it stands now, if you add a duplicate share (that is, you export the same directory twice) the NFS service doesn't start at all. It fails with an error. I don't know enough about NFS to say for sure that this is always the way it is or whether there is any way to export the same directory twice... but I'd think that having the LWRP behave in such a way that it at least tries to makes sure that the exports file is valid and that the service will start after modifications would be desirable.

atomic-penguin commented 9 years ago

I am able to start nfs-kernel-server just fine with the same share exported to two distinct networks, or two sets of distinct options. I can repro the problem if a duplicate shares have the same CIDR ACL. A duplicate share exported to a different CIDR ACL is still a valid declaration in exports.

# This is valid, because the share is not a unique primary key and has nothing to do with why its failing.
/mnt/foo 127.0.0.0/8(ro,sync,no_root_squash)
/mnt/foo 127.0.0.2(rw,sync,no_root_squash)
# So is this.
/mnt/foo 127.0.0.3(ro,sync,no_root_squash) 127.0.0.4(rw,sync,no_root_squash)
#  It can get even more complicated if there is a large set of CIDRs which each need to be checked for uniqueness.
/mnt/foo 127.0.0.5(ro,sync,no_root_squash) 127.0.0.0/8(rw,sync,no_root_squash) 127.0.0.6(ro,sync,no_root_squash)
# This fails because the share, in combination with the CIDR, is a unique primary key.
/mnt/foo 127.0.0.0/8(ro,sync,no_root_squash)
/mnt/foo 127.0.0.0/8(rw,sync,no_root_squash)

Let me reiterate, that I am not opposed to changing the behavior.

StFS commented 9 years ago

well... I agree that since exporting the same directory twice is legal then my suggestion of using that as a primary key of sorts is silly. Using a combination of the directory and network might be an option but it would probably be a bit complicated since the network can be specified in a number of different ways.

I don't suppose it would be possible to store the "before modifications" version of the /etc/exports file and then see if a restart of the NFS service actually works... if it doesn't then reverting back to the old version, starting the service and failing the chef run? That way, a converge at least wouldn't bring down a live NFS server.

atomic-penguin commented 9 years ago

That is exactly the point I was trying to make. I acknowledge that it is a problem. But its not a simple fix that I can just whip out in a few minutes.

I am leaning towards, the replace_or_add resource here. Which could be implemented so that the last thing declared that matches the pattern of share+CIDR wins.

glenjamin commented 9 years ago

What about adding a "name" field to the LWRP, and including this as a comment on the end of the line?

This could default to path+type, but allows users to override if needed.

yoshiwaan commented 9 years ago

I agree that a resource name is the best way to identify an entry in this case (via a comment), as the use case scenario where this crops up is most likely that a user is modifying an existing share and is thus editing the resource in Chef code.

nkadel-skyhook commented 8 years ago

What about providing a template to simply deploy a managed /etc/exports, and not using an LWRP? I'm sure some people would want to continue using the LWRP for tuning local configurations with individual entries. LWRP's have their uses for such configuratons, but managing /etc/exports could avoid overlaps and double entries by working from a template, topped with a "# Chef managed" warning. It would also allow a much more graceful activation via roles, and attributes, without every nfs cookbook user having to write their own wrapper cookbook.

That approach could also provide a much more effective rollback mechanism, if needed. I've worked with just such rollbacks for /etc/sudoers and /etc/named changes, to avoid chef deployments taking out critical managed services due to mismatched merges. If there's any buy-in, I'd be willing to try a pull request for this.

nickkeyzer commented 7 years ago

managing /etc/exports could avoid overlaps and double entries by working from a template, topped with a "# Chef managed" warning. It would also allow a much more graceful activation via roles, and attributes, without every nfs cookbook user having to write their own wrapper cookbook.

This is a perfect example. I'm working with this exact scenario now. My NFS server exports to different networks, each with unique options, depending on the Environment of the node. This is very tricky to do with the LWRP whereas a template file would be much easier to manage via node attributes.