HDDS-2162. Make OM Generic related configuration support HA style config. #1511
Conversation
|
💔 -1 overall
This message was automatically generated. |
bharatviswa504
force-pushed
the
HDDS-2162
branch
from
9f4af60 to
6d4745b
Compare
|
💔 -1 overall
This message was automatically generated. |
| Collection<String> omServiceIds = conf.getTrimmedStringCollection( | ||
| OZONE_OM_SERVICE_IDS_KEY); | ||
|
|
||
| String knownOMNodeId = conf.get(OZONE_OM_NODE_ID_KEY); |
There was a problem hiding this comment.
Isn't there also a key for our own service ID?
There was a problem hiding this comment.
Also you should trim the string here after getting from conf.
There was a problem hiding this comment.
Isn't there also a key for our own service ID?
No, we don't have.
There was a problem hiding this comment.
Oh there should be one then. We cannot assume nodeID is unique across services. We can do so in a separate jira.
There was a problem hiding this comment.
This is generally used in MiniOzoneClusterHA testing. For each OM this is set with different value. (As now we don't have federated OM setup it is not really required at this point, I agree we need this at some point for testing)
There was a problem hiding this comment.
Can you file a jira to add that?
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ha/OMHANodeDetails.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ha/OMHANodeDetails.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ha/OMHANodeDetails.java
Outdated
Show resolved
Hide resolved
bharatviswa504
force-pushed
the
HDDS-2162
branch
from
6d4745b to
fbf5586
Compare
|
Thank You @arp7 for the review. |
|
💔 -1 overall
This message was automatically generated. |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
There was a problem hiding this comment.
I have an uber comment on this JIRA. Under Ozone, what we really need is 3 + 3 six kerberos Identites.
Why don't we just follow the standard Kerberos SPN names? Simple take one config key from the user, either the SPN or file name path the service kerberos identity.
Once you have this, we don't have to do any second guessing or munching of names with any other strings -- after all it is just a service on a host. Code is simpler, and the best part, it is simple enough for any one to understand. In other words, what evil are we trying to prevent here with all the service name munching ?
| String omNode1RpcAddrKey = getOMAddrKeyWithSuffix(null, omNode1Id); | ||
| String omNode2RpcAddrKey = getOMAddrKeyWithSuffix(null, omNode2Id); | ||
| String omNode1RpcAddrKey = getOMAddrKeyWithSuffix(serviceID, omNode1Id); | ||
| String omNode2RpcAddrKey = getOMAddrKeyWithSuffix(serviceID, omNode2Id); | ||
|
|
There was a problem hiding this comment.
Don't need to verify that these strings are in the expected format here?
There was a problem hiding this comment.
I have an uber comment on this JIRA. Under Ozone, what we really need is 3 + 3 six kerberos Identites.
Why don't we just follow the standard Kerberos SPN names? Simple take one config key from the user, either the SPN or file name path the service kerberos identity.
Once you have this, we don't have to do any second guessing or munching of names with any other strings -- after all it is just a service on a host. Code is simpler, and the best part, it is simple enough for any one to understand. In other words, what evil are we trying to prevent here with all the service name munching ?
Not got your last part what is proposed.
This is done for Kerberos settings and also for other configs like OM DB DIrs Http/Https Address. Suppose the user wants to use different keytab file location/principal name it will also help in this situation.
There was a problem hiding this comment.
Not got your last part what is proposed.
https://docs.microsoft.com/en-us/windows/win32/ad/name-formats-for-unique-spns
OzoneManager/host1.example.com/CN=hrdb,OU=mktg,DC=example,DC=com
OzoneManager/host2.example.com/CN=hrdb,OU=mktg,DC=example,DC=com
OzoneManager/host3.example.com/CN=hrdb,OU=mktg,DC=example,DC=com
This is all we need, is what I am trying to say.
Suppose the user wants to use different keytab file location/principal name it will also help in this situation.
Why would you want separate identities to communicate to the same service ? Can you give me an example of why this would be needed ? More over, why support that identity via naming tricks in Ozone instead of creating an new SPN in Kerberos Domain?
There was a problem hiding this comment.
Discussed offline, from my understanding this is being done as to share config across all OM's. And this PR is not changing any config loading code of OM HA, it just added Kerberos/DB config as described in Jira description.
Anu said we don't require the current way, and we shall continue the discussion later to see how we can do it.
There was a problem hiding this comment.
As discussed offline for now removed Kerberos config.
| @@ -309,13 +305,33 @@ private OzoneManager(OzoneConfiguration conf) throws IOException, | |||
| AuthenticationException { | |||
| super(OzoneVersionInfo.OZONE_VERSION_INFO); | |||
| Preconditions.checkNotNull(conf); | |||
| configuration = conf; | |||
| configuration = new OzoneConfiguration(conf); | |||
There was a problem hiding this comment.
This function passes a conf object of type OzoneConfiguration. Why are we reallocating a new object before assigining that configuration obect to the member variable?
There was a problem hiding this comment.
This is being done, so not to change the original values of configuration object passed.
There was a problem hiding this comment.
But you just lost the reference to the original object. I am slightly confused here.
There was a problem hiding this comment.
here configuration = new OzoneConfiguration(conf);
And later I am changing the configuration property values in this new object so that it will not affect to original configuration.
I just checked with below:
OzoneConfiguration configuration = new OzoneConfiguration();
configuration.set(OZONE_METADATA_DIRS,
folder.newFolder().getAbsolutePath());
OzoneConfiguration configuration1 = new OzoneConfiguration(configuration);
configuration1.set(OZONE_METADATA_DIRS, "bharat");
System.out.println(configuration.get(OZONE_METADATA_DIRS));
System.out.println(configuration1.get(OZONE_METADATA_DIRS));
/var/folders/g5/fk451xl14vdf891pq7b6m6v00000gp/T/junit8528754098428361111/junit4506171024308775995
bharat
There was a problem hiding this comment.
Thanks for the catch and for bringing this up. This has caused test failure too. Reverted this back.
bharatviswa504
force-pushed
the
HDDS-2162
branch
from
fbf5586 to
d1f03f2
Compare
bharatviswa504
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
@anuengineer does this change look okay to you? Bharat has removed the principal and keytab generic config that you suggested. |
|
If no further comments, Can I commit these changes? |
|
Yes go ahead. +1 to commit, I believe you have addressed all the feedback. If there's follow up comments we can address in new commit. |
|
I will verify the test failures before I proceed with the commit, as generally, I don't see these many failures in CI run. |
bharatviswa504
force-pushed
the
HDDS-2162
branch
from
d1f03f2 to
821332f
Compare
|
/retest |
|
💔 -1 overall
This message was automatically generated. |
|
I agree, once you do the standard sanity checks; I think we should go ahead and commit. Thank you for working on this. |
|
Test failures are not related to this patch. |
No description provided.