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

Etcd based cluster config #382

Merged
merged 17 commits into from
Nov 29, 2021

Conversation

samutamm
Copy link
Contributor

@samutamm samutamm commented Nov 22, 2021

Close #233. Based on this design #233 (comment).

@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2021

Codecov Report

Merging #382 (9400277) into main (0e61435) will increase coverage by 0.24%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #382      +/-   ##
==========================================
+ Coverage   80.57%   80.82%   +0.24%     
==========================================
  Files          60       60              
  Lines        6888     6961      +73     
==========================================
+ Hits         5550     5626      +76     
- Misses       1043     1044       +1     
+ Partials      295      291       -4     
Impacted Files Coverage Δ
pkg/cluster/layout.go 100.00% <ø> (ø)
pkg/object/mqttproxy/mqttproxy.go 15.18% <0.00%> (-1.03%) ⬇️
pkg/cluster/cluster.go 50.59% <63.88%> (+1.82%) ⬆️
pkg/cluster/member.go 78.12% <71.42%> (+0.43%) ⬆️
pkg/cluster/config.go 88.88% <90.54%> (+19.02%) ⬆️
pkg/object/mqttproxy/client.go 79.87% <0.00%> (-1.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e61435...9400277. Read the comment docs.

@samutamm samutamm marked this pull request as ready for review November 22, 2021 06:57
@samutamm samutamm removed the request for review from qdongxu November 23, 2021 04:32
Copy link
Collaborator

@localvar localvar left a comment

Choose a reason for hiding this comment

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

please also fix the issues reported by Github actions.

pkg/option/option.go Outdated Show resolved Hide resolved
pkg/option/option.go Outdated Show resolved Hide resolved
@xxx7xxxx
Copy link
Contributor

I don't go into detail about the code in the first review, I would like to specify my vision about this PR along with reasons.

  1. Support older and newer roles by changing older names to newer ones in startup options. (You have already fixed it.)
  2. The layout of the option is not a good design IMHO, I'd like to give a clean layout. Example without the trivial:

Primary member:

name: primary-1
cluster-name: cluster-test
cluster-role: primary
cluster-primary:
  listen-peer-urls: [https://127.0.0.1:2380]
  listen-client-urls: [https://127.0.0.1:2379]
  advertise-client-urls: [https://127.0.0.1:2379]
  initial-advertise-peer-urls: [https://127.0.0.1:2380]
  initial-cluster-state: new
  initial-cluster:
   - primary-1: https://127.0.0.1:2380
   - primary-2: https://127.0.0.1:2378
   - primary-3: https://127.0.0.1:2376

Secondary member:

name: secondary-1
cluster-name: cluster-test
cluster-role: secondary
cluster-secondary:
  primary-listen-peer-urls: [https://127.0.0.1:2380]

The reason I designed them like that is to make it cleaner to write/read config by splitting them up.

  1. Please adapt the reader config to new rules.

  2. Please don't add things to cluster/member.go which all will be deleted. What you changed in it will break existing cluster with member file.

@samutamm
Copy link
Contributor Author

@xxx7xxxx Thanks for the comment! I think that the example you proposed makes it simpler to add new reader/secondary members. I would develop it further like this:

Almost the same config for primary member.

name: primary-1
cluster-name: cluster-test
cluster-role: primary # writer
cluster: # only primary member defines cluster entry 
  listen-peer-urls: [https://127.0.0.1:2380]
  listen-client-urls: [https://127.0.0.1:2379]
  advertise-client-urls: [https://127.0.0.1:2379]
  initial-advertise-peer-urls: [https://127.0.0.1:2380]
  initial-cluster-state: new
  initial-cluster:
   - primary-1: https://127.0.0.1:2380
   - primary-2: https://127.0.0.1:2378
   - primary-3: https://127.0.0.1:2376

and then secondary

name: secondary-1
cluster-name: cluster-test
cluster-role: secondary # reader
# secondary member defines, where to connect to primary
primary-listen-peer-urls: [https://127.0.0.1:2380]

@samutamm
Copy link
Contributor Author

@xxx7xxxx I removed the extra logic from members.go, so it's easier to remove in future. The configuration is also as described above.

Copy link
Contributor

@xxx7xxxx xxx7xxxx left a comment

Choose a reason for hiding this comment

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

  1. As I commented last time, please adapt older examples of reader-004/reader-005.
  2. Have you tested adding members, then shutdown all members, then restart all nodes, will it restart successfully?

pkg/cluster/cluster.go Outdated Show resolved Hide resolved
pkg/cluster/cluster.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pkg/option/option.go Show resolved Hide resolved
pkg/cluster/cluster.go Outdated Show resolved Hide resolved
Samu Tamminen added 3 commits November 26, 2021 14:31
@xxx7xxxx xxx7xxxx merged commit 23cb833 into easegress-io:main Nov 29, 2021
@samutamm samutamm deleted the etcd-based-cluster-config branch November 29, 2021 05:35
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.

[proposal] Cluster operation
4 participants