HDDS-3878. Make OMHA serviceID optional if one (but only one) is defined in the config

[elek]

What changes were proposed in this pull request?

om.serviceId is required on case of OM.HA in all the client parameters even if there is only one om.serviceId and it can be chosen.

My goal is:

1. Provide better usability
2. Simplify the documentation task ;-)

With using the om.serviceId from the config if

1. config is available
2. om ha is configured
3. only one service is configured

It also makes easier to run the same tests with/without HA

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3878?filter=12349091

How was this patch tested?

1. Got full green build on my fork.
2. ozone-ha acceptance tests

ozone-ha acceptance tests are turned off long time ago (it included some life-cycle test to start/stop services. In this patch I simplified the ha cluster and added simple smoketest.

Later we can restore the lifecycle tests (start/stop) but I would prefer to use a generic approach for all the clusters.

[codecov-commenter]

Codecov Report

Merging #1149 into master will increase coverage by 2.74%.
The diff coverage is 70.01%.

@@             Coverage Diff              @@
##             master    #1149      +/-   ##
============================================
+ Coverage     70.56%   73.30%   +2.74%     
- Complexity     9427     9988     +561     
============================================
  Files           965      972       +7     
  Lines         49063    49589     +526     
  Branches       4803     4872      +69     
============================================
+ Hits          34620    36351    +1731     
+ Misses        12137    10915    -1222     
- Partials       2306     2323      +17     

Impacted Files	Coverage Δ	Complexity Δ
.../java/org/apache/hadoop/ozone/OzoneConfigKeys.java	100.00% <ø> (ø)	1.00 <0.00> (ø)
...hadoop/hdds/scm/container/SCMContainerManager.java	72.54% <0.00%> (+1.96%)	35.00 <0.00> (+1.00)
...nt/algorithms/ContainerPlacementPolicyFactory.java	83.33% <ø> (ø)	2.00 <0.00> (ø)
...src/main/java/org/apache/hadoop/ozone/OmUtils.java	76.98% <0.00%> (+1.77%)	40.00 <0.00> (+2.00)
.../java/org/apache/hadoop/ozone/om/OMConfigKeys.java	100.00% <ø> (ø)	1.00 <0.00> (ø)
...hadoop/ozone/om/protocol/OzoneManagerProtocol.java	0.00% <ø> (ø)	0.00 <0.00> (ø)
...g/apache/hadoop/ozone/om/codec/OMDBDefinition.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
...one/om/response/bucket/OMBucketCreateResponse.java	100.00% <ø> (ø)	4.00 <0.00> (ø)
...one/om/response/bucket/OMBucketDeleteResponse.java	100.00% <ø> (ø)	5.00 <0.00> (ø)
...m/response/bucket/OMBucketSetPropertyResponse.java	100.00% <ø> (ø)	3.00 <0.00> (ø)
... and 299 more

---

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 dd46a55...8c4500b. Read the comment docs.

[adoroszlai]

Thanks @elek for working on this. The usability improvement is nice.

I think we need additional change in BaseFreonGenerator#createOmClient to make ombg and omkg work with the same setup (single service ID defined in config). Currently these fail with Retrying connect to server: 0.0.0.0/0.0.0.0:9862.

[bharatviswa504]

I think this is a deliberate choice not to pick from config, if it is there. As hive stores the table path in its metastore. So, having a complete path is better.

Just an example scenario/

1. If the user has config of the remote and local cluster, mistakenly he is using remote cluster config when running commands, so the user is talking to a remote cluster, not the local cluster(which users want to do). These kinds of errors cannot be caught. I feel it is better to always provide service id from the user instead of picking from config.

[arp7]

Yeah this was a conscious choice. I feel usage of unqualified paths in HDFS can be ambiguous and error-prone when multiple clusters/federation are involved.

[elek]

If the user has config of the remote and local cluster, mistakenly he is using remote cluster config when running commands

...

when multiple clusters/federation are involved.

If the user has both remote and local (or multiple federated cluster) it means that we have two service ids. This shortcut works only if you have one and only one cluster.

I think it's very important to provide better usability when we have one only one cluster. I agree, if we have two clusters, it should be required to choose.

[arp7]

If the user has both remote and local (or multiple federated cluster) it means that we have two service ids.

We want to support client-less config of HA eventually (using DNS for example). In that case we cannot look at the client side configs to know how many clusters/services there are.

This should be an inconvenience only for manually types o3fs paths. For programmatic usage it shouldn't be such a big deal right? I feel it was a mistake in HDFS to not require fully qualified paths for HA. It made the adoption of viewfs/federation more difficult.

[elek]

We want to support client-less config of HA eventually (using DNS for example). In that case we cannot look at the client side configs to know how many clusters/services there are.

Can you please share more information about this plan? Possible implementations? I need more information to understand the possible problem as with the current behavior it seems to be possible make the configuration optional.

I feel it was a mistake in HDFS to not require fully qualified paths for HA. It made the adoption of viewfs/federation more difficult.

As I understood with defaultFs it's always possible to use implicit defaults. Do you see any risk to use default cluster if only one is defined?

[arp7]

Yes one risk is when people code up their apps or scripts to use implicit paths. Then we add a second cluster and now their paths that used to work previously don't work anymore. Or suppose we want to change the defaultFS from o3fs to viewfs. Fallling back to defaultFS is an anti-pattern IMO.

[bharatviswa504]

As I understood with defaultFs it's always possible to use implicit defaults. Do you see any risk to use default cluster if only one is defined?

Are you referring here defaulFS set to o3fs://bucket.vol.serviceid1

Yes one risk is when people code up their apps or scripts to use implicit paths. Then we add a second cluster and now >their paths that used to work previously don't work anymore.

Agree on this point, this will break all existing tables which are created before assuming with default serviceID which has only one value. Now, those tables cannot be accessed, as hive metastore has path without serviceID.

[arp7]

So iiuc the main convenience argument is that we can type paths like /user/bob/foo instead of o3fs://bucket.volume.om/user/bob/foo right?

With ofs, buckets and volumes now become first class directories i.e. user is a volume and bob is a bucket. So instead of /user/bob/foo, you'd type ofs://om/user/bob/foo. That's lesser characters than the o3fs equivalent. Hopefully that mitigates some of your pain. 🙂

[elek]

Thanks to explain it. I understand why do you think implicit defaultFs can be dangerous. On the other hand (as many other dangerous feature) it provides additional flexibility as you can migrate to a new defaultFs without changing all your app. It cab be a helpful feature even if the same feature can provide some additional risks in some use cases.

But we couldn't change the defaultFs behavior of HDFS, so let's talk about this patch.

You mentioned two concerns:

1. In case of multiple clusters is used, it's better to require an explicit serviceId selection
2. In case of an "automatic", "magic", discovery mechanism, configuration is present.

Fortunately this patch is independent of these problems. This patch modifies the behavior if one (and only one) servceId is defined). In case of (1), at least two serviceIds are defined. In case of (2) no serviceId is defined. Therefore it seems to be safe to merge.

Can you please explain where do you see any risk w.r.t this patch?

[xiaoyuyao]

I agree with @elek that the scope of this change is very limited to where client only has one om service id configured. So accidentally access wrong om cluster when multiple om service ids are defined are not a major issue here.

There are few case that specify om service id explicitly is helpful even only one om service id is configured.

The client specifies different configurations to access two clusters (each with only one service id), e.g., DR(distcp). If we don't require explicit om service id in the uri, the only way to differentiate is the name configuration file locations.

[mukul1987]

I agree with @arp7 that the URI stored in the applications like Hive should be the FullyQualified URI, else the applications assume that the rest of the information is provided by the core-site.xml etc. This might lead to issues when we switch from o3fs to ofs, or to viewfs or something similar.

[elek]

The client specifies different configurations to access two clusters (each with only one service id), e.g., DR(distcp). If we don't require explicit om service id in the uri, the only way to differentiate is the name configuration file locations.

In case of distcp we have two different clusters. This patch makes the serviceId optional only if you have only one configured. serviceId is required if it's not clear which one is used (eg. you have two clusters, configured).

I agree with @arp7 that the URI stored in the applications like Hive should be the FullyQualified URI, else the applications assume that the rest of the information is provided by the core-site.xml etc. This might lead to issues when we switch from o3fs to ofs, or to viewfs or something similar.

Thanks the answer @mukul1987 . I think it's a question which is more related to the Hadoop Compatible File System. As we can use unqualified path (Hadoop feature) I couldn't see any way to disable this.

This patch modifies the behavior when somebody uses unqualified path. If you can convince users to use qualified path it won't change anything.

[elek]

Does the PR summary need an update to describe the new proposal?

No, It doesn't. It's exactly the same as before, but it doesn't apply for ofs/o3fs, only for Java client / cli.

[elek]

So, if the user does not specify it talks to OM HA, if the user specifies hostName it talks non-HA cluster.

Exactly. But only if one (and only one) serviceId is defined. In case of having multiple serviceId, you should always be explicit (unless a default/interal serviceId is configured).

Should we document this behavior, looks like a special case.

Sure. We should document all the cases (including this special case).

I have a big patch where I created documentation for OM HA: #1269

I will improve that page, but only if we have an agreement here and it's merged.

[elek]

@arp7 @bharatviswa504

Are you OK with this approach?

[bharatviswa504]

These files are removed, I think this docker-compose is for testing for ratis log purge usecase.

[elek]

Yes, and it's named as test_disabled.sh because it didn't work. As I wrote in the description, I suggested creating a simplified (but working) test and fix the purge use-case and restart use-case in a separated jira, following the pattern introduced by Attila for the upgrade test.

ozone-ha acceptance tests are turned off long time ago (it included some life-cycle test to start/stop services. In this patch I simplified the ha cluster and added simple smoketest.

Later we can restore the lifecycle tests (start/stop) but I would prefer to use a generic approach for all the clusters.

But I am open to do anything with the tests (I can revert the changes, but in that case the change won't be covered), jut to commit this patch after 37 days and unblock the release.

Just let me know what do you suggest.

[bharatviswa504]

I am okay with it, if there is a cleaner approach to do and if it can be added in the new Jira.
cc @hanishakoneru, any comments on removing this?

[elek]

I think the tests which include restarts / failures can be separated and handled similar to the upgrade test. I think we can keep a simple om-ha (or ozone-ha) test which tests the functionality and collect all the lifecycle related tests (restart, kill dn/om services) in a separated place.

We need more tests there not only for HA but for normal servers, IMHO.

[hanishakoneru]

@elek sorry for jumping into the discussion late. I am working on fixing the robot tests for HA functionality (the old tests covering the restarts and failovers).
Instead of removing those files altogether, can you create another dir for the new docker compose - ozone-ha-basic or something. Or rename the old compose dir to something else.

As for changing the life-cycle test to start/stop services, I am open to discuss a better approach. But I guess we can keep that for a different thread.

Thanks @bharatviswa504 for bringing it to my notice.

[bharatviswa504]

Exactly. But only if one (and only one) serviceId is defined. In case of having multiple serviceId, you should always be >explicit (unless a default/interal serviceId is configured).

Should we document this behavior, looks like a special case.

Using internal service id when multiple is not handled in this?

Other than a few minor comments, Overall LGTM.

[elek]

Thanks the review @bharatviswa504, I updated the patch. See my question about the tests, I am open to do anything, just let me know what do you suggest.

@arp7 Do you have any concept-level concerns?

[bharatviswa504]

@elek I don't see any commits, but the comments are marked as resolved.
Looks like commit push was missing?

[xiaoyuyao]

bq. Using internal service id when multiple is not handled in this?

My understanding of the internal service id is only used internally by OM and Recon.
Normal OM client should just look at the ozone service ids list not the internal one. If it is just one in the list and no service id is explicitly specified, just use that one. If there are more than one, the service id must be explicitly specified.

[bharatviswa504]

bq. Using internal service id when multiple is not handled in this?

My understanding of the internal service id is only used internally by OM and Recon.
Normal OM client should just look at the ozone service ids list not the internal one. If it is just one in the list and no service id is explicitly specified, just use that one. If there are more than one, the service id must be explicitly specified.

And also there is a Jira out for review, to use by S3G also.
But if multiple service ids are there, and if we want to provide convenience, where the commands used to work when only service id was there in the config, to not break user commands(when multiple service ids are added to config) we might need this.

[xiaoyuyao]

But if multiple service ids are there, and if we want to provide convenience, where the commands used to work when only service id was there in the config, to not break user commands(when multiple service ids are added to config) we might need this.

I think we reach consensus "convenience" should not be priority when multiple service ids are added even though they may have only one before. User must specify service id explicitly in that case.

[elek]

@elek I don't see any commits, but the comments are marked as resolved.
Looks like commit push was missing?

Ups, sorry. I pushed to origin instead of my fork. Fixed now.

Update, still hard to find the commits, but 29550ce and 9980b56

[elek]

I think we reach consensus "convenience" should not be priority when multiple service ids are added even though they may have only one before. User must specify service id explicitly in that case.

Based on my understanding we agreed to require explicit serviceId in o3fs/ofs and make it optional (if one is defined) for OzoneClient/API calls.

API calls are used from other servers and CLI tools (both are related to admin use-cases and not related to user activities).

The original argument against making serviceId optional if it can be deducted was the following:

If It's optional in ofs/o3fs url, the url might need modification (make explicit from implicit) when additional HA services are used. Which makes it harder for Hive users to add secondary Ozone HA cluster.

It seemed to be a particular use case, but I accepted this argument. Overall it's a decision between to forces:

1. Make the system safer for Hive users who add additional Ozone HA cluster in the future, but slightly harder for new adopters
2. Make the usability better for new adopters which adds additional risk for Hive users who add additional Ozone HA cluster in the future

@arp7 and @bharatviswa504 argued that 1 is more important, and I accepted it.

But as far as I understood we agreed that we make this improvement on API level (OzoneClient, ozone sh), which also means that S3 can be started easily. The original arguments are not applied to these cases, but I am open to discuss any additional problems, if you see...

[bharatviswa504]

+1 LGTM.
Usage of internal.service.id will be fixed as part of HDDS-4096

[hanishakoneru]

@elek sorry for jumping into the discussion late. I am working on fixing the robot tests for HA functionality (the old tests covering the restarts and failovers).
Instead of removing old ozone-ha files altogether, can you create another dir for the new docker compose - ozone-ha-basic or something. Or rename the old compose dir to something else.

As for changing the life-cycle test to start/stop services, I am open to discuss a better approach. But I guess we can keep that for a different thread.

Thanks @bharatviswa504 for bringing it to my notice.

P.S.: Adding the comment here so that its not missed.

[elek]

@hanishakoneru Sure,

I also thought about separating the test changes and the serviceId changes (I created a separated jira for the test changes and I considered posting them in a separated jira).

But this change can be tested only with fixing the tests. Keeping the old directories and starting the new one -- as you suggested -- seems to be a good approach. I restored the existing om-ha tests but the new ones are added.

[elek]

Summary: got +1 from @bharatviswa504 and @arp7 also confirmed offline, that he is fine with it (if o3fs/ofs are not changed).

Hanisha's suggestion also applied (and I pinged Her offline).

I will commit this soon.

Thanks the (very) long conversation and patient for everybody (@xiaoyuyao, @adoroszlai ...)

It seems to be a long and hard issue, but I am happy with it. As remote work become the norm, I think we should move more and more formal and informal conversations to the pull request threads (or mailing list threads). In this case it took time, but I am glad that we found a consensus.

Proposed change are done, and Hanisha was happy with it (pinged offline)