ghettoVCB icon indicating copy to clipboard operation
ghettoVCB copied to clipboard

NfsIoHack hides errors

Open jokjr opened this issue 3 years ago • 2 comments

We have some cases where the NfsIoHack hides errors, consider this case:

https://github.com/lamw/ghettoVCB/blob/master/ghettoVCB.sh#L643-L652

If the rm fails, we reach

if [[ $? -ne 0 ]]  &&

this overrides $?. Then it checks

[[ "${ENABLE_NFS_IO_HACK}" -eq 1 ]]

If this is 0, the check will fail, but fi without else always returns 0, so

if [[ $? -eq 0 ]]; then

Will always be true.

We could consider something like

                rm -f "${BACKUP_DIR_PATH}/${VM_TO_SEARCH_FOR}-$i.gz"
                rm_status=$?
                # Added the NFS_IO_HACK check and function call here.  Some NAS devices slow at this step.
                if [[ ${rm_status} -ne 0 ]]  && [[ "${ENABLE_NFS_IO_HACK}" -eq 1 ]]; then
                    NfsIoHack
                fi
                if [[ ${rm_status} -eq 0 ]]; then
                    logger "info" "Deleted ${BACKUP_DIR_PATH}/${VM_TO_SEARCH_FOR}-$i.gz"
                else
                    logger "info" "Failure deleting ${BACKUP_DIR_PATH}/${VM_TO_SEARCH_FOR}-$i.gz"
                fi

(I'm not sure what NfsIoHack is supposed to do; will that retry the rm?)

jokjr avatar Mar 15 '21 10:03 jokjr

Agreed, you can't just use $? twice like that. Good catch. But why not write it totally as it's supposed to be then.

rm_status=$?
if [[ ${rm_status} -eq 0 ]]; then
  logger "info" "Deleted ${BACKUP_DIR_PATH}/${VM_TO_SEARCH_FOR}-$i.gz"
else
  if [[ "${ENABLE_NFS_IO_HACK}" -eq 1 ]]; then  
    NfsIoHack
  else
    logger "info" "Failure deleting ${BACKUP_DIR_PATH}/${VM_TO_SEARCH_FOR}-$i.gz"
  fi
fi

TheNetworkIsDown avatar Mar 15 '21 10:03 TheNetworkIsDown

I just noticed in passing, I have not experienced this as a problem and I'm happy with your suggestion. :-) Does the call to NfsIoHack mean that it will be deleted? At a quick glance it looks like NfsIoHack just tries to ensure liveness, but I would have expected a retry of the rm again after?

jokjr avatar Mar 15 '21 11:03 jokjr