Skip to content

Commit 810d106

Browse files
aphralGaphralG
committed
PR feedback
1 parent 4c2a20a commit 810d106

File tree

6 files changed

+79
-70
lines changed

6 files changed

+79
-70
lines changed

‎internal/file/file_manager_service.go‎

Lines changed: 64 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ type (
7171
fileToUpdate *mpi.File,
7272
) error
7373
SetIsConnected(isConnected bool)
74-
MoveFileFromTempDirectory(ctx context.Context, fileAction *model.FileCache, tempDir string) error
74+
renameFile(ctx context.Context, hash, fileName, tempDir string) error
7575
UpdateClient(ctx context.Context, fileServiceClient mpi.FileServiceClient)
7676
}
7777

@@ -191,7 +191,7 @@ func (fms *FileManagerService) ConfigApply(ctx context.Context,
191191
return model.Error, rollbackTempErr
192192
}
193193

194-
rollbackTempFilesErr := fms.RollbackTempFiles(ctx)
194+
rollbackTempFilesErr := fms.backupFiles(ctx)
195195
if rollbackTempFilesErr != nil {
196196
return model.Error, rollbackTempFilesErr
197197
}
@@ -211,47 +211,19 @@ func (fms *FileManagerService) ConfigApply(ctx context.Context,
211211
return model.OK, nil
212212
}
213213

214-
func (fms *FileManagerService) RollbackTempFiles(ctx context.Context) error {
215-
for _, file := range fms.fileActions {
216-
if file.Action == model.Add || file.Action == model.Unchanged {
217-
continue
218-
}
219-
220-
filePath := file.File.GetFileMeta().GetName()
221-
222-
if _, err := os.Stat(filePath); os.IsNotExist(err) {
223-
slog.DebugContext(ctx, "Unable to backup file content since file does not exist",
224-
"file", filePath)
225-
226-
continue
227-
}
228-
229-
tempFilePath := filepath.Join(fms.tempRollbackDir, filePath)
230-
slog.DebugContext(ctx, "Attempting to backup file content since file exists", "temp_path", tempFilePath)
231-
232-
moveErr := fms.fileOperator.MoveFile(ctx, filePath, tempFilePath)
233-
234-
if moveErr != nil {
235-
return moveErr
236-
}
237-
}
238-
239-
return nil
240-
}
241-
242214
func (fms *FileManagerService) ClearCache() {
243215
slog.Debug("Clearing cache and temp files after config apply")
244216
clear(fms.fileActions)
245217
clear(fms.previousManifestFiles)
246218

247219
configErr := os.RemoveAll(fms.tempConfigDir)
248220
if configErr != nil {
249-
slog.Error("error removing temp config directory", "path", fms.tempConfigDir, "err", configErr)
221+
slog.Error("Error removing temp config directory", "path", fms.tempConfigDir, "err", configErr)
250222
}
251223

252224
rollbackErr := os.RemoveAll(fms.tempRollbackDir)
253225
if rollbackErr != nil {
254-
slog.Error("error removing temp rollback directory", "path", fms.tempRollbackDir, "err", rollbackErr)
226+
slog.Error("Error removing temp rollback directory", "path", fms.tempRollbackDir, "err", rollbackErr)
255227
}
256228
}
257229

@@ -273,30 +245,14 @@ func (fms *FileManagerService) Rollback(ctx context.Context, instanceID string)
273245

274246
continue
275247
case model.Delete, model.Update:
276-
fileMeta := fileAction.File.GetFileMeta()
277-
fileName := fileMeta.GetName()
278-
279-
tempFilePath := filepath.Join(fms.tempRollbackDir, fileName)
280-
281-
// Create parent directories for the target file if they don't exist
282-
if err := os.MkdirAll(filepath.Dir(fileName), dirPerm); err != nil {
283-
return fmt.Errorf("failed to create directories for %s: %w", fileName, err)
284-
}
285-
286-
moveErr := os.Rename(tempFilePath, fileName)
287-
if moveErr != nil {
288-
return fmt.Errorf("failed to rename file, %s to %s: %w", tempFilePath, fileName, moveErr)
289-
}
290-
291-
content, readErr := os.ReadFile(fileMeta.GetName())
292-
if readErr != nil {
293-
return fmt.Errorf("error reading file, unable to generate hash: %s error: %w",
294-
fileMeta.GetName(), readErr)
248+
content, err := fms.restoreFiles(fileAction)
249+
if err != nil {
250+
return err
295251
}
296252

297253
// currentFilesOnDisk needs to be updated after rollback action is performed
298-
fileMeta.Hash = files.GenerateHash(content)
299-
fms.currentFilesOnDisk[fileMeta.GetName()] = fileAction.File
254+
fileAction.File.FileMeta.Hash = files.GenerateHash(content)
255+
fms.currentFilesOnDisk[fileAction.File.GetFileMeta().GetName()] = fileAction.File
300256
case model.Unchanged:
301257
fallthrough
302258
default:
@@ -513,6 +469,59 @@ func (fms *FileManagerService) UpdateManifestFile(ctx context.Context,
513469
return fms.fileOperator.WriteManifestFile(ctx, updatedFiles, fms.agentConfig.LibDir, fms.manifestFilePath)
514470
}
515471

472+
func (fms *FileManagerService) backupFiles(ctx context.Context) error {
473+
for _, file := range fms.fileActions {
474+
if file.Action == model.Add || file.Action == model.Unchanged {
475+
continue
476+
}
477+
478+
filePath := file.File.GetFileMeta().GetName()
479+
480+
if _, err := os.Stat(filePath); os.IsNotExist(err) {
481+
slog.DebugContext(ctx, "Unable to backup file content since file does not exist",
482+
"file", filePath)
483+
484+
continue
485+
}
486+
487+
tempFilePath := filepath.Join(fms.tempRollbackDir, filePath)
488+
slog.DebugContext(ctx, "Attempting to backup file content since file exists", "temp_path", tempFilePath)
489+
490+
moveErr := fms.fileOperator.MoveFile(ctx, filePath, tempFilePath)
491+
492+
if moveErr != nil {
493+
return moveErr
494+
}
495+
}
496+
497+
return nil
498+
}
499+
500+
func (fms *FileManagerService) restoreFiles(fileAction *model.FileCache) ([]byte, error) {
501+
fileMeta := fileAction.File.GetFileMeta()
502+
fileName := fileMeta.GetName()
503+
504+
tempFilePath := filepath.Join(fms.tempRollbackDir, fileName)
505+
506+
// Create parent directories for the target file if they don't exist
507+
if err := os.MkdirAll(filepath.Dir(fileName), dirPerm); err != nil {
508+
return nil, fmt.Errorf("failed to create directories for %s: %w", fileName, err)
509+
}
510+
511+
moveErr := os.Rename(tempFilePath, fileName)
512+
if moveErr != nil {
513+
return nil, fmt.Errorf("failed to rename file, %s to %s: %w", tempFilePath, fileName, moveErr)
514+
}
515+
516+
content, readErr := os.ReadFile(fileMeta.GetName())
517+
if readErr != nil {
518+
return nil, fmt.Errorf("error reading file, unable to generate hash: %s error: %w",
519+
fileMeta.GetName(), readErr)
520+
}
521+
522+
return content, nil
523+
}
524+
516525
func (fms *FileManagerService) manifestFile() (map[string]*model.ManifestFile, map[string]*mpi.File, error) {
517526
if _, err := os.Stat(fms.manifestFilePath); err != nil {
518527
return nil, nil, err
@@ -596,7 +605,8 @@ actionsLoop:
596605

597606
continue
598607
case model.Add, model.Update:
599-
err := fms.fileServiceOperator.MoveFileFromTempDirectory(ctx, fileAction, tempDir)
608+
fileMeta := fileAction.File.GetFileMeta()
609+
err := fms.fileServiceOperator.renameFile(ctx, fileMeta.GetHash(), fileMeta.GetName(), tempDir)
600610
if err != nil {
601611
actionError = err
602612

‎internal/file/file_plugin.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -138,12 +138,10 @@ func (fp *FilePlugin) Subscriptions() []string {
138138
}
139139
}
140140

141-
func (fp *FilePlugin) CleanUpConfigApply(ctx context.Context,
141+
func (fp *FilePlugin) enableWatchers(ctx context.Context,
142142
configContext *model.NginxConfigContext,
143143
instanceID string,
144144
) {
145-
fp.fileManagerService.ClearCache()
146-
147145
enableWatcher := &model.EnableWatchers{
148146
ConfigContext: configContext,
149147
InstanceID: instanceID,
@@ -183,7 +181,8 @@ func (fp *FilePlugin) handleConfigApplyComplete(ctx context.Context, msg *bus.Me
183181
}
184182

185183
fp.messagePipe.Process(ctx, &bus.Message{Topic: bus.DataPlaneResponseTopic, Data: response})
186-
fp.CleanUpConfigApply(ctx, &model.NginxConfigContext{}, response.GetInstanceId())
184+
fp.fileManagerService.ClearCache()
185+
fp.enableWatchers(ctx, &model.NginxConfigContext{}, response.GetInstanceId())
187186
}
188187

189188
func (fp *FilePlugin) handleReloadSuccess(ctx context.Context, msg *bus.Message) {
@@ -196,7 +195,8 @@ func (fp *FilePlugin) handleReloadSuccess(ctx context.Context, msg *bus.Message)
196195
return
197196
}
198197

199-
fp.CleanUpConfigApply(ctx, successMessage.ConfigContext, successMessage.DataPlaneResponse.GetInstanceId())
198+
fp.fileManagerService.ClearCache()
199+
fp.enableWatchers(ctx, successMessage.ConfigContext, successMessage.DataPlaneResponse.GetInstanceId())
200200

201201
if successMessage.ConfigContext.Files != nil {
202202
slog.DebugContext(ctx, "Changes made during config apply, update files on disk")

‎internal/file/file_service_operator.go‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
"github.com/nginx/agent/v3/internal/config"
2626
internalgrpc "github.com/nginx/agent/v3/internal/grpc"
2727
"github.com/nginx/agent/v3/internal/logger"
28-
"github.com/nginx/agent/v3/internal/model"
2928
"github.com/nginx/agent/v3/pkg/files"
3029
"github.com/nginx/agent/v3/pkg/id"
3130
"google.golang.org/grpc"
@@ -277,12 +276,12 @@ func (fso *FileServiceOperator) UpdateFile(
277276
return fso.sendUpdateFileStream(ctx, fileToUpdate, fso.agentConfig.Client.Grpc.FileChunkSize)
278277
}
279278

280-
func (fso *FileServiceOperator) MoveFileFromTempDirectory(
281-
ctx context.Context, fileAction *model.FileCache, tempDir string,
279+
// renameFile, renames (moves) file from tempDir to new location to update file.
280+
func (fso *FileServiceOperator) renameFile(
281+
ctx context.Context, hash, fileName, dir string,
282282
) error {
283-
fileName := fileAction.File.GetFileMeta().GetName()
284-
slog.DebugContext(ctx, "Updating file", "file", fileName)
285-
tempFilePath := filepath.Join(tempDir, fileName)
283+
slog.DebugContext(ctx, "Renaming file", "file", fileName)
284+
tempFilePath := filepath.Join(dir, fileName)
286285

287286
// Create parent directories for the target file if they don't exist
288287
if err := os.MkdirAll(filepath.Dir(fileName), dirPerm); err != nil {
@@ -294,7 +293,7 @@ func (fso *FileServiceOperator) MoveFileFromTempDirectory(
294293
return fmt.Errorf("failed to rename file: %w", moveErr)
295294
}
296295

297-
return fso.validateFileHash(fileName, fileAction.File.GetFileMeta().GetHash())
296+
return fso.validateFileHash(fileName, hash)
298297
}
299298

300299
func (fso *FileServiceOperator) validateFileHash(filePath, expectedHash string) error {

‎internal/watcher/file/file_watcher_service.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ func (fws *FileWatcherService) Watch(ctx context.Context, ch chan<- FileUpdateMe
9898
}
9999

100100
func (fws *FileWatcherService) DisableWatcher(ctx context.Context) {
101+
slog.DebugContext(ctx, "Disabling file watcher")
101102
if fws.watcher != nil && fws.watcher.WatchList() != nil {
102103
paths := fws.watcher.WatchList()
103104
slog.DebugContext(ctx, "Removing watchers", "paths", paths)
@@ -112,6 +113,7 @@ func (fws *FileWatcherService) DisableWatcher(ctx context.Context) {
112113
}
113114

114115
func (fws *FileWatcherService) EnableWatcher(ctx context.Context) {
116+
slog.DebugContext(ctx, "Enabling file watcher")
115117
if fws.watcher != nil && fws.watcher.WatchList() != nil && len(fws.watcher.WatchList()) == 0 {
116118
fws.addWatchers(ctx)
117119
}

‎internal/watcher/file/file_watcher_service_test.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ package file
88
import (
99
"bytes"
1010
"context"
11-
"log/slog"
1211
"os"
1312
"path"
1413
"path/filepath"
@@ -266,8 +265,7 @@ func TestFileWatcherService_Watch(t *testing.T) {
266265
defer os.Remove(skippableFile.Name())
267266

268267
select {
269-
case file := <-channel:
270-
slog.Info("Skippable file updated", "", file)
268+
case <-channel:
271269
t.Fatalf("Expected file to be skipped: %v", skippableFile.Name())
272270
case <-time.After(150 * time.Millisecond):
273271
return

‎internal/watcher/watcher_plugin.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ func (w *Watcher) handleEnableWatchers(ctx context.Context, msg *bus.Message) {
161161
slog.DebugContext(ctx, "Watcher plugin received enable watchers message")
162162
enableWatchersMessage, ok := msg.Data.(*model.EnableWatchers)
163163
if !ok {
164-
slog.ErrorContext(ctx, "Unable to cast message payload to *model.EnableWatchers", "payload",
164+
slog.ErrorContext(ctx, "Unable to cast message payload to *model.enableWatchers", "payload",
165165
msg.Data, "topic", msg.Topic)
166166

167167
return

0 commit comments

Comments
 (0)