fix(host-cleanup): ensure no parallel processes of auto host cleanup (#6736)

Signed-off-by: Alexandr Zaytsev <alexandr.zaytsev@flant.com>

Author: nervgh

Diff for: cmd/werf/common/common.go

@@ -19,6 +19,7 @@ import (
 	"github.com/werf/logboek/pkg/types"
 	"github.com/werf/nelm/pkg/action"
 	"github.com/werf/nelm/pkg/log"
+	"github.com/werf/werf/v2/pkg/background"
 	"github.com/werf/werf/v2/pkg/build"
 	"github.com/werf/werf/v2/pkg/build/stage"
 	"github.com/werf/werf/v2/pkg/config"
@@ -1659,7 +1660,7 @@ func GetContextWithLogger() context.Context {
 func WithContext(allowBackgroundMode bool, f func(ctx context.Context) error) error {
 	var ctx context.Context
 
-	if allowBackgroundMode && IsBackgroundModeEnabled() {
+	if allowBackgroundMode && background.IsBackgroundModeEnabled() {
 		out, err := os.OpenFile(GetBackgroundOutputFile(), os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0o644)
 		if err != nil {
 			return fmt.Errorf("unable to open background output file %q: %w", GetBackgroundOutputFile(), err)
@@ -1708,10 +1709,6 @@ func GetAndRemoveLastBackgroundError() (error, error) {
 	return nil, nil
 }
 
-func IsBackgroundModeEnabled() bool {
-	return os.Getenv("_WERF_BACKGROUND_MODE_ENABLED") == "1"
-}
-
 func GetBackgroundOutputFile() string {
 	return filepath.Join(werf.GetServiceDir(), "background_output.log")
 }

Diff for: cmd/werf/common/host_cleanup.go

@@ -26,16 +26,14 @@ func RunAutoHostCleanup(ctx context.Context, cmdData *CmdData, containerBackend
 		return fmt.Errorf("incompatible params --allowed-local-cache-volume-usage=%d and --allowed-local-cache-volume-usage-margin=%d: margin percentage should be less than allowed volume usage level percentage", *cmdData.AllowedLocalCacheVolumeUsage, *cmdData.AllowedLocalCacheVolumeUsageMargin)
 	}
 
-	return host_cleaning.RunAutoHostCleanup(ctx, containerBackend, host_cleaning.AutoHostCleanupOptions{
-		HostCleanupOptions: host_cleaning.HostCleanupOptions{
-			DryRun: false,
-			Force:  false,
-			AllowedBackendStorageVolumeUsagePercentage:       cmdData.AllowedBackendStorageVolumeUsage,
-			AllowedBackendStorageVolumeUsageMarginPercentage: cmdData.AllowedBackendStorageVolumeUsageMargin,
-			AllowedLocalCacheVolumeUsagePercentage:           cmdData.AllowedLocalCacheVolumeUsage,
-			AllowedLocalCacheVolumeUsageMarginPercentage:     cmdData.AllowedLocalCacheVolumeUsageMargin,
-			BackendStoragePath:                               cmdData.BackendStoragePath,
-		},
+	return host_cleaning.RunAutoHostCleanup(ctx, containerBackend, host_cleaning.HostCleanupOptions{
+		DryRun: false,
+		Force:  false,
+		AllowedBackendStorageVolumeUsagePercentage:       cmdData.AllowedBackendStorageVolumeUsage,
+		AllowedBackendStorageVolumeUsageMarginPercentage: cmdData.AllowedBackendStorageVolumeUsageMargin,
+		AllowedLocalCacheVolumeUsagePercentage:           cmdData.AllowedLocalCacheVolumeUsage,
+		AllowedLocalCacheVolumeUsageMarginPercentage:     cmdData.AllowedLocalCacheVolumeUsageMargin,
+		BackendStoragePath:                               cmdData.BackendStoragePath,
 	})
 }
 

Diff for: cmd/werf/main.go

@@ -14,10 +14,18 @@ import (
 	"github.com/werf/nelm/pkg/resrcchangcalc"
 	"github.com/werf/werf/v2/cmd/werf/common"
 	"github.com/werf/werf/v2/cmd/werf/root"
+	"github.com/werf/werf/v2/pkg/background"
 	"github.com/werf/werf/v2/pkg/process_exterminator"
 )
 
 func main() {
+	// IMPORTANT. In background mode we MUST take host-lock to prevent parallel "werf host cleanup" processes.
+	// The processes write data to the same log files that causes "data race".
+	// We don't need to release the lock manually, because it does automatically when the background process will end.
+	if background.IsBackgroundModeEnabled() && !background.TryLock() {
+		return
+	}
+
 	ctx := common.GetContextWithLogger()
 
 	root.PrintStackTraces()

Diff for: pkg/background/background.go

@@ -0,0 +1,27 @@
+package background
+
+import (
+	"os"
+
+	chart "github.com/werf/common-go/pkg/lock"
+	"github.com/werf/lockgate"
+)
+
+func IsBackgroundModeEnabled() bool {
+	return os.Getenv("_WERF_BACKGROUND_MODE_ENABLED") == "1"
+}
+
+// TryLock tries background host-lock or panics if error.
+func TryLock() bool {
+	locker, err := chart.HostLocker()
+	if err != nil {
+		panic(err)
+	}
+	lockName := "werf.background-process"
+	lockOptions := lockgate.AcquireOptions{NonBlocking: true}
+	ok, _, err := locker.Acquire(lockName, lockOptions)
+	if err != nil {
+		panic(err)
+	}
+	return ok
+}

Diff for: pkg/host_cleaning/host_cleanup.go

@@ -30,12 +30,6 @@ type HostCleanupOptions struct {
 	Force  bool
 }
 
-type AutoHostCleanupOptions struct {
-	HostCleanupOptions
-
-	ForceShouldRun bool
-}
-
 func getOptionValueOrDefault(optionValue *uint, defaultValue float64) float64 {
 	var res float64
 	if optionValue != nil {
@@ -46,16 +40,12 @@ func getOptionValueOrDefault(optionValue *uint, defaultValue float64) float64 {
 	return res
 }
 
-func RunAutoHostCleanup(ctx context.Context, backend container_backend.ContainerBackend, options AutoHostCleanupOptions) error {
-	if !options.ForceShouldRun {
-		shouldRun, err := ShouldRunAutoHostCleanup(ctx, backend, options.HostCleanupOptions)
-		if err != nil {
-			logboek.Context(ctx).Warn().LogF("WARNING: unable to check if auto host cleanup should be run: %s\n", err)
-			return nil
-		}
-		if !shouldRun {
-			return nil
-		}
+func RunAutoHostCleanup(ctx context.Context, backend container_backend.ContainerBackend, options HostCleanupOptions) error {
+	if shouldRun, err := shouldRunAutoHostCleanup(ctx, backend, options); err != nil {
+		logboek.Context(ctx).Warn().LogF("WARNING: unable to check if auto host cleanup should be run: %s\n", err)
+		return nil
+	} else if !shouldRun {
+		return nil
 	}
 
 	logboek.Context(ctx).Debug().LogF("RunAutoHostCleanup forking ...\n")
@@ -135,7 +125,7 @@ func RunHostCleanup(ctx context.Context, backend container_backend.ContainerBack
 	})
 }
 
-func ShouldRunAutoHostCleanup(ctx context.Context, backend container_backend.ContainerBackend, options HostCleanupOptions) (bool, error) {
+func shouldRunAutoHostCleanup(ctx context.Context, backend container_backend.ContainerBackend, options HostCleanupOptions) (bool, error) {
 	shouldRun, err := tmp_manager.ShouldRunAutoGC()
 	if err != nil {
 		return false, fmt.Errorf("failed to check tmp manager GC: %w", err)

Diff for: pkg/host_cleaning/host_lock.go

@@ -9,7 +9,8 @@ import (
 	"github.com/werf/logboek"
 )
 
-// withHostLockOrNothing executes callback function if "soft" (NonBlocking=true) lock is acquired. Otherwise, does nothing.
+// withHostLockOrNothing tries a lock. If the lock is acquired, executes callback and releases the lock after.
+// If the lock is NOT acquired, does nothing.
 func withHostLockOrNothing(ctx context.Context, lockName string, callback func() error) error {
 	lockOptions := lockgate.AcquireOptions{NonBlocking: true}
 
@@ -23,5 +24,6 @@ func withHostLockOrNothing(ctx context.Context, lockName string, callback func()
 		return nil
 	}
 
+	// Should we handle panic here and release the lock anyway?
 	return errors.Join(callback(), chart.ReleaseHostLock(lock)) // join non-nil errors or return nil
 }

Diff for: pkg/host_cleaning/host_lock_test.go

@@ -7,34 +7,37 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-var _ = Describe("withHostLockOrNothing", func() {
+var _ = Describe("host lock", func() {
 	var ctx context.Context
-	var spy *spyHostLockCallback
 	BeforeEach(func() {
 		ctx = context.Background()
-		spy = &spyHostLockCallback{}
 	})
-	It("should call callback function if lock is acquired", func() {
-		lockName := "test"
-		err := withHostLockOrNothing(ctx, lockName, spy.Method)
-		Expect(err).To(Succeed())
-		Expect(spy.callsCount).To(Equal(1))
-	})
-	It("should not call callback function if lock isn't acquired", func() {
-		lockName := "test"
-		err := withHostLockOrNothing(ctx, lockName, func() error {
-			return withHostLockOrNothing(ctx, lockName, spy.Method) // lock is already acquired in parent function
+	Describe("withHostLockOrNothing", func() {
+		It("should call callback function if lock is acquired", func() {
+			spy := &spyHostLock{}
+			lockName := "test"
+			err := withHostLockOrNothing(ctx, lockName, spy.Handle1)
+			Expect(err).To(Succeed())
+			Expect(spy.callsCount).To(Equal(1))
+		})
+		It("should not call callback function if lock isn't acquired", func() {
+			spy := &spyHostLock{}
+			lockName := "test"
+			err := withHostLockOrNothing(ctx, lockName, func() error {
+				return withHostLockOrNothing(ctx, lockName, spy.Handle1) // lock is already acquired in parent function
+			})
+			Expect(err).To(Succeed())
+			Expect(spy.callsCount).To(Equal(0))
 		})
-		Expect(err).To(Succeed())
-		Expect(spy.callsCount).To(Equal(0))
 	})
 })
 
-type spyHostLockCallback struct {
+type spyHostLock struct {
 	callsCount int
+	err        error
 }
 
-func (s *spyHostLockCallback) Method() error {
+func (s *spyHostLock) Handle1() error {
 	s.callsCount++
-	return nil
+	return s.err
 }