Skip to content
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.

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

[feat] tools-v2: add bs check server #2463

Closed
wants to merge 4 commits into from

Conversation

pengpengSir
Copy link
Contributor

What problem does this PR solve?

Issue Number: #2355

Problem Summary: support bs check server command in curve tool

What is changed and how it works?

What's Changed:
add tools-v2/pkg/cli/command/curvebs/check/server/server.go
add tools-v2/pkg/cli/command/curvebs/check/server/get_copyset_ids.go
modify tools-v2/internal/utils/row.go
modify tools-v2/pkg/config/bs.go
modify tools-v2/pkg/cli/command/curvebs/check/check.go

How it Works:

Usage:  curve bs check server [flags]

check all copysets infomation on server

Flags:
  -c, --conf string           config file (default is $HOME/.curve/curve.yaml or /etc/curve/curve.yaml)
  -f, --format string         output format (json|plain) (default "plain")
  -h, --help                  print help
      --mdsaddr string        mds address, should be like 127.0.0.1:6700,127.0.0.1:6701,127.0.0.1:6702
      --port int              port
      --rpcretrytimes int32   rpc retry times (default 1)
      --rpctimeout duration   rpc timeout (default 10s)
      --serverid int          server id
      --serverip string       server ip
      --showerror             display all errors in command
      --verbose               show some log

Examples:
$ curve bs check server

command result

+--------+-----------+-------+-------------------------+-------------------------+
| SERVER |    IP     | TOTAL | UNHEALTHY COPYSET COUNT | UNHEALTHY COPYSET RATIO |
+--------+-----------+-------+-------------------------+-------------------------+
| 1      | 127.0.0.1 | 100   | 0                       | 0                       |
+--------+-----------+-------+-------------------------+-------------------------+

Side effects(Breaking backward compatibility? Performance regression?):

Check List

  • Relevant documentation/comments is changed or added
  • I acknowledge that all my contributions will be made under the project's license

@zhanghuidinah zhanghuidinah requested a review from Cyber-SiKu May 9, 2023 02:23
@Cyber-SiKu
Copy link
Contributor

when i try to build this, get such error:

$ go build -gcflags='all=-N -l'  -o sbin/curve ./cmd/curve/main.go
# github.com/opencurve/curve/tools-v2/pkg/cli/command/curvebs/check/server
pkg/cli/command/curvebs/check/server/get_copyset_ids.go:75:9: undefined: config.AddBsChunkServerIDFlag
pkg/cli/command/curvebs/check/server/get_copyset_ids.go:76:9: undefined: config.AddBsHostIpFlag
pkg/cli/command/curvebs/check/server/get_copyset_ids.go:85:22: assignment mismatch: 2 variables but config.GetBsFlagUint32 returns 1 value
pkg/cli/command/curvebs/check/server/get_copyset_ids.go:87:13: assignment mismatch: 2 variables but config.GetBsFlagUint32 returns 1 value
pkg/cli/command/curvebs/check/server/server.go:107:17: assignment mismatch: 2 variables but config.GetBsFlagUint32 returns 1 value
pkg/cli/command/curvebs/check/server/server.go:109:13: assignment mismatch: 2 variables but config.GetBsFlagUint32 returns 1 value
pkg/cli/command/curvebs/check/server/server.go:181:10: undefined: config.AddBsChunkServerIDFlag
pkg/cli/command/curvebs/check/server/server.go:182:10: undefined: config.AddBsHostIpFlag


type CheckServerRpc struct {
Info *basecmd.Rpc
Request *topology.ListChunkServerRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

add listchunkserver in list.
However, other pr is already doing this part of the content, and I have explained the relevant amendments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or you can communicate with him and ask him to reserve a corresponding interface so that you can continue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already talked to him, waiting for his response.

Comment on lines 76 to 77
config.AddBsHostIpFlag(cCmd.Cmd)
config.AddBsPortFlag(cCmd.Cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

chunkaddr is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since hostip and port are separated in topology.GetCopySetsInChunkServerRequest, so i think original version is better.

@@ -1291,6 +1292,27 @@ Output:
+------------+-----------+--------+--------+--------+---------+
```

#### check server
Copy link
Contributor

Choose a reason for hiding this comment

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

Complement the comparison of old and new tools

Comparison of old and new commands

curve bs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

hostip := config.GetBsFlagString(cCmd.Cmd, config.CURVEBS_HOST_IP)
port := config.GetBsFlagUint32(cCmd.Cmd, config.CURVEBS_PORT)

// fmt.Printf("%d %s %d", chunkserverId, hostip, port)
Copy link
Contributor

Choose a reason for hiding this comment

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

rm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@Xinlong-Chen Xinlong-Chen May 23, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 121 to 123
}

// 根据ChunkServerID, HostIP, Port返回 copysetid2poolid
Copy link
Contributor

Choose a reason for hiding this comment

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

please use english

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.gitmodules Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
return output.FinalCmdOutputPlain(&cCmd.FinalCurveCmd)
}

func (cCmd *CopysetidsCommand) RunCommand(cmd *cobra.Command, args []string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can set Cmd.Error = err, to let client use this error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

tools-v2/pkg/cli/command/curvebs/check/server/server.go Outdated Show resolved Hide resolved
tools-v2/pkg/cli/command/curvebs/check/server/server.go Outdated Show resolved Hide resolved
tools-v2/pkg/cli/command/curvebs/check/server/server.go Outdated Show resolved Hide resolved

peerAddr := fmt.Sprintf("%s:%d", hostip, port)
for copysetid, poolid := range *copysetids2poolids {
pCmd.Cmd.ResetFlags()
Copy link
Contributor

Choose a reason for hiding this comment

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

can reuse flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got error when trying to reuse these flags

Copy link
Contributor

Choose a reason for hiding this comment

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

what error?

Got error when trying to reuse these flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I try to use GetCopysetStatus in GetCopysetStatusCommand. Flags I passed to GetCopysetStatusCommand accumulate in its Rpc list, so I think the possible way to reuse flags is GetCopysetStatusCommand offers a method to reset Rpc list.

Signed-off-by: pengpengSir <[email protected]>
@opencurveadmin
Copy link
Collaborator

@Cyber-SiKu PTAL

Copy link
Contributor

@Xinlong-Chen Xinlong-Chen left a comment

Choose a reason for hiding this comment

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

please take care of forementioned problems...
Without more problem for me now!

@@ -1405,6 +1428,7 @@ Output:
| curve_ops_tool check-copyset | curve bs check copyset |
| curve_ops_tool client-status | curve bs status client |
| curve_ops_tool check-operator | curve bs check operator |
| curve_ops_tool check-server | curve bs check server |
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete line 1444, and please don't delete the blank line at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -501,6 +513,38 @@ func AddBSCSUnhealthyOptionFlag(cmd *cobra.Command) {
AddBsBoolOptionFlag(cmd, CURVEBS_CS_UNHEALTHY, "unhealthy")
}

func AddBsServerIdFlag(cmd *cobra.Command) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func AddBsServerIdFlag(cmd *cobra.Command) {
func AddBsServerIdOptionFlag(cmd *cobra.Command) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

AddBSUint32OptionFlag(cmd, CURVEBS_SERVER_ID, "server id")
}

func AddBsServerIpFlag(cmd *cobra.Command) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func AddBsServerIpFlag(cmd *cobra.Command) {
func AddBsServerIpOptionFlag(cmd *cobra.Command) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

AddBsStringOptionFlag(cmd, CURVEBS_SERVER_IP, "server ip")
}

func AddBsPortFlag(cmd *cobra.Command) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func AddBsPortFlag(cmd *cobra.Command) {
func AddBsPortOptionFlag(cmd *cobra.Command) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

AddBsStringOptionFlag(cmd, CURVEBS_CHUNKSERVER_ID, "chunk server id")
}

func AddBsHostIpFlag(cmd *cobra.Command) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func AddBsHostIpFlag(cmd *cobra.Command) {
func AddBsHostIpOptionFlag(cmd *cobra.Command) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 117 to 124
CURVEBS_SERVER_ID = "serverid"
VIPER_CURVEBS_SERVER_ID = "curvebs.serverid"
CURVEBS_SERVER_IP = "serverip"
VIPER_CURVEBS_SERVER_IP = "curvebs.serverip"
CURVEBS_HOST_IP = "hostip"
VIPER_CURVEBS_HOST_IP = "curvebs.hostip"
CURVEBS_PORT = "port"
VIPER_CURVEBS_PORT = "curvebs.port"
Copy link
Contributor

Choose a reason for hiding this comment

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

If some flags are optional, please give default values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 112 to 113
ROW_UNHEALTHY_COPYSET_RATIO = "unhealthy coyset ratio"
ROW_UNHEALTHY_COPYSET_COUNT = "unhealthy copyset count"
Copy link
Contributor

Choose a reason for hiding this comment

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

format
Alignment, and I think, only keep ROW_UNHEALTHY_COPYSET, the following output is also good:
|23(10%)|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,244 @@
/*
* Copyright (c) 2022 NetEase Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2022 NetEase Inc.
* Copyright (c) 2023 NetEase Inc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

config.AddRpcRetryTimesFlag(cCmd.Cmd)
config.AddRpcTimeoutFlag(cCmd.Cmd)
config.AddBsChunkServerIDOptionFlag(cCmd.Cmd)
config.AddBsHostIpFlag(cCmd.Cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

Both hostip and serverip are essentially ip, can these two be called ip? Is it convenient to pass it on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


type ServerCommand struct {
basecmd.FinalCurveCmd
Rpc *listchunkserver.ListChunkServerRpc
Copy link
Contributor

Choose a reason for hiding this comment

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

Such an rpc can be independently used to make a command and then return the result so that other commands can reuse the rpc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: pengpengSir <[email protected]>
Comment on lines 113 to 114
var rpc *listchunkserver.ListChunkServerRpc
if sCmd.ServerIP != "" {
rpc = &listchunkserver.ListChunkServerRpc{
Request: &topology.ListChunkServerRequest{
Ip: &sCmd.ServerIP,
Port: &sCmd.Port,
},
Info: basecmd.NewRpc(mdsAddrs, timeout, retrytimes, "ListChunkServer"),
}
} else {
rpc = &listchunkserver.ListChunkServerRpc{
Request: &topology.ListChunkServerRequest{
ServerID: &sCmd.ServerID,
},
Info: basecmd.NewRpc(mdsAddrs, timeout, retrytimes, "ListChunkServer"),
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Here you can separate the list chunkserver rpc into a separate command, and then call this command. For example fileinfo.go
These things can be processed in the command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: pengpengSir <[email protected]>

for _, status := range copysetid2Status {
total++
if status.String() == "COPYSET_OP_STATUS_SUCCESS" {
Copy link
Contributor

Choose a reason for hiding this comment

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

use constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: pengpengSir <[email protected]>
@zhanghuidinah
Copy link
Member

cicheck

1 similar comment
@Cyber-SiKu
Copy link
Contributor

cicheck

Comment on lines +87 to +104
if ip != "" {
rpc := &GetChunkServerInfosRpc{
Request: &topology.ListChunkServerRequest{
Ip: &ip,
Port: &port,
},
Info: basecmd.NewRpc(mdsAddrs, timeout, retrytimes, "ListChunkServer"),
}
gCmd.Rpc = rpc
} else {
rpc := &GetChunkServerInfosRpc{
Request: &topology.ListChunkServerRequest{
ServerID: &serverid,
},
Info: basecmd.NewRpc(mdsAddrs, timeout, retrytimes, "ListChunkServer"),
}
gCmd.Rpc = rpc
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ip != "" {
rpc := &GetChunkServerInfosRpc{
Request: &topology.ListChunkServerRequest{
Ip: &ip,
Port: &port,
},
Info: basecmd.NewRpc(mdsAddrs, timeout, retrytimes, "ListChunkServer"),
}
gCmd.Rpc = rpc
} else {
rpc := &GetChunkServerInfosRpc{
Request: &topology.ListChunkServerRequest{
ServerID: &serverid,
},
Info: basecmd.NewRpc(mdsAddrs, timeout, retrytimes, "ListChunkServer"),
}
gCmd.Rpc = rpc
}
gCmd.Rpc = &GetChunkServerInfosRpc{
Request: &topology.ListChunkServerRequest{},
Info: basecmd.NewRpc(mdsAddrs, timeout, retrytimes, "ListChunkServer"),
}
if ip != "" {
gCmd.Rpc.Request.Ip = &ip
gCmd.Rpc.Request.Port = &port
} else {
gCmd.Rpc.Request.ServerID = &serverid
}

@Cyber-SiKu
Copy link
Contributor

Cyber-SiKu commented Jun 1, 2023

image
ip && total is wrong

func (cCmd *CopysetidsCommand) Init(cmd *cobra.Command, args []string) error {
timeout := config.GetFlagDuration(cCmd.Cmd, config.RPCTIMEOUT)
retrytimes := config.GetFlagInt32(cCmd.Cmd, config.RPCRETRYTIMES)
mdsAddrs, _ := config.GetBsMdsAddrSlice(cCmd.Cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not check the error

@SeanHai
Copy link
Contributor

SeanHai commented Jul 9, 2023

This pr has not progressed for a while, did you encounter any difficulties?

caoxianfei1 pushed a commit to caoxianfei1/curve that referenced this pull request Jul 23, 2023
Signed-off-by: caoxianfei1 <[email protected]>
caoxianfei1 pushed a commit to caoxianfei1/curve that referenced this pull request Jul 23, 2023
Signed-off-by: caoxianfei1 <[email protected]>
caoxianfei1 pushed a commit to caoxianfei1/curve that referenced this pull request Jul 23, 2023
Signed-off-by: caoxianfei1 <[email protected]>
caoxianfei1 pushed a commit to caoxianfei1/curve that referenced this pull request Jul 23, 2023
Signed-off-by: caoxianfei1 <[email protected]>
@caoxianfei1
Copy link
Contributor

Clost the pt because the feature has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants