2008/10/2 Dejan Muhamedagic <[EMAIL PROTECTED]>: I attached a fixed version for your further comments. And some are replied below Thanks for your time to do the review ;)
> Relative to where? Why and under which circumstances would one
> want to modify this path? Why is it required?
I guess these 2 are about my poor explanation.
This RA needs a place to store information which can be access to the
node actually start the service.
For example, if /dev/sdb is/can be mounted in /share on every node,
and /share is exported via nfs, then we set nfs_share_path to
/share.
>> +<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?
Yes, it is distro/platform dependent. But I intent to code it in RA
instead of making it a parameter, because it is what developer should
know while administrator may not. And the same applies to things like
sm-notify.
I'm looking forward to feedbacks from other distros then openSUSE, so
the RA can be improved to support those distros.
> 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.
>
Point take.
>> +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).
Point take.
>> +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.
Thanks and fixed.
>> + [ -d "$fp" ] || mkdir -p fp
>
> Missing "$".
Thanks for the catch.
>> + [ -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.
Add lines to say so ;)
>> + prepare_direcotry
>
> A typo.
Fixed.
>> +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_*)?
Fixed.
> Thanks,
>
> Dejan
>
> _______________________________________________________
> Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> Home Page: http://linux-ha.org/
>
nfsserver
Description: Binary data
_______________________________________________________ Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev Home Page: http://linux-ha.org/
