Feature: support managing disk information in database #188

legionxiong · 2023-02-24T05:13:02Z

With database disk records commited by disks.yaml,
disk formatting cloud be performed without format.yaml.

And it will get and wirte disk size and URI during
disk formatting, thus record the ID of service(chunkserver)
with associated disk when deploy curvebs cluster. Use
curveadm disks ls to view all disk information.

Usage:
        curveadm disks commit /path/to/disks.yaml
        curveadm disks ls
        curveadm format

[root@curve-1 curvebs]# ./curveadm disks show
global:
  format_percent: 90
  host:
    - curve-1
    - curve-2
    - curve-3
disk:
  - device: /dev/sdb1
    mount: /data/chunkserver0
  - device: /dev/sdc1
    mount: /data/chunkserver1

[root@curve-1 curvebs]# ./curveadm disks ls
Host     Device Path  Device Size  Device URI                                     Disk Format Mount Point  Service ID
----     -----------  -----------  ----------                                     -----------------------  ----------
curve-1  /dev/sdb1    21472739328  fs:uuid//077c677d-144c-4faf-98d5-b1b9f012b5a6  /data/chunkserver0       81793d01ae46
curve-1  /dev/sdc1    21472739328  fs:uuid//fded239b-78bd-4c20-9a53-80c9cdcbcd4c  /data/chunkserver1       3bc6410788cb
curve-2  /dev/sdb1    21472739328  fs:uuid//44198367-0020-4963-9ed4-409eaa2bad68  /data/chunkserver0       15551b243c70
curve-2  /dev/sdc1    21472739328  fs:uuid//286aade7-cb7c-4630-9155-f7935bdc2903  /data/chunkserver1       d24e9354cb01
curve-3  /dev/sdb1    21472739328  fs:uuid//eab438fa-fd11-44b5-93c4-a131a69cfd86  /data/chunkserver0       9cf41f087ef4
curve-3  /dev/sdc1    21472739328  fs:uuid//10e3220b-c598-4cd7-8927-22df166a2a91  /data/chunkserver1       93d4f9eba5fe

Wine93 · 2023-03-01T03:33:51Z

cli/cli/cli.go

-	clusterPoolData     string // cluster pool
+	hosts               string         // hosts
+	disks               string         // disks
+	diskRecords         []storage.Disk // disk


How about diskTable, like fstab?

diskTable refers to a database table that holding records, diskRecords refers to the disk records queried from database.

Wine93 · 2023-03-01T03:34:27Z

cli/command/disks/commit.go

+
+	// write disk records
+	for _, hc := range hcs {
+		for _, dc := range dcs {


You can wrap the below logic into one function.
BTW: i think you can make the function ParseDisk more powerful, and let it return the final disk config which already handle hosts_exclude and hosts_exclude, and then insert the records into database.

Good point.

Wine93 · 2023-03-01T03:34:50Z

cli/command/disks/commit.go

+	}
+
+	// sync disk records
+	diskRecords := curveadm.DiskRecords()


I think the below logic is a bit complex, you can provide a function to get the difference bettwen new records and old records, then to handle the difference, like receive the commit or denied.
Maybe you can refer checkScaleOutTopology, and if you refuse the commit, you can provide some more meaningful error codes, like ERR_DELETE_BINDING_DISK_IS_DENIED.

Wine93 · 2023-03-01T03:36:06Z

configs/bs/cluster/disks.yaml

@@ -0,0 +1,18 @@
+global:
+  format_percent: 90
+


I think maybe we should provide the globalhosts section to let user specify the all host which will to formatted, otherwise user needs to setting many hosts_exclude or hosts_only if there are many hosts. if using hosts, user can focus on a part hosts quicky, and then using hosts_exclude and hosts_only to achieve its aim.

global: format_percent: 95 hosts: - curve1 - curve2 - curve3 disks: ...

BTW, how do you think about the below configure design? I think:

It might be more easier to understand.

The level design SAME AS our other configures, like topology.

The logic of parse disks configure file is more simpler.

global: format_percent: 95 hosts: - curve-1 - curve-2 - curve-3 disks: - device: /dev/sdb1 mount: /data/chunkserver0 - device: /dev/sdc1 mount: /data/chunkserver1 format_percent: 90 - device: /dev/sdd1 mount: /data/chunkserver2 hosts: - curve-1 - curve-2 - device: /dev/sde1 mount: /data/chunkserver3 hosts: - curve1

Wine93 · 2023-03-01T06:23:11Z

cc @tsonglew

tsonglew · 2023-03-01T15:37:59Z

internal/utils/common.go

@@ -216,6 +216,22 @@ func Slice2Map(s []string) map[string]bool {
 	return m
 }

+func InterfaceSlice2Map(s []interface{}) map[interface{}]bool {


it's better to implement a generic function to replace Slice2Map and InterfaceSlice2Map in my opinion, since Go 1.18 is used on develop branch.

Sure, so glad to use Go 1.18.

Add GenericSlice2Map and keep Slice2Map for old references.

tsonglew · 2023-03-01T15:44:08Z

internal/configure/disks/disks.go

+
+				hostExclude := dc.GetHostExclude()
+				if len(hostExclude) > 0 {
+					hosts := []interface{}{}


the variable name hosts collides with "github.com/opencurve/curveadm/internal/configure/hosts", should we consider another name for it?

tsonglew · 2023-03-01T15:56:36Z

internal/configure/disks/disks.go

+		return nil, errno.ERR_PARSE_DISKS_FAILED.E(err)
+	}
+
+	dcs := []*DiskConfig{}


should we use var dcs []*DiskConfig to declare the empty slice, as mentioned in go/wiki/CodeReviewComment

tsonglew · 2023-03-02T04:44:29Z

internal/utils/common.go

@@ -216,6 +222,26 @@ func Slice2Map(s []string) map[string]bool {
 	return m
 }

+func GenericSlice2Map[T string | int | any](t []T) booleanMap {


how about turning Slice2Map into

func Slice2Map[T comparable](t []T) map[T]bool { m := map[T]bool{} for _, item := range t { m[item] = true } return m }

and calling it from internal/configure/disks/disks.go in this way ?

hostExcludeMap := utils.Slice2Map(hostExclude) // ... if _, ok := hostExcludeMap[h]; !ok { // ... }

but interface is not comparable

My bad, Go1.20 is required to do so.

legionxiong · 2023-03-02T06:08:46Z

@tsonglew any other suggestions?

Wine93 · 2023-03-02T08:33:23Z

cli/command/disks/commit.go

+}
+
+func runCommit(curveadm *cli.CurveAdm, options commitOptions) error {
+


redundant: blank line.

Probably we need an action for style check. I'd like to put it into practice with another pr

Probably we need an action for style check. I'd like to put it into practice with another pr

That's great :)

Wine93 · 2023-03-02T08:40:13Z

configs/bs/cluster/disks.yaml

+    mount: /data/chunkserver1
+  - device: /dev/sdd1
+    mount: /data/chunkserver2
+    host_exclude:  # for the use case that all hosts have this disk device except some hosts


We should keep the config template and the implemetion consistent.

disks.yaml template and the implementation is consistent with double check. It would be helpful with more specific comments.

We should keep the config template and the implemetion consistent.

Note, disks configure item names disk and host are used in implementation, not disks and hosts.

global: format_percent: 95 host: - curve-1 - curve-2 - curve-3 disk: - device: /dev/sdb1 mount: /data/chunkserver0 - device: /dev/sdc1 mount: /data/chunkserver1 format_percent: 90 - device: /dev/sdd1 mount: /data/chunkserver2 exclude: - curve-1 - device: /dev/sde1 mount: /data/chunkserver3 host: - curve-1 - curve-2

Disks struct { Global map[string]interface{} `mapstructure:"global"` Disk []map[string]interface{} `mapstructure:"disk"` }

Wine93 · 2023-03-02T08:45:04Z

internal/configure/common/item_set.go

@@ -35,6 +35,7 @@ const (
 	REQUIRE_BOOL
 	REQUIRE_INT
 	REQUIRE_POSITIVE_INTEGER
+	REQUIRE_SLICE


Now we add the type REQUIRE_SLICE, but there is no corresponding checker implementation for it.

Wine93 · 2023-03-02T08:47:55Z

internal/configure/disks/dc_item.go

+	)
+
+	CONFIG_DISK_HOST_EXCLUDE = itemset.Insert(
+		common.DISK_HOST_EXCLUDE,


Maybe we can remove the host_exclude and host_only configure item.

Are there any good solutions to support different disk number among hosts?

What about the use case that disk number is different among hosts?

Hi, guys, for solving the problem that the disk list in each hosts is different while formatting, we have provide two designs which show as below:

Design 1: using host_only and host_exclude

global: format_percent: 95 host: - curve-1 - curve-2 - curve-3 disk: - device: /dev/sdb1 mount: /data/chunkserver0 - device: /dev/sdc1 mount: /data/chunkserver1 - device: /dev/sdd1 mount: /data/chunkserver2 host_exclude: # for the use case that all hosts have this disk device except some hosts - curve-3 - device: /dev/sde1 mount: /data/chunkserver3 host_only: # for the use case that only some hosts have this disk device - curve-1 - curve-2

from @legionxiong

Design 2: using level design

global: format_percent: 95 hosts: - curve-1 - curve-2 - curve-3 disks: - device: /dev/sdb1 mount: /data/chunkserver0 - device: /dev/sdc1 mount: /data/chunkserver1 format_percent: 90 - device: /dev/sdd1 mount: /data/chunkserver2 hosts: - curve-1 - curve-2 - device: /dev/sde1 mount: /data/chunkserver3 hosts: - curve1 - curve2

pros:

It might be more easier to understand.

The level design SAME AS our other configures, like topology.

what's your opinion? cc @aspirer @Cyber-SiKu @tsonglew

Or different disk number among hosts is not expected for curvebs, but what about the different disk device path?

Design 2 is much easier than host_only use case 👍 , but for host_exclude use case it may be a bit painful. And disk devices among hosts may be changed from time to time.

I would prefer the 2nd and the 3rd among designs above. And it seems hosts_exclude is a more explicit name than exclude

Agree, hosts_exclude is more explicit, but disk configure has nothing to exclude except hosts. @Wine93 How do you think about this?

I would prefer the 2nd and the 3rd among designs above. And it seems hosts_exclude is a more explicit name than exclude

Agree, hosts_exclude is more explicit, but disk configure has nothing to exclude except hosts. @Wine93 How do you think about this?

As you said, it is still easy to understand if we clearly describe what exclude does in our WIKI, like hosts. I agree the design 3 shown above.

OK, how to add / update curveadm WiKi? We need add a disks page.

OK, how to add / update curveadm WiKi? We need add a disks page.

You can refer to how-can-i-make-a-pull-request-for-a-wiki-page-on-github, and our wiki git address is https://github.com/opencurve/curveadm.wiki.git.

OK, how to add / update curveadm WiKi? We need add a disks page.

You can refer to how-can-i-make-a-pull-request-for-a-wiki-page-on-github, and our wiki git address is https://github.com/opencurve/curveadm.wiki.git.

It seems I have no privilege to view curveadm wilki repo, got 404 :(

Wine93 · 2023-03-02T08:51:46Z

cli/command/disks/list.go

+	var err error
+	var diskRecords []storage.Disk
+	// diskData := curveadm.Disks()
+	// err = syncDiskRecords(diskData, curveadm, options)


You should fix it, it not works.

Wine93 · 2023-03-03T02:15:23Z

@legionxiong Thank you for your contribution and patience, there are a few small things to do before merge, please:

Merge the commits into one.
Remove the unrelated commit, b2cbbcb.
Modify your commit message: capitalize the first word and let the commit message more clearly and meaningful.

With database disk records commited by `disks.yaml`, disk formatting cloud be performed without `format.yaml`. And it will get and wirte disk `size` and `URI` during disk formatting, thus record the ID of service(chunkserver) with associated disk when deploy curvebs cluster. Use `curveadm disks ls` to view all disk information. Usage: curveadm disks commit /path/to/disks.yaml curveadm disks ls curveadm format Signed-off-by: Lijin Xiong <[email protected]>

legionxiong · 2023-03-03T03:25:08Z

@legionxiong Thank you for your contribution and patience, there are a few small things to do before merge, please:

Merge the commits into one.

Remove the unrelated commit, b2cbbcb.

Modify your commit message: capitalize the first word and let the commit message more clearly and meaningful.

All done.

Wine93

LGTM!

legionxiong · 2023-03-03T03:51:02Z

Shall we merge it?

tsonglew · 2023-03-03T04:32:12Z

LGTM!

legionxiong mentioned this pull request Feb 24, 2023

add another way to get disk config for formatting. #182

Closed

legionxiong requested a review from Wine93 February 24, 2023 05:20

legionxiong force-pushed the new-disk-format branch from 4379c61 to 6798511 Compare February 24, 2023 10:21

Wine93 reviewed Mar 1, 2023

View reviewed changes

tsonglew approved these changes Mar 1, 2023

View reviewed changes

tsonglew self-requested a review March 2, 2023 00:45

legionxiong force-pushed the new-disk-format branch 3 times, most recently from 968ecba to c5db7e7 Compare March 2, 2023 04:08

tsonglew reviewed Mar 2, 2023

View reviewed changes

legionxiong force-pushed the new-disk-format branch from c5db7e7 to 9e1956a Compare March 2, 2023 05:34

legionxiong requested a review from Wine93 March 2, 2023 07:16

legionxiong force-pushed the new-disk-format branch 2 times, most recently from 0ef0519 to 536425e Compare March 2, 2023 08:35

Wine93 reviewed Mar 2, 2023

View reviewed changes

legionxiong force-pushed the new-disk-format branch from 66c8819 to 5f6dd63 Compare March 2, 2023 16:15

legionxiong force-pushed the new-disk-format branch from 5f6dd63 to 3842a59 Compare March 3, 2023 03:23

legionxiong force-pushed the new-disk-format branch from 3842a59 to de36749 Compare March 3, 2023 03:24

legionxiong changed the title ~~add another way to get config for disk formatting~~ Feature: support managing disk information in database Mar 3, 2023

Wine93 approved these changes Mar 3, 2023

View reviewed changes

tsonglew merged commit af342de into opencurve:develop Mar 3, 2023

		}

		func runCommit(curveadm *cli.CurveAdm, options commitOptions) error {

Feature: support managing disk information in database #188

Feature: support managing disk information in database #188

Conversation

legionxiong commented Feb 24, 2023 • 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

Wine93 commented Mar 1, 2023 • 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

legionxiong commented Mar 2, 2023

Choose a reason for hiding this comment

tsonglew Mar 2, 2023 • 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 Mar 2, 2023 • 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

Wine93 Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Design 1: using host_only and host_exclude

Design 2: using level design

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

Wine93 commented Mar 3, 2023 • edited Loading

legionxiong commented Mar 3, 2023

Wine93 left a comment

Choose a reason for hiding this comment

legionxiong commented Mar 3, 2023

tsonglew commented Mar 3, 2023

legionxiong commented Feb 24, 2023 •

edited

Loading

Wine93 commented Mar 1, 2023 •

edited

Loading

tsonglew Mar 2, 2023 •

edited

Loading

Wine93 Mar 2, 2023 •

edited

Loading

Wine93 Mar 2, 2023 •

edited

Loading

Design 1: using `host_only` and `host_exclude`

Wine93 commented Mar 3, 2023 •

edited

Loading