isilon: add Supported() to executor #297
Conversation
Current coverage is 25.01% (diff: 0.00%)@@ master #297 diff @@
==========================================
Files 46 46
Lines 2708 2710 +2
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 678 678
- Misses 1960 1962 +2
Partials 70 70
|
| // resides. | ||
| func Supported(ctx types.Context, opts types.Store) (bool, error) { | ||
| // make sure mount.nfs is present | ||
| if err := exec.Command( |
There was a problem hiding this comment.
This assumes that both the which and mount.nfs commands are in the PATH variable as provided to the executor. Is there perhaps a cleaner way to do this?
There was a problem hiding this comment.
It is definitely assuming that. My (flawed?) reasoning for why this is okay is that libstorage is already making such assumptions for NFS: https://github.com/emccode/libstorage/blob/master/drivers/os/linux/linux.go#L185
Another tactic would be to survey our supported OS's, and see where the binary would be located on each of them, and then just stat for the file in all of the locations, returning true if any of them were successful.
There was a problem hiding this comment.
Your logic works for me :) You could also use this.
There was a problem hiding this comment.
Oh, nice! LookPath() is very convenient. It's which in Go. :)
I'm happy to change it. Do you prefer me to use LookPath() directly, or to import gotil ?
There was a problem hiding this comment.
Feel free to use LookPath() directly. I only abstract it in order to ignore the error and make the code a tad cleaner, but no reason for you to import this here just for this. However, if you build the transitive dependency graph for the executor binary you'll see it's already imported for another dependency on which the executor relies, so why not I suppose? Your call.
codenrhoden
force-pushed
the
feature/isilon_exec_supported
branch
from
56f7be1
to
8da1a0a
Compare
Isilon clients NFS mount the volumes, so make sure that mount.nfs is present.
codenrhoden
force-pushed
the
feature/isilon_exec_supported
branch
from
8da1a0a
to
6929b0c
Compare
Isilon clients NFS mount the volumes, so make sure that mount.nfs
is present.
FWIW, I was hesitant to implement this way, as it is Linux specific. At first I was just going to check if
/usr/sbin/mount.nfsexisted, but that could be distro specific. But the executor already parses files in/proc, which binds it to Linux. So I'm not adding any new restrictions.Also, it seemed possible that a client could by using NFSv4, and if they were they would need
mount.nfs4, but the Linux driver actually doesn't usemount -t nfsormount -t nfs4, it just callsmount, and can't be overriden, so this is also not a current issue. It merely relies on the presence of a:in the device to infer it is NFS.This was inspired by #296