Aaron T. Myers
added a comment - 05/Mar/14 22:10 +1, the latest patch looks good to me. TestHDFSCLI appears to be failing on trunk right now and I'm confident that the findbugs warning is unrelated.
I'm going to commit this momentarily.

That's a good thought, but I think we're fine. The HA NFS stuff doesn't even use the word "nfs" in its configs - you just set the dfs.namenode.shared.dir to point to a path that's on an NFS server, and in any case it's far more critical that we be consistent with the current NFS gateway-related configs, e.g. dfs.nfs.exports.allowed.hosts.

Aaron T. Myers
added a comment - 05/Mar/14 17:30 That's a good thought, but I think we're fine. The HA NFS stuff doesn't even use the word "nfs" in its configs - you just set the dfs.namenode.shared.dir to point to a path that's on an NFS server, and in any case it's far more critical that we be consistent with the current NFS gateway-related configs, e.g. dfs.nfs.exports.allowed.hosts.

Thanks for the review. Are you sure changing 'nfsgateway' to 'nfs' is not going to confuse some people? I wanted to specifically call out that this is the nfs gateway for hdfs, and NOT the nfs mount for the HA namenode.

Abin Shahab
added a comment - 05/Mar/14 17:26 Thanks for the review. Are you sure changing 'nfsgateway' to 'nfs' is not going to confuse some people? I wanted to specifically call out that this is the nfs gateway for hdfs, and NOT the nfs mount for the HA namenode.

Aaron T. Myers
added a comment - 05/Mar/14 16:59 The latest patch looks pretty good to me. My only suggestion is to change the config setting names from being prefixed with 'dfs.nfsgateway' to 'dfs.nfs' for consistency with the other setting names.
+1 once this is addressed.

Quick glance, in the client cache, you don't want to re-initialize the ugi conf or force a new login. Maybe just use realUser.checkTGTAndReloginFromKeytab before creating the proxy user. However that's probably not even necessary because the RPC client will relogin from keytab when the TGT goes bad.

Daryn Sharp
added a comment - 04/Mar/14 18:07 Quick glance, in the client cache, you don't want to re-initialize the ugi conf or force a new login. Maybe just use realUser.checkTGTAndReloginFromKeytab before creating the proxy user. However that's probably not even necessary because the RPC client will relogin from keytab when the TGT goes bad.

Aaron T. Myers, I removed the regression, and applied all the changes you and Jing requested. I did not add MiniKdc, as I need some help on how to move it up to the hadoop-common project so that I can access it from hadoop-hdfs-nfs project.

Abin Shahab
added a comment - 03/Mar/14 17:03 Aaron T. Myers , I removed the regression, and applied all the changes you and Jing requested. I did not add MiniKdc, as I need some help on how to move it up to the hadoop-common project so that I can access it from hadoop-hdfs-nfs project.

Abin Shahab
added a comment - 25/Feb/14 18:24 Aaron T. Myers , sorry for the delay on this.
I'll try to get to this by the end of this week/weekend. What you've asked for is clear to me, and the changes are clear to me.

I don't follow how the change in RpcProgramNfs3 is related to this issue.

Yes, I think the change in RpcProgramNfs3 is a regression of HDFS-5913.

One question: the current patch puts the login into DFSClientCache#getUserGroupInformation, which is called by the load() method of the loading cache. Thus we will call login() every time we miss the cache. Should we put the login call into the constructor of RpcProgramNfs3 instead?

Jing Zhao
added a comment - 19/Feb/14 20:04 I don't follow how the change in RpcProgramNfs3 is related to this issue.
Yes, I think the change in RpcProgramNfs3 is a regression of HDFS-5913 .
One question: the current patch puts the login into DFSClientCache#getUserGroupInformation, which is called by the load() method of the loading cache. Thus we will call login() every time we miss the cache. Should we put the login call into the constructor of RpcProgramNfs3 instead?

I don't follow how the change in RpcProgramNfs3 is related to this issue. Did that perhaps sneak in to this patch accidentally? Or is it somehow related and I'm completely missing it?

We should probably put the new constants in DFSConfigKeys.java instead of Nfs3.java. Also, entries for them should also be added to hdfs-default.xml to document them.

Did you test this with a long-running NFS gateway against a Kerberized cluster? i.e. longer than the ticket lifetime granted by default from the KDC? The reason I ask is because I see no "reloginFromKeytab" anywhere in the patch, which I would expect to be necessary.

I don't think the included tests actually exercises the new functionality. I tried just applying that portion of the patch and it passed without issue. To write an automated test for this, you'll probably have to use the MiniKdc. Or, you could test this manually against a real cluster/KDC and report the findings here.

Aaron T. Myers
added a comment - 19/Feb/14 19:43 Hi Abin, thanks a lot for working on this. A few quick comments:
I don't follow how the change in RpcProgramNfs3 is related to this issue. Did that perhaps sneak in to this patch accidentally? Or is it somehow related and I'm completely missing it?
We should probably put the new constants in DFSConfigKeys.java instead of Nfs3.java. Also, entries for them should also be added to hdfs-default.xml to document them.
Did you test this with a long-running NFS gateway against a Kerberized cluster? i.e. longer than the ticket lifetime granted by default from the KDC? The reason I ask is because I see no "reloginFromKeytab" anywhere in the patch, which I would expect to be necessary.
I don't think the included tests actually exercises the new functionality. I tried just applying that portion of the patch and it passed without issue. To write an automated test for this, you'll probably have to use the MiniKdc. Or, you could test this manually against a real cluster/KDC and report the findings here.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

Hadoop QA
added a comment - 14/Feb/14 08:13 -1 overall . Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12628966/HDFS-5898.patch
against trunk revision .
+1 @author . The patch does not contain any @author tags.
-1 tests included . The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.
+1 javac . The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc . There were no new javadoc warning messages.
+1 eclipse:eclipse . The patch built with eclipse:eclipse.
-1 findbugs . The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.
+1 release audit . The applied patch does not increase the total number of release audit warnings.
+1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs-nfs.
+1 contrib tests . The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6153//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6153//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs-nfs.html
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6153//console
This message is automatically generated.

Abin Shahab
added a comment - 13/Feb/14 18:16 Also, just to be absolutely clear: This feature will add the capability for the NFS gateway running user( lets call it 'nfsuser') to get its own TGT and renew it.
I still see an issue with nfsuser proxying for other users who may have expired TGTs).
Abin

Aaron T. Myers
added a comment - 12/Feb/14 21:58 Daryn Sharp - no, I think UGI#loginUserFromKeytab will be just fine for this use case, it's just that the current code doesn't ever call it, or ever attempt to relogin from the keytab.

Daryn Sharp
added a comment - 12/Feb/14 21:24 Too swamped to investigate, but the NFS gateway is using UGI, correct? If so, presumably UGI.loginUserFromKeytab method is insufficient? If yes again, is HADOOP-9317 closer to what you need?

It's certainly fine to commit the docs change sooner rather than later, but IMO we should also absolutely add the login from keytab capabilities ASAP as HDFS-5804 basically isn't usable in the current state. IMO HDFS-5804 should not have been committed without keytab login support.

Aaron T. Myers
added a comment - 11/Feb/14 22:49 It's certainly fine to commit the docs change sooner rather than later, but IMO we should also absolutely add the login from keytab capabilities ASAP as HDFS-5804 basically isn't usable in the current state. IMO HDFS-5804 should not have been committed without keytab login support.

Abin Shahab, the document change looks good. Do you plan to get this committed first, or wait until you have more code and doc change in a new patch?
Due to the incompatible change of HDFS-5804, I would suggest getting the doc fixed as early as possible if we don't know how soon kerberos keytab related change can be available.

Brandon Li
added a comment - 11/Feb/14 22:45 Abin Shahab , the document change looks good. Do you plan to get this committed first, or wait until you have more code and doc change in a new patch?
Due to the incompatible change of HDFS-5804 , I would suggest getting the doc fixed as early as possible if we don't know how soon kerberos keytab related change can be available.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

Hadoop QA
added a comment - 10/Feb/14 08:48 -1 overall . Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12627926/HDFS-5898-documentation.patch
against trunk revision .
+1 @author . The patch does not contain any @author tags.
-1 tests included . The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.
+1 javac . The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc . There were no new javadoc warning messages.
+1 eclipse:eclipse . The patch built with eclipse:eclipse.
+1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit . The applied patch does not increase the total number of release audit warnings.
-1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:
org.apache.hadoop.hdfs.server.namenode.TestCacheDirectives
+1 contrib tests . The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6094//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6094//console
This message is automatically generated.

Abin Shahab
added a comment - 10/Feb/14 06:15 This patch updates the documentation. As for getting the NFS gateway to handle kerberos keytabs, I will get to it in the next 2/4 weeks(depending on our sprint priorities).

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

-1 eclipse:eclipse. The patch failed to build with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

Recommend changing "These are the hosts from where the proxying is allowed " to explicitly say "this is the host where the NFS gateway is running." That's the only host that should need to be configured.

You might also consider mentioning that the proxyuser config values can be set to "*" for simplicity.

Any update on adding the ability for the NFS gateway to log in from a keytab? That, in my opinion, is the far more important issue.

Aaron T. Myers
added a comment - 06/Feb/14 21:58 Hi Abin, the updates docs look pretty good to me. Two small comments:
Recommend changing "These are the hosts from where the proxying is allowed " to explicitly say "this is the host where the NFS gateway is running." That's the only host that should need to be configured.
You might also consider mentioning that the proxyuser config values can be set to " * " for simplicity.
Any update on adding the ability for the NFS gateway to log in from a keytab? That, in my opinion, is the far more important issue.