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

change replicas to instances #301

Merged
merged 2 commits into from
Sep 12, 2023
Merged

change replicas to instances #301

merged 2 commits into from
Sep 12, 2023

Conversation

tiansuo114
Copy link
Contributor

@tiansuo114 tiansuo114 commented Aug 27, 2023

close #146

change the replicas keyword in topoloy.yaml to instances
but also keep the old version,old version topoloy.yaml still can use

other:
Change the replicas keyword displayed after executing the curveadm status command to instances

将 topoloy.yaml 中的replicas关键字更改为instances
但也保留旧版本,旧版本的topoloy.yaml关键字仍然可以使用

其他:
将执行curveadm status命令后显示的replicas关键字改为instances

@tiansuo114
Copy link
Contributor Author

Sorry I made a terrible mistake in old pull request(change the replicas keyword to service_num #294)😭😭
I have open a new pull request to restart that

@Wine93 Wine93 added this to the v0.3.0 milestone Aug 27, 2023
@Wine93
Copy link
Collaborator

Wine93 commented Aug 27, 2023

Hi @tiansuo114, have you tested it?

@Wine93
Copy link
Collaborator

Wine93 commented Aug 28, 2023

@tiansuo114 You should also handle some related variables, like service_replicas_sequence and service_replica_sequence.

@tiansuo114
Copy link
Contributor Author

@tiansuo114 You should also handle some related variables, like service_replicas_sequence and service_replica_sequence.

my mistake😭i will change it

@tiansuo114
Copy link
Contributor Author

@tiansuo114 You should also handle some related variables, like service_replicas_sequence and service_replica_sequence.

I encountered some problems when changing the fields in LogicalPool in curveadm/internal/configure/pool.go. After I deleted the original replicas field and changed it to instances and compiled and ran it, I encountered a cpp file error, This file comes from curve/tools/curvefsTool.cpp. I don't know if I should change the cpp file or how to deal with this error. I chose to temporarily keep the replicas attribute to solve this problem. For this, I would like to ask for your opinion

The following is the error message I encountered

Error-Code:410019
Error-Description: create logical pool failed
Error-Clue:
E 2023-08-30T14:17:28.23610+0800 134 curvefsTool.cpp:385] logicalpool replicasnum must be uin
E 2023-08-30T14:17:2823646+0800 134 curvefsTool.cpp:132] init logical pool data fail.
E 2023-08-30T14:17:28.023653+0800 134 curvefsTool.cpp:1109] exec fail,ret = -1
How to Solve:

@tiansuo114
Copy link
Contributor Author

@tiansuo114 You should also handle some related variables, like service_replicas_sequence and service_replica_sequence.

I encountered some problems when changing the fields in LogicalPool in curveadm/internal/configure/pool.go. After I deleted the original replicas field and changed it to instances and compiled and ran it, I encountered a cpp file error, This file comes from curve/tools/curvefsTool.cpp. I don't know if I should change the cpp file or how to deal with this error. I chose to temporarily keep the replicas attribute to solve this problem. For this, I would like to ask for your opinion

The following is the error message I encountered

Error-Code:410019 Error-Description: create logical pool failed Error-Clue: E 2023-08-30T14:17:28.23610+0800 134 curvefsTool.cpp:385] logicalpool replicasnum must be uin E 2023-08-30T14:17:2823646+0800 134 curvefsTool.cpp:132] init logical pool data fail. E 2023-08-30T14:17:28.023653+0800 134 curvefsTool.cpp:1109] exec fail,ret = -1 How to Solve:

By the way, I don’t know if the changes in the source code will affect the image file deployed in curveadm (I guess the error of the cpp file in this file is due to a problem with the execution of the image file in docker), I hope Can you give suggestions for modifications?

@Wine93
Copy link
Collaborator

Wine93 commented Sep 1, 2023

@tiansuo114 You should also handle some related variables, like service_replicas_sequence and service_replica_sequence.

I encountered some problems when changing the fields in LogicalPool in curveadm/internal/configure/pool.go. After I deleted the original replicas field and changed it to instances and compiled and ran it, I encountered a cpp file error, This file comes from curve/tools/curvefsTool.cpp. I don't know if I should change the cpp file or how to deal with this error. I chose to temporarily keep the replicas attribute to solve this problem. For this, I would like to ask for your opinion
The following is the error message I encountered
Error-Code:410019 Error-Description: create logical pool failed Error-Clue: E 2023-08-30T14:17:28.23610+0800 134 curvefsTool.cpp:385] logicalpool replicasnum must be uin E 2023-08-30T14:17:2823646+0800 134 curvefsTool.cpp:132] init logical pool data fail. E 2023-08-30T14:17:28.023653+0800 134 curvefsTool.cpp:1109] exec fail,ret = -1 How to Solve:

By the way, I don’t know if the changes in the source code will affect the image file deployed in curveadm (I guess the error of the cpp file in this file is due to a problem with the execution of the image file in docker), I hope Can you give suggestions for modifications?

Please paste your toplogy.yaml.

@@ -96,7 +97,7 @@ type (
* logicalpools:
* - name: pool1
* physicalpool: pool1
* replicasnum: 3
* instancessum: 3
Copy link
Collaborator

@Wine93 Wine93 Sep 1, 2023

Choose a reason for hiding this comment

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

Don't change it, the replicasnum represents the number of copies of the data, not the number of services, this is why we need this PR :)

@@ -119,7 +120,7 @@ type (
* ...
* pools:
* - name: pool1
* replicasnum: 3
* instancessum: 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto.

Name: logicalPool,
Copysets: copysets,
Zones: zones,
Instances: DEFAULT_INSTANCES_PER_COPYSET,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can delete it.

Signed-off-by: tec-rubbish maker <[email protected]>

change topology variable

change service_replicas_sequence and format_replicas_sequence to service_instances_sequence and format_instances_sequence

Signed-off-by: tec-rubbish maker <[email protected]>

fix the worng instances

Signed-off-by: tec-rubbish maker <[email protected]>

mistake

Signed-off-by: tec-rubbish maker <[email protected]>
@tiansuo114
Copy link
Contributor Author

Ok, I corrected my misunderstanding. After testing, the final version of the code can complete the deployment of the curve_bs cluster. I uploaded the deployment file that I executed in my test environment. If you need it, you can find it here download:
https://github.com/tiansuo114/go-tools/tree/main/curveadm_configs

My test is still not perfect, if you find any other mistakes, please ask me to modify the code again😭😭

@@ -73,7 +73,7 @@ func NewStatusCommand(curveadm *cli.CurveAdm) *cobra.Command {
flags.StringVar(&options.role, "role", "*", "Specify service role")
flags.StringVar(&options.host, "host", "*", "Specify service host")
flags.BoolVarP(&options.verbose, "verbose", "v", false, "Verbose output for status")
flags.BoolVarP(&options.showReplicas, "show-replicas", "s", false, "Display service replicas")
flags.BoolVarP(&options.showInstances, "show-instances", "s", false, "Display service num")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
flags.BoolVarP(&options.showInstances, "show-instances", "s", false, "Display service num")
flags.BoolVarP(&options.showInstances, "show-instances", "s", false, "Display service instances")

Copy link
Collaborator

@Wine93 Wine93 left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

[develop activities] misunderstand of replica in topology.yaml
3 participants