feat: support one-click stop format #162

DemoLiang · 2022-11-22T07:58:11Z

feat: 支持一键停止磁盘格式化，包括：停止容器，删除fstab记录，umount 挂载的磁盘
test：简单测试了：单节点单盘、单节点多盘、多节点多盘

curveadm run result:

step1: check node docker,fstab, mountpoint not exist

step2: curveadm run format ,and check docker,fstab,mountpoint exist

step3: curveadm stop format ,check docker, fstab,mountpoint delete

#156

Wine93 · 2022-11-22T09:13:55Z

cli/command/format.go

@@ -38,7 +38,8 @@ import (
 const (
 	FORMAT_EXAMPLE = `Examples:
  $ curveadm format -f /path/to/format.yaml           # Format chunkfile pool with specified configure file
-  $ curveadm format --status -f /path/to/format.yaml  # Display formatting status`
+  $ curveadm format --status -f /path/to/format.yaml  # Display formatting status
+  $ curveadm format --stop -f /path/to/format.yaml  # stop formatting`


Maybe we should align the comment of command examples.

Maybe we should align the comment of command examples.

resolve

Wine93 · 2022-11-22T09:19:21Z

cli/command/format.go

@@ -73,6 +79,7 @@ func NewFormatCommand(curveadm *cli.CurveAdm) *cobra.Command {
 	flags := cmd.Flags()
 	flags.StringVarP(&options.filename, "formatting", "f", "format.yaml", "Specify the configure file for formatting chunkfile pool")
 	flags.BoolVar(&options.showStatus, "status", false, "Show formatting status")
+	flags.BoolVar(&options.stopFormat, "stop", false, "stop formatting status")


The usage message of stop option should be Stop formatting progress.

BTW: i think we shoud use subcommands to instead of all options, like:

curveadm format start curveadm format status curveadm format stop

resolve

current subcommands not support in format?

so I keep the same write as status

I real forget why we use option instead of subcommands, but you can change it if you interested it.

The usage message of stop option should be Stop formatting progress.

BTW: i think we shoud use subcommands to instead of all options, like:

curveadm format start curveadm format status curveadm format stop

Usage message:

flags.BoolVar(&options.stopFormat, "stop", false, "Stop formatting progress")

not

flags.BoolVar(&options.stopFormat, "stop", false, "stop formatting status")

resove the usage

i think we should keep the option with pre version ,stop and status

subcommands can rewrite in next version for status , stop and others

Wine93 · 2022-11-22T09:20:45Z

cli/command/format.go

@@ -49,11 +50,16 @@ var (
 	FORMAT_STATUS_PLAYBOOK_STEPS = []int{
 		playbook.GET_FORMAT_STATUS,
 	}
+	// FORMAT_STOP_PLAYBOOK_STEPS 停止格式化step


Please use english comment here.

Wine93 · 2022-11-22T09:21:22Z

internal/playbook/factory.go

@@ -114,6 +114,9 @@ const (
 	REMOVE_PLAYGROUND
 	GET_PLAYGROUND_STATUS

+	// STOP_FORMAT 停止format


Wine93 · 2022-11-22T09:21:50Z

internal/task/task/bs/format_stop.go

+	}
+)
+
+// skipStopFormat 跳过停止format操作，当前不存在format的容器


Wine93 · 2022-11-22T09:21:58Z

internal/task/task/bs/format_stop.go

+	}
+}
+
+// expression 生成要删除或者添加的fstab的sed表达式


Wine93 · 2022-11-22T09:22:12Z

internal/task/task/bs/format_stop.go

+	}
+}
+
+// execute 执行删除fstab记录


Wine93 · 2022-11-22T09:23:32Z

internal/task/task/bs/format_stop.go

+	})
+}
+
+// stopContainer 停止container


ilixiaocui · 2022-11-22T09:23:34Z

Thanks you for the pr!

Please post the screenshot of the execution result of curveadm fromat.

Wine93 · 2022-11-22T09:23:42Z

internal/task/task/bs/format_stop.go

+	return nil
+}
+
+// NewStopFormatTask 新建停止stop format任务


Wine93 · 2022-11-22T09:30:04Z

internal/task/task/bs/format_stop.go

+}
+
+// execute 执行删除fstab记录
+func (s *deleteFSTab) execute(ctx *context.Context) error {


The step deleteFSTab is similar to step step2EditFSTab, you can union them into one, and provide one parameter to determine whether to add record into fstab.

resolve this issue ,remove deleteFsTabe
add skipAdd for edit fstab to skip add record for delete

Wine93 · 2022-11-22T09:35:56Z

internal/task/task/bs/format_stop.go

+	device := fc.GetDevice()
+	mountPoint := fc.GetMountPoint()
+	usagePercent := fc.GetFormatPercent()
+	subname := fmt.Sprintf("host=%s device=%s mountPoint=%s usage=%d%%",


Maybe the container id (or name) is more useful than the usage percent in stop task.

but we can't get containerid ,if task step not run?

but we can't get containerid ,if task step not run?

There is a fixed method to generate container name for format task:

containerName := device2ContainerName(device)

modify it ,replace the usagePercent with containerName

aspirer · 2022-11-22T09:41:51Z

cli/command/format.go

@@ -38,7 +38,8 @@ import (
 const (
 	FORMAT_EXAMPLE = `Examples:
  $ curveadm format -f /path/to/format.yaml           # Format chunkfile pool with specified configure file
-  $ curveadm format --status -f /path/to/format.yaml  # Display formatting status`
+  $ curveadm format --status -f /path/to/format.yaml  # Display formatting status
+  $ curveadm format --stop -f /path/to/format.yaml  # stop formatting`


you can use gofmt or ide auto format the code style.

resolve ,format it
gofmt not support strings usage format ,

Wine93 · 2022-11-22T09:48:23Z

internal/task/task/bs/format_stop.go

+	})
+
+	// 2: stop container
+	t.AddStep(&stopContainer{


Maybe we shoud place the step stopContainer to the after of step deleteFSTab, because we will skip this task if container not exist even if the record in fstab is not deleted, and the last stop task which abort after stop container will make this problem.

yeh ,we have place the step of stopContainer and deleteFSTab
first delete fstab record
next stop container

Wine93 · 2022-11-22T09:53:33Z

Thank for you contribute @DemoLiang , maybe some improvements can make this feature become better :)

Wine93 · 2022-11-23T01:48:52Z

internal/task/task/bs/format.go

-		InPlace:     true,
-		ExecOptions: curveadm.ExecOptions(),
-	})
+	if s.skipAdd {


Wrong judge, i think you want to express is that the step will not add new record if the parameter skipAdd is true, is it right?

yeh ,Thanks you review it
we have two curveadm project for backup
and test is deleteFSTab ,so I did not find the error
I've resolve it and retested uploading tested pass images

Wine93

LGTM!

Cyber-SiKu · 2022-11-25T06:16:18Z

internal/task/task/bs/format_stop.go

@@ -0,0 +1,133 @@
+/*
+ *  Copyright (c) 2021 NetEase Inc.


Cyber-SiKu · 2022-11-25T06:31:18Z

cli/command/format.go

 	flags.BoolVar(&options.showStatus, "status", false, "Show formatting status")
+	flags.BoolVar(&options.stopFormat, "stop", false, "Stop formatting progress")


Are the flag stop and status mutually exclusive?

check the option ,If both option is true, report an error config and return

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

Cyber-SiKu

LGTM!

DemoLiang changed the title ~~feat: #156 support one-click stop format~~ feat: support one-click stop format Nov 22, 2022

ilixiaocui requested review from Wine93, ilixiaocui and cw123 and removed request for ilixiaocui and cw123 November 22, 2022 08:55

Wine93 reviewed Nov 22, 2022

View reviewed changes

aspirer reviewed Nov 22, 2022

View reviewed changes

Wine93 reviewed Nov 22, 2022

View reviewed changes

Wine93 reviewed Nov 23, 2022

View reviewed changes

Wine93 previously approved these changes Nov 23, 2022

View reviewed changes

Cyber-SiKu reviewed Nov 25, 2022

View reviewed changes

feat: #156 support one-click stop format

0070f43

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

DemoLiang dismissed Wine93’s stale review via 0070f43 November 27, 2022 06:15

Cyber-SiKu approved these changes Nov 28, 2022

View reviewed changes

ilixiaocui removed their request for review November 28, 2022 02:59

Wine93 approved these changes Nov 28, 2022

View reviewed changes

Wine93 merged commit 6a46cc5 into opencurve:master Nov 28, 2022

		flags.BoolVar(&options.showStatus, "status", false, "Show formatting status")
		flags.BoolVar(&options.stopFormat, "stop", false, "Stop formatting progress")

feat: support one-click stop format #162

feat: support one-click stop format #162

Conversation

DemoLiang commented Nov 22, 2022 • edited Loading

Wine93 Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilixiaocui commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 commented Nov 22, 2022 • edited Loading

Wine93 Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

DemoLiang Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Wine93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cyber-SiKu left a comment

Choose a reason for hiding this comment

DemoLiang commented Nov 22, 2022 •

edited

Loading

Wine93 Nov 22, 2022 •

edited

Loading

Wine93 Nov 22, 2022 •

edited

Loading

Wine93 Nov 22, 2022 •

edited

Loading

Wine93 Nov 22, 2022 •

edited

Loading

Wine93 commented Nov 22, 2022 •

edited

Loading

Wine93 Nov 23, 2022 •

edited

Loading

DemoLiang Nov 23, 2022 •

edited

Loading