You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2017/09/01 20:37:19 UTC
[Impala-ASF-CR] IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7938
Change subject: IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
Impala currently kinits by forking off a subprocess. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_krpc_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_development.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 203 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/1
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc@842
PS6, Line 842: if (FLAGS_use_krpc_kinit) {
> can this path only be used with KRPC, or could it be used with thrift?
It can be used with both. The flag is there in case we run into unforeseen issues, so we can commit to removing it after one release.
I'll rename it to "use_kudu_kinit", since it's from the kudu security library and not the krpc library.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 18:29:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1359/
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 10
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 20:58:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Michael Brown, Jim Apple, Bikramjeet Vig, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7938
to look at the new patch set (#14).
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.
TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).
Updated bin/bootstrap_system.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Also fixed a bug that didn't transfer the environment into 'sudo'
in bin/bootstrap_system.sh.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_system.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 234 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/14
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 14
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Bikramjeet Vig, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7938
to look at the new patch set (#12).
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.
TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).
Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Some of these binaries are installed via sudo to avoid the
interactive dialog screens. If we install without sudo, they do
not respect the 'noninteractive' option and still wait for user
input.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_system.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 239 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/12
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 12
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Bikramjeet Vig, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7938
to look at the new patch set (#7).
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.
Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/gtest-util.h
M bin/bootstrap_development.sh
A cmake_modules/FindKerberosPrograms.cmake
12 files changed, 230 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/7
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Bikramjeet Vig, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7938
to look at the new patch set (#9).
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.
TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).
Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
A cmake_modules/FindKerberosPrograms.cmake
9 files changed, 229 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/9
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 9
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1368/
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 11
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Oct 2017 18:38:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 10: Code-Review+2
Carrying +2.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 10
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 20:58:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Bikramjeet Vig,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7938
to look at the new patch set (#6).
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_krpc_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_krpc_kinit' flag.
Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/gtest-util.h
M bin/bootstrap_development.sh
A cmake_modules/FindKerberosPrograms.cmake
12 files changed, 230 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/6
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 8: Code-Review+1
(1 comment)
Conditional on the rebase on top of https://gerrit.cloudera.org/#/c/8247/
http://gerrit.cloudera.org:8080/#/c/7938/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/7938/8//COMMIT_MSG@7
PS8, Line 7: KRPC
Kudu
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Sat, 14 Oct 2017 01:57:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7938/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/7938/4//COMMIT_MSG@21
PS4, Line 21: Converted existing tests in thrift-server-test to run with and
: without kerberos.
we can also add tests that run with and without FLAGS_use_krpc_kinit set to make sure both paths work and also make sure that KRPC's Kinit was successful.
http://gerrit.cloudera.org:8080/#/c/7938/4/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/7938/4/be/src/rpc/thrift-server-test.cc@129
PS4, Line 129: ASSERT_TRUE(kudu::Env::Default()->GetExecutablePath(¤t_executable_path).ok())
since this assert is done a bunch of times here and would probably be done more in the future as we have added the KPRC code, would it make sense to add a macro for this (for kudu:status object) like we have ASSERT_OK for the impala:status object?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2017 21:38:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc@247
PS7, Line 247: }
> This makes the test infra quite flakey. I tried for quite some time, but it
OK. That's fine as long as we eventually will get back to this.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Sat, 14 Oct 2017 01:10:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 13
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 13:25:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 2:
(8 comments)
Some quick comments.
http://gerrit.cloudera.org:8080/#/c/7938/2//COMMIT_MSG
Commit Message:
PS2, Line 9: subprocess
child process
PS2, Line 23: util
utility
PS2, Line 33: Verified with thrift-server-test and also manually on a
: live kerberized cluster.
Did you verify that our current Jenkins AMI will be able to find the necessary util to run the tests ?
http://gerrit.cloudera.org:8080/#/c/7938/2/CMakeLists.txt
File CMakeLists.txt:
PS2, Line 305: find_package(KerberosPrograms REQUIRED)
Wouldn't build fail if it's not found ? Did you try packaging build ?
http://gerrit.cloudera.org:8080/#/c/7938/2/be/src/kudu/security/CMakeLists.txt
File be/src/kudu/security/CMakeLists.txt:
PS2, Line 89: set(SECURITY_TEST_SRCS_FOR_IMPALA
: test/mini_kdc.cc)
nit: this can be one line
http://gerrit.cloudera.org:8080/#/c/7938/2/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
PS2, Line 74: Remove this flag in a compatibility-breaking release
Is there a JIRA for that ?
Line 843: KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(), "Could not init kerberos");
long line.
http://gerrit.cloudera.org:8080/#/c/7938/2/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
PS2, Line 231: ASSERT_FALSE(non_ssl_client.Open().ok());
Is it possible to match against the expected error status instead ?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 3:
Passed the packaging build on multiple platforms.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#2).
Change subject: IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
Impala currently kinits by forking off a subprocess. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_krpc_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc util. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail.
Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_development.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 203 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/2
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 8: Verified-1 Code-Review+2
> (1 comment)
Manually setting Verified: -1.
Running the test in a loop shows that it's flaky. Not a functional issue, but something to do with setting up and tearing down the KDC so often. Or possibly a test infra issue because our renew threads don't have a shutdown mechanism.
Will work on fixing it.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Oct 2017 17:35:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc
File be/src/kudu/security/init.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc@460
PS5, Line 460: //setenv("KRB5CCNAME", "MEMORY:kudu", 1);
: //setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1);
: // We commented out the above since we don't want to overwrite our own Impala
: // set environment variables.
> Is this function used for testing only ? If so, would it be better to move
No, this function is what initializes Kerberos in the kudu security library. See L843 in authentication.cc
Unfortunately, there isn't a clean way to make this configurable, unless we pass in strings to the InitKerberosForServer() function as parameters for these environment variables, which makes little sense for Kudu to have in their code base.
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc@469
PS5, Line 469: //setenv("KRB5RCACHETYPE", "none", 1);
> Same comment.
Same as above.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 19:10:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 12:
The GVO was hanging since the kerberos package installations expected user input. It turns out that the DEBIAN_FRONTEND=noninteractive option was not being respected by these packages for some reason, but doing the same as sudo made it work. There's no mention anywhere online of why this works, but just other online threads with people who claim that this worked for them too.
I don't like having things around that I can't explain, but unfortunately, I don't see any other way to do this now.
@Michael Brown and/or @Jim Apple, could you please review/sign-off on bootstrap_system.sh when you get the chance?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 12
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 05:11:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/12/bin/bootstrap_system.sh
File bin/bootstrap_system.sh:
http://gerrit.cloudera.org:8080/#/c/7938/12/bin/bootstrap_system.sh@111
PS12, Line 111: # Install these as sudo so that we can avoid the interactive dialog screens.
The apt-get on line 106 is calling the function on line 79. Maybe the problem with the env var was the sudo pass-through blockage on line 82? What if you added DF=ni on that line.
Maybe also gate it on ther terminal interactivity, like on line 41?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 12
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 05:26:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Bikramjeet Vig, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7938
to look at the new patch set (#11).
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.
TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).
Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_system.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 233 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/11
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 11
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 14:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1398/
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 14
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 20:29:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc
File be/src/kudu/security/init.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc@460
PS5, Line 460: //setenv("KRB5CCNAME", "MEMORY:kudu", 1);
: //setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1);
: // We commented out the above since we don't want to overwrite our own Impala
: // set environment variables.
> No, this function is what initializes Kerberos in the kudu security library
Yes, I should have looked at the Kudu code base instead.
So, in the Kudu code base, this is called from:
- ServerBase::Init()
- TestBasicOperation() in test_mini_kdc.cc
Do you think it makes sense to do some small refactoring in the Kudu side to only keep the common code in this function ?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 19:19:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc@842
PS6, Line 842: if (FLAGS_use_krpc_kinit) {
can this path only be used with KRPC, or could it be used with thrift?
If the latter, why have the flag? (Or at least, can we commit to removing the flag and the old code after one release?)
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 18:21:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/exec/kudu-util.h
File be/src/exec/kudu-util.h:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/exec/kudu-util.h@45
PS6, Line 45: Status&
> Should this be Status instead of Status& ?
Nevermind. Using const reference is fine too. The returned object's lifetime is extended to that of the reference although I would expect the compiler to figure it out too even without using a reference.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 00:51:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 11: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1368/
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 11
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Oct 2017 04:38:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/test/mini_kdc.cc
File be/src/kudu/security/test/mini_kdc.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/test/mini_kdc.cc@41
PS5, Line 41: // test_util.h has a dependancy on gmock which Impala doesn't depend on, so we rewrite
: // parts of this code that use test_util members.
: //#include "kudu/util/test_util.h"
Doesn't this problem exist for all files which #include "test_util.h" ? I suppose we don't build kudu tests by default so this is an exception, right ? Otherwise, it'd be better in the long run to figure out how to resolve the dependency on gmock or we will keep doing the same change for every files which include this file.
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/test/mini_kdc.cc@66
PS5, Line 66: //options_.data_root = JoinPathSegments(GetTestDataDirectory(), "krb5kdc");
May as well delete this line instead of commenting it out. Add a comment instead.
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/auth-provider.h
File be/src/rpc/auth-provider.h:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/auth-provider.h@143
PS5, Line 143: /// Runs "RunKinit" below if needs_kinit_ is true and FLAGS_use_krpc_kinit is false.
Not your change but please clarify this is a long running thread which periodically forks Impala to run kinit.
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/authentication.cc@843
PS5, Line 843: KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(),
: "Could not init kerberos");
May help to point out that a thread is created in this function which will periodically carry out kinit.
What's the life cycle story for this thread ?
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@44
PS5, Line 44: #define ASSERT_KUDU_OK(status) \
: do { \
: const Status& status_ = (FromKuduStatus(status)); \
: ASSERT_TRUE(status_.ok()) << "Error: " << status_.GetDetail(); \
: } while (0)
> Should this live in gtest-util.h or kudu-util.h ?
Also, based on the convention of KUDU_RETURN_IF_ERROR(), may be this one should be called KUDU_ASSERT_OK() ?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 23:42:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 5:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc
File be/src/kudu/security/init.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc@460
PS5, Line 460: //setenv("KRB5CCNAME", "MEMORY:kudu", 1);
: //setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1);
: // We commented out the above since we don't want to overwrite our own Impala
: // set environment variables.
> Yes, I should have looked at the Kudu code base instead.
Yes, I asked the Kudu guys and they said that they were okay with it. I posted a patch here:
https://gerrit.cloudera.org/#/c/8247/
Once that's merged, I'll rebase this patch on top of that.
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/test/mini_kdc.cc
File be/src/kudu/security/test/mini_kdc.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/test/mini_kdc.cc@41
PS5, Line 41: // test_util.h has a dependancy on gmock which Impala doesn't depend on, so we rewrite
: // parts of this code that use test_util members.
: //#include "kudu/util/test_util.h"
> Doesn't this problem exist for all files which #include "test_util.h" ? I s
Yes it does due to the dependenct on gmock. However, we don't include any of those files in our build. This would be the first.
In the long run, I think we would fare better by including gmock as a dependency to pull in more of Kudu's test utils which I think are beneficial.
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/test/mini_kdc.cc@66
PS5, Line 66: //options_.data_root = JoinPathSegments(GetTestDataDirectory(), "krb5kdc");
> May as well delete this line instead of commenting it out. Add a comment in
Done
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/auth-provider.h
File be/src/rpc/auth-provider.h:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/auth-provider.h@143
PS5, Line 143: /// Runs "RunKinit" below if needs_kinit_ is true and FLAGS_use_krpc_kinit is false.
> Not your change but please clarify this is a long running thread which peri
Done
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/authentication.cc@843
PS5, Line 843: KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(),
: "Could not init kerberos");
> Not sure how easy it is to do so but it'd be great to have a test which inj
When a renewal fails, there's nothing we can do. The only thing that happens is that we add an error message to the logs and the cluster will eventually come to a halt.
So, since there's no real error handling, I don't think a test for that would be necessary. What do you think?
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/authentication.cc@843
PS5, Line 843: KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(),
: "Could not init kerberos");
> May help to point out that a thread is created in this function which will
Added a comment with the life cycle.
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@44
PS5, Line 44: #define ASSERT_KUDU_OK(status) \
: do { \
: const Status& status_ = (FromKuduStatus(status)); \
: ASSERT_TRUE(status_.ok()) << "Error: " << status_.GetDetail(); \
: } while (0)
> Also, based on the convention of KUDU_RETURN_IF_ERROR(), may be this one sh
Done
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@44
PS5, Line 44: #define ASSERT_KUDU_OK(status) \
: do { \
: const Status& status_ = (FromKuduStatus(status)); \
: ASSERT_TRUE(status_.ok()) << "Error: " << status_.GetDetail(); \
: } while (0)
> Should this live in gtest-util.h or kudu-util.h ?
Moved to gtest-util.h
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@104
PS5, Line 104: int kdc_port = GetServerPort();
> Does this need to be global ?
Yes, we need to get only the server port only once. If we make it a part of the class, we would get a new port for every test.
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@116
PS5, Line 116:
> A class level comment explaining the purpose of this test class would be he
Done
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@119
PS5, Line 119: GetParam()
> Is this defined in the kudu library ?
No, this is a function that's exposed by the Gtest library. It runs the test multiple times with the options in L189.
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@120
PS5, Line 120: FLAGS_use_krpc_kinit = (GetParam() == USE_KUDU_KERBEROS) ? true : false;
> FLAGS_use_krpc_kinit = GetParam() == USE_KUDU_KERBEROS;
Done
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@123
PS5, Line 123: filesystem::create_directories(unique_test_dir)
> Should we check for return value of this ? Why don't we use the wrappers in
I missed that function. I just changed it to use RemoveAndCreateDirectory()
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@157
PS5, Line 157: kudu::MiniKdc* kdc_
> Why not scope_ptr ?
Done
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@174
PS5, Line 174: DCHECK(!kdc_);
> DCHECK(kdc_ == nullptr);
Done
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@176
PS5, Line 176: DCHECK(kdc_);
> DCHECK(kdc_ != nullptr);
Done
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@186
PS5, Line 186: ASSERT_KUDU_OK(kdc_->Stop());
> Are we leaking kdc_ too ?
I think we were. I fixed it by making it a scoped_ptr.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 06:33:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/12/bin/bootstrap_system.sh
File bin/bootstrap_system.sh:
http://gerrit.cloudera.org:8080/#/c/7938/12/bin/bootstrap_system.sh@111
PS12, Line 111: if ! { service --status-all | grep -E '^ \[ \+ \] ssh$'; }
> The apt-get on line 106 is calling the function on line 79. Maybe the probl
Aha, looks like that was the problem. When running sudo apt-get in L79, we didn't transfer the environment variables into the sudo scope. I fixed that by adding -E. I also tested that it works.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 13
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 06:10:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7938/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/7938/4//COMMIT_MSG@21
PS4, Line 21: Converted existing tests in thrift-server-test to run with and
: without kerberos.
> we can also add tests that run with and without FLAGS_use_krpc_kinit set to
Done
http://gerrit.cloudera.org:8080/#/c/7938/4/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/7938/4/be/src/rpc/thrift-server-test.cc@129
PS4, Line 129: FLAGS_krb5_conf = Substitute("$0/$1", keytab_dir, "krb5.conf");
> since this assert is done a bunch of times here and would probably be done
Good point. We have a FromKuduStatus() function which will come in with Michael's RpcMgr patch here:
https://gerrit.cloudera.org/#/c/7901/9/be/src/exec/kudu-util.h
I'll add it for now and it in this patch as well and use it; and it should resolve itself in the rebase after his patch is merged in.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Oct 2017 23:27:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 10: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1359/
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 10
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Sat, 21 Oct 2017 01:00:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Bikramjeet Vig,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7938
to look at the new patch set (#5).
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_krpc_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_krpc_kinit' flag.
Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/gtest-util.h
M bin/bootstrap_development.sh
A cmake_modules/FindKerberosPrograms.cmake
12 files changed, 234 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/5
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 13: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1394/
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 13
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 19:34:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 11: Code-Review+2
After a rebase, the binaries listed in bootstrap_development moved to bootstrap_system, causing all the changes I had in that file to be lost. Hence the Jenkins -1.
I just added the binaries to install back to bootstrap_system.
Rebase, carry +2.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 11
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Oct 2017 18:38:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#3).
Change subject: IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_krpc_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail.
Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/gtest-util.h
M bin/bootstrap_development.sh
A cmake_modules/FindKerberosPrograms.cmake
11 files changed, 208 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/3
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Michael Brown, Jim Apple, Bikramjeet Vig, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7938
to look at the new patch set (#13).
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.
TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).
Updated bin/bootstrap_system.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Also fixed a bug that didn't transfer the environment into 'sudo'
in bin/bootstrap_system.sh.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_system.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 234 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/13
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 13
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/exec/kudu-util.h
File be/src/exec/kudu-util.h:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/exec/kudu-util.h@45
PS6, Line 45: (FromKuduStatus(status))
> nit: parenthesis not needed.
Done
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/auth-provider.h
File be/src/rpc/auth-provider.h:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/auth-provider.h@144
PS6, Line 144: forks
> nit: forks Impalad and exec 'kinit'
Done
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc@77
PS6, Line 77: "Only used when FLAGS_use_krpc is false");
> Okay. So in other words, we could have fixed the old code as well, and elim
Yes, we could have, but we couldn't deprecate this flag in a minor release anyway, so I never went ahead with it.
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/thrift-server-test.cc@117
PS6, Line 117: !(GetParam() == USE_KUDU_KERBEROS)
> Did you flip the polarity ?
Oops, I did that for some local testing verification and forgot to turn it back. Switched it back.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 05:33:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/exec/kudu-util.h
File be/src/exec/kudu-util.h:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/exec/kudu-util.h@45
PS6, Line 45: Status&
Should this be Status instead of Status& ?
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/exec/kudu-util.h@45
PS6, Line 45: (FromKuduStatus(status))
nit: parenthesis not needed.
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/auth-provider.h
File be/src/rpc/auth-provider.h:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/auth-provider.h@144
PS6, Line 144: forks
nit: forks Impalad and exec 'kinit'
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/thrift-server-test.cc@117
PS6, Line 117: !(GetParam() == USE_KUDU_KERBEROS)
Did you flip the polarity ?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 00:36:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc@96
PS7, Line 96: int kdc_port = GetServerPort();
static
http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc@247
PS7, Line 247:
As discussed offline, may be it'd help to test with short renewal period to exercise the renewal path too if possible.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 22:06:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#4).
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_krpc_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail.
Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/gtest-util.h
M bin/bootstrap_development.sh
A cmake_modules/FindKerberosPrograms.cmake
11 files changed, 208 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/4
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc@77
PS6, Line 77: "Only used when FLAGS_use_krpc is false");
in what case would a user need to set this flag? and how is that case handled in the kudu kinit case?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 18:35:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 7:
I think this is really close. Please consider adding more tests as suggested in the comments.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 22:06:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 13:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1394/
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 13
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 15:46:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/7938/2//COMMIT_MSG
Commit Message:
PS2, Line 9: subprocess
> child process
Done
PS2, Line 23: util
> utility
Done
PS2, Line 33: Verified with thrift-server-test and also manually on a
: live kerberized cluster.
> Did you verify that our current Jenkins AMI will be able to find the necess
I have a fix for our AMI and tested it with that fix, I'll update that separately as well.
http://gerrit.cloudera.org:8080/#/c/7938/2/CMakeLists.txt
File CMakeLists.txt:
PS2, Line 305: find_package(KerberosPrograms REQUIRED)
> Wouldn't build fail if it's not found ? Did you try packaging build ?
I've tested it on a machine without the binaries and the builds passed, but that may have been on old bits. Let me run a packaging build on multiple platforms and see if it works fine.
http://gerrit.cloudera.org:8080/#/c/7938/2/be/src/kudu/security/CMakeLists.txt
File be/src/kudu/security/CMakeLists.txt:
PS2, Line 89: set(SECURITY_TEST_SRCS_FOR_IMPALA
: test/mini_kdc.cc)
> nit: this can be one line
Done
http://gerrit.cloudera.org:8080/#/c/7938/2/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
PS2, Line 74: Remove this flag in a compatibility-breaking release
> Is there a JIRA for that ?
Yes, I just filed IMPALA-5893.
Line 843: KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(), "Could not init kerberos");
> long line.
Done
http://gerrit.cloudera.org:8080/#/c/7938/2/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
PS2, Line 231: ASSERT_FALSE(non_ssl_client.Open().ok());
> Is it possible to match against the expected error status instead ?
Yes, I added a new macro that matches against an expected substring.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc@77
PS6, Line 77: "Only used when FLAGS_use_krpc is false");
> in what case would a user need to set this flag? and how is that case handl
Currently all our users use this flag. This needs to be set when we use our current way of forking kinit (i.e. in other words when FLAGS_use_krpc is false, in the context of this patch), since our RunKinit() function uses that to periodically wake up.
I'm not sure of the history of this flag, but it is a completely unnecessary flag. Our reinit interval should be based on the configured ticket lifetime in the system's krb5.conf file, which is what we've done in the kudu kinit case. It detects this ticket lifetime and wakes up periodically according to that to do the reinit.
Allowing a user to control this flag is very redundant and possibly confusing to users that understand kerberos.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 18:43:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc@77
PS6, Line 77: "Only used when FLAGS_use_krpc is false");
> Currently all our users use this flag. This needs to be set when we use our
Okay. So in other words, we could have fixed the old code as well, and eliminated this flag in that case. (Let's not bother with that though, given we will rip that code out soon).
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 18:45:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 14: Code-Review+2
Fix minor clang-tidy issues.
Carry +2.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 14
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 20:29:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 8: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/8/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/8/be/src/rpc/authentication.cc@109
PS8, Line 109: "off a kinit process.");
can you file a jira to remove this flag (and the old code) within a release or two?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Oct 2017 23:48:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 9:
(2 comments)
> > (1 comment)
>
> Manually setting Verified: -1.
>
> Running the test in a loop shows that it's flaky. Not a functional
> issue, but something to do with setting up and tearing down the KDC
> so often. Or possibly a test infra issue because our renew threads
> don't have a shutdown mechanism.
>
> Will work on fixing it.
On more investigation, it looks like the cause of this problem is that our security code (and Kudu's security code) does not have a shutdown mechanism. So, Init-ing the security library multiple times can have undefined behavior, since every time it is init-ed, a new renewal thread runs in the background and is never stopped, and these renewal threads have access to common state. Our security code is written with the assumption that it will be init-ed only once, which holds true for live clusters, but not for test purposes we aim to do in this patch.
I've opened IMPALA-6085 to track this to fix it in the long term. In the short term, I've changed the code to using kerberos only for one test per process. (in this case, the SslConnectivity test).
Documenting the other things I tried to fix this:
- Start and stop the KDC in the main() function: The problem is that we can't run it with and without kerberos in this case. Also, Gtest doesn't guarantee calling RUN_ALL_TESTS() twice will work, if we want to run with and without kerberos.
- Keep around some extra global state to start and stop the KDC according to the test we're in. This is possible, but is a big hack, and I'm against checking that kind of code in.
http://gerrit.cloudera.org:8080/#/c/7938/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/7938/8//COMMIT_MSG@7
PS8, Line 7: Kudu
> Kudu
Done
http://gerrit.cloudera.org:8080/#/c/7938/8/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/8/be/src/rpc/authentication.cc@109
PS8, Line 109: // (IMPALA-5893)
> can you file a jira to remove this flag (and the old code) within a release
Done
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 9
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Oct 2017 20:33:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc
File be/src/kudu/security/init.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc@460
PS5, Line 460: //setenv("KRB5CCNAME", "MEMORY:kudu", 1);
: //setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1);
: // We commented out the above since we don't want to overwrite our own Impala
: // set environment variables.
Is this function used for testing only ? If so, would it be better to move it to the tests in the Kudu side too to avoid unnecessary divergence in the code ?
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc@469
PS5, Line 469: //setenv("KRB5RCACHETYPE", "none", 1);
Same comment.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 18:42:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 5:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@44
PS5, Line 44: #define ASSERT_KUDU_OK(status) \
: do { \
: const Status& status_ = (FromKuduStatus(status)); \
: ASSERT_TRUE(status_.ok()) << "Error: " << status_.GetDetail(); \
: } while (0)
Should this live in gtest-util.h or kudu-util.h ?
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@104
PS5, Line 104: int kdc_port = GetServerPort();
Does this need to be global ?
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@116
PS5, Line 116:
A class level comment explaining the purpose of this test class would be helpful.
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@119
PS5, Line 119: GetParam()
Is this defined in the kudu library ?
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@120
PS5, Line 120: FLAGS_use_krpc_kinit = (GetParam() == USE_KUDU_KERBEROS) ? true : false;
FLAGS_use_krpc_kinit = GetParam() == USE_KUDU_KERBEROS;
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@123
PS5, Line 123: filesystem::create_directories(unique_test_dir)
Should we check for return value of this ? Why don't we use the wrappers in FileSystemUtil class instead ?
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@157
PS5, Line 157: kudu::MiniKdc* kdc_
Why not scope_ptr ?
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@174
PS5, Line 174: DCHECK(!kdc_);
DCHECK(kdc_ == nullptr);
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@176
PS5, Line 176: DCHECK(kdc_);
DCHECK(kdc_ != nullptr);
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@186
PS5, Line 186: ASSERT_KUDU_OK(kdc_->Stop());
Are we leaking kdc_ too ?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 19:55:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.
TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).
Updated bin/bootstrap_system.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Also fixed a bug that didn't transfer the environment into 'sudo'
in bin/bootstrap_system.sh.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Reviewed-on: http://gerrit.cloudera.org:8080/7938
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_system.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 234 insertions(+), 21 deletions(-)
Approvals:
Sailesh Mukil: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 15
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
......................................................................
Patch Set 14: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 14
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Oct 2017 00:19:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Bikramjeet Vig, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7938
to look at the new patch set (#8).
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.
This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.
Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.
Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.
Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/gtest-util.h
A cmake_modules/FindKerberosPrograms.cmake
11 files changed, 229 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/8
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Oct 2017 18:42:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/authentication.cc@843
PS5, Line 843: KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(),
: "Could not init kerberos");
> May help to point out that a thread is created in this function which will
Not sure how easy it is to do so but it'd be great to have a test which injects failure during renewal and verifies the error handling behavior. May a stress flag or something but that probably involves modifying the kudu library code and leads to divergence. What do you think ?
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 23:57:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc@96
PS7, Line 96: static int kdc_port = GetServerPort();
> static
Done
http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc@247
PS7, Line 247: }
> As discussed offline, may be it'd help to test with short renewal period to
This makes the test infra quite flakey. I tried for quite some time, but it seems as though starting many KDCs close to each other with different configurations seems to be problematic. I'm not sure why yet though.
It is a test infra issue rather than a functional kerberos issue. I think it will be better if I took time later to see what the problem could be and add the test in a separate patch to save some time.
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Oct 2017 05:38:23 +0000
Gerrit-HasComments: Yes