fix: Fix the problem of recovering application and website task logs

[ssongliu]

No description provided.

[f2c-ci-robot]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[f2c-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ssongliu. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[wanghe-fit2cloud]

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.

[wanghe-fit2cloud]

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.

[wanghe-fit2cloud]

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.

[wanghe-fit2cloud]

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.

[wanghe-fit2cloud]

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.

[wanghe-fit2cloud]

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.

[sonarqubecloud]

Quality Gate failed

Failed conditions
D Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE