diff --git a/changelog/unreleased/clean-dfs1.md b/changelog/unreleased/clean-dfs1.md new file mode 100644 index 00000000000..436b05f5421 --- /dev/null +++ b/changelog/unreleased/clean-dfs1.md @@ -0,0 +1,5 @@ +Enhancement: Error handling cleanup in decomposed FS + +- Avoid inconsensitencies by cleaning up actions in case of err + +https://github.com/cs3org/reva/pull/2511 diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index 501d3f77fde..002baf8ea8a 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -217,6 +217,17 @@ func (fs *Decomposedfs) CreateHome(ctx context.Context) (err error) { } return nil }) + + // make sure to delete the created directory if things go wrong + defer func() { + if err != nil { + // do not catch the error to not shadow the original error + if tmpErr := fs.tp.Delete(ctx, n); tmpErr != nil { + appctx.GetLogger(ctx).Error().Err(tmpErr).Msg("Can not revert file system change after error") + } + } + }() + if err != nil { return } @@ -333,6 +344,9 @@ func (fs *Decomposedfs) CreateDir(ctx context.Context, ref *provider.Reference) // mark the home node as the end of propagation if err = xattr.Set(nodePath, xattrs.PropagationAttr, []byte("1")); err != nil { appctx.GetLogger(ctx).Error().Err(err).Interface("node", n).Msg("could not mark node to propagate") + + // FIXME: This does not return an error at all, but results in a severe situation that the + // part tree is not marked for propagation return } } @@ -345,9 +359,10 @@ func (fs *Decomposedfs) TouchFile(ctx context.Context, ref *provider.Reference) } // CreateReference creates a reference as a node folder with the target stored in extended attributes -// There is no difference between the /Shares folder and normal nodes because the storage is not supposed to be accessible without the storage provider. -// In effect everything is a shadow namespace. +// There is no difference between the /Shares folder and normal nodes because the storage is not supposed to be accessible +// without the storage provider. In effect everything is a shadow namespace. // To mimic the eos and owncloud driver we only allow references as children of the "/Shares" folder +// FIXME: This comment should explain briefly what a reference is in this context. func (fs *Decomposedfs) CreateReference(ctx context.Context, p string, targetURI *url.URL) (err error) { ctx, span := rtrace.Provider.Tracer("reva").Start(ctx, "CreateReference") defer span.End() @@ -368,39 +383,62 @@ func (fs *Decomposedfs) CreateReference(ctx context.Context, p string, targetURI } // create Shares folder if it does not exist - var n *node.Node - if n, err = fs.lu.NodeFromResource(ctx, &provider.Reference{Path: fs.o.ShareFolder}); err != nil { + var parentNode *node.Node + var parentCreated, childCreated bool // defaults to false + if parentNode, err = fs.lu.NodeFromResource(ctx, &provider.Reference{Path: fs.o.ShareFolder}); err != nil { err := errtypes.InternalError(err.Error()) span.SetStatus(codes.Error, err.Error()) return err - } else if !n.Exists { - if err = fs.tp.CreateDir(ctx, n); err != nil { + } else if !parentNode.Exists { + if err = fs.tp.CreateDir(ctx, parentNode); err != nil { span.SetStatus(codes.Error, err.Error()) return err } - } + parentCreated = true + } + + var childNode *node.Node + // clean up directories created here on error + defer func() { + if err != nil { + // do not catch the error to not shadow the original error + if childCreated && childNode != nil { + if tmpErr := fs.tp.Delete(ctx, childNode); tmpErr != nil { + appctx.GetLogger(ctx).Error().Err(tmpErr).Str("node_id", childNode.ID).Msg("Can not clean up child node after error") + } + } + if parentCreated && parentNode != nil { + if tmpErr := fs.tp.Delete(ctx, parentNode); tmpErr != nil { + appctx.GetLogger(ctx).Error().Err(tmpErr).Str("node_id", parentNode.ID).Msg("Can not clean up parent node after error") + } + + } + } + }() - if n, err = n.Child(ctx, parts[1]); err != nil { + if childNode, err = parentNode.Child(ctx, parts[1]); err != nil { return errtypes.InternalError(err.Error()) } - if n.Exists { + if childNode.Exists { // TODO append increasing number to mountpoint name err := errtypes.AlreadyExists(p) span.SetStatus(codes.Error, err.Error()) return err } - if err := fs.tp.CreateDir(ctx, n); err != nil { + if err := fs.tp.CreateDir(ctx, childNode); err != nil { span.SetStatus(codes.Error, err.Error()) return err } + childCreated = true - internal := n.InternalPath() - if err := xattr.Set(internal, xattrs.ReferenceAttr, []byte(targetURI.String())); err != nil { + internalPath := childNode.InternalPath() + if err := xattr.Set(internalPath, xattrs.ReferenceAttr, []byte(targetURI.String())); err != nil { + // the reference could not be set - that would result in an lost reference? err := errors.Wrapf(err, "Decomposedfs: error setting the target %s on the reference file %s", targetURI.String(), - internal, + internalPath, ) span.SetStatus(codes.Error, err.Error()) return err diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go index b3ce5bb7629..867c02ad1c6 100644 --- a/pkg/storage/utils/decomposedfs/spaces.go +++ b/pkg/storage/utils/decomposedfs/spaces.go @@ -511,13 +511,15 @@ func (fs *Decomposedfs) createStorageSpace(ctx context.Context, spaceType, space if err != nil { if isAlreadyExists(err) { appctx.GetLogger(ctx).Debug().Err(err).Str("space", spaceID).Str("spacetype", spaceType).Msg("symlink already exists") + // FIXME: is it ok to wipe this err if the symlink already exists? + err = nil } else { // TODO how should we handle error cases here? appctx.GetLogger(ctx).Error().Err(err).Str("space", spaceID).Str("spacetype", spaceType).Msg("could not create symlink") } } - return nil + return err } func (fs *Decomposedfs) storageSpaceFromNode(ctx context.Context, n *node.Node, spaceType, nodePath string, canListAllSpaces bool) (*provider.StorageSpace, error) {