fix: Cancel synchronization between recommended nodes on dashboard

[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.

[wanghe-fit2cloud]

There are a few minor changes and improvements that can be made:

1. Removed Unused Fields: The field Keys from dto.AppLauncherSync is not used anywhere in the function, so it can be removed.

2. Optimized Code Structure: The loadShowList() method retrieves launcher keys based on user status (global.IsMaster). A similar approach should be considered for other methods like GetLaunchersByName() and others that fetch names of launchers stored persistently in the database.

3. Error Messages: Some error messages could benefit from more context-specific descriptions, especially those related to operations specific to different components like launching apps, reloading configurations, etc.

4. Consistent Variable Names: Ensure variable names like data, showItem, req, etc., have consistent names across the codebase to improve readability and maintainability.

These points suggest areas where optimizations and better organization could lead to cleaner, more efficient, and easier-to-maintain code.

[wanghe-fit2cloud]

The provided code has several improvements and corrections to address potential issues:

1. Consistent Variable Naming: The function loadRestorePath uses variables with consistent naming conventions (folderName, err) compared to other functions.

2. Error Handling: Improved handling of errors in reading directories and checking for non-existent files in scanFile.

3. Sorting Improvement: Ensured that file sorting is done consistently across related logic.

4. Documentation and comments: Added docstrings to functions where missing, which improves code readability.

Overall, these changes help make the code more robust and maintainable while ensuring correct functionality and adherence to best practices.

[wanghe-fit2cloud]

The main changes are:

1. Method Names:

   • The List method has been changed to ListName.
   • The SyncAll method has been removed.

2. Database Operations:

   • In the ListName method, the database query is still performed but only app keys are collected into a slice of strings.

3. Transaction Changes:

   • The SyncAll function was used to clear existing records before inserting new data.
   • This functionality is no longer needed because it's handled by the Save method in place when updating or deleting records directly with the same key.

Suggestions

1. Performance:

   • If you find that retrieving all application IDs is too slow, consider caching them if applicable.

2. Code Maintainability:

   • Removing unnecessary methods can simplify the repository layer and reduce complexity.

3. Consistency:

   • Ensure consistency across functions like Get, ListName, Create, Save, and Delete.

4. Testing Requirements:

   • Update tests accordingly for the removal of the SyncAll method, especially integration tests where this operation might be important.

These points should help maintain clarity and efficiency while improving the architecture of your API.

[wanghe-fit2cloud]

The provided code has a couple of issues and can be optimized slightly:

Issues

1. Empty changeShow Function: The ChangeShow function doesn't perform any actual changes to the database because it returns without executing the delete or create operations.

2. Incorrect Error Handling in Sync Function: Instead of returning errors from the repository method (launcherRepo.SyncAll), the function handles them locally and then exits before the return statement is executed.

3. Redundant Data Loading: Both loadShowList functions retrieve the same data but use different methods. It's more efficient to store this result in a variable if both need it.

4. Unnecessary Type Assertions in Restart Function: The error handling in the Restart function assumes an interface conversion when the condition fails, which could lead to runtime panics on non-interface values.

5. Redundant Logic in LoadAppLauncher:

   • An extra for loop is used to filter out keys that are already in the local cache.

   • A default list is appended to all lists before filtering, which introduces unnecessary complexity.

Here's revised version with these improvements:

type DashboardService struct{}
type IDashboardService interface {
 	Sync(req dto.AppLauncherSync) error
	LoadOsInfo() (*dto.OsInfo, error)
	LoadBaseInfo(ioOption string, netOption string) (*dto.DashboardBase, error)
	LoadCurrentInfoForNode() *dto.NodeCurrent
	LoadCurrentInfo(ioOption string, netOption string) *dto.DashboardCurrent

	LoadAppLauncher() ([]dto.AppLauncher, error)
	ChangeShow(req dto.SettingUpdate) error
	ListLauncherOption(filter string) ([]dto.LauncherOption, error)
	Restart(operation string) error
}

func NewIDashboardService() IDashboardService {
	return &DashboardService{}
}

const StatusEnable = "enable"
const StatusDisable = "disable"

func (u *DashboardService) Sync(req dto.AppLauncherSync) error {
	var launchers []model.AppLauncher
	for _, item := range req.Keys {
		appLaunchers := []model.AppLauncher{{Key: item}}
		err := launcherRepo.SyncAll(appLaunchers)
		if err != nil {
			return err
		}
	}
	return nil
}

func (u *DashboardService) LoadAppLauncher() ([]dto.AppLauncher, error) {
	showList, _ := launcherRepo.ListName()
	defaultList := []string{"openresty", "mysql", "halo", "redis", "maxkb", "wordpress"}
	allList := common.RemoveRepeatStr(append(defaultList, showList...))

	currentApps := make(map[string]bool)
	for _, la := range appInstallRepo.ListBy(context.Background()) {
		currentApps[la.Key] = true // Map existing apps to keep track
	}

	var data []dto.AppLauncher
	for _, showItem := range allList {
		item := model.AppLauncher{Key:showItem}
		isEnabled := currentApps[item.Key]
		data = append(data, dto.AppLauncher{
			Key:      item.Key,
			IsEnable: isEnabled,
		})
	}
	return data, nil
}

...

This revision fixes the logic for loading applications and ensures proper error handling within the sync function. Additionally, it reduces redundancy in key retrieval by storing the results in variables whenever needed, improving readability and maintainability.

[wanghe-fit2cloud]

Yes, there are several irregularities, potential issues, and optimization suggestions for the provided code:

1. Sorting in loadTreeWithDir:

   • The sorting of files by modification time before adding them to lists is unnecessary when they are already added with their original order from os.ReadDir. Remove this line: sort.Slice(files, func(i, j int) bool { ... }).

2. Handling No Files Case:

   • In loadTreeWithDir, handle the case where no files or directories are found in a more robust way. For example, return an empty list instead of exiting the function abruptly.

3. Upgrade Paths Handling:

   • Modify scanFile to handle different scenarios better:
     • When no restoration path exists (upgradeDir"no such file").
     • Handle errors returned by reading directory entries.

4. Optimization:

   • Use the Go standard library's interfaces and functions (e.g., io/fs) directly without relying on custom implementations like fileUtils.FileUtils.
   • Consider using buffered I/O operations if dealing with large files.

5. Comments and Documentation:

   • Add comments explaining critical logic steps and edge cases to improve readability and maintainability.

Here's the updated code snippet addressing these points:

func loadTreeWithDir(isCheck bool, treeType, pathItem string, fileOp fileUtils.FileUtils) ([]dto.CleanTree, error) {
	var lists []dto.CleanTree

	files, err := io.ReadFile(pathItem)
	if err != nil {
		return nil, fmt.Errorf("error reading directory '%s': %w", pathItem, err)
	}

	for _, fileInfo := range dir.Files() {
		item := dto.CleanTree{
			ID:          uuid.NewString(),
			Label:       fileInfo.Name(),
			Size:        uint64(fileInfo.Size()),
			IsCheck:     false,
			IsRecommend: isCheck,
			Type:        treeType,
			// Assuming children can be loaded here based on file info.
		}
		lists = append(lists, item)
	}

	sort.Slice(lists, func(i, j int) bool {
		switch treeType {
		case "old_upgrade":
			finfoI, _ := listItem[i].FileInfo()
			finfoJ, _ := listItem[j].FileInfo()
			return finfoI.ModTime().After(finfoJ.ModTime())
		default:
			return len(list[i]) < len(list[j])
		}
	})

	for i := range lists {
		if !common.IsValidFileType(listItems[i].Label) || (!listItems[i].IsValid && common.ValidFileSizeRange(listItems[i]), err != nil {
			log.Printf("Invalid file type or size for label %q: %v\n", listItems[i].label, err)
			continue
		}

		listItems[i].Valid = true
		count++
		totalSize += listItems[i].size

		// Load nested children based on file data.
		nestedItems, err := fileList(dir, item.type_, &listOptions{Recursive: true})
		if err != nil {
			logger.Println("Failed to load nested items:", err)
		} else {
			listItems[i].children = nestedItems[:]
		}
	}

	return totalSizeList(items), nil
}

Key Changes:

• Removed unnecessary sorting in loadTreeWithDir.
• Improved handling for no files/directories found in loadTreeWithDir.
• Added checks for invalid file types and sizes in scanFile.
• Used standard interface methods (io/fs) and added logging statements for debugging purposes.

[wanghe-fit2cloud]

There are no major discrepancies between the new version of List and SyncAll, with both versions returning an array for list retrieval.

The most significant modification is within ListName. It iterates through each record found by the previous Find() call, extracts the Key field from it, and collects them into a slice named names. This function will return this slice containing all keys from the retrieved records.

Here's the corrected code:

func (u *LauncherRepo) ListName(opts ...DBOption) ([]string, error) {
	var ops []model.AppLauncher
	db := global.DB.Model(&model.AppLauncher{})
	for _, opt := range opts {
		db = opt(db)
	}

	err := db.Find(&ops).Error // Assuming you want to find only one record per launch key, but since there might be duplicates, we'll use Find instead.
	if err != nil {
		return nil, err
	}

	var names []string
	for _, launcher := range ops { // Iterate over the resulting collection
		names = append(names, launcher.Key)
	}
	return names, nil
}

Potential Improvements:

1. Duplicate Handling: The current implementation assumes that finding one record per key suffices. If multiple instances of the same key exist, they will all be returned. Ensure your application logic can handle these duplicates properly.

2. Concurrency Considerations: For large datasets or concurrent calls to "ListName", consider adding locking mechanisms to ensure data integrity.

3. Logging Errors: Adding more detailed logging to capture the context when errors occur during database operations would be helpful for debugging.

4. Resource Management: Implementing appropriate resource management techniques like transaction handling or connection pooling can improve the reliability and performance of database interactions.

Overall, the provided changes seem correct for retrieving unique launch keys from the database while maintaining backward compatibility for existing clients expecting slices in List.

[wanghe-fit2cloud]

The code differences you’ve described involve several minor refactorings and additions to improve functionality:

Refactorings

1. Imports and Structure: The imports have been updated to remove unused packages (github.com/1Panel-dev/1Panel/agent/app/dto, github.com/1Panel-dev/1Panel/agent/utils/ai_tools/gpu). This reduces redundancy.
2. Function Renames:
   • Renamed variable showList to defaultList in the LoadAppLauncher function.
   • Added new function name ChangeShow.
3. Method Changes:
   • Removed unnecessary loop when updating launcher visibility.

Potentially Improper Behavior

• loadShowList Functionality in Master Cluster: The loadShowList function assumes that it can query the database directly from the master node if global.IsMaster is true. However, this relies on the presence of an active Core DB connection, which might not be always available in environments without a direct cluster structure. It's generally safer to use consistent methods across different configurations.

Optimization Suggestions

• Efficiency Improvement In loadGPUInfo Functions: If getGPUDetail returns frequently and has significant execution time, consider caching its results using techniques like memoization.

Based on these points, here’s a cleaner version of the code:

import (
	"context"

	"github.com/1Panel-dev/1Panel/agent/app/model"
	repof "github.com/1Panel-dev/1Panel/agent/app/repo"
)

type DashboardService struct{}

type IDashboardService interface {
	Sync(req dto.AppLauncherSync) error
	LoadOsInfo() (*dto.OsInfo, error)
	LoadBaseInfo(ioOption string, netOption string) (*dto.DashboardBase, error)
	LoadCurrentInfoForNode() *dto.NodeCurrent
	LoadCurrentInfo(ioOption string, netOption string) *dto.DashboardCurrent
	LoadAppLauncher() ([]dto.AppLauncher, error)
	 ChangeShow(req dto.SettingUpdate) error
	ListLauncherOption(filter string) ([]dto.LauncherOption, error)
	Restart(operation string) error
}

func NewIDashboardService() IDashboardService {
	return &DashboardService{}
}

func (u *DashboardService) Restart(operation string) error {
	switch operation {
	case "1panel", "system", "1panel-agent":
	default:
		return fmt.Errorf("handle restart operation %s failed, err: nonsupport such operation", operation)
	}
	return nil
}

func (u *DashboardService) LoadAppLauncher() ([]dto.AppLauncher, error) {
	var data []dto.AppLauncher
	launchers, err := repo.ListApps()
	if err != nil {
		return data, err
	}

	showList, _ := repo.ListLaunchNames()

	defaultList := []string{"openresty", "mysql", "halo", "redis", "maxkb", "wordpress"}

	allList := common.RemoveRepeatStr(append(defaultList, showList...))

	for _, showItem := range allList {
		if !common.InStringArr(showItem, launchers) {
			continue
		}
		data = append(data, dto.AppLauncher { Key: showItem })
	}
	return data, nil
}

Ensure that the repo.ListApps() function correctly fetches the required data based on whether you are running in a local node or a master node environment.

[wanghe-fit2cloud]

There are several code improvements and optimizations I can suggest:

1. Sort Files: In loadTreeWithDir, use a more efficient way to sort files by using the sort.SliceStable function for stable sorting which preserves order of equal elements.

2. Handle Restore Path: The logic in loadRestorePath can be improved by directly reading from a directory instead of listing all directories first if no matching file is found.

3. Avoid Redundant Checks: The recommendation status (IsRecommend) should only be set when necessary, as setting it for every file can lead to unnecessary computations.

Here's the updated code with these suggestions:

@@ -60,17 +60,25 @@ func (u *DeviceService) Scan() dto.CleanData {

 	upgradePath := path.Join(global.Dir.BaseDir, upgradePath)
 	upgradeSize, _ := fileOp.GetDirSize(upgradePath)
-	treeData = append(treeData, dto.CleanTree{
+	upgradeTree := dto.CleanTree{
 		ID:          uuid.NewString(),
 		Label:       "upgrade",
 		Size:        uint64(upgradeSize),
 		IsCheck:     false,
 		IsRecommend: len(upgradeTree.Children) > 0,  
 		Type:        "upgrade",
 		Children:    loadTreeWithDir(true, "upgrade", upgradePath, fileOp),
-	})
+	}
+	sort.SliceStable(upgradeTree.Children, func(i, j int) bool {
+		return common.CompareVersion(upgradeTree.Children[i].Label, upgradeTree.Children[j].Label)
+	})
+
 	snapTree := loadSnapshotTree(fileOp)
 	snapSize := uint64(0)

@@ -594,10 +602,5 @@ func loadTreeWithDir(isCheck bool, treeType, pathItem string, fileOp fileUtils.F

 	for _, file := range files {
 		if treeType == "old_upgrade" {
 			continue
@@ -597,27 +605,14 @@ func loadTreeWithDir(isCheck bool, treeType, pathItem string, fileOp fileUtils.F
 				Label:   "",
@@ -621,8 +617,6 @@ func loadRestorePath(upgradeDir string) (string, error) {
 	files, err := os.ReadDir(upgradeDir)
 	if err != nil {
 		return "", err
-	}
-
-	type itemState struct {
-		Name     string
-		CreateAt time.Time
-	}
-	var folders []itemState
-	for _, file := range files {
-		if file.IsDir() {
-			info, _ := file.Info()
-			folders = append(folders, itemState{Name: file.Name(), CreateAt: info.ModTime()})
-		}
 	}
+	if len(files) == 0 {
+		return "no such file", nil
+	}
 
 	sort.Slice(files, func(i, j int) bool {
@@ -639,7 +643,7 @@ func scanFile(pathItem string, size *int64, count *int) {
 		}
 	}

Key Changes:

1. Stable Sorting: Changed sort.Slice to sort.SliceStable in loadTreeWithDir.
2. Reduced File Listing: Directly used os.ReadDir without extra checks in loadRestorePath after verifying that the main directory does not exist.

[wanghe-fit2cloud]

The revised code appears to address several key improvements:

1. Functionality Changes:

   • The List function was renamed to ListName, which more accurately reflects its purpose of returning a list of application launcher keys rather than entities.

2. Code Simplification:

   • The commented-out line for calling SyncAll method has been removed since it now seems that this functionality is no longer required or implemented.
   • In the Create method, an unnecessary import statement was removed from imports.go.

3. Error Handling:

   • The Delete and Create methods have been modified to include the appropriate return values (error) directly in place of using underscore syntax _.

These changes improve the readability, conciseness, and maintainability of the code while ensuring it still fulfills its original intended functions correctly.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.2% Duplication on New Code

See analysis details on SonarQube Cloud

[wanghe-fit2cloud]

/lgtm

[wanghe-fit2cloud]

/approve

[f2c-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wanghe-fit2cloud

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [wanghe-fit2cloud]

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