Skip to content

Commit

Permalink
chore: refactor Mount() for clarity
Browse files Browse the repository at this point in the history 
It looks like the Mount() method onced used goroutines, and while this
was removed, the complexity was not. This change does not change any
behavior, but removes overly-complex structure.
  • Loading branch information
@blgm
blgm committed Jul 6, 2023
1 parent 39b2964 commit 5f8b98a
Showing 1 changed file with 46 additions and 80 deletions.
126 changes: 46 additions & 80 deletions volume_driver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"fmt"
"os"
"path/filepath"
"sync"
"time"

"code.cloudfoundry.org/dockerdriver"
Expand All @@ -23,7 +22,6 @@ import (

type NfsVolumeInfo struct {
Opts map[string]interface{} `json:"-"` // don't store opts
wg sync.WaitGroup
mountError string
dockerdriver.VolumeInfo // see dockerdriver.resources.go
}
Expand Down Expand Up @@ -134,103 +132,61 @@ func (d *VolumeDriver) Mount(env dockerdriver.Env, mountRequest dockerdriver.Mou
return dockerdriver.MountResponse{Err: "Missing mandatory 'volume_name'"}
}

var doMount bool
var opts map[string]interface{}
var mountPath string
var wg *sync.WaitGroup

ret := func() dockerdriver.MountResponse {

volume, ok := d.volumes.Get(mountRequest.Name)
if !ok {
return dockerdriver.MountResponse{Err: fmt.Sprintf("Volume '%s' must be created before being mounted", mountRequest.Name)}
}

mountPath = d.mountPath(driverhttp.EnvWithLogger(logger, env), volume.Name)

logger.Info("mounting-volume", lager.Data{"id": volume.Name, "mountpoint": mountPath})
logger.Info("mount-source", lager.Data{"source": volume.Opts["source"].(string)})

if volume.MountCount < 1 {
doMount = true
volume.wg.Add(1)
opts = map[string]interface{}{}
for k, v := range volume.Opts {
opts[k] = v
}
}

volume.Mountpoint = mountPath
volume.MountCount++

logger.Info("volume-ref-count-incremented", lager.Data{"name": volume.Name, "count": volume.MountCount})
volume, ok := d.volumes.Get(mountRequest.Name)
if !ok {
return dockerdriver.MountResponse{Err: fmt.Sprintf("Volume '%s' must be created before being mounted", mountRequest.Name)}
}

if err := d.persistState(driverhttp.EnvWithLogger(logger, env)); err != nil {
logger.Error("persist-state-failed", err)
return dockerdriver.MountResponse{Err: fmt.Sprintf("persist state failed when mounting: %s", err.Error())}
}
mountPath := d.mountPath(driverhttp.EnvWithLogger(logger, env), volume.Name)
volume.Mountpoint = mountPath
logger.Info("mounting-volume", lager.Data{"id": volume.Name, "mountpoint": mountPath})
logger.Info("mount-source", lager.Data{"source": volume.Opts["source"].(string)})

wg = &volume.wg
return dockerdriver.MountResponse{Mountpoint: volume.Mountpoint}
}()
doMount := volume.MountCount < 1
volume.MountCount++
logger.Info("volume-ref-count-incremented", lager.Data{"name": volume.Name, "count": volume.MountCount})

if ret.Err != "" {
return ret
if err := d.persistState(driverhttp.EnvWithLogger(logger, env)); err != nil {
logger.Error("persist-state-failed", err)
return dockerdriver.MountResponse{Err: fmt.Sprintf("persist state failed when mounting: %s", err.Error())}
}

if doMount {
mountStartTime := d.time.Now()

err := d.mount(driverhttp.EnvWithLogger(logger, env), opts, mountPath)
err := d.mount(driverhttp.EnvWithLogger(logger, env), copyOpts(volume.Opts), mountPath)

mountEndTime := d.time.Now()
mountDuration := mountEndTime.Sub(mountStartTime)
if mountDuration > 8*time.Second {
logger.Error("mount-duration-too-high", nil, lager.Data{"mount-duration-in-second": mountDuration / time.Second, "warning": "This may result in container creation failure!"})
}

func() {
volume, ok := d.volumes.Get(mountRequest.Name)
if !ok {
ret = dockerdriver.MountResponse{Err: fmt.Sprintf("Volume '%s' not found", mountRequest.Name)}
} else if err != nil {
if _, ok := err.(dockerdriver.SafeError); ok {
errBytes, m_err := json.Marshal(err)
if m_err != nil {
logger.Error("failed-to-marshal-safeerror", m_err)
volume.mountError = err.Error()
}
volume.mountError = string(errBytes)
} else {
volume.mountError = err.Error()
}
switch err.(type) {
case nil:
return dockerdriver.MountResponse{Mountpoint: volume.Mountpoint}
case dockerdriver.SafeError:
errBytes, mErr := json.Marshal(err)
if mErr != nil {
logger.Error("failed-to-marshal-safeerror", mErr)
volume.mountError = err.Error()
}
}()

wg.Done()
}

wg.Wait()

return func() dockerdriver.MountResponse {
volume, ok := d.volumes.Get(mountRequest.Name)
if !ok {
return dockerdriver.MountResponse{Err: fmt.Sprintf("Volume '%s' not found", mountRequest.Name)}
} else if volume.mountError != "" {
volume.mountError = string(errBytes)
return dockerdriver.MountResponse{Err: volume.mountError}
} else {
// Check the volume to make sure it's still mounted before handing it out again.
if !doMount && !d.mounter.Check(driverhttp.EnvWithLogger(logger, env), volume.Name, volume.Mountpoint) {
wg.Add(1)
defer wg.Done()
if err := d.mount(driverhttp.EnvWithLogger(logger, env), volume.Opts, mountPath); err != nil {
logger.Error("remount-volume-failed", err)
return dockerdriver.MountResponse{Err: fmt.Sprintf("Error remounting volume: %s", err.Error())}
}
default:
volume.mountError = err.Error()
return dockerdriver.MountResponse{Err: volume.mountError}
}
} else {
// Check the volume to make sure it's still mounted before handing it out again.
if !d.mounter.Check(driverhttp.EnvWithLogger(logger, env), volume.Name, volume.Mountpoint) {
if err := d.mount(driverhttp.EnvWithLogger(logger, env), volume.Opts, mountPath); err != nil {
logger.Error("remount-volume-failed", err)
return dockerdriver.MountResponse{Err: fmt.Sprintf("Error remounting volume: %s", err.Error())}
}
return dockerdriver.MountResponse{Mountpoint: volume.Mountpoint}
}
}()
return dockerdriver.MountResponse{Mountpoint: volume.Mountpoint}
}
}

func (d *VolumeDriver) Path(env dockerdriver.Env, pathRequest dockerdriver.PathRequest) dockerdriver.PathResponse {
Expand Down Expand Up @@ -258,6 +214,8 @@ func (d *VolumeDriver) Path(env dockerdriver.Env, pathRequest dockerdriver.PathR

func (d *VolumeDriver) Unmount(env dockerdriver.Env, unmountRequest dockerdriver.UnmountRequest) dockerdriver.ErrorResponse {
logger := env.Logger().Session("unmount", lager.Data{"volume": unmountRequest.Name})
logger.Info("start")
defer logger.Info("end")

if unmountRequest.Name == "" {
return dockerdriver.ErrorResponse{Err: "Missing mandatory 'volume_name'"}
Expand Down Expand Up @@ -544,3 +502,11 @@ func (d *VolumeDriver) Drain(env dockerdriver.Env) error {

return nil
}

func copyOpts(input map[string]any) map[string]any {
output := make(map[string]any)
for k, v := range input {
output[k] = v
}
return output
}

0 comments on commit 5f8b98a

Please sign in to comment.