Skip to content
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
2 changes: 1 addition & 1 deletion agent/app/service/backup_app.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}
Copy link
Copy Markdown
Member

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:

  1. Parameter Passing: The handleAppRecover function now takes an additional parameter, recoverTask, which represents the context of the current recover operation.

  2. File Path Calculation:

    • rollbackFile is constructed using the same logic but includes taskID in its construction (path.Base(rollbackFile)).
  3. Backup Handling:

    • If it's not a rollback, a backup file is prepared with the name including both the instance ID and the time.
    • This file path is passed to the handleAppBackup function along with other necessary parameters like directories and base names.
  4. Logging:

    • An error log is added if the backup process fails during the rollback phase.

Optimization Suggestions:

  • Ensure that global.Dir.TmpDir and 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 recoverTask first, like this:

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 recoverTask is available, potentially reducing redundant computations in edge cases where recoverTask is nil.

Expand Down
28 changes: 20 additions & 8 deletions agent/app/service/backup_website.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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()
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code has several issues:

  1. handleWebsiteRecover function's recoverTask.Execute() call is missing the proper context, which could lead to errors.
  2. In handleWebsiteBackup, if err occurs during creating a backup task with operations (task.NewTaskWithOps), it should return this error instead of continuing.
  3. When calling handleWebsiteRecover inside another Go routine, use a waitgroup to ensure all goroutines complete properly before moving on.
  4. The logic for restoring website data seems unnecessary as the file path is hardcoded.

Here’s an optimized version of the handleWebsiteBackup function that removes redundant code and uses channels to manage concurrency safely:

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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:

  1. Unused Variables:

    • The exclude variable is declared but not used within the handleWebsiteBackup function.
  2. Incorrect Task Addition:

    • In handleWebsiteBackup, there's an issue with adding tasks to backupTask. It should use a different naming convention for the task name (GetTaskName would suggest it's intended for another scope).
  3. Task Execution Logic:

    • In handleWebsiteRecover, the rollback process attempts to execute an empty string as recoverTask.Execute(), which will likely raise an error if the task object has not been properly initialized or configured.
  4. Security Considerations:

    • The secret parameter is passed directly into fmt.Sprintf without any sanitization, potentially exposing sensitive information if not handled carefully.
  5. Error Handling:

    • Some errors are logged using LOG.Errorf, while others (like those returned by task.GetTaskName and missing file operations) aren't wrapped appropriately in return statements.
  6. Concurrency Concerns:

    • The call to go func() creates new goroutines for each request; ensure that these routines have proper handling of resources, especially in larger applications.
  7. Documentation and Commenting:

    • There may be improvements needed for comments explaining what certain sections of the code do, making maintenance easier for future developers.

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.

Expand Down
2 changes: 1 addition & 1 deletion agent/app/service/cronjob_backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided code snippet has two main areas of concern:

  1. Null Backup Directory Initialization: In the line record.FileName, you're initializing record.FileName without checking if backupDir is nil. It's generally good practice to ensure that all variables used in file operations have valid values before attempting them.

  2. Error Handling in Upload Function: The error return type from u.uploadCronjobBackFile() is explicitly typed as error, which is a common practice but doesn't provide much context about what might go wrong during the upload process. While this isn't directly related to the logic errors, it can be improved by handling specific error types or providing more detailed feedback when an error occurs.

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:

  • Initialization Check: Added a check for backupDir being nil and returning an early error if so.
  • Error Context Logging: Improved logging within functions where errors could likely occur, such as handleWebsiteBackup and uploadCronjobBackFile.
  • Deferred Cleanup: Ensured temporary directories are cleaned up using defer.

These tweaks help make the function more robust, easier to maintain, and provide clearer insights into potential problems during execution.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code change makes cronjob.SourceAccountIDs equal to the default empty slice model.Cronjob.SourcAccountIDs, which is likely unintentional because these IDs should come from the cronjob structure passed into this function. Additionally, if you're not passing a valid directory name into handleWebsiteBackup, it might cause errors or undefined behavior when preparing backups.

Suggested Modification: Change record.FileName before checking for backup error:

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 taskID is correctly used somewhere within your codebase as it seems unused at the moment. If no task ID is necessary, you can remove it from the call.

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.

Expand Down
3 changes: 3 additions & 0 deletions core/i18n/lang/en.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ ErrBackupPublic: "Detected that this backup account is not public, please check
ErrOSSConn: "Unable to get the latest version, please check the server's external network connectivity."

#license
LicenseCheck: 'Check if the license is available'
ErrLicenseInUsed: 'The license is already bound. Please check and try again!'
ErrLicenseExpired: 'The license has expired. Please check and try again!'
ErrLicense: "License format error, please check and retry!"
ErrLicenseCheck: "License validation failed, please check and retry!"
ErrXpackVersion: "License validation failed, this license is version-limited, cannot import, please check and retry!"
Expand Down
3 changes: 3 additions & 0 deletions core/i18n/lang/ja.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ ErrOSSConn: "最新バージョンを取得できません。外部ネットワ


#license
LicenseCheck: 'ライセンスが利用可能か確認する'
ErrLicenseInUsed: 'このライセンスは既に紐付けられています。確認して再試行してください!'
ErrLicenseExpired: 'このライセンスは期限切れです。確認して再試行してください!'
ErrLicense: "ライセンスフォーマットエラー、確認して再試行してください!"
ErrLicenseCheck: "ライセンスの確認に失敗しました、確認して再試行してください!"
ErrXpackVersion: "ライセンスの確認に失敗しました、このライセンスはバージョン制限があり、正常にインポートできません、確認して再試行してください!"
Expand Down
3 changes: 3 additions & 0 deletions core/i18n/lang/ko.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ ErrBackupPublic: "이 백업 계정이 공개된 것으로 감지되지 않았
ErrOSSConn: "최신 버전을 가져올 수 없습니다. 외부 네트워크 연결을 확인하십시오."

#license
LicenseCheck: '라이선스 사용 가능 여부 확인'
ErrLicenseInUsed: '해당 라이선스가 이미 연결되었습니다. 확인 후 다시 시도하세요!'
ErrLicenseExpired: '해당 라이선스가 만료되었습니다. 확인 후 다시 시도하세요!'
ErrLicense: "라이선스 형식이 잘못되었습니다. 다시 확인하고 시도해 주세요!"
ErrLicenseCheck: "라이선스 검증 실패. 다시 확인하고 시도해 주세요!"
ErrXpackVersion: "라이선스 검증 실패, 이 라이선스는 버전 제한이 있습니다. 성공적으로 가져올 수 없습니다. 다시 확인하고 시도해 주세요!"
Expand Down
3 changes: 3 additions & 0 deletions core/i18n/lang/ms.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ ErrBackupPublic: "Akaun sandaran ini dikesan tidak awam, sila semak semula dan c
ErrOSSConn: "Tidak dapat mendapatkan versi terkini, sila semak sambungan rangkaian luar pelayan."

#license
LicenseCheck: 'Periksa sama ada lesen tersedia'
ErrLicenseInUsed: 'Lesen ini telah terikat. Sila periksa dan cuba lagi!'
ErrLicenseExpired: 'Lesen ini telah tamat tempoh. Sila periksa dan cuba lagi!'
ErrLicense: "Format lesen tidak sah, sila semak dan cuba lagi!"
ErrLicenseCheck: "Pengesahan lesen gagal, sila semak dan cuba lagi!"
ErrXpackVersion: "Pengesahan lesen gagal, lesen ini terhad kepada versi tertentu, sila semak dan cuba lagi!"
Expand Down
3 changes: 3 additions & 0 deletions core/i18n/lang/pt-BR.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ ErrBackupPublic: "A conta de backup detectada não é pública, por favor verifi
ErrOSSConn: "Não foi possível obter a versão mais recente, verifique a conectividade externa do servidor."

#license
LicenseCheck: 'Verificar se a licença está disponível'
ErrLicenseInUsed: 'Esta licença já está vinculada. Por favor, verifique e tente novamente!'
ErrLicenseExpired: 'Esta licença expirou. Por favor, verifique e tente novamente!'
ErrLicense: "Formato de licença inválido, por favor verifique e tente novamente!"
ErrLicenseCheck: "Falha na verificação da licença, por favor verifique e tente novamente!"
ErrXpackVersion: "Falha na verificação da licença, esta licença é restrita a uma versão específica, por favor verifique e tente novamente!"
Expand Down
3 changes: 3 additions & 0 deletions core/i18n/lang/ru.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ ErrBackupPublic: "Обнаружено, что эта учетная запис
ErrOSSConn: "Не удалось получить последнюю версию, проверьте подключение сервера к внешней сети."

#license
LicenseCheck: 'Проверить доступность лицензии'
ErrLicenseInUsed: 'Лицензия уже привязана. Пожалуйста, проверьте и повторите попытку!'
ErrLicenseExpired: 'Срок действия лицензии истек. Пожалуйста, проверьте и повторите попытку!'
ErrLicense: "Неверный формат лицензии, проверьте и повторите попытку!"
ErrLicenseCheck: "Ошибка проверки лицензии, проверьте и повторите попытку!"
ErrXpackVersion: "Ошибка проверки лицензии, лицензия ограничена по версии, проверьте и повторите попытку!"
Expand Down
3 changes: 3 additions & 0 deletions core/i18n/lang/zh-Hant.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ ErrBackupPublic: "檢測到該備份帳號為非公用,請檢查後再試!"
ErrOSSConn: "無法獲取最新版本,請確認伺服器是否能夠連接外部網路。"

#license
LicenseCheck: '檢查許可證是否可用'
ErrLicenseInUsed: '檢測到該許可證已被綁定,請檢查後重試!'
ErrLicenseExpired: '檢測到該許可證已過期,請檢查後重試!'
ErrLicense: "許可證格式錯誤,請檢查後重試!"
ErrLicenseCheck: "許可證校驗失敗,請檢查後重試!"
ErrXpackVersion: "許可證校驗失敗,該許可證受版本限制,無法成功導入,請檢查後重試!"
Expand Down
3 changes: 3 additions & 0 deletions core/i18n/lang/zh.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ ErrBackupPublic: "检测到该备份账号为非公用,请检查后重试!"
ErrOSSConn: "无法获取最新版本,请确认服务器是否能够连接外部网络。"

#license
LicenseCheck: '检查许可证是否可用'
ErrLicenseInUsed: '检查到该许可证已被绑定,请检查后重试!'
ErrLicenseExpired: '检查到该许可证已过期,请检查后重试!'
ErrLicense: "许可证格式错误,请检查后重试!"
ErrLicenseCheck: "许可证校验失败,请检查后重试!"
ErrXpackVersion: "许可证校验失败,该许可证受版本限制,无法成功导入,请检查后重试!"
Expand Down
Loading