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

Jump to bottom

HDDS-4730. Use separate Ratis admin and client ports #1887

Merged
merged 28 commits into from Feb 8, 2021
Merged

HDDS-4730. Use separate Ratis admin and client ports #1887

merged 28 commits into from Feb 8, 2021

Conversation

adoroszlai
Copy link
Contributor

What changes were proposed in this pull request?

Move Datanode's Ratis server-to-server and admin communication to other ports and use separate TLS config.

  • Add new ports with default config.
  • Only use new ports if datanode has not been initialized previously. Ratis stores peer addresses with port in Raft log, so if the ring is already created, we cannot simply move server-to-server communication to another port.
  • Store "separate ports" as feature in datanode details YAML. Currently this is the only feature, but I think it's better to have a list of features than to introduce "random" fields.
  • Add acceptance test for restarting Ozone (with the same version, no upgrade).
  • Enabled TLS for datanode GRPC in ozonesecure cluster.

https://issues.apache.org/jira/browse/HDDS-4730

How was this patch tested?

Regular CI:
https://github.com/adoroszlai/hadoop-ozone/actions/runs/531656927

@adoroszlai
Reproduce the bug with cross-compatibility test
206e9af
@adoroszlai
Avoid duplicate build()
bdcbc0b
@adoroszlai
Ignore unknown port type
749986a
@adoroszlai
HDDS-4731. Only include public ports in DatanodeDetails proto by default
95ae22d
@adoroszlai
Fix missing version number
53b9ce0
@adoroszlai
Also log replication source
06a517d
@adoroszlai
Include ports in DatanodeDetails toString
9d910b8
@adoroszlai
Add integration test
7672919
@adoroszlai
Include all ports by default, explicitly limit ports for pipeline and…
bf0921a 
… block location
@adoroszlai
trigger new CI check
221871f
@adoroszlai
Add client version to proto requests
73d7a5f
@adoroszlai
trigger new CI check
1577cb8
@adoroszlai
Merge remote-tracking branch 'origin/master' into HDDS-4731
98b22b7
@adoroszlai
adjust whitespace
b8bff1a
@adoroszlai
HDDS-4730. Use separate Ratis admin and client ports
52c8527
@adoroszlai
Use different default ports for admin/server
2b72972
@adoroszlai
Add version marker in datanode details yaml
1a40f59
@adoroszlai
Fix checkstyle
58e6e7a
@adoroszlai
Avoid null port for admin/server
8ec490d
@adoroszlai
Fix persistence of feature flag in DatanodeDetailsYaml
1023372
@adoroszlai
Fix mini cluster
d5a712b
@adoroszlai
Remove leftover debug
cfcfe4e
@adoroszlai
Merge remote-tracking branch 'origin/master' into HDDS-4730
d9b677a
@adoroszlai adoroszlai self-assigned this Feb 2, 2021
amaliujia
amaliujia reviewed Feb 2, 2021
View reviewed changes
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java Outdated
@@ -73,6 +74,7 @@
private String buildDate;
private HddsProtos.NodeOperationalState persistedOpState;
private long persistedOpStateExpiryEpochSec = 0;
private final List<String> features = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I might have missed something:

Is there a JIRA or design doc that says what features in Datanode are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a way to help distinguish newly initialized datanodes from existing ones being restarted with new code version. No further features planned currently.

lokeshj1703
lokeshj1703 reviewed Feb 5, 2021
View reviewed changes
Copy link
Contributor

@lokeshj1703 lokeshj1703 left a comment

Choose a reason for hiding this comment

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

@adoroszlai Thanks for working on this! The changes look good to me. I have few minor comments inline.

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java Outdated
Comment on lines 132 to 133
public static final String SEPARATE_RATIS_PORTS_AVAILABLE = "RatisPorts";

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we define features in a separate class? We could also define them as enums.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can move it to separate class. I'd rather avoid enums, which are more type-safe, but worse for compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although not sure if it's worth extracting to another class at this time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was suggesting separate class so that in future if new features are defined, it could all be in the same class for better accessibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @lokeshj1703 for the suggestion. Extracted to separate class. Also changed to using version numbers instead of "features". I think this will be simpler in the long run. Features of the datanode can be mapped at runtime (eg. if version > 0, then we have this "separate ports" feature).

...java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java Show resolved Hide resolved
@adoroszlai adoroszlai marked this pull request as draft February 5, 2021 18:10
@adoroszlai adoroszlai marked this pull request as ready for review February 6, 2021 11:38
@adoroszlai
Copy link
Contributor Author

@elek @bshashikant @lokeshj1703 @amaliujia Thanks for the reviews so far. Do you have any comment on the latest patch, can I merge it?

lokeshj1703
lokeshj1703 approved these changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@lokeshj1703 lokeshj1703 left a comment

Choose a reason for hiding this comment

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

@adoroszlai Thanks for updating the PR! The changes look good to me. +1.

@adoroszlai adoroszlai merged commit 375da4d into apache:master Feb 8, 2021
@adoroszlai adoroszlai deleted the HDDS-4730 branch February 8, 2021 12:25
@adoroszlai
Copy link
Contributor Author

Thanks all for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amaliujia amaliujia amaliujia left review comments

@lokeshj1703 lokeshj1703 lokeshj1703 approved these changes

@bshashikant bshashikant bshashikant approved these changes

@elek elek Awaiting requested review from elek

Assignees

@adoroszlai adoroszlai

Labels
datanode
Projects
None yet
Milestone
No milestone
4 participants
@adoroszlai @amaliujia @lokeshj1703 @bshashikant