IBM / ubiquity

Ubiquity
Apache License 2.0
90 stars 26 forks source link

UB-1407: added timeouts to umount operations #237

Closed olgashtivelman closed 6 years ago

olgashtivelman commented 6 years ago

added timeout to some commands as part of the unmount process as follows:

unmount (timeout 30s) clean up command : dmsetup and multipath -f (each should be with timeout of 30s) multipath -ll (timeout 20s) multipath -r (timeout 60s) rescan -r (timeout 2 minutes) iscsiadm rescan (timeout 1 minutes)


This change is Reviewable

coveralls commented 6 years ago

Coverage Status

Coverage increased (+3.8%) to 51.961% when pulling f7fadb90f9bd56639c1c62dbab2644e97d0f0cfe on fix/UB-1407_add_timeout_to_commands into 382fc6e1cff9874a86702fc265ff97ac9a4388d8 on dev.

beckmani commented 6 years ago

remote/mounter/block_device_utils/fs.go, line 95 at r1 (raw file):


  args := []string{mpoint}
  if _, err := b.exec.ExecuteWithTimeout(30*1000, umountCmd, args); err != nil {

Please define as const (umountTimeout) - see MultipathTimeout in mpath.go

beckmani commented 6 years ago

remote/mounter/block_device_utils/mpath.go, line 30 at r1 (raw file):


const multipathCmd = "multipath"
const MultipathTimeout = 60*1000

Nice :-)

beckmani commented 6 years ago

remote/mounter/block_device_utils/mpath.go, line 60 at r1 (raw file):

  }
  args := []string{"-ll"}
  outputBytes, err := b.exec.ExecuteWithTimeout(20*1000, multipathCmd, args)

Should use MultipathTimeout

beckmani commented 6 years ago

remote/mounter/block_device_utils/mpath.go, line 234 at r1 (raw file):

func (b *blockDeviceUtils) Cleanup(mpath string) error {
  defer b.logger.Trace(logs.DEBUG)()
  cleanupTimeout := 30*1000

Should be defined as const

beckmani commented 6 years ago

remote/mounter/block_device_utils/rescan.go, line 44 at r1 (raw file):

  }
  args := []string{"-m", "session", "--rescan"}
  if _, err := b.exec.ExecuteWithTimeout(1*60*1000, rescanCmd, args); err != nil {

Should be defined as const

shay-berman commented 6 years ago

@olgashtivelman finished to review - please apply few comments and then it LGTM