Repository: kubeclipper/kubeclipper
Pull request #674 (2023-10-08 06:58:28)

fix: kcctl optimization
1. registry deploy add flag skip-image-load
2. ssh and cmd util add exit code check
3. ssh and cmd util add log print

<!-- Thanks for sending a pull request! Here are some tips for you:

1. If you want **faster** PR reviews, read how: https://github.com/kubeclipper/community/blob/master/developer-guide/development/the-pr-author-guide-to-getting-through-code-review.md
2. In case you want to know how your PR got reviewed, read: https://github.com/kubeclipper/community/blob/master/developer-guide/development/code-review-guide.md
3. Here are some coding convetions followed by Kubeclipper community: https://github.com/kubeclipper/community/blob/master/developer-guide/development/coding-conventions.md
-->

### What type of PR is this?
<!--
Add one of the following kinds:
/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind design

Optionally add one or more of the following kinds if applicable:
/kind api-change
/kind deprecation
/kind failing-test
/kind flake
/kind regression
-->

/kind flake
### What this PR does / why we need it:

### Which issue(s) this PR fixes:
<!--
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

### Special notes for reviewers:
```
```

### Does this PR introduced a user-facing change?
<!--
If no, just write "None" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".

For more information on release notes see: https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md
-->
```release-note
None
```

### Additional documentation, usage docs, etc.:
<!--
This section can be blank if this pull request does not require a release note.
Please use the following format for linking documentation or pass the
section below:
- [KEP]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```

kubeclipper-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubeclipper/kubeclipper/pull/674#" title="Author self-approved">lixd</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **x893675** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @x893675` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubeclipper%2Fkubeclipper).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubeclipper/kubeclipper/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["x893675"]} -->
lixd: /assign @x893675
kubeclipper-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubeclipper/kubeclipper/pull/674#" title="Author self-approved">lixd</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **x893675** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubeclipper%2Fkubeclipper).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubeclipper/kubeclipper/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["x893675"]} -->
codecov-commenter: ## [Codecov](https://app.codecov.io/gh/kubeclipper/kubeclipper/pull/674?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubeclipper) Report
> Merging [#674](https://app.codecov.io/gh/kubeclipper/kubeclipper/pull/674?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubeclipper) (e0bcc98) into [master](https://app.codecov.io/gh/kubeclipper/kubeclipper/commit/59beab9bcb51be4659f21497f21bd3fb57c8b2cb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubeclipper) (59beab9) will **decrease** coverage by `0.10%`.
> The diff coverage is `59.45%`.

:exclamation: Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/kubeclipper/kubeclipper/pull/674/graphs/tree.svg?width=650&height=150&src=pr&token=3PWFG2MHTE&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubeclipper)](https://app.codecov.io/gh/kubeclipper/kubeclipper/pull/674?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubeclipper)

```diff
@@            Coverage Diff             @@
##           master     #674      +/-   ##
==========================================
- Coverage   14.23%   14.13%   -0.10%     
==========================================
  Files         142      142              
  Lines       20043    20071      +28     
==========================================
- Hits         2853     2837      -16     
- Misses      16891    16944      +53     
+ Partials      299      290       -9     
```


| [Files](https://app.codecov.io/gh/kubeclipper/kubeclipper/pull/674?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubeclipper) | Coverage Δ | |
|---|---|---|
| [pkg/utils/sshutils/cmd.go](https://app.codecov.io/gh/kubeclipper/kubeclipper/pull/674?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubeclipper#diff-cGtnL3V0aWxzL3NzaHV0aWxzL2NtZC5nbw==) | `78.26% <100.00%> (+0.98%)` | :arrow_up: |
| [pkg/cli/registry/registry.go](https://app.codecov.io/gh/kubeclipper/kubeclipper/pull/674?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubeclipper#diff-cGtnL2NsaS9yZWdpc3RyeS9yZWdpc3RyeS5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/utils/sshutils/ssh\_v2.go](https://app.codecov.io/gh/kubeclipper/kubeclipper/pull/674?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubeclipper#diff-cGtnL3V0aWxzL3NzaHV0aWxzL3NzaF92Mi5nbw==) | `27.40% <62.96%> (+7.76%)` | :arrow_up: |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/kubeclipper/kubeclipper/pull/674/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubeclipper)

</details>
x893675: /lgtm
/approve
kubeclipper-bot: LGTM label has been added.  <details>Git tree hash: c3a91193460945295a43ba6cfbf92301558ff818</details>
kubeclipper-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubeclipper/kubeclipper/pull/674#" title="Author self-approved">lixd</a>*, *<a href="https://github.com/kubeclipper/kubeclipper/pull/674#issuecomment-1756669035" title="Approved">x893675</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubeclipper%2Fkubeclipper).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubeclipper/kubeclipper/blob/master/OWNERS)~~ [x893675]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
x893675: /cherrypick release-1.4
kubeclipper-bot: @x893675: once the present PR merges, I will cherry-pick it on top of release-1.4 in a new PR and assign it to you.

<details>

In response to [this](https://github.com/kubeclipper/kubeclipper/pull/674#issuecomment-1756669207):

>/cherrypick release-1.4


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

Patch:
diff --git a/pkg/cli/registry/registry.go b/pkg/cli/registry/registry.go
index fdc7e84..2294182 100644
--- a/pkg/cli/registry/registry.go
+++ b/pkg/cli/registry/registry.go
@@ -60,6 +60,8 @@ const (
 	registryExample = `
   # Deploy docker registry
   kcctl registry deploy --pk-file key --node 10.0.0.111 --pkg kc.tar.gz
+  # Deploy docker registry without image load
+  kcctl registry deploy --pk-file key --node 10.0.0.111 --pkg kc.tar.gz --skip-image-load
   # Clean docker registry
   kcctl registry clean --pk-file key --node 10.0.0.111
   # Push docker image to registry
@@ -134,10 +136,11 @@ type RegistryOptions struct {
 	DataRoot     string
 	RegistryPort int
 
-	Type   string
-	Name   string
-	Tag    string
-	Number int
+	Type          string
+	Name          string
+	Tag           string
+	Number        int
+	SkipImageLoad bool
 }
 
 var (
@@ -197,6 +200,7 @@ func NewCmdRegistryDeploy(o *RegistryOptions) *cobra.Command {
 	cmd.Flags().StringVar(&o.Pkg, "pkg", o.Pkg, "registry service and images pkg.")
 	cmd.Flags().StringVar(&o.DataRoot, "data-root", o.DataRoot, "set registry data root directory.")
 	cmd.Flags().IntVar(&o.RegistryPort, "registry-port", o.RegistryPort, "set registry port")
+	cmd.Flags().BoolVar(&o.SkipImageLoad, "skip-image-load", o.SkipImageLoad, "set to skip image load,if set true will skip image load when deploy registry")
 
 	utils.CheckErr(cmd.MarkFlagRequired("node"))
 	utils.CheckErr(cmd.MarkFlagRequired("pkg"))
@@ -453,6 +457,7 @@ func (o *RegistryOptions) Install() error {
 	if err := o.deployRegistry(); err != nil {
 		return fmt.Errorf("deploy registry error: %s", err.Error())
 	}
+
 	// load images
 	if err := o.loadImages(); err != nil {
 		return fmt.Errorf("load images error: %s", err.Error())
@@ -676,6 +681,10 @@ func (o *RegistryOptions) processPackage() error {
 }
 
 func (o *RegistryOptions) loadImages() error {
+	if o.SkipImageLoad {
+		logger.Info("skip image load")
+		return nil
+	}
 	// TODO: for historical reasons and version stability, kcctl temporarily skips the load of the kc-extension image when deploying a private repository
 	// e.g. find /tmp/kc/resource -name images*.tar.gz ｜ grep -v kc-extension  | awk '{print}'
 
diff --git a/pkg/utils/sshutils/cmd.go b/pkg/utils/sshutils/cmd.go
index aeaf84f..02d51c4 100644
--- a/pkg/utils/sshutils/cmd.go
+++ b/pkg/utils/sshutils/cmd.go
@@ -25,6 +25,8 @@ import (
 	"os/exec"
 	"path"
 	"strings"
+
+	"github.com/kubeclipper/kubeclipper/pkg/cli/logger"
 )
 
 func IsFileExist(filepath string) (bool, error) {
@@ -57,7 +59,9 @@ func CmdToString(name string, arg ...string) (Result, error) {
 	}
 	ret.Stdout = bout.String()
 	ret.Stderr = berr.String()
-	return ret, nil
+
+	logger.V(5).Info(ret.Table())
+	return ret, ret.error()
 }
 
 func RunCmdAsSSH(cmdStr string) (Result, error) {
@@ -86,7 +90,8 @@ func RunCmdAsSSH(cmdStr string) (Result, error) {
 		ret.ExitCode = exitCode
 		return ret, nil
 	}
-	return ret, nil
+	logger.V(5).Info(ret.Table())
+	return ret, ret.error()
 }
 
 func Whoami() string {
diff --git a/pkg/utils/sshutils/ssh_v2.go b/pkg/utils/sshutils/ssh_v2.go
index a0507ee..7aa2fdb 100644
--- a/pkg/utils/sshutils/ssh_v2.go
+++ b/pkg/utils/sshutils/ssh_v2.go
@@ -21,9 +21,14 @@ package sshutils
 import (
 	"bytes"
 	"fmt"
+	"strconv"
 	"strings"
 	"sync"
 
+	"github.com/olekukonko/tablewriter"
+
+	"github.com/kubeclipper/kubeclipper/pkg/cli/logger"
+
 	"golang.org/x/crypto/ssh"
 )
 
@@ -41,16 +46,43 @@ func (r Result) Short() string {
 	return fmt.Sprintf("(run `%s` on %s@%s)", r.PrintCmd, r.User, r.Host)
 }
 
+// Deprecated
+// need not use this method to check error,will automic check when command run.
 func (r Result) Error() error {
+	return r.error()
+}
+
+func (r Result) error() error {
 	if r.ExitCode == 0 {
 		return nil
 	}
-	return fmt.Errorf("%s err: %s", r.Short(), r.Stderr)
+	if r.Stderr != "" {
+		return fmt.Errorf("%s exitcode %v msg: %s", r.Short(), r.ExitCode, r.Stderr)
+	}
+	return fmt.Errorf("%s exitcode %v msg: %s", r.Short(), r.ExitCode, r.Stdout)
 }
 
+// Deprecated
+// use Table to replace
 func (r Result) String() string {
-	return fmt.Sprintf("(run `%s` on %s@%s) \nstdout: \n%s\nstderr: %s\nexitcode: %v\n", r.PrintCmd, r.User, r.Host,
-		r.Stdout, r.Stderr, r.ExitCode)
+	return r.Table()
+}
+
+// Table format as table
+func (r Result) Table() string {
+	buff := new(bytes.Buffer)
+
+	table := tablewriter.NewWriter(buff)
+	table.SetHeader([]string{"USER", "HOST", "COMMAND", "STDOUT", "STDERR", "EXIT CODE"})
+	data := [][]string{
+		{r.User, r.Host, r.PrintCmd, r.Stdout, r.Stderr, strconv.Itoa(r.ExitCode)},
+	}
+
+	for _, v := range data {
+		table.Append(v)
+	}
+	table.Render()
+	return "\n" + buff.String() + "\n"
 }
 
 func (r Result) StdoutToString(place string) string {
@@ -99,7 +131,13 @@ func SSHCmd(sshConfig *SSH, host, cmd string) (Result, error) {
 		Stderr:   stderr,
 		ExitCode: code,
 	}
-	return result, err
+	logger.V(5).Infof(result.Table())
+
+	if err != nil {
+		return result, err
+	}
+	// check exit code again when run cmd success
+	return result, result.error()
 }
 
 // runSSHCommand returns the stdout, stderr, and exit code from running cmd on


Test patch:
diff --git a/pkg/utils/ipvsutil/ipvs_linux_test.go b/pkg/utils/ipvsutil/ipvs_linux_test.go
index bd6514d..1585ee5 100644
--- a/pkg/utils/ipvsutil/ipvs_linux_test.go
+++ b/pkg/utils/ipvsutil/ipvs_linux_test.go
@@ -23,6 +23,8 @@ import (
 )
 
 func TestClear(t *testing.T) {
+	t.Skip("can't run in github action")
+
 	type args struct {
 		dryRun bool
 	}
@@ -47,6 +49,8 @@ func TestClear(t *testing.T) {
 }
 
 func TestCreateIPVS(t *testing.T) {
+	t.Skip("can't run in github action")
+
 	type args struct {
 		vs     *VirtualServer
 		dryRun bool
@@ -82,6 +86,8 @@ func TestCreateIPVS(t *testing.T) {
 }
 
 func TestDeleteIPVS(t *testing.T) {
+	t.Skip("can't run in github action")
+
 	type args struct {
 		vs     *VirtualServer
 		dryRun bool
diff --git a/pkg/utils/sshutils/ssh_v2_test.go b/pkg/utils/sshutils/ssh_v2_test.go
index a42a999..15ee68c 100644
--- a/pkg/utils/sshutils/ssh_v2_test.go
+++ b/pkg/utils/sshutils/ssh_v2_test.go
@@ -55,3 +55,11 @@ func TestSSHCmd(t *testing.T) {
 	}
 	assert.Equal(t, "54321", ret.StdoutToString(""))
 }
+
+func TestSSHTable(t *testing.T) {
+	ret, err := SSHCmdWithSudo(nil, "", "ls .")
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Log(ret.Table())
+}