Add test with external locking process

Author: pchila

internal/pkg/agent/application/upgrade/step_mark.go

@@ -263,9 +263,12 @@ func markerFilePath(dataDirPath string) string {
 
 func lockMarkerFile(markerFileName string) (*flock.Flock, error) {
 	fLock := flock.New(markerFileName + ".lock")
-	_, err := fLock.TryLock()
+	locked, err := fLock.TryLock()
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("locking %s: %w", fLock, err)
+	}
+	if !locked {
+		return nil, fmt.Errorf("failed locking %s", fLock)
 	}
 
 	return fLock, nil

internal/pkg/agent/application/upgrade/step_mark_test.go

@@ -5,9 +5,16 @@
 package upgrade
 
 import (
+	"bufio"
+	"context"
 	"fmt"
 	"os"
+	"os/exec"
+	"path/filepath"
+	"runtime"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -52,11 +59,11 @@ func Test_markUpgradeLocking(t *testing.T) {
 	}{
 		{
 			name: "Lock file is created when writing update marker", args: args{
-				agent:          newAgent456,
-				previousAgent:  prevAgent123,
-				action:         nil,
-				upgradeDetails: nil,
-			},
+			agent:          newAgent456,
+			previousAgent:  prevAgent123,
+			action:         nil,
+			upgradeDetails: nil,
+		},
 			afterUpdateMarkerCreation: func(t *testing.T, dataDir string) {
 				assert.FileExists(t, markerFilePath(dataDir), "Update marker file must exist")
 				assert.FileExists(t, markerFilePath(dataDir)+".lock", "Update marker lock file must exist")
@@ -69,11 +76,11 @@ func Test_markUpgradeLocking(t *testing.T) {
 		},
 		{
 			name: "Update marker is re-lockable after writing", args: args{
-				agent:          newAgent456,
-				previousAgent:  prevAgent123,
-				action:         nil,
-				upgradeDetails: nil,
-			},
+			agent:          newAgent456,
+			previousAgent:  prevAgent123,
+			action:         nil,
+			upgradeDetails: nil,
+		},
 			afterUpdateMarkerCreation: func(t *testing.T, dataDir string) {
 				assert.FileExists(t, markerFilePath(dataDir), "Update marker file must exist")
 				assert.FileExists(t, markerFilePath(dataDir)+".lock", "Update marker lock file must exist")
@@ -88,11 +95,11 @@ func Test_markUpgradeLocking(t *testing.T) {
 		},
 		{
 			name: "Update marker creation should not fail if marker is already locked by the same process", args: args{
-				agent:          newAgent456,
-				previousAgent:  prevAgent123,
-				action:         nil,
-				upgradeDetails: nil,
-			},
+			agent:          newAgent456,
+			previousAgent:  prevAgent123,
+			action:         nil,
+			upgradeDetails: nil,
+		},
 			beforeUpdateMarkerCreation: func(t *testing.T, dataDir string) {
 				// write some fake data in update marker file
 				updateMarkerFilePath := markerFilePath(dataDir)
@@ -115,6 +122,62 @@ func Test_markUpgradeLocking(t *testing.T) {
 			},
 			wantErr: assert.NoError,
 		},
+		{
+			name: "Update marker creation should fail if marker is already locked by another process", args: args{
+			agent:          newAgent456,
+			previousAgent:  prevAgent123,
+			action:         nil,
+			upgradeDetails: nil,
+		},
+			beforeUpdateMarkerCreation: func(t *testing.T, dataDir string) {
+				// write some fake data in update marker file
+				updateMarkerFilePath := markerFilePath(dataDir)
+				err := os.WriteFile(updateMarkerFilePath, []byte("this: is not a real update marker"), 0o664)
+				require.NoError(t, err, "error creating fake update marker")
+
+				// lock the fake update marker using an external process
+				lockFilePath := updateMarkerFilePath + ".lock"
+				cmdCancel, lockFileCmd := createFileLockerCmd(t, lockFilePath)
+
+				fileLockerStdErr, err := lockFileCmd.StderrPipe()
+				require.NoError(t, err, "Error getting stderr pipe from filelocker")
+
+				fileLockedCh := make(chan struct{})
+
+				// consume stderr to check for locking
+				go func() {
+					scanner := bufio.NewScanner(fileLockerStdErr)
+					for scanner.Scan() {
+						line := scanner.Text()
+						if strings.Contains(line, "Acquired lock on file") {
+							fileLockedCh <- struct{}{}
+						}
+					}
+				}()
+
+				err = lockFileCmd.Start()
+				require.NoError(t, err, "running filelocker should not fail")
+
+				t.Cleanup(func() {
+					cmdCancel()
+					_ = lockFileCmd.Wait()
+				})
+
+				select {
+				case <-fileLockedCh:
+					// file was locked from the external process: all good
+					t.Log("external filelocker acquired the lock!")
+				case <-time.After(30 * time.Second):
+					t.Fatalf("timed out waiting for file locker to lock the file")
+				}
+			},
+			afterUpdateMarkerCreation: func(t *testing.T, dataDir string) {
+				// verify we can't read the actual update marker since it's still locked
+				_, err := LoadMarker(dataDir)
+				require.Error(t, err, "loading update marker should fail")
+			},
+			wantErr: assert.Error,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -131,6 +194,24 @@ func Test_markUpgradeLocking(t *testing.T) {
 	}
 }
 
+func createFileLockerCmd(t *testing.T, lockFilePath string) (context.CancelFunc, *exec.Cmd) {
+	executableName := "filelocker"
+	if runtime.GOOS == "windows" {
+		executableName += ".exe"
+	}
+	filelockerExecutablePath := filepath.Join("test", "filelocker", executableName)
+	require.FileExistsf(
+		t,
+		filelockerExecutablePath,
+		"filelocker executable %s should exist. Please ensure that mage build:testbinaries has been executed.",
+		filelockerExecutablePath,
+	)
+
+	cmdCtx, cmdCancel := context.WithCancel(t.Context())
+	lockFileCmd := exec.CommandContext(cmdCtx, filelockerExecutablePath, "-lockfile", lockFilePath)
+	return cmdCancel, lockFileCmd
+}
+
 func checkUpgradeMarker(t *testing.T, updateMarker *UpdateMarker, prevAgent agentInstall, newAgent agentInstall) {
 	t.Helper()
 	require.NotNil(t, updateMarker, "update marker should not be nil")

internal/pkg/agent/application/upgrade/test/filelocker/.gitignore

@@ -0,0 +1,2 @@
+# Ignore test binary
+filelocker*

internal/pkg/agent/application/upgrade/test/filelocker/main.go

@@ -0,0 +1,51 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License 2.0;
+// you may not use this file except in compliance with the Elastic License 2.0.
+
+// This is a simple program that will lock a file passed using the -lockfile option, used for testing file lock works properly.
+// os.Interrupt or signal.SIGTERM will make the program release the lock and exit
+package main
+
+import (
+	"flag"
+	"log"
+	"os"
+	"os/signal"
+	"syscall"
+
+	"github.com/gofrs/flock"
+)
+
+const AcquiredLockLogFmt = "Acquired lock on file %s\n"
+
+const lockFileFlagName = "lockfile"
+
+var lockFile = flag.String(lockFileFlagName, "", "path to lock file")
+
+func main() {
+
+	signalCh := make(chan os.Signal, 1)
+	signal.Notify(signalCh, os.Interrupt, syscall.SIGTERM)
+
+	flag.Parse()
+	if *lockFile == "" {
+		log.Fatalf("No lockfile specified. Please run %s -%s <path to lockfile>", os.Args[0], lockFileFlagName)
+	}
+
+	fLock := flock.New(*lockFile)
+
+	locked, err := fLock.TryLock()
+	if err != nil {
+		log.Fatalf("Error locking %s: %s", *lockFile, err.Error())
+	}
+
+	if !locked {
+		log.Fatalf("Failed acquiring lock on %s", *lockFile)
+	}
+	defer fLock.Unlock()
+
+	log.Printf(AcquiredLockLogFmt, *lockFile)
+
+	s := <-signalCh
+	log.Printf("Received signal: %s, exiting", s.String())
+}

magefile.go

@@ -435,6 +435,7 @@ func getTestBinariesPath() ([]string, error) {
 		filepath.Join(wd, "pkg", "component", "fake", "component"),
 		filepath.Join(wd, "internal", "pkg", "agent", "install", "testblocking"),
 		filepath.Join(wd, "pkg", "core", "process", "testsignal"),
+		filepath.Join(wd, "internal", "pkg", "agent", "application", "upgrade", "test", "filelocker"),
 	}
 	return testBinaryPkgs, nil
 }