Skip to content

Commit

Permalink
do not try to resize block volumes
Browse files Browse the repository at this point in the history
Block volumes cannot be resized, so there is no point in even attempting it.
An attempt would fail because it could not determine the FS one the volume, as
there likely isn't any. A retry would then succeed as there is an earlier check
if we need to mount at all.

The fix is to rearrange the logic to only attempt to resize filesystem volumes.

While at it, also remove the deprecated "IsNotMountPoint". The double negative
was getting confusing in any case.

Signed-off-by: Moritz Wanzenböck <[email protected]>
  • Loading branch information
WanzenBug authored and rck committed Oct 8, 2024
1 parent 03b4410 commit 798b960
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 24 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- Fixed an issue that caused the node publish call to attempt to resize a block volume: block volumes are now never
attempted to be resized.

## [1.6.3] - 2024-06-28

### Fixed
Expand Down
30 changes: 16 additions & 14 deletions pkg/client/linstor.go
Original file line number Diff line number Diff line change
Expand Up @@ -1975,28 +1975,30 @@ func (s *Linstor) Mount(ctx context.Context, source, target, fsType string, read
}
}

needsMount, err := mount.IsNotMountPoint(s.mounter, target)
isMounted, err := s.mounter.IsMountPoint(target)
if err != nil {
return fmt.Errorf("unable to determine mount status of %s %v", target, err)
}

if !needsMount {
return nil
if !isMounted {
err = s.mounter.Mount(source, target, fsType, mntOpts)
if err != nil {
return err
}
}

err = s.mounter.Mount(source, target, fsType, mntOpts)
if err != nil {
return err
if block {
return nil
}

resizerFs := mount.NewResizeFs(s.mounter.Exec)

resize, err := resizerFs.NeedResize(source, target)
needResize, err := resizerFs.NeedResize(source, target)
if err != nil {
return fmt.Errorf("unable to determine if resize required: %w", err)
}

if !block && resize {
if needResize {
_, err := resizerFs.Resize(source, target)
if err != nil {
unmountErr := s.Unmount(target)
Expand All @@ -2021,20 +2023,20 @@ func (s *Linstor) setDevReadWrite(ctx context.Context, srcPath string) error {
return err
}

// IsNotMountPoint determines if a directory is a mountpoint.
// IsMountPoint determines if a directory is a mountpoint.
//
// Non-existent paths return (true, nil).
func (s *Linstor) IsNotMountPoint(target string) (bool, error) {
notMounted, err := mount.IsNotMountPoint(s.mounter, target)
// Non-existent paths return (false, nil).
func (s *Linstor) IsMountPoint(target string) (bool, error) {
mounted, err := s.mounter.IsMountPoint(target)
if err != nil {
if os.IsNotExist(err) {
return true, nil
return false, nil
}

return false, err
}

return notMounted, nil
return mounted, nil
}

// Unmount unmounts the target. Operates locally on the machines where it is called.
Expand Down
17 changes: 10 additions & 7 deletions pkg/client/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,27 +258,30 @@ func (s *MockStorage) Mount(ctx context.Context, source, target, fsType string,
return nil
}

func (s *MockStorage) IsNotMountPoint(target string) (bool, error) {
func (s *MockStorage) IsMountPoint(target string) (bool, error) {
_, err := os.Stat(target)
if err != nil {
if os.IsNotExist(err) {
return true, nil
return false, nil
}

return false, err
}
return false, nil

return true, nil
}

func (s *MockStorage) Unmount(target string) error {
notMounted, err := s.IsNotMountPoint(target)
mounted, err := s.IsMountPoint(target)
if err != nil {
return err
}
if notMounted {
return nil

if mounted {
return os.RemoveAll(target)
}

return os.RemoveAll(target)
return nil
}

func (s *MockStorage) GetVolumeStats(path string) (volume.VolumeStats, error) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/driver/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -430,12 +430,12 @@ func (d Driver) NodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeSt
return nil, missingAttr("NodeGetVolumeStats", req.GetVolumeId(), "VolumeId")
}

notMounted, err := d.Mounter.IsNotMountPoint(req.GetVolumePath())
mounted, err := d.Mounter.IsMountPoint(req.GetVolumePath())
if err != nil {
return nil, status.Errorf(codes.Internal, "NodeGetVolumeStats failed for %s: failed to check if path %v is mounted: %v", req.GetVolumeId(), req.GetVolumePath(), err)
}

if notMounted {
if !mounted {
return nil, status.Errorf(codes.NotFound, "NodeGetVolumeStats failed for %s: path %v is not mounted", req.GetVolumeId(), req.GetVolumePath())
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ type Querier interface {
type Mounter interface {
Mount(ctx context.Context, source, target, fsType string, readonly bool, mntOpts []string) error
Unmount(target string) error
IsNotMountPoint(target string) (bool, error)
IsMountPoint(target string) (bool, error)
}

// VolumeStats provides details about filesystem usage.
Expand Down

0 comments on commit 798b960

Please sign in to comment.