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 status cluster #2530

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

caoxianfei1
Copy link
Contributor

@caoxianfei1 caoxianfei1 commented Jun 13, 2023

What problem does this PR solve?

Issue Number: #xxx

Problem Summary:

What is changed and how it works?

What's Changed:

How it Works:

curve bs status cluster

The output is as follows:

Services:

ROLE LEADER ONLINE OFFLINE
etcd 10.0.0.1:23790 10.0.0.2:23790
10.0.0.3:23790
mds 10.0.0.1:6700 10.0.0.2:6700
10.0.0.3:6700
chunkserver - 10.0.0.1:8200
10.0.0.2:8200
10.0.0.3:8200
snapshot 10.0.0.1:5555 10.0.0.2:5555
10.0.0.3:5555

Copyset:

POOLID TOTAL OK WARN ERROR
1 100 94 3 3

Space:

TYPE USED LEFT RECYCLABLE CREATED
physical 0 B 98 GiB - -

Cluster health is: ok

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

@Cyber-SiKu Cyber-SiKu requested review from Cyber-SiKu and Xinlong-Chen and removed request for Cyber-SiKu June 13, 2023 06:49
@Cyber-SiKu
Copy link
Contributor

cicheck

Copy link
Contributor

@Cyber-SiKu Cyber-SiKu left a comment

Choose a reason for hiding this comment

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

run fail

curve bs status cluster      
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xec8ef8]

goroutine 1 [running]:
github.com/opencurve/curve/tools-v2/pkg/cli/command/curvebs/status/cluster.(*ClusterCommand).RunCommand(0xc0001b89a0, 0xc0004ddb00, {0x1795258, 0x0, 0x0})
        /home/******/code/curve-backup/tools-v2/pkg/cli/command/curvebs/status/cluster/cluster.go:112 +0x238

@@ -459,6 +459,9 @@ var (
ErrBsListScanStatus = func() *CmdError {
return NewInternalCmdError(66, "list scan-status fail, err: %s")
}
ErrBsListSpaceStatus = func() *CmdError {
return NewInternalCmdError(66, "list space status fail, err: %s")
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
return NewInternalCmdError(66, "list space status fail, err: %s")
return NewInternalCmdError(67, "list space status fail, err: %s")

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.

good job

@@ -187,3 +188,20 @@ func (sCmd *SpaceCommand) RunCommand(cmd *cobra.Command, args []string) error {
func (sCmd *SpaceCommand) ResultPlainOutput() error {
return output.FinalCmdOutputPlain(&sCmd.FinalCurveCmd)
}

func GetSpaceStatus(caller *cobra.Command) (*interface{}, *tablewriter.Table, *cmderror.CmdError, cobrautil.ClUSTER_HEALTH_STATUS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

return *tablewriter.Table is ugly, user can use sCmd.Result by parsing it.

Copy link
Contributor Author

@caoxianfei1 caoxianfei1 Jun 14, 2023

Choose a reason for hiding this comment

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

What does mean? cmd.Result is all rows not a table obj and can not render it directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

What does mean? cmd.Result is all rows not a table obj and can not render it directly.

The table object is a list, and it is very ugly to match.

@@ -120,3 +121,19 @@ func NewStatusChunkServerCommand() *ChunkServerCommand {
func NewChunkServerCommand() *cobra.Command {
return NewStatusChunkServerCommand().Cmd
}

func GetChunkserverStatus(caller *cobra.Command) (*interface{}, *tablewriter.Table, *cmderror.CmdError, cobrautil.ClUSTER_HEALTH_STATUS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -238,3 +242,19 @@ func NewStatusClientCommand() *ClientCommand {
basecmd.NewFinalCurveCli(&clientCmd.FinalCurveCmd, clientCmd)
return clientCmd
}

func GetClientStatus(caller *cobra.Command) (*interface{}, *tablewriter.Table, *cmderror.CmdError, cobrautil.ClUSTER_HEALTH_STATUS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -92,9 +94,7 @@ func (cCmd *ClientCommand) Init(cmd *cobra.Command, args []string) error {
}

if len((*results).([]map[string]string)) == 0 {
retErr := cmderror.ErrBsGetClientStatus()
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this error number in tools-v2/internal/error/error.go?

Copy link
Contributor Author

@caoxianfei1 caoxianfei1 Jun 14, 2023

Choose a reason for hiding this comment

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

The err code is used by func GetClientStatus.

@@ -123,6 +123,10 @@ func (cCmd *ClientCommand) Print(cmd *cobra.Command, args []string) error {
}

func (cCmd *ClientCommand) RunCommand(cmd *cobra.Command, args []string) error {
if len(cCmd.metrics) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

when users use this command directly(he don't create any client), they will get nothings by this way. This is strange for users.

}

func (cCmd *ClusterCommand) AddFlags() {
config.AddRpcRetryTimesFlag(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.

all sub-command flags ought to add this command.

@@ -104,3 +105,19 @@ func NewStatusCopysetCommand() *CopysetCommand {
basecmd.NewFinalCurveCli(&copysetCmd.FinalCurveCmd, copysetCmd)
return copysetCmd
}

func GetCopysetsStatus(caller *cobra.Command) (*interface{}, *tablewriter.Table, *cmderror.CmdError, cobrautil.ClUSTER_HEALTH_STATUS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -204,7 +205,7 @@ func NewStatusEtcdCommand() *EtcdCommand {
return etcdCmd
}

func GetEtcdStatus(caller *cobra.Command) (*interface{}, *cmderror.CmdError, cobrautil.ClUSTER_HEALTH_STATUS) {
func GetEtcdStatus(caller *cobra.Command) (*interface{}, *tablewriter.Table, *cmderror.CmdError, cobrautil.ClUSTER_HEALTH_STATUS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Cyber-SiKu please take a look this change. It's conflicting with what we discussed earlier.

Copy link
Contributor

@Cyber-SiKu Cyber-SiKu Jun 14, 2023

Choose a reason for hiding this comment

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

@Cyber-SiKu please take a look this change. It's conflicting with what we discussed earlier.

Indeed, it seems strange to directly return a table here. I am thinking about whether this is possible? Directly synchronize the status cluster output format to the subcommand instead of simply setting it to noout? Calling this command in this way also has output, but the status cluster only cares about the inspection results of this step? But in this case, the output in jason format is difficult to handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why feel strange? I think that it should be understandable if the output of a command corresponds to a table object. status cluster is collection of these tables. The cmd.Result is all rows that seems like need to combine them into a table for output?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Xinlong-Chen @caoxianfei1
At present, the output of the status cluster is too much, and it is difficult to recognize if the table is directly output. Can you combine the output like this:
services

role leader online offline
mds
etcd
snapshot
chunkserver -

copyset

total ok warn error

space

... ... ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, I think both methods are ok.

@@ -195,7 +196,7 @@ func NewStatusMdsCommand() *MdsCommand {
return mdsCmd
}

func GetMdsStatus(caller *cobra.Command) (*interface{}, *cmderror.CmdError, cobrautil.ClUSTER_HEALTH_STATUS) {
func GetMdsStatus(caller *cobra.Command) (*interface{}, *tablewriter.Table, *cmderror.CmdError, cobrautil.ClUSTER_HEALTH_STATUS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -205,7 +206,7 @@ func NewStatusSnapshotCommand() *SnapshotCommand {
return snapshotCmd
}

func GetSnapshotStatus(caller *cobra.Command) (*interface{}, *cmderror.CmdError, cobrautil.ClUSTER_HEALTH_STATUS) {
func GetSnapshotStatus(caller *cobra.Command) (*interface{}, *tablewriter.Table, *cmderror.CmdError, cobrautil.ClUSTER_HEALTH_STATUS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

for key, function := range cCmd.type2Func {
result, table, err, health := function(cmd)
cCmd.type2Table[key] = table
results[key] = *result
Copy link
Contributor

Choose a reason for hiding this comment

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

The case where result is nil is not handled.

}
finalErr := cmderror.MergeCmdErrorExceptSuccess(errs)
cCmd.Error = finalErr
results["health"] = cobrautil.ClusterHealthStatus_Str[int32(cCmd.health)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the definition of the constant in cobrautil.

@caoxianfei1 caoxianfei1 force-pushed the feature/status-cluster branch 3 times, most recently from 03e7029 to 27fbff4 Compare June 20, 2023 09:49
@caoxianfei1 caoxianfei1 force-pushed the feature/status-cluster branch 2 times, most recently from 94f86e7 to ca20577 Compare June 21, 2023 06:33
Copy link
Contributor

@Cyber-SiKu Cyber-SiKu left a comment

Choose a reason for hiding this comment

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

LGTM

@Cyber-SiKu
Copy link
Contributor

cicheck

@Cyber-SiKu
Copy link
Contributor

@Xinlong-Chen you can take a look.

@caoxianfei1
Copy link
Contributor Author

@Xinlong-Chen 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.

still have some small questions~

retErr.Format(err.Error())
return nil, retErr, cobrautil.HEALTH_ERROR
}
fmt.Sprintln(sCmd.Result)
Copy link
Contributor

Choose a reason for hiding this comment

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

can delete debug 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

tools-v2/pkg/cli/command/curvebs/check/copyset/copyset.go Outdated Show resolved Hide resolved
Signed-off-by: caoxianfei1 <caoxianfei@corp.netease.com>
@Xinlong-Chen
Copy link
Contributor

cicheck

@Cyber-SiKu Cyber-SiKu merged commit 746f2c1 into opencurve:master Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants