Christian, I'm generally fine with most of the changes, thanks very much for your continued contribution. I have a few comments on your patch, though.
> # Check for mandatory files presence and consistency
> vmware_validate() {
> if [ -z "`pidof vmware-hostd`" ]; then
> - ocf_log warn "vmware-hostd is not running"
> - return $OCF_ERR_GENERIC
> + ocf_log err "vmware-hostd is not running: aborting"
> + exit $OCF_ERR_GENERIC
> fi
Changing warn to err and return to exit is obviously fine, but you may
actually want to make this $OCF_ERR_INSTALLED.
> @@ -253,12 +260,20 @@
> else
> ocf_log info "Virtual machine $VM is already stopped"
> fi
> + # VMware randomly fails to unregister VMs,
> + # so we send the command twice
> ocf_log info "Unregistering virtual machine $VM"
> vmware_unregister_vm $VMID
> VMID=`vmware_get_vid "$RELVMXPATH"`
> if [ -n "$VMID" ]; then
> - ocf_log err "Could not unregister virtual machine $VM: aborting."
> - exit $OCF_ERR_GENERIC
> + ocf_log warn "Could not unregister virtual machine $VM: retrying."
> + sleep 10
> + vmware_unregister_vm $VMID
> + VMID=`vmware_get_vid "$RELVMXPATH"`
> + if [ -n "$VMID" ]; then
> + ocf_log err "Could not unregister virtual machine $VM: aborting."
> + exit $OCF_ERR_GENERIC
> + fi
> fi
> ocf_log info "Virtual machine $VM is stopped"
> return $OCF_SUCCESS
If you are running into VMware "randomly unregistering" VMs, can you be
confident that it'll work on the second try? Wouldn't it be smarter to
just keep trying until you are timed out?
> @@ -278,11 +293,11 @@
>
> # Print metadata informations
> meta_data() {
> - cat <<END
> + cat <<END
> <?xml version="1.0"?>
> <!DOCTYPE resource-agent SYSTEM "ra-api-1.dtd">
> -<resource-agent name="vmware">
> -<version>0.1</version>
> +<resource-agent name="VMwareVM">
> +<version>0.2</version>
> <longdesc lang="en">
> OCF compliant script to control vmware server 2.0 virtual machines.
> </longdesc>
Sorry, we can't do that. The resource agent has already been released
under the name "vmware", so people are probably using this is
production. Renaming the RA now would break their setups. Of course this
can be worked around by adding a symlink, but I really don't see the point.
> @@ -301,15 +316,15 @@
> <longdesc lang="en">
> vmware-vim-cmd executable path
> </longdesc>
> -<shortdesc lang="en">vmware-vimsh path</shortdesc>
> +<shortdesc lang="en">vmware-vim-cmd path</shortdesc>
> <content type="string" default="/usr/bin/vmware-vim-cmd"/>
> </parameter>
> </parameters>
>
> <actions>
> -<action name="start" timeout="300" />
> -<action name="stop" timeout="300" />
> -<action name="monitor" timeout="30" interval="300" depth="0" />
> +<action name="start" timeout="900" />
> +<action name="stop" timeout="900" />
> +<action name="monitor" timeout="30" interval="300" depth="0"
> start-delay="60" />
> <action name="meta-data" timeout="5" />
> </actions>
> </resource-agent>
Those timeouts look really excessive. Are you sure you want to declare
_15 minutes_ the recommended minimum?
And, please kill start-delay. People can configure that if they really
want to, but the RA really shouldn't advertise this.
Cheers,
Florian
signature.asc
Description: OpenPGP digital signature
_______________________________________________________ Linux-HA-Dev: [email protected] http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev Home Page: http://linux-ha.org/
