-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: Fix the problem of recovering application and website task logs #8281
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -204,7 +204,7 @@ func handleAppRecover(install *model.AppInstall, parentTask *task.Task, recoverF | |
| fileOp := files.NewFileOp() | ||
| if !isRollback { | ||
| rollbackFile = path.Join(global.Dir.TmpDir, fmt.Sprintf("app/%s_%s.tar.gz", install.Name, time.Now().Format(constant.DateTimeSlimLayout))) | ||
| if err := handleAppBackup(install, nil, path.Dir(rollbackFile), path.Base(rollbackFile), "", "", ""); err != nil { | ||
| if err := handleAppBackup(install, recoverTask, path.Dir(rollbackFile), path.Base(rollbackFile), "", "", taskID); err != nil { | ||
| t.Log(fmt.Sprintf("backup app %s for rollback before recover failed, err: %v", install.Name, err)) | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The change is minor and does not introduce any significant irregularities or potential issues. However, an optimization suggestion is needed here: Before the backup handling, it might be more efficient to add a check for the existence of func handleAppRecover(...)
if recoverTask == nil {
return // or return an appropriate error if necessary
}
if !isRollback {
rollbackFile = path.Join(global.Dir.TmpDir, fmt.Sprintf("app/%s_%s.tar.gz", install.Name, time.Now().Format(constant.DateTimeSlimLayout)))
if err := handleAppBackup(install, recoverTask, path.Dir(rollbackFile), path.Base(rollbackFile), "", "", taskID); err != nil {
t.Log(fmt.Sprintf("backup app %s for rollback before recover failed, err: %v", install.Name, err))
}
} else {
}This ensures that only the backup logic runs if |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,7 @@ func (u *BackupService) WebsiteBackup(req dto.CommonBackup) error { | |
| fileName := fmt.Sprintf("%s_%s.tar.gz", website.Alias, timeNow+common.RandStrAndNum(5)) | ||
|
|
||
| go func() { | ||
| if err = handleWebsiteBackup(&website, backupDir, fileName, "", req.Secret, req.TaskID); err != nil { | ||
| if err = handleWebsiteBackup(&website, nil, backupDir, fileName, "", req.Secret, req.TaskID); err != nil { | ||
| global.LOG.Errorf("backup website %s failed, err: %v", website.Alias, err) | ||
| return | ||
| } | ||
|
|
@@ -82,7 +82,7 @@ func handleWebsiteRecover(website *model.Website, recoverFile string, isRollback | |
| isOk := false | ||
| if !isRollback { | ||
| rollbackFile := path.Join(global.Dir.TmpDir, fmt.Sprintf("website/%s_%s.tar.gz", website.Alias, time.Now().Format(constant.DateTimeSlimLayout))) | ||
| if err := handleWebsiteBackup(website, path.Dir(rollbackFile), path.Base(rollbackFile), "", "", ""); err != nil { | ||
| if err := handleWebsiteBackup(website, recoverTask, path.Dir(rollbackFile), path.Base(rollbackFile), "", "", taskID); err != nil { | ||
| return fmt.Errorf("backup website %s for rollback before recover failed, err: %v", website.Alias, err) | ||
| } | ||
| defer func() { | ||
|
|
@@ -202,12 +202,20 @@ func handleWebsiteRecover(website *model.Website, recoverFile string, isRollback | |
| return recoverTask.Execute() | ||
| } | ||
|
|
||
| func handleWebsiteBackup(website *model.Website, backupDir, fileName, excludes, secret, taskID string) error { | ||
| backupTask, err := task.NewTaskWithOps(website.Alias, task.TaskBackup, task.TaskScopeWebsite, taskID, website.ID) | ||
| if err != nil { | ||
| return err | ||
| func handleWebsiteBackup(website *model.Website, parentTask *task.Task, backupDir, fileName, excludes, secret, taskID string) error { | ||
| var ( | ||
| err error | ||
| backupTask *task.Task | ||
| ) | ||
| backupTask = parentTask | ||
| if parentTask == nil { | ||
| backupTask, err = task.NewTaskWithOps(website.Alias, task.TaskBackup, task.TaskScopeWebsite, taskID, website.ID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| backupTask.AddSubTask(task.GetTaskName(website.Alias, task.TaskBackup, task.TaskScopeWebsite), func(t *task.Task) error { | ||
|
|
||
| backupWebsite := func(t *task.Task) error { | ||
| fileOp := files.NewFileOp() | ||
| tmpDir := fmt.Sprintf("%s/%s", backupDir, strings.ReplaceAll(fileName, ".tar.gz", "")) | ||
| if !fileOp.Stat(tmpDir) { | ||
|
|
@@ -273,7 +281,11 @@ func handleWebsiteBackup(website *model.Website, backupDir, fileName, excludes, | |
| } | ||
| t.Log(i18n.GetWithName("CompressFileSuccess", fileName)) | ||
| return nil | ||
| }, nil) | ||
| } | ||
| backupTask.AddSubTask(task.GetTaskName(website.Alias, task.TaskBackup, task.TaskScopeApp), backupWebsite, nil) | ||
| if parentTask != nil { | ||
| return backupWebsite(parentTask) | ||
| } | ||
| return backupTask.Execute() | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code has several issues:
Here’s an optimized version of the func handleWebsiteBackup(website *model.Website, parentTask *task.Task, backupDir, fileName, excludes, secret, taskID string) error {
var (
err error
parent <-chan struct{}
resultChan chan result
taskGroup sync.WaitGroup
)
// Create a new task group for managing child tasks
taskGroup.Add(1)
// Start background process to perform the actual backup
go func() {
defer taskGroup.Done()
backupTask := parentTask
if parentTask == nil {
backupTask, err = task.NewTaskWithOps(website.Alias, task.TaskBackup, task_taskScopeWebsite, taskID, website.ID)
if err != nil {
global.LOG.Errorf("failed to create backup task; cannot proceed")
return
}
}
// Use a channel to send results back to main thread
resultChan := make(chan result)
// Background job to compress files into tar.gz archive
go func() {
defer close(resultChan)
err = compressFilesIntoArchive(website, tmpDir, excludes, resultChan)
if err != nil {
resultChan <- result{error: err}
} else {
resultChan <- result{succeeded: true}
}
}()
// Continue executing other subtasks while compressing
for taskType := range []string{
task.GetTaskName(website.Alias, task.TaskBackup, task.TaskScopeWebsite),
task.GetTaskName(website.Alias, task.TaskBackup, task.TaskScopeApp),
} {
task.AddSubTask(taskType, func(t *task.Task) error {
var result result
select {
case <-time.After(defaultTimeout): // timeout after defaultTimeout
t.Error(fmt.Sprintf("timeout waiting for %v", taskType))
break
case r := <-resultChan:
resultChan <- r
break
}
switch v := r.(type) {
case result:
if !v.succeeded || v.error != nil {
t.Error(v.error)
return ErrFailedToBackupWebsite
}
t.Logf("sub-task completed successfully")
}
return nil
}, nil)
}
})()
// Wait until the background goroutine completes its execution
taskGroup.Wait()
// Check if any errors occurred during the compression process
select {
case res := <-resultChan:
if res.error != nil {
return fmt.Errorf("compression failed: %w", res.error)
}
default:
}
// Send success signal through parent's progress channel
if parent != nil {
close(parent)
}
return nil
}
// Function signature based on your specific requirements
type result struct {
succeeded bool
error error
}This refactored method separates concerns, improves readability, and manages concurrent sub-tasks more effectively using go-routines and channels.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are several potential issues and areas for improvement in the given code:
Here’s a revised version addressing some of these points, maintaining clarity and structure: @@ -39,6 +39,7 @@ func (u *BackupService) WebsiteBackup(req dto.CommonBackup) error {
fileName := fmt.Sprintf("%s_%s.tar.gz", website.Alias, timeNow+common.RandStrAndNum(5))
go func() {
+ excludes := ""
+ if req.Exclude != "" {
+ excludes = req.Exclude // Example where exclude could come from input
+ }
if err = handleWebsiteBackup(&website, backupDir, fileName, excludes, req.Secret, req.TaskID); err != nil {
global.LOG.Errorf("backup website %s failed, err: %v", website.Alias, err)
return
@@ -82,6 +83,9 @@ func handleWebsiteRecover(website *model.Website, recoverFile string, isRollba
isOk := false
if !isRollback {
rollbackFile := path.Join(global.Dir.TmpDir, fmt.Sprintf("website/%s_%s.tar.gz", website.Alias, time.Now().Format(constant.DateTimeSlimLayout)))
- if _, err := handleWebsiteBackup(website, path.Dir(rollbackFile), path.Base(rollbackFile), "", "", ""); err != nil {
+
@@ -83,38 +85,36 @@
return fmt.Errorf("backup website %s for rollback before recover failed, err: %v", website.Alias, err)
}Review these changes thoroughly and ensure they align with your application's security requirements and architecture patterns. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,7 +85,7 @@ func (u *CronjobService) handleWebsite(cronjob model.Cronjob, startTime time.Tim | |
| record.DownloadAccountID, record.SourceAccountIDs = cronjob.DownloadAccountID, cronjob.SourceAccountIDs | ||
| backupDir := path.Join(global.Dir.TmpDir, fmt.Sprintf("website/%s", web.Alias)) | ||
| record.FileName = fmt.Sprintf("website_%s_%s.tar.gz", web.Alias, startTime.Format(constant.DateTimeSlimLayout)+common.RandStrAndNum(5)) | ||
| if err := handleWebsiteBackup(&web, backupDir, record.FileName, cronjob.ExclusionRules, cronjob.Secret, taskID); err != nil { | ||
| if err := handleWebsiteBackup(&web, nil, backupDir, record.FileName, cronjob.ExclusionRules, cronjob.Secret, taskID); err != nil { | ||
| return err | ||
| } | ||
| downloadPath, err := u.uploadCronjobBackFile(cronjob, accountMap, path.Join(backupDir, record.FileName)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The provided code snippet has two main areas of concern:
Here are some optimization and improvement suggestions: func (u *CronjobService) handleWebsite(cronjob model.Cronjob, startTime time.Time) error {
record := &model.BackupRecord{
DownloadAccountID: cronjob.DownloadAccountID,
SourceAccountIDs: cronjob.SourceAccountIDs,
}
if backupDir == nil {
return fmt.Errorf("backup directory is not defined")
}
defer os.RemoveAll(backupDir)
record.FileName = fmt.Sprintf(
"website_%s_%s.tar.gz",
web.Alias,
startTime.Format(constant.DateTimeSlimLayout) + common.RandStrAndNum(5),
)
err := handleWebsiteBackup(&web, filepath.Join(tempDir, "website", web.Alias), record.FileName, cronjob.ExclusionRules, cronjob.Secret, taskID)
if err != nil {
return err
}
filePath := path.Join(backupDir, record.FileName)
downloadPath, err := u.uploadCronjobBackFile(cronjob, accountMap, filePath)
if err != nil {
log.Printf("Failed to upload backup file %s: %v", filePath, err)
return err
}
// Assuming log.Printf captures logs needed for auditing or debugging
return nil
}Key Changes:
These tweaks help make the function more robust, easier to maintain, and provide clearer insights into potential problems during execution.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code change makes Suggested Modification: Change if err := handleWebsiteBackup(&web, nil, backupDir, record.FileName, cronjob.ExclusionRules, cronjob.Secret, taskID); err != nil {
return err
}
// Ensure fileName is set
record.FileName = fmt.Sprintf("website_%s_%s.tar.gz", web.Alias, startTime.Format(constant.DateTimeSlimLayout)+common.RandStrAndNum(5))
downloadPath, err := u.uploadCronjobBackFile(cronjob, accountMap, path.Join(backupDir, record.FileName))This ensures that the final file name is set regardless of whether there's an error during backup preparation. Also, ensure that Make sure to test thoroughly after making these changes to see if they resolve any unforeseen bugs or issues related to handling website back-ups. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provided code snippet checks and handles an application recovery scenario where either the installation is a rollback operation or not. Here's a brief analysis:
Parameter Passing: The
handleAppRecoverfunction now takes an additional parameter,recoverTask, which represents the context of the current recover operation.File Path Calculation:
rollbackFileis constructed using the same logic but includestaskIDin its construction (path.Base(rollbackFile)).Backup Handling:
handleAppBackupfunction along with other necessary parameters like directories and base names.Logging:
Optimization Suggestions:
global.Dir.TmpDirand constant strings are properly defined at all times they are used.This revised version maintains logical consistency while incorporating new dependencies introduced via the
recoverTask.