Fixed file watcher not disabling correctly

Author: aphralG

internal/file/file_manager_service.go

@@ -102,8 +102,6 @@ type FileManagerService struct {
 	fileServiceOperator fileServiceOperatorInterface
 	// map of files and the actions performed on them during config apply
 	fileActions map[string]*model.FileCache // key is file path
-	// map of the contents of files which have been updated or deleted during config apply, used during rollback
-	rollbackFileContents map[string][]byte // key is file path
 	// map of the files currently on disk, used to determine the file action during config apply
 	currentFilesOnDisk    map[string]*mpi.File // key is file path
 	previousManifestFiles map[string]*model.ManifestFile
@@ -123,7 +121,6 @@ func NewFileManagerService(fileServiceClient mpi.FileServiceClient, agentConfig
 		fileOperator:          NewFileOperator(manifestLock),
 		fileServiceOperator:   NewFileServiceOperator(agentConfig, fileServiceClient, manifestLock),
 		fileActions:           make(map[string]*model.FileCache),
-		rollbackFileContents:  make(map[string][]byte),
 		currentFilesOnDisk:    make(map[string]*mpi.File),
 		previousManifestFiles: make(map[string]*model.ManifestFile),
 		rollbackManifest:      true,
@@ -182,15 +179,14 @@ func (fms *FileManagerService) ConfigApply(ctx context.Context,
 		return model.NoChange, nil
 	}
 
-	// fms.rollbackFileContents = fileContent
 	fms.fileActions = diffFiles
 
-	fms.tempConfigDir, configTempErr = fms.createTempConfigDirectory(ctx, "config")
+	fms.tempConfigDir, configTempErr = fms.createTempConfigDirectory("config")
 	if configTempErr != nil {
 		return model.Error, configTempErr
 	}
 
-	fms.tempRollbackDir, rollbackTempErr = fms.createTempConfigDirectory(ctx, "rollback")
+	fms.tempRollbackDir, rollbackTempErr = fms.createTempConfigDirectory("rollback")
 	if rollbackTempErr != nil {
 		return model.Error, rollbackTempErr
 	}
@@ -245,7 +241,6 @@ func (fms *FileManagerService) RollbackTempFiles(ctx context.Context) error {
 
 func (fms *FileManagerService) ClearCache() {
 	slog.Debug("Clearing cache and temp files after config apply")
-	clear(fms.rollbackFileContents)
 	clear(fms.fileActions)
 	clear(fms.previousManifestFiles)
 
@@ -258,8 +253,6 @@ func (fms *FileManagerService) ClearCache() {
 	if rollbackErr != nil {
 		slog.Error("error removing temp rollback directory", "path", fms.tempRollbackDir, "err", rollbackErr)
 	}
-
-	slog.Info("Cleaned up temp files")
 }
 
 //nolint:revive,cyclop // cognitive-complexity of 13 max is 12, loop is needed cant be broken up
@@ -290,7 +283,6 @@ func (fms *FileManagerService) Rollback(ctx context.Context, instanceID string)
 				return fmt.Errorf("failed to create directories for %s: %w", fileName, err)
 			}
 
-			slog.InfoContext(ctx, "Moving files during rollback")
 			moveErr := os.Rename(tempFilePath, fileName)
 			if moveErr != nil {
 				return fmt.Errorf("failed to rename file, %s to %s: %w", tempFilePath, fileName, moveErr)
@@ -407,6 +399,11 @@ func (fms *FileManagerService) DetermineFileActions(
 	for fileName, manifestFile := range filesMap {
 		_, exists := modifiedFiles[fileName]
 
+		if _, err := os.Stat(fileName); os.IsNotExist(err) {
+			slog.DebugContext(ctx, "File already deleted, skipping", "file", fileName)
+			continue
+		}
+
 		if !exists {
 			fileDiff[fileName] = &model.FileCache{
 				File:   manifestFile,
@@ -697,17 +694,11 @@ func (fms *FileManagerService) convertToFile(manifestFile *model.ManifestFile) *
 	}
 }
 
-func (fms *FileManagerService) createTempConfigDirectory(ctx context.Context, pattern string) (string, error) {
+func (fms *FileManagerService) createTempConfigDirectory(pattern string) (string, error) {
 	tempDir, tempDirError := os.MkdirTemp(fms.configPath, pattern)
 	if tempDirError != nil {
 		return "", fmt.Errorf("failed creating temp config directory: %w", tempDirError)
 	}
-	defer func(path string) {
-		err := os.RemoveAll(path)
-		if err != nil {
-			slog.ErrorContext(ctx, "error removing temp config directory", "path", path, "err", err)
-		}
-	}(tempDir)
 
 	return tempDir, nil
 }

internal/file/file_manager_service_test.go

@@ -59,6 +59,7 @@ func TestFileManagerService_ConfigApply_Add(t *testing.T) {
 	agentConfig.AllowedDirectories = []string{tempDir}
 
 	fileManagerService := NewFileManagerService(fakeFileServiceClient, agentConfig, &sync.RWMutex{})
+	fileManagerService.configPath = filepath.Dir(filePath)
 	fileManagerService.agentConfig.LibDir = manifestDirPath
 	fileManagerService.manifestFilePath = manifestFilePath
 
@@ -108,6 +109,7 @@ func TestFileManagerService_ConfigApply_Add_LargeFile(t *testing.T) {
 	agentConfig.AllowedDirectories = []string{tempDir}
 	fileManagerService := NewFileManagerService(fakeFileServiceClient, agentConfig, &sync.RWMutex{})
 	fileManagerService.agentConfig.LibDir = manifestDirPath
+	fileManagerService.configPath = filepath.Dir(filePath)
 	fileManagerService.manifestFilePath = manifestFilePath
 
 	request := protos.CreateConfigApplyRequest(overview)
@@ -168,6 +170,7 @@ func TestFileManagerService_ConfigApply_Update(t *testing.T) {
 
 	fileManagerService := NewFileManagerService(fakeFileServiceClient, agentConfig, &sync.RWMutex{})
 	fileManagerService.agentConfig.LibDir = manifestDirPath
+	fileManagerService.configPath = filepath.Dir(tempFile.Name())
 	fileManagerService.manifestFilePath = manifestFilePath
 	err := fileManagerService.UpdateCurrentFilesOnDisk(ctx, filesOnDisk, false)
 	require.NoError(t, err)
@@ -179,7 +182,11 @@ func TestFileManagerService_ConfigApply_Update(t *testing.T) {
 	data, readErr := os.ReadFile(tempFile.Name())
 	require.NoError(t, readErr)
 	assert.Equal(t, fileContent, data)
-	assert.Equal(t, fileManagerService.rollbackFileContents[tempFile.Name()], previousFileContent)
+
+	content, err := os.ReadFile(fileManagerService.tempRollbackDir + tempFile.Name())
+	require.NoError(t, err)
+	assert.Equal(t, previousFileContent, content)
+
 	assert.Equal(t, fileManagerService.fileActions[tempFile.Name()].File, overview.GetFiles()[0])
 	assert.True(t, fileManagerService.rollbackManifest)
 }
@@ -219,6 +226,7 @@ func TestFileManagerService_ConfigApply_Delete(t *testing.T) {
 	fileManagerService := NewFileManagerService(fakeFileServiceClient, agentConfig, &sync.RWMutex{})
 	fileManagerService.agentConfig.LibDir = manifestDirPath
 	fileManagerService.manifestFilePath = manifestFilePath
+	fileManagerService.configPath = filepath.Dir(tempFile.Name())
 	err := fileManagerService.UpdateCurrentFilesOnDisk(ctx, filesOnDisk, false)
 	require.NoError(t, err)
 
@@ -236,7 +244,11 @@ func TestFileManagerService_ConfigApply_Delete(t *testing.T) {
 	writeStatus, err := fileManagerService.ConfigApply(ctx, request)
 	require.NoError(t, err)
 	assert.NoFileExists(t, tempFile.Name())
-	assert.Equal(t, fileManagerService.rollbackFileContents[tempFile.Name()], fileContent)
+
+	content, err := os.ReadFile(fileManagerService.tempRollbackDir + tempFile.Name())
+	require.NoError(t, err)
+	assert.Equal(t, fileContent, content)
+
 	assert.Equal(t,
 		fileManagerService.fileActions[tempFile.Name()].File.GetFileMeta().GetName(),
 		filesOnDisk[tempFile.Name()].GetFileMeta().GetName(),
@@ -278,6 +290,7 @@ func TestFileManagerService_ConfigApply_Failed(t *testing.T) {
 
 	fileManagerService := NewFileManagerService(fakeFileServiceClient, agentConfig, &sync.RWMutex{})
 	fileManagerService.agentConfig.LibDir = manifestDirPath
+	fileManagerService.configPath = filepath.Dir(filePath)
 	fileManagerService.manifestFilePath = manifestFilePath
 
 	request := protos.CreateConfigApplyRequest(overview)
@@ -322,9 +335,18 @@ func TestFileManagerService_checkAllowedDirectory(t *testing.T) {
 	require.Error(t, err)
 }
 
+//nolint:usetesting // need to use MkDirTemp instead of t.tempDir for rollback as t.tempDir does not accept a pattern
 func TestFileManagerService_ClearCache(t *testing.T) {
+	tempDir := t.TempDir()
+	rollbackDir, err := os.MkdirTemp(tempDir, "rollback")
+	require.NoError(t, err)
+	configDir, err := os.MkdirTemp(tempDir, "config")
+	require.NoError(t, err)
+
 	fakeFileServiceClient := &v1fakes.FakeFileServiceClient{}
 	fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{})
+	fileManagerService.tempConfigDir = configDir
+	fileManagerService.tempRollbackDir = rollbackDir
 
 	filesCache := map[string]*model.FileCache{
 		"file/path/test.conf": {
@@ -340,25 +362,27 @@ func TestFileManagerService_ClearCache(t *testing.T) {
 		},
 	}
 
-	contentsCache := map[string][]byte{
-		"file/path/test.conf": []byte("some test data"),
-	}
-
 	fileManagerService.fileActions = filesCache
-	fileManagerService.rollbackFileContents = contentsCache
 	assert.NotEmpty(t, fileManagerService.fileActions)
-	assert.NotEmpty(t, fileManagerService.rollbackFileContents)
 
 	fileManagerService.ClearCache()
 
 	assert.Empty(t, fileManagerService.fileActions)
-	assert.Empty(t, fileManagerService.rollbackFileContents)
+
+	_, statErr := os.Stat(fileManagerService.tempRollbackDir)
+	assert.True(t, os.IsNotExist(statErr))
+	_, statConfigErr := os.Stat(fileManagerService.tempConfigDir)
+	assert.True(t, os.IsNotExist(statConfigErr))
 }
 
+//nolint:usetesting // need to use MkDirTemp instead of t.tempDir for rollback as t.tempDir does not accept a pattern
 func TestFileManagerService_Rollback(t *testing.T) {
 	ctx := context.Background()
 	tempDir := t.TempDir()
 
+	rollbackDir, mkdirErr := os.MkdirTemp(tempDir, "rollback")
+	require.NoError(t, mkdirErr)
+
 	deleteFilePath := filepath.Join(tempDir, "nginx_delete.conf")
 
 	newFileContent := []byte("location /test {\n    return 200 \"This config needs to be rolled back\\n\";\n}")
@@ -373,6 +397,25 @@ func TestFileManagerService_Rollback(t *testing.T) {
 	_, writeErr = updateFile.Write(newFileContent)
 	require.NoError(t, writeErr)
 
+	helpers.CreateDirWithErrorCheck(t, rollbackDir+tempDir)
+
+	tempAddFile, createErr := os.Create(rollbackDir + addFile.Name())
+	require.NoError(t, createErr)
+	_, writeErr = tempAddFile.Write(oldFileContent)
+	require.NoError(t, writeErr)
+
+	tempUpdateFile, createErr := os.Create(rollbackDir + updateFile.Name())
+	require.NoError(t, createErr)
+	_, writeErr = tempUpdateFile.Write(oldFileContent)
+	require.NoError(t, writeErr)
+	t.Log(tempUpdateFile.Name())
+
+	tempDeleteFile, createErr := os.Create(rollbackDir + tempDir + "/nginx_delete.conf")
+	require.NoError(t, createErr)
+	_, writeErr = tempDeleteFile.Write(oldFileContent)
+	require.NoError(t, writeErr)
+	t.Log(tempDeleteFile.Name())
+
 	manifestDirPath := tempDir
 	manifestFilePath := manifestDirPath + "/manifest.json"
 	helpers.CreateFileWithErrorCheck(t, manifestDirPath, "manifest.json")
@@ -430,17 +473,14 @@ func TestFileManagerService_Rollback(t *testing.T) {
 			},
 		},
 	}
-	fileContentCache := map[string][]byte{
-		deleteFilePath:    oldFileContent,
-		updateFile.Name(): oldFileContent,
-	}
 
 	instanceID := protos.NginxOssInstance([]string{}).GetInstanceMeta().GetInstanceId()
 	fakeFileServiceClient := &v1fakes.FakeFileServiceClient{}
 	fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{})
-	fileManagerService.rollbackFileContents = fileContentCache
 	fileManagerService.fileActions = filesCache
 	fileManagerService.agentConfig.LibDir = manifestDirPath
+	fileManagerService.tempRollbackDir = rollbackDir
+	fileManagerService.configPath = filepath.Dir(updateFile.Name())
 	fileManagerService.manifestFilePath = manifestFilePath
 
 	err := fileManagerService.Rollback(ctx, instanceID)
@@ -622,6 +662,7 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
 			fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{})
 			fileManagerService.agentConfig.LibDir = manifestDirPath
 			fileManagerService.manifestFilePath = manifestFilePath
+			fileManagerService.configPath = filepath.Dir(updateTestFile.Name())
 
 			require.NoError(tt, err)
 
@@ -1013,20 +1054,22 @@ func TestFileManagerService_deleteTempFiles(t *testing.T) {
 
 func TestFileManagerService_createTempConfigDirectory(t *testing.T) {
 	agentConfig := types.AgentConfig()
-	agentConfig.LibDir = t.TempDir()
+	tempDir := t.TempDir()
+	configPath := tempDir
 
 	fileManagerService := FileManagerService{
 		agentConfig: agentConfig,
+		configPath:  configPath,
 	}
 
-	dir, err := fileManagerService.createTempConfigDirectory(t.Context(), "config")
+	dir, err := fileManagerService.createTempConfigDirectory("config")
 	assert.NotEmpty(t, dir)
 	require.NoError(t, err)
 
 	// Test for unknown directory path
-	agentConfig.LibDir = "/unknown/"
+	fileManagerService.configPath = "/unknown/"
 
-	dir, err = fileManagerService.createTempConfigDirectory(t.Context(), "config")
+	dir, err = fileManagerService.createTempConfigDirectory("config")
 	assert.Empty(t, dir)
 	require.Error(t, err)
 }

internal/file/file_operator_test.go

@@ -68,7 +68,6 @@ func TestFileOperator_MoveFile_fileExists(t *testing.T) {
 	err = fileOperator.MoveFile(t.Context(), tempFile, newFile)
 	require.NoError(t, err)
 
-	assert.NoFileExists(t, tempFile)
 	assert.FileExists(t, newFile)
 }
 

internal/file/file_plugin.go

@@ -144,7 +144,6 @@ func (fp *FilePlugin) CleanUpConfigApply(ctx context.Context,
 ) {
 	fp.fileManagerService.ClearCache()
 
-	slog.InfoContext(ctx, "Cleaned up temp files")
 	enableWatcher := &model.EnableWatchers{
 		ConfigContext: configContext,
 		InstanceID:    instanceID,
@@ -188,7 +187,7 @@ func (fp *FilePlugin) handleConfigApplyComplete(ctx context.Context, msg *bus.Me
 }
 
 func (fp *FilePlugin) handleReloadSuccess(ctx context.Context, msg *bus.Message) {
-	slog.InfoContext(ctx, "File plugin received reload success message", "data", msg.Data)
+	slog.DebugContext(ctx, "File plugin received reload success message", "data", msg.Data)
 
 	successMessage, ok := msg.Data.(*model.ReloadSuccess)
 
@@ -210,7 +209,6 @@ func (fp *FilePlugin) handleReloadSuccess(ctx context.Context, msg *bus.Message)
 			slog.ErrorContext(ctx, "Unable to update current files on disk", "error", updateError)
 		}
 	}
-
 	fp.messagePipe.Process(ctx, &bus.Message{Topic: bus.DataPlaneResponseTopic, Data: successMessage.DataPlaneResponse})
 }
 

internal/watcher/credentials/credential_watcher_service.go

@@ -146,7 +146,7 @@ func (cws *CredentialWatcherService) handleEvent(ctx context.Context, event fsno
 		return
 	}
 
-	slog.InfoContext(ctx, "Credential Processing FSNotify event", "event", event)
+	slog.DebugContext(ctx, "Processing FSNotify event", "event", event)
 
 	switch {
 	case event.Has(fsnotify.Remove):

internal/watcher/file/file_watcher_service.go

@@ -89,7 +89,6 @@ func (fws *FileWatcherService) Watch(ctx context.Context, ch chan<- FileUpdateMe
 		if fws.watcher != nil {
 			select {
 			case event := <-fws.watcher.Events:
-				slog.InfoContext(ctx, "--------- Event received", "event", event)
 				fws.handleEvent(ctx, event)
 			case watcherError := <-fws.watcher.Errors:
 				slog.ErrorContext(ctx, "Unexpected error in file watcher", "error", watcherError)
@@ -98,8 +97,25 @@ func (fws *FileWatcherService) Watch(ctx context.Context, ch chan<- FileUpdateMe
 	}
 }
 
-func (fws *FileWatcherService) SetEnabled(enabled bool) {
-	fws.enabled.Store(enabled)
+func (fws *FileWatcherService) DisableWatcher(ctx context.Context) {
+	if fws.watcher != nil && fws.watcher.WatchList() != nil {
+		paths := fws.watcher.WatchList()
+		slog.DebugContext(ctx, "Removing watchers", "paths", paths)
+		for _, filePath := range paths {
+			err := fws.watcher.Remove(filePath)
+			if err != nil {
+				slog.ErrorContext(ctx, "Unable to remove watcher file", "path", filePath, "error", err)
+			}
+		}
+	}
+	fws.enabled.Store(false)
+}
+
+func (fws *FileWatcherService) EnableWatcher(ctx context.Context) {
+	if fws.watcher != nil && fws.watcher.WatchList() != nil && len(fws.watcher.WatchList()) == 0 {
+		fws.addWatchers(ctx)
+	}
+	fws.enabled.Store(true)
 }
 
 func (fws *FileWatcherService) Update(ctx context.Context, nginxConfigContext *model.NginxConfigContext) {
@@ -173,16 +189,13 @@ func (fws *FileWatcherService) removeWatchers(ctx context.Context) {
 }
 
 func (fws *FileWatcherService) handleEvent(ctx context.Context, event fsnotify.Event) {
-	slog.InfoContext(ctx, "Is enabled", "bool", fws.enabled.Load())
 	if fws.enabled.Load() {
 		if fws.isEventSkippable(event) {
 			return
 		}
 
-		slog.InfoContext(ctx, "Processing FSNotify event", "event", event)
+		slog.DebugContext(ctx, "Processing FSNotify event", "event", event)
 		fws.filesChanged.Store(true)
-	} else {
-		slog.InfoContext(ctx, "Ignoring FSNotify event", "event", event)
 	}
 }