Hi Xinwei,
A few questions below:
On Sun, Sep 28, 2008 at 10:28:59PM +0800, Xinwei Hu wrote:
> Hi all,
>
> I send an implementation of nfsserver to the user list a while ago.
> As there's no interests show around, I resend it here as a patch.
>
> Please consider it for upstream. Thanks.
> +<parameter name="nfs_shared_path" unique="0" required="1">
> +<longdesc lang="en">
> +The directory which physically shared among all servers. Typically, it's a
> filesystem on a shared storage.
I don't entirely understand. I guess that on a cluster running
this RA there's going to be some kind of shared storage. What
does "physically shared" mean? How does "filesystem" relate to a
"directory": are you saying that there is "typically a
filesystem" used only for this purpose?
What's the purpose of this location? I guess one could figure
that out from the code, but this is user documentation.
> +<parameter name="nfs_info_path" unique="0" required="1">
> +<longdesc lang="en">
> +The relative directory where to store nfs related information. It defaults
> to .nfsserver.
Relative to where? Why and under which circumstances would one
want to modify this path? Why is it required?
> +<parameter name="sharedip" unique="0" required="1">
> +<longdesc lang="en">
> +The IP address to access the nfs service
> +nfsserver_monitor ()
> +{
> + /etc/init.d/nfsserver status
a) How do you know that all distributions/platforms have
/etc/init.d/nfsserver? On one Debian box I find
/etc/init.d/nfs-kernel-server. There could be more. Shouldn't
this be a parameter (attribute) as well?
b) This seems to be very verbose
# /etc/init.d/nfsserver status
Checking for kernel based NFS server: idmapd unused
mountd unused
statd unused
nfsd unused
and it is going to be logged whenever monitor is invoked.
> +nfsserver_daemon_start ()
> +{
> + ocf_log info "Starting NFS Server via init script"
> + /etc/init.d/nfsserver start
> + if [ $? -ne 0 ]; then
> + ocf_log err "Failed to start NFS Server via init script"
> + return 1
Information is lost here. The nfs server init script should be a
LSB compliant script, right? Then, the exit codes should be
translated to the OCF codes (I guess that for the most part they
are equal).
> +prepare_directory ()
> +{
> + declare fp="$OCF_RESKEY_nfs_shared_path/$OCF_RESKEY_nfs_info_path"
Drop "declare". Or do you need bash for this script? If so, which
I doubt, then change #!/bin/sh to #!/bin/bash.
> + [ -d "$fp" ] || mkdir -p fp
Missing "$".
> + [ -d "$fp/rpc_pipefs" ] || mkdir -p $fp/rpc_pipefs
> + [ -d "$fp/sm" ] || mkdir -p $fp/sm
> + [ -d "$fp/sm.ha" ] || mkdir -p $fp/sm.ha
> + [ -d "$fp/sm.bak" ] || mkdir -p $fp/sm.bak
> + [ -d "$fp/v4recovery" ] || mkdir -p $fp/v4recovery
> +}
> +
> +is_bound ()
> +{
> + mount | grep -q "$1 on $2 type none (.*bind)"
...
> + mount --bind $fp /var/lib/nfs
This is Linux specific. Is this RA supposed to run only on Linux?
If so, please document that and explain why.
> + prepare_direcotry
A typo.
> +nfsserver_validate ()
> +{
> + if [ -x /etc/init.d/nfsserver && -x /usr/sbin/sm-notify ]; then
> + return $OCF_SUCCESS
> + else
> + return $OCF_ERR_INSTALLED
> + fi
Shouldn't you also check the attributes (OCF_RESKEY_*)?
Thanks,
Dejan
_______________________________________________________
Linux-HA-Dev: [email protected]
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/
