From a9705a710006bdd11cc2d5e9ad60f7fc37225aed Mon Sep 17 00:00:00 2001 From: nabil salah Date: Tue, 29 Apr 2025 18:32:38 +0300 Subject: [PATCH 1/7] fix: dummy instances add cleanup Signed-off-by: nabil salah --- pkg/flist/flist.go | 13 +++++++++++++ pkg/network/networker.go | 9 ++++----- pkg/primitives/zdb/zdb.go | 41 ++++++++++++++++++++++++++++++++++++--- 3 files changed, 55 insertions(+), 8 deletions(-) diff --git a/pkg/flist/flist.go b/pkg/flist/flist.go index 0a99390e..0d69f8da 100644 --- a/pkg/flist/flist.go +++ b/pkg/flist/flist.go @@ -517,6 +517,19 @@ func (f *flistModule) valid(path string) error { } if err := f.isMountpoint(path); err == nil { + // Check if the mountpoint is empty + contents, err := os.ReadDir(path) + if err != nil { + log.Warn().Err(err).Str("path", path).Msg("failed to check mountpoint contents") + } else if len(contents) == 0 { + log.Warn().Str("path", path).Msg("found empty mountpoint, attempting to unmount") + if err := f.system.Unmount(path, 0); err != nil { + log.Error().Err(err).Str("path", path).Msg("failed to unmount empty mountpoint") + return ErrAlreadyMounted + } + return nil + } + return ErrAlreadyMounted } diff --git a/pkg/network/networker.go b/pkg/network/networker.go index 59746ff7..1aab1671 100644 --- a/pkg/network/networker.go +++ b/pkg/network/networker.go @@ -355,12 +355,11 @@ func (n *networker) EnsureZDBPrepare(id string) (string, error) { // ZDBDestroy is the opposite of ZDPrepare, it makes sure network setup done // for zdb is rewind. ns param is the namespace return by the ZDBPrepare func (n *networker) ZDBDestroy(ns string) error { - panic("not implemented") - // if !strings.HasPrefix(ns, zdbNamespacePrefix) { - // return fmt.Errorf("invalid zdb namespace name '%s'", ns) - // } + if !strings.HasPrefix(ns, zdbNamespacePrefix) { + return fmt.Errorf("invalid zdb namespace name '%s'", ns) + } - // return n.destroy(ns) + return n.destroy(ns) } // func (n *networker) createMacVlan(iface string, master string, hw net.HardwareAddr, ips []*net.IPNet, routes []*netlink.Route, netNs ns.NetNS) error { diff --git a/pkg/primitives/zdb/zdb.go b/pkg/primitives/zdb/zdb.go index 79f10194..b506f125 100644 --- a/pkg/primitives/zdb/zdb.go +++ b/pkg/primitives/zdb/zdb.go @@ -64,7 +64,30 @@ func (se *safeError) Error() string { func (z *tZDBContainer) DataMount() (string, error) { for _, mnt := range z.Mounts { if mnt.Target == zdbContainerDataMnt { - return mnt.Source, nil + // Verify that this is a valid ZDB data directory by checking for expected subdirectories + source := mnt.Source + if stat, err := os.Stat(source); err != nil || !stat.IsDir() { + return "", fmt.Errorf("container '%s' data mount path doesn't exist or isn't a directory", z.Name) + } + + // Check for data and index directories which should exist in a valid ZDB mount + requiredDirs := []string{"data", "index"} + valid := true + + for _, dir := range requiredDirs { + path := filepath.Join(source, dir) + if stat, err := os.Stat(path); err != nil || !stat.IsDir() { + valid = false + log.Warn().Str("container", string(z.Name)).Str("path", path).Msg("missing required ZDB directory") + break + } + } + + if !valid { + return "", fmt.Errorf("container '%s' data mount doesn't appear to be a valid ZDB directory structure", z.Name) + } + + return source, nil } } @@ -92,13 +115,25 @@ func (p *Manager) Provision(ctx context.Context, wl *gridtypes.WorkloadWithID) ( } func (p *Manager) zdbListContainers(ctx context.Context) (map[pkg.ContainerID]tZDBContainer, error) { - contmod := stubs.NewContainerModuleStub(p.zbus) + var ( + contmod = stubs.NewContainerModuleStub(p.zbus) + ) containerIDs, err := contmod.List(ctx, zdbContainerNS) if err != nil { return nil, errors.Wrap(err, "failed to list running containers") } + cleanup := func(z tZDBContainer) { + for _, mnt := range z.Mounts { + if mnt.Target == zdbContainerDataMnt { + source := mnt.Source + if err := os.RemoveAll(source); err != nil { + log.Error().Err(err).Str("path", source).Msg("failed to delete invalid ZDB source directory") + } + } + } + } // for each container we try to find a free space to jam in this new zdb namespace // request m := make(map[pkg.ContainerID]tZDBContainer) @@ -113,7 +148,7 @@ func (p *Manager) zdbListContainers(ctx context.Context) (map[pkg.ContainerID]tZ if _, err = cont.DataMount(); err != nil { log.Error().Err(err).Msg("failed to get data directory of zdb container") - continue + cleanup(cont) } m[containerID] = cont } From 3fc8e241b2eece020543924827d3129474a10991 Mon Sep 17 00:00:00 2001 From: nabil salah Date: Wed, 30 Apr 2025 16:05:05 +0300 Subject: [PATCH 2/7] fix: destroy network on zdb destroy Signed-off-by: nabil salah --- pkg/primitives/zdb/zdb.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/primitives/zdb/zdb.go b/pkg/primitives/zdb/zdb.go index b506f125..35c44982 100644 --- a/pkg/primitives/zdb/zdb.go +++ b/pkg/primitives/zdb/zdb.go @@ -116,15 +116,21 @@ func (p *Manager) Provision(ctx context.Context, wl *gridtypes.WorkloadWithID) ( func (p *Manager) zdbListContainers(ctx context.Context) (map[pkg.ContainerID]tZDBContainer, error) { var ( + flist = stubs.NewFlisterStub(p.zbus) contmod = stubs.NewContainerModuleStub(p.zbus) + network = stubs.NewNetworkerStub(p.zbus) ) containerIDs, err := contmod.List(ctx, zdbContainerNS) if err != nil { return nil, errors.Wrap(err, "failed to list running containers") } + rootFS, err := p.zdbRootFS(ctx) + if err != nil { + return nil, errors.Wrap(err, "failed to get zdb root fs") + } - cleanup := func(z tZDBContainer) { + cleanup := func(z tZDBContainer, containerID pkg.ContainerID) { for _, mnt := range z.Mounts { if mnt.Target == zdbContainerDataMnt { source := mnt.Source @@ -133,6 +139,10 @@ func (p *Manager) zdbListContainers(ctx context.Context) (map[pkg.ContainerID]tZ } } } + if err := flist.Unmount(ctx, string(containerID)); err != nil { + log.Error().Err(err).Str("path", rootFS).Msgf("failed to unmount") + } + network.ZDBDestroy(ctx, string(containerID)) } // for each container we try to find a free space to jam in this new zdb namespace // request @@ -148,7 +158,7 @@ func (p *Manager) zdbListContainers(ctx context.Context) (map[pkg.ContainerID]tZ if _, err = cont.DataMount(); err != nil { log.Error().Err(err).Msg("failed to get data directory of zdb container") - cleanup(cont) + cleanup(cont, containerID) } m[containerID] = cont } From a64aa6a4233ee0eca60e1a2667e03bc6cacb6210 Mon Sep 17 00:00:00 2001 From: nabil salah Date: Wed, 30 Apr 2025 18:13:59 +0300 Subject: [PATCH 3/7] fix: linting Signed-off-by: nabil salah --- pkg/flist/flist.go | 2 +- pkg/primitives/zdb/zdb.go | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/flist/flist.go b/pkg/flist/flist.go index 0d69f8da..ea8f8515 100644 --- a/pkg/flist/flist.go +++ b/pkg/flist/flist.go @@ -529,7 +529,7 @@ func (f *flistModule) valid(path string) error { } return nil } - + return ErrAlreadyMounted } diff --git a/pkg/primitives/zdb/zdb.go b/pkg/primitives/zdb/zdb.go index 35c44982..aa010242 100644 --- a/pkg/primitives/zdb/zdb.go +++ b/pkg/primitives/zdb/zdb.go @@ -69,25 +69,25 @@ func (z *tZDBContainer) DataMount() (string, error) { if stat, err := os.Stat(source); err != nil || !stat.IsDir() { return "", fmt.Errorf("container '%s' data mount path doesn't exist or isn't a directory", z.Name) } - + // Check for data and index directories which should exist in a valid ZDB mount requiredDirs := []string{"data", "index"} valid := true - + for _, dir := range requiredDirs { path := filepath.Join(source, dir) if stat, err := os.Stat(path); err != nil || !stat.IsDir() { valid = false - log.Warn().Str("container", string(z.Name)).Str("path", path).Msg("missing required ZDB directory") + log.Warn().Str("container", z.Name).Str("path", path).Msg("missing required ZDB directory") break } } - + if !valid { return "", fmt.Errorf("container '%s' data mount doesn't appear to be a valid ZDB directory structure", z.Name) } - - return source, nil + + return mnt.Source, nil } } @@ -116,9 +116,9 @@ func (p *Manager) Provision(ctx context.Context, wl *gridtypes.WorkloadWithID) ( func (p *Manager) zdbListContainers(ctx context.Context) (map[pkg.ContainerID]tZDBContainer, error) { var ( - flist = stubs.NewFlisterStub(p.zbus) + flist = stubs.NewFlisterStub(p.zbus) contmod = stubs.NewContainerModuleStub(p.zbus) - network = stubs.NewNetworkerStub(p.zbus) + network = stubs.NewNetworkerStub(p.zbus) ) containerIDs, err := contmod.List(ctx, zdbContainerNS) @@ -142,7 +142,7 @@ func (p *Manager) zdbListContainers(ctx context.Context) (map[pkg.ContainerID]tZ if err := flist.Unmount(ctx, string(containerID)); err != nil { log.Error().Err(err).Str("path", rootFS).Msgf("failed to unmount") } - network.ZDBDestroy(ctx, string(containerID)) + _ = network.ZDBDestroy(ctx, string(containerID)) } // for each container we try to find a free space to jam in this new zdb namespace // request From 72021a0a70468f853b78977f68fe868ce56429d4 Mon Sep 17 00:00:00 2001 From: nabil salah Date: Thu, 1 May 2025 15:51:13 +0300 Subject: [PATCH 4/7] fix: remove wrong validation way of checking mount Signed-off-by: nabil salah --- pkg/flist/flist.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/pkg/flist/flist.go b/pkg/flist/flist.go index ea8f8515..0a99390e 100644 --- a/pkg/flist/flist.go +++ b/pkg/flist/flist.go @@ -517,19 +517,6 @@ func (f *flistModule) valid(path string) error { } if err := f.isMountpoint(path); err == nil { - // Check if the mountpoint is empty - contents, err := os.ReadDir(path) - if err != nil { - log.Warn().Err(err).Str("path", path).Msg("failed to check mountpoint contents") - } else if len(contents) == 0 { - log.Warn().Str("path", path).Msg("found empty mountpoint, attempting to unmount") - if err := f.system.Unmount(path, 0); err != nil { - log.Error().Err(err).Str("path", path).Msg("failed to unmount empty mountpoint") - return ErrAlreadyMounted - } - return nil - } - return ErrAlreadyMounted } From 854fc1299d2a0e00d2a80d4593d3e2a7b4553ebc Mon Sep 17 00:00:00 2001 From: nabil salah Date: Mon, 5 May 2025 17:01:05 +0300 Subject: [PATCH 5/7] fix: cleaning before trying to mount Signed-off-by: nabil salah --- pkg/flist/flist.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/flist/flist.go b/pkg/flist/flist.go index 0a99390e..94bc70e9 100644 --- a/pkg/flist/flist.go +++ b/pkg/flist/flist.go @@ -430,11 +430,9 @@ func (f *flistModule) mountInNamespace(name, url string, opt pkg.MountOptions, n sublog := log.With().Str("name", name).Str("url", url).Str("storage", opt.Storage).Logger() sublog.Info().Msgf("request to mount flist: %+v", opt) - defer func() { - if err := f.cleanUnusedMounts(); err != nil { - log.Error().Err(err).Msg("failed to run clean up") - } - }() + if err := f.cleanUnusedMounts(); err != nil { + log.Error().Err(err).Msg("failed to run clean up") + } // mount overlay mountpoint, err := f.mountpath(name) if err != nil { From 5273891b738eb1cca8d15753ab6e8efba07c3f87 Mon Sep 17 00:00:00 2001 From: nabil salah Date: Tue, 6 May 2025 16:22:22 +0300 Subject: [PATCH 6/7] feat:force unmount cleaning Signed-off-by: nabil salah --- pkg/flist/cleanup.go | 22 +++++++++++++++++--- pkg/flist/flist.go | 3 ++- pkg/flist/flist_test.go | 2 ++ pkg/primitives/zdb/zdb.go | 43 ++++++++++++++++++++------------------- 4 files changed, 45 insertions(+), 25 deletions(-) diff --git a/pkg/flist/cleanup.go b/pkg/flist/cleanup.go index d169daf8..56f4730d 100644 --- a/pkg/flist/cleanup.go +++ b/pkg/flist/cleanup.go @@ -10,6 +10,7 @@ import ( "github.com/pkg/errors" "github.com/rs/zerolog/log" "github.com/threefoldtech/zosbase/pkg/app" + "golang.org/x/sys/unix" ) // Cleaner interface, implementer of this interface @@ -134,12 +135,27 @@ func (f *flistModule) cleanUnusedMounts() error { for _, entry := range entries { path := filepath.Join(f.mountpoint, entry.Name()) - if err := f.isMountpoint(path); err == nil { + + // First check if it's a mountpoint - clean non-mountpoints as before + if err := f.isMountpoint(path); err != nil { + if err := os.Remove(path); err != nil { + log.Error().Err(err).Msgf("failed to clean mountpoint %s", path) + } continue } - if err := os.Remove(path); err != nil { - log.Error().Err(err).Msgf("failed to clean mountpoint %s", path) + // New check: If it's a mountpoint, verify it's not empty + contents, readErr := os.ReadDir(path) + if readErr == nil && len(contents) == 0 { + log.Debug().Str("path", path).Msg("cleaning empty mountpoint") + if unmountErr := f.system.Unmount(path, unix.MNT_DETACH|unix.MNT_FORCE); unmountErr != nil { + log.Error().Err(unmountErr).Str("path", path).Msg("failed to unmount empty mountpoint") + continue + } + + if removeErr := os.RemoveAll(path); removeErr != nil { + log.Error().Err(removeErr).Str("path", path).Msg("failed to clean empty mountpoint") + } } } diff --git a/pkg/flist/flist.go b/pkg/flist/flist.go index 94bc70e9..4e314f33 100644 --- a/pkg/flist/flist.go +++ b/pkg/flist/flist.go @@ -27,6 +27,7 @@ import ( "github.com/threefoldtech/zosbase/pkg/kernel" "github.com/threefoldtech/zosbase/pkg/network/namespace" "github.com/threefoldtech/zosbase/pkg/stubs" + "golang.org/x/sys/unix" ) const ( @@ -597,7 +598,7 @@ func (f *flistModule) Unmount(name string) error { } if f.valid(mountpoint) == ErrAlreadyMounted { - if err := f.system.Unmount(mountpoint, 0); err != nil { + if err := f.system.Unmount(mountpoint, unix.MNT_DETACH|unix.MNT_FORCE); err != nil { log.Error().Err(err).Str("path", mountpoint).Msg("fail to umount flist") } } diff --git a/pkg/flist/flist_test.go b/pkg/flist/flist_test.go index 24025e36..e5f35023 100644 --- a/pkg/flist/flist_test.go +++ b/pkg/flist/flist_test.go @@ -217,6 +217,8 @@ func TestIsolation(t *testing.T) { strg.On("VolumeCreate", mock.Anything, mock.Anything, mock.Anything, uint64(256*mib)). Return(backend, nil) + sys.On("Unmount", mock.Anything, 3).Return(nil) + name1 := "test1" sys.On("Mount", "overlay", filepath.Join(root, "mountpoint", name1), "overlay", uintptr(syscall.MS_NOATIME), mock.Anything).Return(nil) diff --git a/pkg/primitives/zdb/zdb.go b/pkg/primitives/zdb/zdb.go index aa010242..470d747a 100644 --- a/pkg/primitives/zdb/zdb.go +++ b/pkg/primitives/zdb/zdb.go @@ -114,36 +114,37 @@ func (p *Manager) Provision(ctx context.Context, wl *gridtypes.WorkloadWithID) ( return res, newSafeError(err) } -func (p *Manager) zdbListContainers(ctx context.Context) (map[pkg.ContainerID]tZDBContainer, error) { +func (p *Manager) zdbCleanUp(ctx context.Context, z tZDBContainer, containerID pkg.ContainerID) { var ( flist = stubs.NewFlisterStub(p.zbus) - contmod = stubs.NewContainerModuleStub(p.zbus) network = stubs.NewNetworkerStub(p.zbus) ) - - containerIDs, err := contmod.List(ctx, zdbContainerNS) - if err != nil { - return nil, errors.Wrap(err, "failed to list running containers") - } rootFS, err := p.zdbRootFS(ctx) if err != nil { - return nil, errors.Wrap(err, "failed to get zdb root fs") + log.Error().Err(err).Msg("failed to get zdb root fs") } - - cleanup := func(z tZDBContainer, containerID pkg.ContainerID) { - for _, mnt := range z.Mounts { - if mnt.Target == zdbContainerDataMnt { - source := mnt.Source - if err := os.RemoveAll(source); err != nil { - log.Error().Err(err).Str("path", source).Msg("failed to delete invalid ZDB source directory") - } + for _, mnt := range z.Mounts { + if mnt.Target == zdbContainerDataMnt { + source := mnt.Source + if err := os.RemoveAll(source); err != nil { + log.Error().Err(err).Str("path", source).Msg("failed to delete invalid ZDB source directory") } } - if err := flist.Unmount(ctx, string(containerID)); err != nil { - log.Error().Err(err).Str("path", rootFS).Msgf("failed to unmount") - } - _ = network.ZDBDestroy(ctx, string(containerID)) } + if err := flist.Unmount(ctx, string(containerID)); err != nil { + log.Error().Err(err).Str("path", rootFS).Msgf("failed to unmount") + } + _ = network.ZDBDestroy(ctx, string(containerID)) +} + +func (p *Manager) zdbListContainers(ctx context.Context) (map[pkg.ContainerID]tZDBContainer, error) { + var contmod = stubs.NewContainerModuleStub(p.zbus) + + containerIDs, err := contmod.List(ctx, zdbContainerNS) + if err != nil { + return nil, errors.Wrap(err, "failed to list running containers") + } + // for each container we try to find a free space to jam in this new zdb namespace // request m := make(map[pkg.ContainerID]tZDBContainer) @@ -158,7 +159,7 @@ func (p *Manager) zdbListContainers(ctx context.Context) (map[pkg.ContainerID]tZ if _, err = cont.DataMount(); err != nil { log.Error().Err(err).Msg("failed to get data directory of zdb container") - cleanup(cont, containerID) + p.zdbCleanUp(ctx, cont, containerID) } m[containerID] = cont } From df6715adc5c43c8c2e6685f953a11644f6b534d5 Mon Sep 17 00:00:00 2001 From: nabil salah Date: Tue, 6 May 2025 16:29:58 +0300 Subject: [PATCH 7/7] fix: flist tests Signed-off-by: nabil salah --- pkg/flist/flist_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/flist/flist_test.go b/pkg/flist/flist_test.go index e5f35023..4b6b1170 100644 --- a/pkg/flist/flist_test.go +++ b/pkg/flist/flist_test.go @@ -166,7 +166,7 @@ func TestMountUnmount(t *testing.T) { os.Remove(cmder.m["pid"]) strg.On("VolumeDelete", mock.Anything, filepath.Base(mnt)).Return(nil) - sys.On("Unmount", mnt, 0).Return(nil) + sys.On("Unmount", mnt, 3).Return(nil) err = flister.Unmount(name) require.NoError(t, err) @@ -193,7 +193,7 @@ func TestMountUnmountRO(t *testing.T) { os.Remove(cmder.m["pid"]) strg.On("VolumeDelete", mock.Anything, filepath.Base(mnt)).Return(nil) - sys.On("Unmount", mnt, 0).Return(nil) + sys.On("Unmount", mnt, 3).Return(nil) err = flister.Unmount(name) require.NoError(t, err)