You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/11/16 01:34:22 UTC

[kudu-CR] el6: fix krb5 realm workaround for static builds 

Hello Dan Burkert, Adar Dembo,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/5100

to review the following change.

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................

el6: fix krb5 realm workaround for static builds

Here's another attempt at enabling the workaround for the numeric realm
problem on el6 (see ba2ae3de4a7c43ff2f5873e822410e066ea99667 for
background).

The previous attempt didn't succeed for static builds because we were
setting LD_PRELOAD to the non-absolute path of the override library.
This worked OK in debug builds because our binary RUNPATHs included the
build/lib/ directory. Static builds didn't have such a RUNPATH and thus
the LD_PRELOAD was unable to locate the specified shared object.

The two options to fix this were (1) determine and specify an absolute
path, or (2) build this workaround into all of our binaries rather than
try to preload it at runtime.

The issue with #1 is that, when we get to Java-based testing, it would
have been inconvenient to determine the absolute path of the override
library. In addition, it would make test binaries non-relocatable, which
is somewhat of a hassle.

So, this takes approach #2. The build incantations are a bit nasty,
since for dynamically linked builds, it's critical that the override
library be listed on the linker invocation prior to the krb5 library
itself. Otherwise, we'll resolve the symbol from libkrb5.so and our
"override" won't work. To solve this, I added the override library just
prior to the 'krb5' library in the list of target dependencies for
'security'. Since this is the only place that krb5 is referenced, CMake
should retain our provided order.

In addition, we need to make sure that the linker doesn't entirely skip
the override binary. I accomplished this using the '--undefined' linker
flag.

I tested this patch using both sasl_rpc-test and
external_mini_cluster-test on an el6 box in both debug (dynamic linked)
and release (static-linked) builds. I also fully built both build types
to ensure there were no linker issues in unrelated tests.

Change-Id: I050d03d58658b650891566b9f955c867b15731e0
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/security/CMakeLists.txt
M src/kudu/security/test/krb5_realm_override.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
M src/kudu/util/test_util.cc
7 files changed, 25 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/5100/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................


el6: fix krb5 realm workaround for static builds

Here's another attempt at enabling the workaround for the numeric realm
problem on el6 (see ba2ae3de4a7c43ff2f5873e822410e066ea99667 for
background).

The previous attempt didn't succeed for static builds because we were
setting LD_PRELOAD to the non-absolute path of the override library.
This worked OK in debug builds because our binary RUNPATHs included the
build/lib/ directory. Static builds didn't have such a RUNPATH and thus
the LD_PRELOAD was unable to locate the specified shared object.

The two options to fix this were (1) determine and specify an absolute
path, or (2) build this workaround into all of our binaries rather than
try to preload it at runtime.

The issue with #1 is that, when we get to Java-based testing, it would
have been inconvenient to determine the absolute path of the override
library. In addition, it would make test binaries non-relocatable, which
is somewhat of a hassle.

So, this takes approach #2. The build incantations are a bit nasty. In
order to make sure that the symbol shows up everywhere, and shows up
before the krb5 library, and doesn't show up more than once (to avoid
ODR violations), we have to manually add it to the link list for the
the executables that need the override. This includes the tests (and
thus the 'test_main' library) as well as any executables which are run
by the tests ((tserver, master, and 'kudu' tool).

In addition, we need to make sure that the linker doesn't entirely skip
the override binary. I accomplished this using the '--undefined' linker
flag.

I tested this patch using both sasl_rpc-test and
external_mini_cluster-test on an el6 box in both debug (dynamic linked)
and release (static-linked) builds. I also fully built both build types
to ensure there were no linker issues in unrelated tests.

Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Reviewed-on: http://gerrit.cloudera.org:8080/5100
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Dan Burkert <da...@apache.org>
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
R src/kudu/security/krb5_realm_override.cc
M src/kudu/tools/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
M src/kudu/util/test_util.cc
11 files changed, 39 insertions(+), 38 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5100

to look at the new patch set (#2).

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................

el6: fix krb5 realm workaround for static builds

Here's another attempt at enabling the workaround for the numeric realm
problem on el6 (see ba2ae3de4a7c43ff2f5873e822410e066ea99667 for
background).

The previous attempt didn't succeed for static builds because we were
setting LD_PRELOAD to the non-absolute path of the override library.
This worked OK in debug builds because our binary RUNPATHs included the
build/lib/ directory. Static builds didn't have such a RUNPATH and thus
the LD_PRELOAD was unable to locate the specified shared object.

The two options to fix this were (1) determine and specify an absolute
path, or (2) build this workaround into all of our binaries rather than
try to preload it at runtime.

The issue with #1 is that, when we get to Java-based testing, it would
have been inconvenient to determine the absolute path of the override
library. In addition, it would make test binaries non-relocatable, which
is somewhat of a hassle.

So, this takes approach #2. The build incantations are a bit nasty,
since for dynamically linked builds, it's critical that the override
library be listed on the linker invocation prior to the krb5 library
itself. Otherwise, we'll resolve the symbol from libkrb5.so and our
"override" won't work. To solve this, I added the override library just
prior to the 'krb5' library in the list of target dependencies for
'security'. Since this is the only place that krb5 is referenced, CMake
should retain our provided order.

In addition, we need to make sure that the linker doesn't entirely skip
the override binary. I accomplished this using the '--undefined' linker
flag.

I tested this patch using both sasl_rpc-test and
external_mini_cluster-test on an el6 box in both debug (dynamic linked)
and release (static-linked) builds. I also fully built both build types
to ensure there were no linker issues in unrelated tests.

Change-Id: I050d03d58658b650891566b9f955c867b15731e0
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/security/CMakeLists.txt
R src/kudu/security/krb5_realm_override.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
M src/kudu/util/test_util.cc
7 files changed, 28 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/5100/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5100

to look at the new patch set (#3).

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................

el6: fix krb5 realm workaround for static builds

Here's another attempt at enabling the workaround for the numeric realm
problem on el6 (see ba2ae3de4a7c43ff2f5873e822410e066ea99667 for
background).

The previous attempt didn't succeed for static builds because we were
setting LD_PRELOAD to the non-absolute path of the override library.
This worked OK in debug builds because our binary RUNPATHs included the
build/lib/ directory. Static builds didn't have such a RUNPATH and thus
the LD_PRELOAD was unable to locate the specified shared object.

The two options to fix this were (1) determine and specify an absolute
path, or (2) build this workaround into all of our binaries rather than
try to preload it at runtime.

The issue with #1 is that, when we get to Java-based testing, it would
have been inconvenient to determine the absolute path of the override
library. In addition, it would make test binaries non-relocatable, which
is somewhat of a hassle.

So, this takes approach #2. The build incantations are a bit nasty,
since for dynamically linked builds, it's critical that the override
library be listed on the linker invocation prior to the krb5 library
itself. Otherwise, we'll resolve the symbol from libkrb5.so and our
"override" won't work. To solve this, I added the override library just
prior to the 'krb5' library in the list of target dependencies for
'security'. Since this is the only place that krb5 is referenced, CMake
should retain our provided order.

In addition, we need to make sure that the linker doesn't entirely skip
the override binary. I accomplished this using the '--undefined' linker
flag.

I tested this patch using both sasl_rpc-test and
external_mini_cluster-test on an el6 box in both debug (dynamic linked)
and release (static-linked) builds. I also fully built both build types
to ensure there were no linker issues in unrelated tests.

Change-Id: I050d03d58658b650891566b9f955c867b15731e0
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
R src/kudu/security/krb5_realm_override.cc
M src/kudu/tools/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
M src/kudu/util/test_util.cc
11 files changed, 30 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/5100/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5100/4/CMakeLists.txt
File CMakeLists.txt:

Line 947: set(KRB5_REALM_OVERRIDE -Wl,--undefined=krb5_realm_override_loaded krb5_realm_override)
This line causes the macOS build to fail, but it appears if you wrap it in an if (NOT APPLE) the build succeeds.


-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................


Patch Set 5: Code-Review+2

oops wrong patch

-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5100

to look at the new patch set (#4).

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................

el6: fix krb5 realm workaround for static builds

Here's another attempt at enabling the workaround for the numeric realm
problem on el6 (see ba2ae3de4a7c43ff2f5873e822410e066ea99667 for
background).

The previous attempt didn't succeed for static builds because we were
setting LD_PRELOAD to the non-absolute path of the override library.
This worked OK in debug builds because our binary RUNPATHs included the
build/lib/ directory. Static builds didn't have such a RUNPATH and thus
the LD_PRELOAD was unable to locate the specified shared object.

The two options to fix this were (1) determine and specify an absolute
path, or (2) build this workaround into all of our binaries rather than
try to preload it at runtime.

The issue with #1 is that, when we get to Java-based testing, it would
have been inconvenient to determine the absolute path of the override
library. In addition, it would make test binaries non-relocatable, which
is somewhat of a hassle.

So, this takes approach #2. The build incantations are a bit nasty. In
order to make sure that the symbol shows up everywhere, and shows up
before the krb5 library, and doesn't show up more than once (to avoid
ODR violations), we have to manually add it to the link list for the
important executables (tserver, master, and 'kudu' tool) as well as the
'test_main' library.

In addition, we need to make sure that the linker doesn't entirely skip
the override binary. I accomplished this using the '--undefined' linker
flag.

I tested this patch using both sasl_rpc-test and
external_mini_cluster-test on an el6 box in both debug (dynamic linked)
and release (static-linked) builds. I also fully built both build types
to ensure there were no linker issues in unrelated tests.

Change-Id: I050d03d58658b650891566b9f955c867b15731e0
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
R src/kudu/security/krb5_realm_override.cc
M src/kudu/tools/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
M src/kudu/util/test_util.cc
11 files changed, 30 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/5100/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5100/4//COMMIT_MSG
Commit Message:

PS4, Line 32: important executables (tserver, master, and 'kudu' tool) 
> What about one-off executables like tpch, tpch_real_world, etc? And the cli
yea, I'll rephrase this to say that it's the executables that are used by tests


http://gerrit.cloudera.org:8080/#/c/5100/4/CMakeLists.txt
File CMakeLists.txt:

PS4, Line 945: Binaries which are used by tests need to link this in.
> Can you update this to be very precise about what kind of binaries need it?
Done


Line 947: set(KRB5_REALM_OVERRIDE -Wl,--undefined=krb5_realm_override_loaded krb5_realm_override)
> This line causes the macOS build to fail, but it appears if you wrap it in 
Done


http://gerrit.cloudera.org:8080/#/c/5100/4/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

Line 22: target_link_libraries(krb5_realm_override glog dl)
> I don't think we want to link libdl on macOS.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5100/1/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

PS1, Line 30:   ${KRB5_REALM_OVERRIDE}
            :   krb5)
> If the ordering is critical (as I think it is, based on your commit descrip
Done


http://gerrit.cloudera.org:8080/#/c/5100/1/src/kudu/security/test/krb5_realm_override.cc
File src/kudu/security/test/krb5_realm_override.cc:

> We should move this out of the 'test' subdirectory so it's not confusing wh
Done


PS1, Line 32: The linkage invocation uses the '-Wl,--undefined'
            : // linker flag to force linking even though no symbol here is explicitly
            : // referenced.
> Now that this is expected to be found in every Kudu binary, you could add a
I tried that and couldn't quite get it to work without various link errors, etc. Already blew 3 hours trying to get the precise combination of jenga pieces here so I don't want to screw with it anymore.


-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5100/1/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

PS1, Line 30:   ${KRB5_REALM_OVERRIDE}
            :   krb5)
If the ordering is critical (as I think it is, based on your commit description), we should document that.


http://gerrit.cloudera.org:8080/#/c/5100/1/src/kudu/security/test/krb5_realm_override.cc
File src/kudu/security/test/krb5_realm_override.cc:

We should move this out of the 'test' subdirectory so it's not confusing when it's found linked to a production binary.


PS1, Line 32: The linkage invocation uses the '-Wl,--undefined'
            : // linker flag to force linking even though no symbol here is explicitly
            : // referenced.
Now that this is expected to be found in every Kudu binary, you could add a no-op symbol that is referenced out of ServerBase/TestMain (etc.) to avoid using -Wl,--undefined, no?


-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5100/5/CMakeLists.txt
File CMakeLists.txt:

Line 952: set(KRB5_REALM_OVERRIDE -Wl,--undefined=krb5_realm_override_loaded krb5_realm_override)
Nit: indent a bit.


-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5100

to look at the new patch set (#5).

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................

el6: fix krb5 realm workaround for static builds

Here's another attempt at enabling the workaround for the numeric realm
problem on el6 (see ba2ae3de4a7c43ff2f5873e822410e066ea99667 for
background).

The previous attempt didn't succeed for static builds because we were
setting LD_PRELOAD to the non-absolute path of the override library.
This worked OK in debug builds because our binary RUNPATHs included the
build/lib/ directory. Static builds didn't have such a RUNPATH and thus
the LD_PRELOAD was unable to locate the specified shared object.

The two options to fix this were (1) determine and specify an absolute
path, or (2) build this workaround into all of our binaries rather than
try to preload it at runtime.

The issue with #1 is that, when we get to Java-based testing, it would
have been inconvenient to determine the absolute path of the override
library. In addition, it would make test binaries non-relocatable, which
is somewhat of a hassle.

So, this takes approach #2. The build incantations are a bit nasty. In
order to make sure that the symbol shows up everywhere, and shows up
before the krb5 library, and doesn't show up more than once (to avoid
ODR violations), we have to manually add it to the link list for the
the executables that need the override. This includes the tests (and
thus the 'test_main' library) as well as any executables which are run
by the tests ((tserver, master, and 'kudu' tool).

In addition, we need to make sure that the linker doesn't entirely skip
the override binary. I accomplished this using the '--undefined' linker
flag.

I tested this patch using both sasl_rpc-test and
external_mini_cluster-test on an el6 box in both debug (dynamic linked)
and release (static-linked) builds. I also fully built both build types
to ensure there were no linker issues in unrelated tests.

Change-Id: I050d03d58658b650891566b9f955c867b15731e0
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
R src/kudu/security/krb5_realm_override.cc
M src/kudu/tools/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
M src/kudu/util/test_util.cc
11 files changed, 39 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/5100/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5100/4//COMMIT_MSG
Commit Message:

PS4, Line 32: important executables (tserver, master, and 'kudu' tool) 
What about one-off executables like tpch, tpch_real_world, etc? And the client library (which is to say, third party applications that use the client library)?

I presume the answer is 'no' because it's not important for them to work under the following conditions:
1) el6 (i.e. old krb5), and
2) numeric IPs in service principals, and
3) no functional reverse DNS for those IPs.


http://gerrit.cloudera.org:8080/#/c/5100/4/CMakeLists.txt
File CMakeLists.txt:

PS4, Line 945: Binaries which are used by tests need to link this in.
Can you update this to be very precise about what kind of binaries need it? It's not just tests, since it's also being used in the daemons. What's the rule of thumb? Binaries that use krpc to communicate with a mini cluster as part of an integration test?


http://gerrit.cloudera.org:8080/#/c/5100/4/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

Line 22: target_link_libraries(krb5_realm_override glog dl)
I don't think we want to link libdl on macOS.


-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] el6: fix krb5 realm workaround for static builds

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
......................................................................


Patch Set 5: Code-Review-1

Still broken on OS X - see previous comment with gist.

-- 
To view, visit http://gerrit.cloudera.org:8080/5100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No