HDDS-5919. In kubernetes OM HA has circular dependency on the service availability #3185

sokui · 2022-03-11T22:05:11Z

What changes were proposed in this pull request?

we can control to disable JVM network address cache to be enabled or not2.
OM can use hostname (ozone-om-0) instead of fqdn (ozone-om-o.ozone-om-service) to resolve itself's hostname during the launching time.

What is the link to the Apache JIRA

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

How was this patch tested?

Test it in k8s production with kerberos enabled. works well. In the launching time OM can resolve itself with the hostname.

kerneltime · 2022-03-12T00:54:35Z

cc @sodonnel

…e availability

kerneltime · 2022-04-21T21:23:47Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/ha/FlexibleFQDNResolution.java

+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_JVM_NETWORK_ADDRESS_CACHE_ENABLED_DEFAULT;
+
+/**
+ * FQDN related utils.


Please add more context as to why this is needed.

will do . Thanks

kerneltime · 2022-04-21T21:27:32Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/ha/TestFlexibleFQDNResolution.java

+/**
+ * Tests for {@link FlexibleFQDNResolution} class.
+ */
+public class TestFlexibleFQDNResolution {


Would this work only in k8s environment?

By turning on ozone.flexible.fqdn.resolution.enabled, it should work for both traditional servers and k8s based servers. Of course, the traditional servers do not require it. You can turn it off when deploying to the traditional servers

adoroszlai

Thanks @sokui for working on this.

adoroszlai · 2022-05-04T20:19:08Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

+  public static final String OZONE_FLEXIBLE_FQDN_RESOLUTION_ENABLED =
+          "ozone.flexible.fqdn.resolution.enabled";
+  public static final boolean OZONE_FLEXIBLE_FQDN_RESOLUTION_ENABLED_DEFAULT =
+          false;
+
+  public static final String OZONE_JVM_NETWORK_ADDRESS_CACHE_ENABLED =
+          "ozone.jvm.network.address.cache.enabled";
+  public static final boolean OZONE_JVM_NETWORK_ADDRESS_CACHE_ENABLED_DEFAULT =
+          true;
+


I think it would be useful to group these config keys by adding a common prefix (after ozone.):

ozone.network.flexible.fqdn.resolution.enabled

ozone.network.jvm.address.cache.enabled

adoroszlai · 2022-05-04T20:21:21Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/ha/ConfUtils.java

+    if (addr.getAddress() == null) {
+      return false;
+    }
    return NetUtils.isLocalAddress(addr.getAddress());


Nit: store value locally and simplify condition.

Suggested change

if (addr.getAddress() == null) {

return false;

}

return NetUtils.isLocalAddress(addr.getAddress());

InetAddress addr = addr.getAddress();

return addr != null && NetUtils.isLocalAddress(addr.getAddress());

adoroszlai · 2022-05-04T20:31:54Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHANodeDetails.java

+        if (!flexibleFqdnResolutionEnabled && addr.isUnresolved()
+                || flexibleFqdnResolutionEnabled
+                && !isAddressHostNameLocal(addr)) {


I think it would be nice to create a helper method to avoid repeating this condition.

Suggested change

if (!flexibleFqdnResolutionEnabled && addr.isUnresolved()

|| flexibleFqdnResolutionEnabled

&& !isAddressHostNameLocal(addr)) {

if (isUnresolved(addr, conf)) {

adoroszlai · 2022-05-04T20:32:03Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHANodeDetails.java

+        if (!isPeer && (!flexibleFqdnResolutionEnabled
+                && !addr.isUnresolved()
+                && ConfUtils.isAddressLocal(addr)
+                || flexibleFqdnResolutionEnabled
+                && isAddressHostNameLocal(addr))) {


Also for this condition, something like this:

Suggested change

if (!isPeer && (!flexibleFqdnResolutionEnabled

&& !addr.isUnresolved()

&& ConfUtils.isAddressLocal(addr)

|| flexibleFqdnResolutionEnabled

&& isAddressHostNameLocal(addr))) {

if (!isPeer && isAddressLocal(addr, conf)) {

adoroszlai · 2022-05-04T20:42:17Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/ha/FlexibleFQDNResolution.java

+ * the FQDN [pod_name].[service_name] is not resolvable at the service
+ * starting time.
+ */
+public final class FlexibleFQDNResolution {


I would change this class to a bit more generic Ozone-specific network utility.

Rename to OzoneNetUtils (or similar)

Move to a non-ha-specific package, e.g. org/apache/hadoop/ozone/util or org/apache/hadoop/hdds/utils.

Move isAddressLocal from ConfUtils to this class.

Also add the suggested new isUnresolved and isAddressLocal methods in this class.

adoroszlai · 2022-05-04T20:45:04Z

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java

+      FlexibleFQDNResolution.disableJvmNetworkAddressCacheIfRequired(
+              new OzoneConfiguration());


I think S3 Gateway and Recon could benefit from the same setting.

added this to both S3G and Recon

adoroszlai · 2022-05-04T20:48:24Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerStarter.java

+    FlexibleFQDNResolution.disableJvmNetworkAddressCacheIfRequired(
+            new OzoneConfiguration());


Instead of creating a new OzoneConfiguration, can we place this call in commonInit? Or is that too late?

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerStarter.java

Lines 166 to 168 in a3f5021

private void commonInit() {

conf = createOzoneConfiguration();

TracingUtil.initTracing("OzoneManager", conf);

yeah. I think I tried that before. It is too late

adoroszlai · 2022-05-04T20:50:08Z

...rver-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManagerStarter.java

+    FlexibleFQDNResolution.disableJvmNetworkAddressCacheIfRequired(
+            new OzoneConfiguration());


Same comment here about commonInit().

same as above

adoroszlai · 2022-05-07T21:15:37Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/ha/ConfUtils.java
 23: Unused import - org.apache.hadoop.net.NetUtils.
 26: Unused import - java.net.InetAddress.
 27: Unused import - java.net.InetSocketAddress.

adoroszlai · 2022-05-08T09:24:30Z

Thanks @sokui for updating the patch. LGTM.

adoroszlai · 2022-05-08T09:27:16Z

@GeorgeJahad @xBis7 can you please take a look at this Kubernetes-related fix?

GeorgeJahad · 2022-05-09T18:20:36Z

I'll have some time later today @adoroszlai

GeorgeJahad · 2022-05-11T22:05:01Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/HASecurityUtils.java


    // Get host name.
-    String hostname = scmAddress.getAddress().getHostName();
+    String hostname = scmAddress.getHostName();


Is this change necessary?

I cannot exactly remember. I think I got some cert issue when deploying to k8s. But when I just tested this, it seems both of these correctly return the hostname. I cannot remember now why I have this change. Are you OK to keep my change, or you think I should revert it? Please let me know. Thanks

I'm ok with keeping it, I think it does the same thing. I was just wondering.

GeorgeJahad · 2022-05-11T22:08:24Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/util/OzoneNetUtils.java

+   * @param addr a FQDN address
+   * @return The address of host name
+   */
+  public static InetSocketAddress getAddressWithHostName(


Nit: for me, the OM changes would be easier to understand if this method were called "getAddressWithHostNameLocal()", (which would parallel the "isAddressHostNameLocal()" method above.)

GeorgeJahad · 2022-05-11T22:11:03Z

lgtm

adoroszlai · 2022-05-13T14:58:15Z

@kerneltime would you like to take another look?
@JacksonYao287 would you like to take a look?

If not, I would like to merge this.

JacksonYao287 · 2022-05-14T01:38:12Z

LGTM +1

adoroszlai · 2022-05-14T13:48:22Z

Thanks @sokui for the patch, @GeorgeJahad, @JacksonYao287, @kerneltime for the review.

* master: (96 commits) HDDS-6738. Migrate tests with rules in hdds-server-framework to JUnit5 (apache#3415) HDDS-6650. S3MultipartUpload support update bucket usedNamespace. (apache#3404) HDDS-6491. Support FSO keys in getExpiredOpenKeys (apache#3226) HDDS-6596. EC: Support ListBlock from CoordinatorDN (apache#3410) HDDS-6737. Migrate parameterized tests in hdds-server-framework to JUnit5 (apache#3414) HDDS-6660: EC: Add the DN side Reconstruction Handler class. (apache#3399) HDDS-6750. Migrate simple tests in hdds-server-scm to JUnit5 (apache#3417) HDDS-6749. SCM includes itself as peer in addSCM request (apache#3413) HDDS-6657. Improve Ozone integrated Ranger configuration instructions (apache#3365) HDDS-6742. Audit operation category mismatch (apache#3407) HDDS-6748. Intermittent timeout in TestECBlockReconstructedInputStream#testReadDataWithUnbuffer (apache#3416) HDDS-6731. Migrate simple tests in hdds-server-framework to JUnit5 (apache#3412) HDDS-5919. In kubernetes OM HA has circular dependency on service availability (apache#3185) HDDS-6730. Migrate tests in hdds-tools to JUnit5 (apache#3402) HDDS-6630. Explicitly remove node after being chosen (apache#3332) HDDS-6560. Add general Grafana dashboard (apache#3285) HDDS-6704. EC: ReplicationManager - create version of ContainerReplicaCounts applicable to EC (apache#3405) HDDS-6680. Pre-Finalize behaviour for Bucket Layout Feature. (apache#3377) HDDS-6619. Add freon command to run r/w mix workload using ObjectStore APIs (apache#3383) HDDS-6734. ozone admin pipeline list CLI is not backward compatible (apache#3406) ... Conflicts: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMMetadataStore.java hadoop-hdds/interface-server/src/main/proto/SCMRatisProtocol.proto hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMMetadataStoreImpl.java hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java

zhiheng xie added 4 commits March 30, 2022 12:42

HDDS-5919: In kubernettes om HA has circular dependency on the servic…

95403d9

…e availability

fix rat and checkstyle errors

4a9bd9e

fix findbugs

a26290a

modify ozone-default.xml file

9a1efdb

sokui force-pushed the HDDS-5919-OM-fqdn branch from 1de7b7c to 9a1efdb Compare March 30, 2022 19:42

kerneltime reviewed Apr 21, 2022

View reviewed changes

zhiheng xie and others added 2 commits April 21, 2022 17:12

add more comments

12ac016

Merge branch 'master' into HDDS-5919-OM-fqdn

531039f

adoroszlai self-requested a review May 4, 2022 16:52

adoroszlai changed the title ~~HDDS-5919: In kubernettes om HA has circular dependency on the servic…~~ HDDS-5919: In kubernetes OM HA has circular dependency on the service availability May 4, 2022

adoroszlai changed the title ~~HDDS-5919: In kubernetes OM HA has circular dependency on the service availability~~ HDDS-5919. In kubernetes OM HA has circular dependency on the service availability May 4, 2022

adoroszlai reviewed May 4, 2022

View reviewed changes

address more comments

34f90c2

fix checkstyle error

096c622

adoroszlai requested a review from JacksonYao287 May 8, 2022 09:25

GeorgeJahad reviewed May 11, 2022

View reviewed changes

address PR comment

ad58130

adoroszlai merged commit f131de5 into apache:master May 14, 2022

		FlexibleFQDNResolution.disableJvmNetworkAddressCacheIfRequired(
		new OzoneConfiguration());

	private void commonInit() {
	conf = createOzoneConfiguration();
	TracingUtil.initTracing("OzoneManager", conf);

HDDS-5919. In kubernetes OM HA has circular dependency on the service availability #3185

HDDS-5919. In kubernetes OM HA has circular dependency on the service availability #3185

Uh oh!

Conversation

sokui commented Mar 11, 2022 • edited by adoroszlai Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

kerneltime commented Mar 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented May 7, 2022

Uh oh!

adoroszlai commented May 8, 2022

Uh oh!

adoroszlai commented May 8, 2022

Uh oh!

GeorgeJahad commented May 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeorgeJahad commented May 11, 2022

Uh oh!

adoroszlai commented May 13, 2022

Uh oh!

JacksonYao287 commented May 14, 2022

Uh oh!

adoroszlai commented May 14, 2022

sokui commented Mar 11, 2022 •

edited by adoroszlai

Loading