Improve(hosts): use more meaningful name for host name #330

tiansuo114 · 2023-11-05T14:00:46Z

This PR is about issue #260. I followed the advice of the instructor in the last PR and changed the host (keyword used to express the host identifier) in the configuration file required by curveadm hosts to name, and retained it. hostname keyword

Tests I did:

Under the curveadm hosts command, this keyword can work normally, and all commands under hosts can work normally.
Under curveadm format, the disk can be formatted normally.
The entire process of deploying curvefs can be carried out normally.

The tests I have done may still be incomplete. I hope to get the code review from the instructor and give further suggestions for modifications.

这个pr是有关于issue #260的,我遵循了上一次pr中导师提出的意见,将curveadm hosts 所需的配置文件中的host(用来表达主机标识符的关键字)更改为了name,并保留了hostname关键字

我所做的测试:
1.curveadm hosts命令下,该关键字可以正常发挥效果,hosts下的所有指令均能正常工作
2.curveadm format下,可以正常对磁盘进行格式化
3.可以正常进行整套部署curvefs的流程

我所做的测试可能仍不完备,希望能得到导师的代码审查,并给出更进一步的修改意见

caoxianfei1 · 2023-11-06T02:18:29Z

It's a good job, but we need to backwards compatible。It means that host and name are allowed to be configured

tiansuo114 · 2023-11-06T02:52:53Z

It's a good job, but we need to backwards compatible。It means that host and name are allowed to be configured

Yes, I know that backward compatibility is a very important option, so I modified the get method in internal/configure/hosts/hc_get.go so that when using the GetName method, it first retrieves whether there is an attribute corresponding to the host keyword. Make a judgment and then decide to return the corresponding attribute. Through this operation, backward compatibility can be achieved. The hosts.yaml file with the attributes host and name can be deployed normally. My compatibility method may still not be correct. If so, Please guide me if there is anything that needs correction or a better way. Thank you very much.

caoxianfei1 · 2023-11-07T02:12:48Z

It's a good job, but we need to backwards compatible。It means that host and name are allowed to be configured

Yes, I know that backward compatibility is a very important option, so I modified the get method in internal/configure/hosts/hc_get.go so that when using the GetName method, it first retrieves whether there is an attribute corresponding to the host keyword. Make a judgment and then decide to return the corresponding attribute. Through this operation, backward compatibility can be achieved. The hosts.yaml file with the attributes host and name can be deployed normally. My compatibility method may still not be correct. If so, Please guide me if there is anything that needs correction or a better way. Thank you very much.

got. but you must ensue replace all getHost() with getName().

tiansuo114 · 2023-11-07T03:47:01Z

It's a good job, but we need to backwards compatible。It means that host and name are allowed to be configured

Yes, I know that backward compatibility is a very important option, so I modified the get method in internal/configure/hosts/hc_get.go so that when using the GetName method, it first retrieves whether there is an attribute corresponding to the host keyword. Make a judgment and then decide to return the corresponding attribute. Through this operation, backward compatibility can be achieved. The hosts.yaml file with the attributes host and name can be deployed normally. My compatibility method may still not be correct. If so, Please guide me if there is anything that needs correction or a better way. Thank you very much.

got. but you must ensue replace all getHost() with getName().

Ok,my mistake😭😭,my understanding of the entire system is still not deep enough. There may still be some places that need to be changed that I have not noticed. I will search the code again to determine if there are other places that need to be changed. I will modify the code as soon as possible and submit it. I hope to get it again. Tutor's help

Wine93 · 2023-11-22T02:27:23Z

Hi @tiansuo114, thank you for your contribute :)
Have you tested all related commands for this commit, the hosts module is the basic module, mant commands rely on it, we need gurantee all related commands works well.

tiansuo114 · 2023-11-22T14:25:09Z

Hi @tiansuo114, thank you for your contribute :)您好，感谢您的贡献:) Have you tested all related commands for this commit, the hosts module is the basic module, mant commands rely on it, we need gurantee all related commands works well.您是否测试了此提交的所有相关命令，该 hosts 模块是基本模块，mant 命令依赖于它，我们需要保证所有相关命令都能正常工作。

I have tested the deployment of the curvefs/bs cluster, curvefs/bs client, console, curvefs/bs web monitoring, disk formatting, and other modules. Except for the cluster deployment module, all other modules run correctly when using the host/name keywords.

However, I have a question regarding the cluster topology module. In the configuration file of this module, there are already host keywords that should be replaced and a name keyword that is automatically set according to rules if not explicitly declared. I'm unsure how to handle the host keyword in the cluster topology module.

If no handling is required, I will finalize the code and push the final version after conducting another round of testing.

Wine93 · 2023-11-23T01:42:58Z

cli/command/client/map.go

-  $ curveadm map user:/volume --host machine1 --size=10GiB --create     # Map volume which size is 10GiB and created by automatic
-  $ curveadm map user:/volume --host machine1 --create --poolset ssd    # Map volume created by automatic in poolset 'ssd'
-  $ curveadm map user:/volume --host machine1 -c /path/to/client.yaml   # Map volume with specified configure file`
+  $ curveadm map user:/volume --name machine1 --create                  # Map volume which created by automatic


Maybe we should keep the command option, i think the host is more meaningful.

Wine93 · 2023-11-23T01:46:00Z

BTW: I think we should only convert the host to name in hosts.yaml.

tiansuo114 · 2023-11-23T01:59:05Z

BTW: I think we should only convert the host to name in hosts.yaml.顺便说一句：我认为我们应该只将 host 转换为 name hosts.yaml .

Okay, this is my understanding issue, I will modify the code as soon as possible

caoxianfei1 · 2023-12-04T08:27:30Z

@tiansuo114 Pls commit to develop branch again instead of tombstone

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

montaguelhz

Commit message requires attention to the specification

montaguelhz · 2023-12-06T02:53:24Z

cli/cli/cli.go

 			return hc, nil
 		}
 	}
 	return nil, errno.ERR_HOST_NOT_FOUND.
-		F("host: %s", host)
+		F("host: %s", name)


Does the host printed here need to be replaced？

montaguelhz · 2023-12-06T02:54:04Z

internal/configure/hosts/hosts.go

-				F("duplicate host: %s", hc.GetHost())
+		if _, ok := exist[hc.GetName()]; ok {
+			return nil, errno.ERR_DUPLICATE_NAME.
+				F("duplicate host: %s", hc.GetName())


tiansuo114 · 2023-12-06T07:34:55Z

errno.ERR_DUPLICATE_NAME.
F("duplicate host: %s", hc.GetName())

The contributor's suggested modification in the previous comment is as follows:
In the output string of errno.ERR_DUPLICATE_NAME.F("duplicate host: %s", hc.GetName()), the word "host" should be replaced with a more meaningful identifier.

I guess that maintaining this format, similar to "i-class item: j-item", can make it easier to identify the part that needs modification.

Additionally, the contributor also raised the question of whether it should be changed to "host name" identifier because the "hostname" keyword is already used in the hosts module to represent the host's IP address. I guess that using this output format might cause confusion.

If there is a mistake in my understanding, I will make corrections as soon as possible according to the correction opinions of community members.

Wine93 · 2023-12-07T05:34:59Z

internal/configure/hosts/hc_item.go

+		comm.REQUIRE_STRING,
+		false,
+		nil,
+	)


注意换行

my mistake, i will fix it

Wine93 · 2023-12-07T05:36:06Z

internal/configure/hosts/hc_get.go

@@ -67,7 +67,17 @@ func (hc *HostConfig) getBool(i *comm.Item) bool {
 	return v.(bool)
 }

+func (hc *HostConfig) getName() string {


这个函数可以去掉，每个配置项都提供了一个默认值的钩子，你可以在上面做事情，可以参见 CONFIG_USER.

Thank you for the guidance. Following your advice, I have re-implemented this functionality in the hc_item. If there are further areas that need modification, I would appreciate your guidance once again.

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

Wine93 reviewed Nov 23, 2023

View reviewed changes

tiansuo114 changed the base branch from tombstone to develop December 5, 2023 03:14

change host to name in hosts moudle

aadfa52

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

montaguelhz reviewed Dec 6, 2023

View reviewed changes

tiansuo114 changed the title ~~change host to name~~ Improve(hosts): use more meaningful name for host name Dec 6, 2023

tiansuo114 requested a review from Wine93 December 7, 2023 02:15

Wine93 reviewed Dec 7, 2023

View reviewed changes

remove getName in hc_get

648d3ba

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

tiansuo114 requested review from Wine93 and montaguelhz December 7, 2023 08:41

Wine93 approved these changes Dec 11, 2023

View reviewed changes

montaguelhz approved these changes Dec 11, 2023

View reviewed changes

caoxianfei1 merged commit 1376628 into opencurve:develop Dec 14, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve(hosts): use more meaningful name for host name #330

Improve(hosts): use more meaningful name for host name #330

tiansuo114 commented Nov 5, 2023

caoxianfei1 commented Nov 6, 2023

tiansuo114 commented Nov 6, 2023

caoxianfei1 commented Nov 7, 2023

tiansuo114 commented Nov 7, 2023

Wine93 commented Nov 22, 2023

tiansuo114 commented Nov 22, 2023

Wine93 Nov 23, 2023

Wine93 commented Nov 23, 2023

tiansuo114 commented Nov 23, 2023

caoxianfei1 commented Dec 4, 2023

montaguelhz left a comment

montaguelhz Dec 6, 2023

montaguelhz Dec 6, 2023

tiansuo114 commented Dec 6, 2023

Wine93 Dec 7, 2023

tiansuo114 Dec 7, 2023

Wine93 Dec 7, 2023

tiansuo114 Dec 7, 2023

Improve(hosts): use more meaningful name for host name #330

Improve(hosts): use more meaningful name for host name #330

Conversation

tiansuo114 commented Nov 5, 2023

caoxianfei1 commented Nov 6, 2023

tiansuo114 commented Nov 6, 2023

caoxianfei1 commented Nov 7, 2023

tiansuo114 commented Nov 7, 2023

Wine93 commented Nov 22, 2023

tiansuo114 commented Nov 22, 2023

Wine93 Nov 23, 2023

Choose a reason for hiding this comment

Wine93 commented Nov 23, 2023

tiansuo114 commented Nov 23, 2023

caoxianfei1 commented Dec 4, 2023

montaguelhz left a comment

Choose a reason for hiding this comment

montaguelhz Dec 6, 2023

Choose a reason for hiding this comment

montaguelhz Dec 6, 2023

Choose a reason for hiding this comment

tiansuo114 commented Dec 6, 2023

Wine93 Dec 7, 2023

Choose a reason for hiding this comment

tiansuo114 Dec 7, 2023

Choose a reason for hiding this comment

Wine93 Dec 7, 2023

Choose a reason for hiding this comment

tiansuo114 Dec 7, 2023

Choose a reason for hiding this comment