From 8a50f616af33897c4138aa0efba5ba5ca1702347 Mon Sep 17 00:00:00 2001 From: Fatih Arslan Date: Tue, 21 Aug 2018 22:06:45 +0300 Subject: [PATCH] driver: do not use source to check mount --- driver/driver_test.go | 2 +- driver/mounter.go | 26 ++++++++++++------------ driver/node.go | 47 ++++++++++++++----------------------------- 3 files changed, 29 insertions(+), 46 deletions(-) diff --git a/driver/driver_test.go b/driver/driver_test.go index 8ad95bd32..124db87d5 100644 --- a/driver/driver_test.go +++ b/driver/driver_test.go @@ -202,6 +202,6 @@ func (f *fakeMounter) Unmount(target string) error { func (f *fakeMounter) IsFormatted(source string) (bool, error) { return true, nil } -func (f *fakeMounter) IsMounted(source, target string) (bool, error) { +func (f *fakeMounter) IsMounted(target string) (bool, error) { return true, nil } diff --git a/driver/mounter.go b/driver/mounter.go index 6a7373419..56f349c64 100644 --- a/driver/mounter.go +++ b/driver/mounter.go @@ -53,11 +53,10 @@ type Mounter interface { // returns true if the source device is already formatted. IsFormatted(source string) (bool, error) - // IsMounted checks whether the source device is mounted to the target path - // in a correct way (i.e: propagated). It returns true if it's mounted. An - // error is returned in case of system errors or if it's mounted - // incorrectly. - IsMounted(source, target string) (bool, error) + // IsMounted checks whether the target path is a correct mount (i.e: + // propagated). It returns true if it's mounted. An error is returned in + // case of system errors or if it's mounted incorrectly. + IsMounted(target string) (bool, error) } // TODO(arslan): this is Linux only for now. Refactor this into a package with @@ -215,11 +214,7 @@ func (m *mounter) IsFormatted(source string) (bool, error) { return true, nil } -func (m *mounter) IsMounted(source, target string) (bool, error) { - if source == "" { - return false, errors.New("source is not specified for checking the mount") - } - +func (m *mounter) IsMounted(target string) (bool, error) { if target == "" { return false, errors.New("target is not specified for checking the mount") } @@ -233,12 +228,12 @@ func (m *mounter) IsMounted(source, target string) (bool, error) { return false, err } - findmntArgs := []string{"-o", "TARGET,PROPAGATION,FSTYPE,OPTIONS", source, "-J"} + findmntArgs := []string{"-o", "TARGET,PROPAGATION,FSTYPE,OPTIONS", "-M", target, "-J"} m.log.WithFields(logrus.Fields{ "cmd": findmntCmd, "args": findmntArgs, - }).Info("checking if source is mounted") + }).Info("checking if target is mounted") out, err := exec.Command(findmntCmd, findmntArgs...).CombinedOutput() if err != nil { @@ -251,6 +246,11 @@ func (m *mounter) IsMounted(source, target string) (bool, error) { err, findmntCmd, string(out)) } + // no response means there is no mount + if string(out) == "" { + return false, nil + } + var resp *findmntResponse err = json.Unmarshal(out, &resp) if err != nil { @@ -261,7 +261,7 @@ func (m *mounter) IsMounted(source, target string) (bool, error) { for _, fs := range resp.FileSystems { // check if the mount is propagated correctly. It should be set to shared. if fs.Propagation != "shared" { - return true, fmt.Errorf("mount propagation for target %q is not enabled or the block device %q does not exist anymore", target, source) + return true, fmt.Errorf("mount propagation for target %q is not enabled", target) } // the mountpoint should match as well diff --git a/driver/node.go b/driver/node.go index ad5261b1d..09b580137 100644 --- a/driver/node.go +++ b/driver/node.go @@ -28,7 +28,6 @@ import ( "context" "net/http" "path/filepath" - "strings" csi "github.com/container-storage-interface/spec/lib/go/csi/v0" "github.com/sirupsen/logrus" @@ -104,7 +103,7 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe ll.Info("mounting the volume for staging") - mounted, err := d.mounter.IsMounted(source, target) + mounted, err := d.mounter.IsMounted(target) if err != nil { return nil, err } @@ -138,17 +137,7 @@ func (d *Driver) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstageVolu }) ll.Info("node unstage volume called") - vol, resp, err := d.doClient.Storage.GetVolume(ctx, req.VolumeId) - if err != nil { - if resp != nil && resp.StatusCode == http.StatusNotFound { - // we assume it's deleted already for idempotency - return &csi.NodeUnstageVolumeResponse{}, nil - } - return nil, err - } - - source := getDiskSource(vol.Name) - mounted, err := d.mounter.IsMounted(source, req.StagingTargetPath) + mounted, err := d.mounter.IsMounted(req.StagingTargetPath) if err != nil { return nil, err } @@ -186,16 +175,6 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu return nil, status.Error(codes.InvalidArgument, "NodePublishVolume Volume Capability must be provided") } - vol, resp, err := d.doClient.Storage.GetVolume(ctx, req.VolumeId) - if err != nil { - if resp != nil && resp.StatusCode == http.StatusNotFound { - return nil, status.Errorf(codes.NotFound, "volume %q not found", req.VolumeId) - } - return nil, err - } - - diskSource := getDiskSource(vol.Name) - source := req.StagingTargetPath target := req.TargetPath @@ -223,9 +202,7 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu "method": "node_publish_volume", }) - // we can only check if target is mounted with the diskSource directly. - // The staging target path (which is a directory itself) won't work in this case - mounted, err := d.mounter.IsMounted(diskSource, target) + mounted, err := d.mounter.IsMounted(target) if err != nil { return nil, err } @@ -260,15 +237,21 @@ func (d *Driver) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublish }) ll.Info("node unpublish volume called") - ll.Info("unmounting the target path") - err := d.mounter.Unmount(req.TargetPath) - // Invalid argument is returned by the `mount` command in this container - // image when a path is already umounted. We only return an error for - // anything else - if err != nil && !strings.Contains(err.Error(), "Invalid argument") { + mounted, err := d.mounter.IsMounted(req.TargetPath) + if err != nil { return nil, err } + if mounted { + ll.Info("unmounting the target path") + err := d.mounter.Unmount(req.TargetPath) + if err != nil { + return nil, err + } + } else { + ll.Info("target path is already unmounted") + } + ll.Info("unmounting volume is finished") return &csi.NodeUnpublishVolumeResponse{}, nil }