From 200ed7230e47aa09991092ac2aa29375617c1e85 Mon Sep 17 00:00:00 2001 From: Christophe de Carvalho Date: Wed, 27 Nov 2019 18:39:15 +0100 Subject: [PATCH 1/4] deletes qgroups when deleting sub volumes fixes #397 fixes 409 The sub-volume delete method has been updated to also delete the associated qgroup at the same time. A new method "Maintenance" has been added to the Volume interface so any left over qgroups from before this commit will be also cleaned as soon as storaged is updated and restarted --- pkg/storage/filesystem/btrfs.go | 84 ++++++++++++++++++++++++--- pkg/storage/filesystem/btrfs_utils.go | 7 +++ pkg/storage/filesystem/filesystem.go | 7 ++- pkg/storage/storage.go | 24 ++++++++ 4 files changed, 114 insertions(+), 8 deletions(-) diff --git a/pkg/storage/filesystem/btrfs.go b/pkg/storage/filesystem/btrfs.go index ee2030fef..7d5e8921a 100644 --- a/pkg/storage/filesystem/btrfs.go +++ b/pkg/storage/filesystem/btrfs.go @@ -174,6 +174,10 @@ func (p *btrfsPool) mounted(fs *Btrfs) (string, bool) { return "", false } +func (p *btrfsPool) ID() int { + return 0 +} + func (p *btrfsPool) Name() string { return p.name } @@ -287,6 +291,7 @@ func (p *btrfsPool) Volumes() ([]Volume, error) { for _, sub := range subs { volumes = append(volumes, newBtrfsVolume( + sub.ID, filepath.Join(mnt, sub.Path), p.utils, )) @@ -296,11 +301,17 @@ func (p *btrfsPool) Volumes() ([]Volume, error) { } func (p *btrfsPool) addVolume(root string) (*btrfsVolume, error) { - if err := p.utils.SubvolumeAdd(context.Background(), root); err != nil { + ctx := context.Background() + if err := p.utils.SubvolumeAdd(ctx, root); err != nil { + return nil, err + } + + volume, err := p.utils.SubvolumeInfo(ctx, root) + if err != nil { return nil, err } - return newBtrfsVolume(root, p.utils), nil + return newBtrfsVolume(volume.ID, root, p.utils), nil } func (p *btrfsPool) AddVolume(name string) (Volume, error) { @@ -314,7 +325,19 @@ func (p *btrfsPool) AddVolume(name string) (Volume, error) { } func (p *btrfsPool) removeVolume(root string) error { - return p.utils.SubvolumeRemove(context.Background(), root) + ctx := context.Background() + qgroups, err := p.utils.QGroupList(ctx, root) + if err != nil { + return err + } + + for _, qgroup := range qgroups { + log.Debug().Msgf("delete qgroup %s", qgroup.ID) + if err := p.utils.QGroupDestroy(ctx, qgroup.ID, root); err != nil { + return err + } + } + return p.utils.SubvolumeRemove(ctx, root) } func (p *btrfsPool) RemoveVolume(name string) error { @@ -393,18 +416,59 @@ func (p *btrfsPool) Reserved() (uint64, error) { return total, nil } +func (p *btrfsPool) Maintenance() error { + // this method cleans up all the unused + // qgroups that could exists on a filesystem + + volumes, err := p.Volumes() + if err != nil { + return err + } + subVolsIDs := map[string]struct{}{} + for _, volume := range volumes { + // use the 0/X notation to match the qgroup IDs format + subVolsIDs[fmt.Sprintf("0/%d", volume.ID())] = struct{}{} + } + + ctx := context.Background() + qgroups, err := p.utils.QGroupList(ctx, p.Path()) + if err != nil { + return err + } + + for qgroupID := range qgroups { + // for all qgroup that doesn't have an linked + // volume, delete the qgroup + _, ok := subVolsIDs[qgroupID] + if !ok { + log.Debug().Str("id", qgroupID).Msg("destroy qgroup") + if err := p.utils.QGroupDestroy(ctx, qgroupID, p.Path()); err != nil { + return err + } + } + } + + return nil +} + type btrfsVolume struct { + id int path string utils *BtrfsUtil } -func newBtrfsVolume(path string, utils *BtrfsUtil) *btrfsVolume { +func newBtrfsVolume(ID int, path string, utils *BtrfsUtil) *btrfsVolume { return &btrfsVolume{ + id: ID, path: path, utils: utils, } } +func (v *btrfsVolume) ID() int { + return v.id +} + func (v *btrfsVolume) Path() string { return v.path } @@ -418,19 +482,25 @@ func (v *btrfsVolume) Volumes() ([]Volume, error) { } for _, sub := range subs { - volumes = append(volumes, newBtrfsVolume(filepath.Join(v.Path(), sub.Path), v.utils)) + volumes = append(volumes, newBtrfsVolume(sub.ID, filepath.Join(v.Path(), sub.Path), v.utils)) } return volumes, nil } func (v *btrfsVolume) AddVolume(name string) (Volume, error) { + ctx := context.Background() mnt := filepath.Join(v.Path(), name) - if err := v.utils.SubvolumeAdd(context.Background(), mnt); err != nil { + if err := v.utils.SubvolumeAdd(ctx, mnt); err != nil { + return nil, err + } + + volume, err := v.utils.SubvolumeInfo(ctx, mnt) + if err != nil { return nil, err } - return newBtrfsVolume(mnt, v.utils), nil + return newBtrfsVolume(volume.ID, mnt, v.utils), nil } func (v *btrfsVolume) RemoveVolume(name string) error { diff --git a/pkg/storage/filesystem/btrfs_utils.go b/pkg/storage/filesystem/btrfs_utils.go index 52187db43..3be7742fc 100644 --- a/pkg/storage/filesystem/btrfs_utils.go +++ b/pkg/storage/filesystem/btrfs_utils.go @@ -174,6 +174,13 @@ func (u *BtrfsUtil) QGroupLimit(ctx context.Context, size uint64, path string) e return err } +// QGroupDestroy deletes a qgroup on a subvol +func (u *BtrfsUtil) QGroupDestroy(ctx context.Context, id, path string) error { + _, err := u.run(ctx, "btrfs", "qgroup", "destroy", id, path) + + return err +} + // GetDiskUsage get btrfs usage func (u *BtrfsUtil) GetDiskUsage(ctx context.Context, path string) (usage BtrfsDiskUsage, err error) { output, err := u.run(ctx, "btrfs", "filesystem", "df", "--raw", path) diff --git a/pkg/storage/filesystem/filesystem.go b/pkg/storage/filesystem/filesystem.go index bd2ae42ee..955a3f8ea 100644 --- a/pkg/storage/filesystem/filesystem.go +++ b/pkg/storage/filesystem/filesystem.go @@ -14,6 +14,8 @@ type Usage struct { // Volume represents a logical volume in the pool. Volumes can be nested type Volume interface { + // Volume ID + ID() int // Path of the volume Path() string // Volumes are all subvolumes of this volume @@ -50,7 +52,10 @@ type Pool interface { Type() pkg.DeviceType // Reserved is reserved size of the devices in bytes Reserved() (uint64, error) - + // Maintenance is a routine that is called at boot + // and that all implementer can use to do some clean up and + // other maintenance on the pool + Maintenance() error // Health() ? } diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 342b93f13..d0ac2925f 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -60,6 +60,10 @@ func New() (pkg.StorageModule, error) { log.Info().Msgf("Finished initializing storage module") } + if err := s.Maintenance(); err != nil { + log.Error().Err(err).Msg("storage devices maintenance failed") + } + return s, err } @@ -211,6 +215,26 @@ func (s *storageModule) initialize(policy pkg.StoragePolicy) error { return s.ensureCache() } +func (s *storageModule) Maintenance() error { + + for _, pool := range s.volumes { + log.Info(). + Str("pool", pool.Name()). + Msg("start storage pool maintained") + if err := pool.Maintenance(); err != nil { + log.Error(). + Err(err). + Str("pool", pool.Name()). + Msg("error during maintainace") + return err + } + log.Info(). + Str("pool", pool.Name()). + Msg("finished storage pool maintained") + } + return nil +} + // CreateFilesystem with the given size in a storage pool. func (s *storageModule) CreateFilesystem(name string, size uint64, poolType pkg.DeviceType) (string, error) { log.Info().Msgf("Creating new volume with size %d", size) From 020c1b7ab3819a65d508dd49644d5229e0529a6c Mon Sep 17 00:00:00 2001 From: Christophe de Carvalho Date: Thu, 28 Nov 2019 10:52:08 +0100 Subject: [PATCH 2/4] add tests --- pkg/storage/filesystem/btrfs.go | 7 +++- pkg/storage/filesystem/btrfs_ci_test.go | 53 +++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/pkg/storage/filesystem/btrfs.go b/pkg/storage/filesystem/btrfs.go index 7d5e8921a..2061e52f6 100644 --- a/pkg/storage/filesystem/btrfs.go +++ b/pkg/storage/filesystem/btrfs.go @@ -326,6 +326,11 @@ func (p *btrfsPool) AddVolume(name string) (Volume, error) { func (p *btrfsPool) removeVolume(root string) error { ctx := context.Background() + + if err := p.utils.SubvolumeRemove(ctx, root); err != nil { + return err + } + qgroups, err := p.utils.QGroupList(ctx, root) if err != nil { return err @@ -337,7 +342,7 @@ func (p *btrfsPool) removeVolume(root string) error { return err } } - return p.utils.SubvolumeRemove(ctx, root) + return nil } func (p *btrfsPool) RemoveVolume(name string) error { diff --git a/pkg/storage/filesystem/btrfs_ci_test.go b/pkg/storage/filesystem/btrfs_ci_test.go index 60d954106..a025b0971 100644 --- a/pkg/storage/filesystem/btrfs_ci_test.go +++ b/pkg/storage/filesystem/btrfs_ci_test.go @@ -301,3 +301,56 @@ func TestBtrfsListCI(t *testing.T) { ok := assert.Len(t, names, 0) assert.True(t, ok, "not all pools were listed") } + +func TestCLeanUpQgroupsCI(t *testing.T) { + if SkipCITests { + t.Skip("test requires ability to create loop devices") + } + + devices, err := SetupDevices(1) + require.NoError(t, err, "failed to initialize devices") + + defer devices.Destroy() + loops := devices.Loops() + fs := NewBtrfs(&TestDeviceManager{loops}) + + names := make(map[string]struct{}) + for idx := range loops { + loop := &loops[idx] + name := fmt.Sprintf("test-list-%d", idx) + names[name] = struct{}{} + _, err := fs.Create(context.Background(), name, pkg.Single, loop) + require.NoError(t, err) + } + pools, err := fs.List(context.Background(), func(p Pool) bool { + return strings.HasPrefix(p.Name(), "test-") + }) + pool := pools[0] + + _, err = pool.Mount() + require.NoError(t, err) + defer pool.UnMount() + + volume, err := pool.AddVolume("vol1") + require.NoError(t, err) + + err = volume.Limit(256 * 1024 * 1024) + require.NoError(t, err) + + btrfsVol, ok := volume.(*btrfsVolume) + require.True(t, ok, "volume should be a btrfsVolume") + + qgroups, err := btrfsVol.utils.QGroupList(context.TODO(), pool.Path()) + require.NoError(t, err) + assert.Equal(t, 2, len(qgroups)) + + _, ok = qgroups[fmt.Sprintf("0/%d", btrfsVol.id)] + assert.True(t, ok, "qgroups should contains a qgroup linked to the subvolume") + + err = pool.RemoveVolume("vol1") + require.NoError(t, err) + + qgroups, err = btrfsVol.utils.QGroupList(context.TODO(), pool.Path()) + require.NoError(t, err) + assert.Equal(t, 0, len(qgroups), "qgroups should have been deleted with the subvolume") +} From 0408e3b252203967351042b8ebb506983b96728f Mon Sep 17 00:00:00 2001 From: Christophe de Carvalho Date: Thu, 28 Nov 2019 12:00:27 +0100 Subject: [PATCH 3/4] storaged: remove possibility to create subvolume of subvolume --- pkg/storage/filesystem/btrfs.go | 67 ++++++------------------- pkg/storage/filesystem/btrfs_ci_test.go | 7 --- pkg/storage/filesystem/filesystem.go | 14 +++--- 3 files changed, 24 insertions(+), 64 deletions(-) diff --git a/pkg/storage/filesystem/btrfs.go b/pkg/storage/filesystem/btrfs.go index 2061e52f6..4320d9765 100644 --- a/pkg/storage/filesystem/btrfs.go +++ b/pkg/storage/filesystem/btrfs.go @@ -186,6 +186,16 @@ func (p *btrfsPool) Path() string { return filepath.Join("/mnt", p.name) } +// Limit on a pool is not supported yet +func (p *btrfsPool) Limit(size uint64) error { + return fmt.Errorf("not implemented") +} + +// FsType of the filesystem of this volume +func (p *btrfsPool) FsType() string { + return "btrfs" +} + // Mount mounts the pool in it's default mount location under /mnt/name func (p *btrfsPool) Mount() (string, error) { ctx := context.Background() @@ -385,16 +395,6 @@ func (p *btrfsPool) Usage() (usage Usage, err error) { return Usage{Size: totalSize / raidSizeDivisor[du.Data.Profile], Used: uint64(fsi[0].Used)}, nil } -// Limit on a pool is not supported yet -func (p *btrfsPool) Limit(size uint64) error { - return fmt.Errorf("not implemented") -} - -// FsType of the filesystem of this volume -func (p *btrfsPool) FsType() string { - return "btrfs" -} - // Type of the physical storage used for this pool func (p *btrfsPool) Type() pkg.DeviceType { // We only create heterogenous pools for now @@ -478,39 +478,14 @@ func (v *btrfsVolume) Path() string { return v.path } -func (v *btrfsVolume) Volumes() ([]Volume, error) { - var volumes []Volume - - subs, err := v.utils.SubvolumeList(context.Background(), v.Path()) - if err != nil { - return nil, err - } - - for _, sub := range subs { - volumes = append(volumes, newBtrfsVolume(sub.ID, filepath.Join(v.Path(), sub.Path), v.utils)) - } - - return volumes, nil -} - -func (v *btrfsVolume) AddVolume(name string) (Volume, error) { - ctx := context.Background() - mnt := filepath.Join(v.Path(), name) - if err := v.utils.SubvolumeAdd(ctx, mnt); err != nil { - return nil, err - } - - volume, err := v.utils.SubvolumeInfo(ctx, mnt) - if err != nil { - return nil, err - } - - return newBtrfsVolume(volume.ID, mnt, v.utils), nil +// Name of the filesystem +func (v *btrfsVolume) Name() string { + return filepath.Base(v.Path()) } -func (v *btrfsVolume) RemoveVolume(name string) error { - mnt := filepath.Join(v.Path(), name) - return v.utils.SubvolumeRemove(context.Background(), mnt) +// FsType of the filesystem +func (v *btrfsVolume) FsType() string { + return "btrfs" } // Usage return the volume usage @@ -545,13 +520,3 @@ func (v *btrfsVolume) Limit(size uint64) error { return v.utils.QGroupLimit(ctx, size, v.Path()) } - -// Name of the filesystem -func (v *btrfsVolume) Name() string { - return filepath.Base(v.Path()) -} - -// FsType of the filesystem -func (v *btrfsVolume) FsType() string { - return "btrfs" -} diff --git a/pkg/storage/filesystem/btrfs_ci_test.go b/pkg/storage/filesystem/btrfs_ci_test.go index a025b0971..6642b963b 100644 --- a/pkg/storage/filesystem/btrfs_ci_test.go +++ b/pkg/storage/filesystem/btrfs_ci_test.go @@ -162,13 +162,6 @@ func basePoolTest(t *testing.T, pool Pool) { assert.Equal(t, uint64(1024*1024*1024), usage.Size) }) - t.Run("test subvolume list no subvolumes", func(t *testing.T) { - volumes, err := volume.Volumes() - require.NoError(t, err) - - assert.Empty(t, volumes) - }) - t.Run("test limit subvolume", func(t *testing.T) { usage, err := volume.Usage() require.NoError(t, err) diff --git a/pkg/storage/filesystem/filesystem.go b/pkg/storage/filesystem/filesystem.go index 955a3f8ea..f047319c0 100644 --- a/pkg/storage/filesystem/filesystem.go +++ b/pkg/storage/filesystem/filesystem.go @@ -18,12 +18,6 @@ type Volume interface { ID() int // Path of the volume Path() string - // Volumes are all subvolumes of this volume - Volumes() ([]Volume, error) - // AddVolume adds a new subvolume with the given name - AddVolume(name string) (Volume, error) - // RemoveVolume removes a subvolume with the given name - RemoveVolume(name string) error // Usage reports the current usage of the volume Usage() (Usage, error) // Limit the maximum size of the volume @@ -56,7 +50,15 @@ type Pool interface { // and that all implementer can use to do some clean up and // other maintenance on the pool Maintenance() error + // Health() ? + + // Volumes are all subvolumes of this volume + Volumes() ([]Volume, error) + // AddVolume adds a new subvolume with the given name + AddVolume(name string) (Volume, error) + // RemoveVolume removes a subvolume with the given name + RemoveVolume(name string) error } // Filter closure for Filesystem list From aa4f8d9d3d6bb568af958c4e0d36c234d15d27df Mon Sep 17 00:00:00 2001 From: Christophe de Carvalho Date: Fri, 29 Nov 2019 10:26:20 +0100 Subject: [PATCH 4/4] storaged: only delete the qgroup associated with a volume --- pkg/storage/filesystem/btrfs.go | 16 ++++++++-------- pkg/storage/filesystem/btrfs_ci_test.go | 8 ++++++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/storage/filesystem/btrfs.go b/pkg/storage/filesystem/btrfs.go index 4320d9765..3c674e2e7 100644 --- a/pkg/storage/filesystem/btrfs.go +++ b/pkg/storage/filesystem/btrfs.go @@ -8,6 +8,7 @@ import ( "strings" "syscall" + "github.com/pkg/errors" "github.com/rs/zerolog/log" "github.com/threefoldtech/zos/pkg" @@ -337,21 +338,20 @@ func (p *btrfsPool) AddVolume(name string) (Volume, error) { func (p *btrfsPool) removeVolume(root string) error { ctx := context.Background() - if err := p.utils.SubvolumeRemove(ctx, root); err != nil { + info, err := p.utils.SubvolumeInfo(ctx, root) + if err != nil { return err } - qgroups, err := p.utils.QGroupList(ctx, root) - if err != nil { + if err := p.utils.SubvolumeRemove(ctx, root); err != nil { return err } - for _, qgroup := range qgroups { - log.Debug().Msgf("delete qgroup %s", qgroup.ID) - if err := p.utils.QGroupDestroy(ctx, qgroup.ID, root); err != nil { - return err - } + qgroupID := fmt.Sprintf("0/%d", info.ID) + if err := p.utils.QGroupDestroy(ctx, qgroupID, p.Path()); err != nil { + return errors.Wrapf(err, "failed to delete qgroup %s", qgroupID) } + return nil } diff --git a/pkg/storage/filesystem/btrfs_ci_test.go b/pkg/storage/filesystem/btrfs_ci_test.go index 6642b963b..5a6272d16 100644 --- a/pkg/storage/filesystem/btrfs_ci_test.go +++ b/pkg/storage/filesystem/btrfs_ci_test.go @@ -302,8 +302,8 @@ func TestCLeanUpQgroupsCI(t *testing.T) { devices, err := SetupDevices(1) require.NoError(t, err, "failed to initialize devices") - defer devices.Destroy() + loops := devices.Loops() fs := NewBtrfs(&TestDeviceManager{loops}) @@ -326,6 +326,7 @@ func TestCLeanUpQgroupsCI(t *testing.T) { volume, err := pool.AddVolume("vol1") require.NoError(t, err) + t.Logf("volume ID: %v\n", volume.ID()) err = volume.Limit(256 * 1024 * 1024) require.NoError(t, err) @@ -336,6 +337,7 @@ func TestCLeanUpQgroupsCI(t *testing.T) { qgroups, err := btrfsVol.utils.QGroupList(context.TODO(), pool.Path()) require.NoError(t, err) assert.Equal(t, 2, len(qgroups)) + t.Logf("qgroups before delete: %v", qgroups) _, ok = qgroups[fmt.Sprintf("0/%d", btrfsVol.id)] assert.True(t, ok, "qgroups should contains a qgroup linked to the subvolume") @@ -345,5 +347,7 @@ func TestCLeanUpQgroupsCI(t *testing.T) { qgroups, err = btrfsVol.utils.QGroupList(context.TODO(), pool.Path()) require.NoError(t, err) - assert.Equal(t, 0, len(qgroups), "qgroups should have been deleted with the subvolume") + + t.Logf("remaining qgroups: %+v", qgroups) + assert.Equal(t, 1, len(qgroups), "qgroups should have been deleted with the subvolume") }