Skip to content

Handle scenario where agent tries to delete a file that does not exist during a config apply #1123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions internal/file/file_manager_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,19 @@ type (

fileManagerServiceInterface interface {
UpdateOverview(ctx context.Context, instanceID string, filesToUpdate []*mpi.File, iteration int) error
ConfigApply(ctx context.Context, configApplyRequest *mpi.ConfigApplyRequest) (writeStatus model.WriteStatus,
err error)
ConfigApply(
ctx context.Context,
configApplyRequest *mpi.ConfigApplyRequest,
) (writeStatus model.WriteStatus, err error)
Rollback(ctx context.Context, instanceID string) error
UpdateFile(ctx context.Context, instanceID string, fileToUpdate *mpi.File) error
ClearCache()
UpdateCurrentFilesOnDisk(ctx context.Context, updateFiles map[string]*mpi.File, referenced bool) error
DetermineFileActions(currentFiles map[string]*mpi.File, modifiedFiles map[string]*model.FileCache) (
map[string]*model.FileCache, map[string][]byte, error)
DetermineFileActions(
ctx context.Context,
currentFiles map[string]*mpi.File,
modifiedFiles map[string]*model.FileCache,
) (map[string]*model.FileCache, map[string][]byte, error)
IsConnected() bool
SetIsConnected(isConnected bool)
}
Expand Down Expand Up @@ -510,8 +515,11 @@ func (fms *FileManagerService) ConfigApply(ctx context.Context,
return model.Error, allowedErr
}

diffFiles, fileContent, compareErr := fms.DetermineFileActions(fms.currentFilesOnDisk,
ConvertToMapOfFileCache(fileOverview.GetFiles()))
diffFiles, fileContent, compareErr := fms.DetermineFileActions(
ctx,
fms.currentFilesOnDisk,
ConvertToMapOfFileCache(fileOverview.GetFiles()),
)

if compareErr != nil {
return model.Error, compareErr
Expand Down Expand Up @@ -546,7 +554,7 @@ func (fms *FileManagerService) ClearCache() {

// nolint:revive,cyclop
func (fms *FileManagerService) Rollback(ctx context.Context, instanceID string) error {
slog.InfoContext(ctx, "Rolling back config for instance", "instanceid", instanceID)
slog.InfoContext(ctx, "Rolling back config for instance", "instance_id", instanceID)

fms.filesMutex.Lock()
defer fms.filesMutex.Unlock()
Expand Down Expand Up @@ -714,6 +722,7 @@ func (fms *FileManagerService) checkAllowedDirectory(checkFiles []*mpi.File) err
// that have changed and a map of the contents for each updated and deleted file. Key to both maps is file path
// nolint: revive,cyclop,gocognit
func (fms *FileManagerService) DetermineFileActions(
ctx context.Context,
currentFiles map[string]*mpi.File,
modifiedFiles map[string]*model.FileCache,
) (
Expand All @@ -736,6 +745,7 @@ func (fms *FileManagerService) DetermineFileActions(
return nil, nil, manifestFileErr
}
}

// if file is in manifestFiles but not in modified files, file has been deleted
// copy contents, set file action
for fileName, manifestFile := range filesMap {
Expand All @@ -745,7 +755,12 @@ func (fms *FileManagerService) DetermineFileActions(
// Read file contents before marking it deleted
fileContent, readErr := os.ReadFile(fileName)
if readErr != nil {
return nil, nil, fmt.Errorf("error reading file %s: %w", fileName, readErr)
if errors.Is(readErr, os.ErrNotExist) {
slog.DebugContext(ctx, "Unable to backup file contents since file does not exist", "file", fileName)
continue
} else {
return nil, nil, fmt.Errorf("error reading file %s: %w", fileName, readErr)
}
}
fileContents[fileName] = fileContent

Expand Down
21 changes: 18 additions & 3 deletions internal/file/file_manager_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,7 @@ func TestFileManagerService_Rollback(t *testing.T) {
}

func TestFileManagerService_DetermineFileActions(t *testing.T) {
ctx := context.Background()
tempDir := os.TempDir()

deleteTestFile := helpers.CreateFileWithErrorCheck(t, tempDir, "nginx_delete.conf")
Expand All @@ -573,7 +574,6 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {

addTestFile := helpers.CreateFileWithErrorCheck(t, tempDir, "nginx_add.conf")
defer helpers.RemoveFileWithErrorCheck(t, addTestFile.Name())
t.Logf("Adding file: %s", addTestFile.Name())
addFileContent := []byte("test add file")
addErr := os.WriteFile(addTestFile.Name(), addFileContent, 0o600)
require.NoError(t, addErr)
Expand Down Expand Up @@ -683,6 +683,18 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
expectedContent: make(map[string][]byte),
expectedError: nil,
},
{
name: "Test 3: File being deleted already doesn't exist",
modifiedFiles: make(map[string]*model.FileCache),
currentFiles: map[string]*mpi.File{
"/unknown/file.conf": {
FileMeta: protos.FileMeta("/unknown/file.conf", files.GenerateHash(fileContent)),
},
},
expectedCache: make(map[string]*model.FileCache),
expectedContent: make(map[string][]byte),
expectedError: nil,
},
}

for _, test := range tests {
Expand All @@ -697,8 +709,11 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig())
require.NoError(tt, err)

diff, contents, fileActionErr := fileManagerService.DetermineFileActions(test.currentFiles,
test.modifiedFiles)
diff, contents, fileActionErr := fileManagerService.DetermineFileActions(
ctx,
test.currentFiles,
test.modifiedFiles,
)
require.NoError(tt, fileActionErr)
assert.Equal(tt, test.expectedContent, contents)
assert.Equal(tt, test.expectedCache, diff)
Expand Down
26 changes: 14 additions & 12 deletions internal/file/filefakes/fake_file_manager_service_interface.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.