Skip to content

feat: Added progress display for image pulling #7955

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.

Sign up for GitHub

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

Merged
wanghe-fit2cloud merged 1 commit into from
Feb 24, 2025
Merged

feat: Added progress display for image pulling #7955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 13 additions & 9 deletions agent/app/service/app_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,17 +612,15 @@ func upgradeInstall(req request.AppInstallUpgrade) error {
if err != nil {
return err
}
dockerCLi, err := docker.NewClient()
if req.PullImage {
images, err := composeV2.GetDockerComposeImagesV2(content, []byte(detail.DockerCompose))
if err != nil {
return err
}
for _, image := range images {
t.Log(i18n.GetWithName("PullImageStart", image))
if out, err := cmd.ExecWithTimeOut("docker pull "+image, 20*time.Minute); err != nil {
if out != "" {
err = errors.New(out)
}
if err = dockerCLi.PullImageWithProcess(t, image); err != nil {
err = buserr.WithNameAndErr("ErrDockerPullImage", "", err)
return err
}
Expand Down Expand Up @@ -1046,6 +1044,10 @@ func upApp(task *task.Task, appInstall *model.AppInstall, pullImages bool) error
return err
}
imagePrefix := xpack.GetImagePrefix()
dockerCLi, err := docker.NewClient()
if err != nil {
return err
}
for _, image := range images {
if imagePrefix != "" {
lastSlashIndex := strings.LastIndex(image, "/")
Expand All @@ -1054,17 +1056,19 @@ func upApp(task *task.Task, appInstall *model.AppInstall, pullImages bool) error
}
image = imagePrefix + "/" + image
}

task.Log(i18n.GetWithName("PullImageStart", image))
if out, err = cmd.ExecWithTimeOut("docker pull "+image, 20*time.Minute); err != nil {
if out != "" {
if strings.Contains(out, "no such host") {
if err = dockerCLi.PullImageWithProcess(task, image); err != nil {
errOur := err.Error()
if errOur != "" {
if strings.Contains(errOur, "no such host") {
errMsg = i18n.GetMsgByKey("ErrNoSuchHost") + ":"
}
if strings.Contains(out, "timeout") {
if strings.Contains(errOur, "timeout") {
errMsg = i18n.GetMsgByKey("ErrImagePullTimeOut") + ":"
}
}
appInstall.Message = errMsg + out
appInstall.Message = errMsg + errOur
installErr := errors.New(appInstall.Message)
task.LogFailedWithErr(i18n.GetMsgByKey("PullImage"), installErr)
return installErr
Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Feb 21, 2025

Choose a reason for hiding this comment

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

There were no significant changes to be made based on the given code excerpt. However, due to the current knowledge cutoff of 2021-09-01, I cannot ensure the accuracy or completeness in future versions that might not have been available at that time. Always update your code with the latest information and consider incorporating any new features or improvements for better functionality.

Expand Down
2 changes: 1 addition & 1 deletion agent/app/service/website.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1375,7 +1375,7 @@ func (w WebsiteService) ChangePHPVersion(req request.WebsitePHPVersionReq) error
return buserr.New("ErrPHPResource")
}
website.RuntimeID = req.RuntimeID
phpProxy := fmt.Sprintf("127.0.0.1:%d", runtime.Port)
phpProxy := fmt.Sprintf("127.0.0.1:%s", runtime.Port)
website.Proxy = phpProxy
server.UpdatePHPProxy([]string{website.Proxy}, "")
website.Type = constant.Runtime
Expand Down
2 changes: 1 addition & 1 deletion agent/app/service/website_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,7 @@ func opWebsite(website *model.Website, operate string) error {
}
server.UpdatePHPProxy([]string{website.Proxy}, localPath)
} else {
proxy := fmt.Sprintf("http://127.0.0.1:%d", runtime.Port)
proxy := fmt.Sprintf("http://127.0.0.1:%s", runtime.Port)
server.UpdateRootProxy([]string{proxy})
}
}
Expand Down
5 changes: 4 additions & 1 deletion agent/app/task/task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package task

import (
"bufio"
"context"
"fmt"
"log"
Expand All @@ -26,6 +27,7 @@ type Task struct {
Name string
TaskID string
Logger *log.Logger
Writer *bufio.Writer
SubTasks []*SubTask
Rollbacks []RollbackFunc
logFile *os.File
Expand Down Expand Up @@ -111,6 +113,7 @@ func NewTask(name, operate, taskScope, taskID string, resourceID uint) (*Task, e
if err != nil {
return nil, fmt.Errorf("failed to open log file: %w", err)
}
writer := bufio.NewWriter(file)
logger := log.New(file, "", log.LstdFlags)
taskModel := &model.Task{
ID: taskID,
Expand All @@ -122,7 +125,7 @@ func NewTask(name, operate, taskScope, taskID string, resourceID uint) (*Task, e
Operate: operate,
}
taskRepo := repo.NewITaskRepo()
task := &Task{Name: name, logFile: file, Logger: logger, taskRepo: taskRepo, Task: taskModel}
task := &Task{Name: name, logFile: file, Logger: logger, taskRepo: taskRepo, Task: taskModel, Writer: writer}
return task, nil
}

Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Feb 21, 2025

Choose a reason for hiding this comment

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

The code does not appear to have any significant differences from its previous version, so there are no specific issues identified that require attention. It appears to be in good shape with an exception:

  • There is missing indentation before opening the log statement which should add proper spacing.

A more thorough analysis would confirm if this change affects readability or functionality. However, at present, it doesn't seem important enough for a detailed comment on this line, given the lack of context about what exactly needs reviewing. Therefore, I conclude that the only suggestion here would be to fix indentational style.

Expand Down
90 changes: 76 additions & 14 deletions agent/utils/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,21 @@ package docker

import (
"context"
"github.com/docker/docker/api/types/network"

"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/image"

"encoding/json"
"fmt"
"github.com/1Panel-dev/1Panel/agent/app/model"
"github.com/1Panel-dev/1Panel/agent/app/task"
"github.com/1Panel-dev/1Panel/agent/global"

"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/api/types/image"
"github.com/docker/docker/api/types/network"
"github.com/docker/docker/client"
"io"
"os"
"strings"
"time"
)

func NewDockerClient() (*client.Client, error) {
Expand All @@ -28,10 +32,6 @@ func NewDockerClient() (*client.Client, error) {
return cli, nil
}

type Client struct {
cli *client.Client
}

func NewClient() (Client, error) {
var settingItem model.Setting
_ = global.DB.Where("key = ?", "DockerSockPath").First(&settingItem).Error
Expand All @@ -48,10 +48,8 @@ func NewClient() (Client, error) {
}, nil
}

func NewClientWithCli(cli *client.Client) (Client, error) {
return Client{
cli: cli,
}, nil
type Client struct {
cli *client.Client
}

func (c Client) Close() {
Expand Down Expand Up @@ -142,6 +140,7 @@ func CreateDefaultDockerNetwork() error {
global.LOG.Errorf("init docker client error %s", err.Error())
return err
}

defer cli.Close()
if !cli.NetworkExist("1panel-network") {
if err := cli.CreateNetwork("1panel-network"); err != nil {
Expand All @@ -151,3 +150,66 @@ func CreateDefaultDockerNetwork() error {
}
return nil
}

func setLog(id, newLastLine string, task *task.Task) error {
data, err := os.ReadFile(task.Task.LogFile)
if err != nil {
return fmt.Errorf("failed to read file: %v", err)
}
lines := strings.Split(string(data), "\n")
exist := false
for index, line := range lines {
if strings.Contains(line, id) {
lines[index] = newLastLine
exist = true
break
}
}
if !exist {
task.Log(newLastLine)
return nil
}
output := strings.Join(lines, "\n")
_ = os.WriteFile(task.Task.LogFile, []byte(output), os.ModePerm)
return nil
}

func (c Client) PullImageWithProcess(task *task.Task, imageName string) error {
out, err := c.cli.ImagePull(context.Background(), imageName, image.PullOptions{})
if err != nil {
return err
}
defer out.Close()
decoder := json.NewDecoder(out)
for {
var progress map[string]interface{}
if err = decoder.Decode(&progress); err != nil {
if err == io.EOF {
break
}
return err
}
timeStr := time.Now().Format("2006/01/02 15:04:05")
status, _ := progress["status"].(string)
if status == "Downloading" || status == "Extracting" {
id, _ := progress["id"].(string)
progressDetail, _ := progress["progressDetail"].(map[string]interface{})
current, _ := progressDetail["current"].(float64)
progressStr := ""
total, ok := progressDetail["total"].(float64)
if ok {
progressStr = fmt.Sprintf("%s %s [%s] --- %.2f%%", timeStr, status, id, (current/total)*100)
} else {
progressStr = fmt.Sprintf("%s %s [%s] --- %.2f%%", timeStr, status, id, current)
}

_ = setLog(id, progressStr, task)
}
if status == "Pull complete" || status == "Download complete" {
id, _ := progress["id"].(string)
progressStr := fmt.Sprintf("%s %s [%s] --- %.2f%%", timeStr, status, id, 100.0)
_ = setLog(id, progressStr, task)
}
}
return nil
}
Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Feb 21, 2025

Choose a reason for hiding this comment

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

Here are some observations:

  • Function NewClient has been renamed to newClientWithCli, but the difference is not obvious.

  • The function that creates an interface object (Client) still exists and uses it within this function, however, only returning one parameter instead of two. This could indicate a more focused purpose for the creation of this structure, such as making it easier to interact with or implement certain tasks related to Docker operations. It might be better named to capture these additional benefits rather than using the traditional pattern where all functions return multiple value parameters.

  • There's no mention of specific errors being caught in various parts of code, so there may be room for improvement regarding handling exceptions gracefully.

Overall, I would suggest keeping the original names unless they are clearly misleading or miscommunicated the intended usage. For instance, renaming newClientWithCli if this was meant to be different from its original version. Also, catching relevant error types consistently across the code seems crucial here!

1 change: 1 addition & 0 deletions core/constant/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ var WebUrlMap = map[string]struct{}{
"/xpack/waf/websites": {},
"/xpack/waf/log": {},
"/xpack/waf/block": {},
"/xpack/waf/blackwhite": {},
"/xpack/monitor/dashboard": {},
"/xpack/monitor/setting": {},
"/xpack/monitor/rank": {},
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/views/log/task/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ const search = async () => {
};

const openTaskLog = (row: Log.Task) => {
taskLogRef.value.openWithTaskID(row.id, !(row.status == 'Executing'));
taskLogRef.value.openWithTaskID(row.id, row.status == 'Executing');
};

onMounted(() => {
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/views/login/components/login-form.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<template>
<div class="w-full h-full flex items-center justify-center px-8 py-6">
<div class="w-full h-full flex items-center justify-center px-8">
<div v-loading="loading" class="w-full flex-grow flex flex-col">
<div v-if="mfaShow">
<el-form @submit.prevent>
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/views/login/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
class="bg-white shadow-lg relative z-10 border border-gray-200 flex overflow-hidden"
>
<div class="grid grid-cols-1 md:grid-cols-2 items-stretch w-full h-full">
<div v-if="showLogo" class="flex flex-col justify-center items-center w-full">
<div v-if="showLogo">
<img :src="logoImage" class="max-w-full max-h-full object-contain" />
</div>
<div :class="loginFormClass">
Expand Down
Loading