Skip to content

Commit

Permalink
feat(CSI-239): moveToTrash does not return error to upper layers (#312)
Browse files Browse the repository at this point in the history
### TL;DR

Improved error handling in volume garbage collection process.

### What changed?

- Modified `triggerGcVolume` and `moveVolumeToTrash` functions to return errors.
- Updated `Trash` method in `Volume` struct to handle the error returned by `triggerGcVolume`.
- Removed synchronous comment from `triggerGcVolume` as it's no longer relevant.

### How to test?

1. Attempt to trash a volume that requires garbage collection.
2. Verify that any errors during the process are properly propagated and handled.
3. Test scenarios where moving the volume to trash might fail (e.g., permission issues, disk full) and ensure errors are returned.

### Why make this change?

This change improves error handling and propagation in the volume garbage collection process. By returning errors from `triggerGcVolume` and `moveVolumeToTrash`, we can better detect and respond to issues that may occur during the trashing process. This enhancement allows for more robust error handling and logging, potentially improving debugging and system reliability.

---
  • Loading branch information
sergeyberezansky authored Sep 12, 2024
2 parents 08fc758 + abefc0f commit ded86a2
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 6 deletions.
10 changes: 6 additions & 4 deletions pkg/wekafs/gc.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,17 @@ func initInnerPathVolumeGc(mounter AnyMounter) *innerPathVolGc {
return &gc
}

func (gc *innerPathVolGc) triggerGcVolume(ctx context.Context, volume *Volume) {
func (gc *innerPathVolGc) triggerGcVolume(ctx context.Context, volume *Volume) error {
op := "triggerGcVolume"
ctx, span := otel.Tracer(TracerName).Start(ctx, op)
defer span.End()
ctx = log.With().Str("trace_id", span.SpanContext().TraceID().String()).Str("span_id", span.SpanContext().SpanID().String()).Str("op", op).Logger().WithContext(ctx)
logger := log.Ctx(ctx).With().Str("volume_id", volume.GetId()).Logger()
logger.Info().Msg("Triggering garbage collection of volume")
gc.moveVolumeToTrash(ctx, volume) // always do it synchronously
return gc.moveVolumeToTrash(ctx, volume)
}

func (gc *innerPathVolGc) moveVolumeToTrash(ctx context.Context, volume *Volume) {
func (gc *innerPathVolGc) moveVolumeToTrash(ctx context.Context, volume *Volume) error {
op := "moveVolumeToTrash"
ctx, span := otel.Tracer(TracerName).Start(ctx, op)
defer span.End()
Expand All @@ -54,7 +54,7 @@ func (gc *innerPathVolGc) moveVolumeToTrash(ctx context.Context, volume *Volume)
defer unmount()
if err != nil {
logger.Error().Err(err).Msg("Failed to mount filesystem for GC processing")
return
return err
}
volumeTrashLoc := filepath.Join(path, garbagePath)
if err := os.MkdirAll(volumeTrashLoc, DefaultVolumePermissions); err != nil {
Expand All @@ -68,6 +68,7 @@ func (gc *innerPathVolGc) moveVolumeToTrash(ctx context.Context, volume *Volume)
if err := os.Rename(fullPath, newPath); err != nil {
logger.Error().Err(err).Str("full_path", fullPath).
Str("volume_trash_location", volumeTrashLoc).Msg("Failed to move volume contents to volumeTrashLoc")
return err
}
// NOTE: there is a problem of directory leaks here. If the volume innerPath is deeper than /csi-volumes/vol-name,
// e.g. if using statically provisioned volume, we move only the deepest directory
Expand All @@ -77,6 +78,7 @@ func (gc *innerPathVolGc) moveVolumeToTrash(ctx context.Context, volume *Volume)
// 2024-07-29: apparently seems this is not a real problem since static volumes are not deleted this way
// and dynamic volumes are always created inside the /csi-volumes
logger.Debug().Str("full_path", fullPath).Str("volume_trash_location", volumeTrashLoc).Msg("Volume contents moved to trash")
return nil
}

func (gc *innerPathVolGc) purgeLeftovers(ctx context.Context, fs string, apiClient *apiclient.ApiClient) {
Expand Down
3 changes: 1 addition & 2 deletions pkg/wekafs/volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -633,8 +633,7 @@ func (v *Volume) updateCapacityXattr(ctx context.Context, enforceCapacity *bool,

func (v *Volume) Trash(ctx context.Context) error {
if v.requiresGc() {
v.server.getMounter().getGarbageCollector().triggerGcVolume(ctx, v)
return nil
return v.server.getMounter().getGarbageCollector().triggerGcVolume(ctx, v)
}
return v.Delete(ctx)
}
Expand Down

0 comments on commit ded86a2

Please sign in to comment.