You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2019/03/28 06:58:11 UTC
[kudu-CR] [sentry] enable sentry integration for master stress test
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12877
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
[sentry] enable sentry integration for master stress test
This patch adds more coverage for master authorization enforcement via
enabling Sentry integration for master stress and failover tests.
Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/integration-tests/master_sentry-itest.cc
7 files changed, 183 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12877/1
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc
File src/kudu/integration-tests/alter_table-randomized-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@108
PS1, Line 108: public ::testing::WithParamInterface<pair<HmsMode, bool>> {
> We should probably avoid conflating the two. A disabled HMS integration wou
+1 for SentryMode or at least have boolean constants named bool SENTRY_DISABLED = false;
bool SENTRY_ENABLED = true;
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@140
PS1, Line 140: GetServerPrivilege
Would Dababase privilege be enough for these new scenarios?
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@176
PS1, Line 176: ::testing::ValuesIn
nit: will it be able to infer the type if having just
...
::testing::Values(
{ HmsMode::NONE, false },
{ HmsMode::ENABLE_METASTORE_INTEGRATION, false },
{ HmsMode::ENABLE_METASTORE_INTEGRATION, true },
)
...
?
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc
File src/kudu/integration-tests/master-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@118
PS1, Line 118: MasterStressTest
Does it make sense to restart Sentry time to time during this test? That would be something orthogonal to restarting masters and since we are expecting NetworkError status (right?) everything should run smooth?
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@522
PS1, Line 522: ::testing::ValuesIn(
: vector<pair<HmsMode, bool>> {
: { HmsMode::NONE, false },
:
: { HmsMode::ENABLE_METASTORE_INTEGRATION, false },
:
: { HmsMode::ENABLE_METASTORE_INTEGRATION, true },
: }
nit: will it compile with some the reduced option below
::testing::Values(
{ HmsMode::NONE, false },
{ HmsMode::ENABLE_METASTORE_INTEGRATION, false },
{ HmsMode::ENABLE_METASTORE_INTEGRATION, true }
)
?
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 02 Apr 2019 04:49:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12877/4/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:
http://gerrit.cloudera.org:8080/#/c/12877/4/src/kudu/integration-tests/cluster_itest_util.h@454
PS4, Line 454:
: // When Sentry is enabled, set up super privilege for 'admin' group (via
: // creating an admin role and grant privilege 'ALL' on server to the role),
: // so that 'test-admin' user, as who all integration tests are running,
: // belong to 'admin' group can operate on the cluster.
As a user of this function, it isn't clear what super privileges are. Also the second part of the sentence doesn't read quite right. This also doesn't document what 'kdc' or 'address' are, or the fact that a user should expect 'test-admin' to be logged in after calling this. Consider:
"Grants the 'test-admin' user Sentry privileges to perform any operation, using 'kdc' to authenticate with the Sentry instance at 'address'. Once called, the 'test-admin' user will be logged in."
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.h@458
PS2, Line 458: // belong to 'admin' group can operate on the cluster.
: Status SetupAdministratorPrivileges(MiniKdc* kdc,
: const HostPort& address);
:
> Added more clarification.
If this is tied to ExternalMiniCluster::Start(), did you consider putting it there? Why does this need to be a stand-alone piece? Wouldn't we always want to do this when Sentry is enabled with an EMC?
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Apr 2019 05:36:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.h@458
PS2, Line 458: Status SetupAdministratorPrivileges(MiniKdc* kdc,
: const HostPort& address);
:
: } // namespace itest
> I am hesitate to do that in case there are tests that don't want super priv
Yeah, this is boilerplate code, and it's easy to not be aware of this tooling, and it's strongly tied to ExternalMiniCluster for the reason you mentioned. I don't feel strongly about it, as long as it is a conscious decision.
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Apr 2019 06:34:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12877
to look at the new patch set (#4).
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
[sentry] enable sentry integration for master stress test
This patch adds more coverage for master authorization enforcement via
enabling Sentry integration for master stress and failover tests.
I looped each of the following tests 2000 times:
alter_table-randomized-test: http://dist-test.cloudera.org/job?job_id=hao.hao.1555900144.107645
master-stress-test: http://dist-test.cloudera.org/job?job_id=hao.hao.1555897983.99429
master_failover-itest: http://dist-test.cloudera.org/job?job_id=hao.hao.1555893155.81719
All of the failures are due to known flakiness (KUDU-2621, KUDU-2774,
KUDU-2779, and KUDU-1358).
Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/integration-tests/master_sentry-itest.cc
9 files changed, 205 insertions(+), 107 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12877/4
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12877/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12877/3//COMMIT_MSG@17
PS3, Line 17: KUDU-2279
> Did you mean KUDU-2779 here?
Ah, right. Thanks for catching this.
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.h@458
PS2, Line 458: Status SetupSentryPrivilege(MiniKdc* kdc,
: const HostPort& address);
:
: } // namespace itest
Added more clarification.
> Seems we will be Kinitted as 'test-admin'?
It is because all tests using ExternalMiniCluster is kinitted as 'test-admin'.
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1244
PS2, Line 1244:
> Why don't we have to do this for test-admin?
This is already created at external_mini_cluster.cc.
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1257
PS2, Line 1257: (), "admin-role", privilege)
> Seems like we are trying to be very permissive in granting the admin all on
So far, all the tests using this method don't require grant option. If it is no longer the case, we can certainly do that.
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Apr 2019 04:35:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has removed a vote on this change.
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc
File src/kudu/integration-tests/master-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@118
PS1, Line 118: MasterStressTest
> Does it make sense to restart Sentry time to time during this test? That w
I meant since restarting masters is the crucial point of those scenarios, I was curious what we want to test when Sentry integration is enabled.
As of now, our clients (and here we test our C++ client) are implemented such a way that they retry if they get transient errors due to short periods of unavailability of Kudu masters. What behavior do we expect if Sentry is not available for a short period of time? Will the errors be seen as transient to the clients, and will they retry operations in that case?
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 11 Apr 2019 23:01:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/alter_table-randomized-test.cc
File src/kudu/integration-tests/alter_table-randomized-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/alter_table-randomized-test.cc@122
PS2, Line 122: >address
> Add a using for this?
Done
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1241
PS2, Line 1241:
> nit: Could we just pass in the Sentry address?
Done
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1254
PS2, Line 1254: // perform any operations required in tests.
: RETURN_NOT_OK(m
> nit: "Create an admin role for the "admin" group specified in mini_sentry.c
Done
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1256
PS2, Line 1256: tryGrantOption::D
> Can you rename one of these to admin-role? It's confusing as is.
Done
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/master-stress-test.cc
File src/kudu/integration-tests/master-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/master-stress-test.cc@124
PS2, Line 124:
> using here too
Done
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/master_failover-itest.cc
File src/kudu/integration-tests/master_failover-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/master_failover-itest.cc@92
PS2, Line 92:
> using here too
Done
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 22 Apr 2019 05:54:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 1:
(5 comments)
I'm a little concerned that HMS (and Sentry) integration will make these tests more flaky. Could you loop them on dist-test to gauge the situation?
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc
File src/kudu/integration-tests/alter_table-randomized-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@106
PS1, Line 106: enable
Nit: to enable
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@108
PS1, Line 108: public ::testing::WithParamInterface<pair<HmsMode, bool>> {
Not that it changes anything, but might be more obvious to refer to the second parameter as "enabling Kerberos" rather than "enabling Sentry" since a lot of our tests parameterize in that way.
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@126
PS1, Line 126: if (enable_sentry) {
Is there another test fixture we could leverage to avoid duplicating all of this?
I wonder if we should start consolidating logic like this into the ExternalMiniCluster itself since so many tests need it.
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc
File src/kudu/integration-tests/master-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@117
PS1, Line 117: // Parameterized based on HmsMode and and whether or not enable Sentry integration.
Same comments, etc.
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master_failover-itest.cc
File src/kudu/integration-tests/master_failover-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master_failover-itest.cc@85
PS1, Line 85: // Parameterized based on HmsMode and and whether or not enable Sentry integration.
Same comments here as in alter_table-randomized-test.
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 28 Mar 2019 20:49:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12877
to look at the new patch set (#5).
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
[sentry] enable sentry integration for master stress test
This patch adds more coverage for master authorization enforcement via
enabling Sentry integration for master stress and failover tests.
I looped each of the following tests 2000 times:
alter_table-randomized-test: http://dist-test.cloudera.org/job?job_id=hao.hao.1555900144.107645
master-stress-test: http://dist-test.cloudera.org/job?job_id=hao.hao.1555897983.99429
master_failover-itest: http://dist-test.cloudera.org/job?job_id=hao.hao.1555893155.81719
All of the failures are due to known flakiness (KUDU-2621, KUDU-2774,
KUDU-2779, and KUDU-1358).
Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/integration-tests/master_sentry-itest.cc
9 files changed, 204 insertions(+), 107 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12877/5
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 1: Verified+1
Unrelated flaky of isolate race KUDU-2432.
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 28 Mar 2019 17:07:18 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 3: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.h@458
PS2, Line 458: Status SetupSentryPrivilege(MiniKdc* kdc,
: const HostPort& address);
:
: } // namespace itest
Seems we will be Kinitted as 'test-admin'? I'm not sure if that's important.
Maybe reword as:
"Grants all privileges on the 'server1' to the 'admin' user. <explanation about what kinitting as 'test-admin' does and why a user of this function should care>"
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1240
PS2, Line 1240:
rename: SetupAdministratorPrivileges
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1244
PS2, Line 1244:
Why don't we have to do this for test-admin?
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1257
PS2, Line 1257: (), "admin-role", privilege)
Seems like we are trying to be very permissive in granting the admin all on server. Why not grant option enabled?
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 22 Apr 2019 21:11:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 1:
Unrelated flaky of isolate race KUDU-2432.
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 28 Mar 2019 17:06:52 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 2:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc
File src/kudu/integration-tests/alter_table-randomized-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@106
PS1, Line 106: _SHU
> nit: remove
Done
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@106
PS1, Line 106:
> Nit: to enable
Done
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@108
PS1, Line 108: const vector<int32_t> kBlockSizes = {0, 2 * 1024 * 1024,
> +1 for SentryMode or at least have boolean constants named bool SENTRY_DISA
Done
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@108
PS1, Line 108: const vector<int32_t> kBlockSizes = {0, 2 * 1024 * 1024,
> We should probably avoid conflating the two. A disabled HMS integration wou
Done
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@108
PS1, Line 108: const vector<int32_t> kBlockSizes = {0, 2 * 1024 * 1024,
> Not that it changes anything, but might be more obvious to refer to the sec
I agree with Andrew and use a SentryMode enum for it.
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@126
PS1, Line 126: // we end up using quite a bit of disk space. So, we disable it.
> Is there another test fixture we could leverage to avoid duplicating all of
I added this to cluster_itest_util.cc which seems to be a central place for itest util functions.
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@126
PS1, Line 126: // we end up using quite a bit of disk space. So, we disable it.
> +1 seems this is copied in three tests; probably worth sticking it somewher
Done
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@140
PS1, Line 140:
> Would Dababase privilege be enough for these new scenarios?
If granting Database level privilege, we need to ensure to grant the privilege on all involved databases. So I found it is easier use server level privilege here.
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@176
PS1, Line 176:
> nit: will it be able to infer the type if having just
No, it doesn't seem to work. Though I can explicit specify the type, which I don't see why is better than the existing way.
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@178
PS1, Line 178: // NULLable columns.
: static const int32_t kNullValue = 0xdeadbeef;
: // We use this special value to denote default values.
: // We ensure that we never insert or update to this value except when the
: //
> nit: extra lines
Done
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc
File src/kudu/integration-tests/master-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@107
PS1, Line 107: using kudu::tools::LeaderMasterProxy;
> warning: using decl 'tuple' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@117
PS1, Line 117:
> Same comments, etc.
Done
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@118
PS1, Line 118: const MonoDelta
> I meant since restarting masters is the crucial point of those scenarios, I
Thanks a lot for the explanation! I don't expect Kudu's client to retry when receives NetworkError() (when Sentry is unavailable). Because Kudu master connecting to Sentry as the client which already retry (thift/client.h) for transient errors due to short periods of unavailability. And how client should handled NetworkError() (when Sentry is unavailable) are covered in master_sentry-itest.cc. So I don't see we need to restart Sentry in this test.
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@522
PS1, Line 522:
: OverrideFlagForSlowTests("num_delete_table_threads", "5");
: OverrideFlagForSlowTests("num_replace_tablet_threads", "5");
: OverrideFlagForSlowTests("num_seconds_to_run", "30");
:
: // This is for SyncRpc() calls using LeaderMasterProxy.
: FLAGS_timeout_ms = kDefaultAdminTimeout.ToMilliseconds();
:
> nit: will it compile with some the reduced option below
Same as my other comment for alter_table-randomized-test.cc.
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master_failover-itest.cc
File src/kudu/integration-tests/master_failover-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master_failover-itest.cc@85
PS1, Line 85: namespace client {
> Same comments here as in alter_table-randomized-test.
Done
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master_failover-itest.cc@196
PS1, Line 196: // leader master has been paused.
: TEST_P(MasterFailoverTest, TestCreateTableSync) {
: const char* kTableName = "default.test_create_table_sync";
:
: if (!AllowSlowTests()) {
: LOG(INFO) << "This
> nit: extra lines
Done
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 19 Apr 2019 04:52:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc
File src/kudu/integration-tests/master-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@118
PS1, Line 118: num_tables_delet
> Thanks a lot for the explanation! I don't expect Kudu's client to retry wh
SGTM, thanks for the clarification.
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Apr 2019 06:48:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc
File src/kudu/integration-tests/alter_table-randomized-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@106
PS1, Line 106: and
nit: remove
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@108
PS1, Line 108: public ::testing::WithParamInterface<pair<HmsMode, bool>> {
> Not that it changes anything, but might be more obvious to refer to the sec
We should probably avoid conflating the two. A disabled HMS integration wouldn't work with an enabled Sentry integration, but I don't think that's the case for Kerberos.
Maybe instead have a SentryMode enum?
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@126
PS1, Line 126: if (enable_sentry) {
> Is there another test fixture we could leverage to avoid duplicating all of
+1 seems this is copied in three tests; probably worth sticking it somewhere central.
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@178
PS1, Line 178: { HmsMode::NONE, false },
:
: { HmsMode::ENABLE_METASTORE_INTEGRATION, false },
:
:
nit: extra lines
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master_failover-itest.cc
File src/kudu/integration-tests/master_failover-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master_failover-itest.cc@196
PS1, Line 196: vector<pair<HmsMode, bool>> {
: { HmsMode::NONE, false },
:
: { HmsMode::ENABLE_METASTORE_INTEGRATION, false },
:
: { HmsMode::ENABL
nit: extra lines
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 29 Mar 2019 04:29:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
[sentry] enable sentry integration for master stress test
This patch adds more coverage for master authorization enforcement via
enabling Sentry integration for master stress and failover tests.
I looped each of the following tests 2000 times:
alter_table-randomized-test: http://dist-test.cloudera.org/job?job_id=hao.hao.1555900144.107645
master-stress-test: http://dist-test.cloudera.org/job?job_id=hao.hao.1555897983.99429
master_failover-itest: http://dist-test.cloudera.org/job?job_id=hao.hao.1555893155.81719
All of the failures are due to known flakiness (KUDU-2621, KUDU-2774,
KUDU-2779, and KUDU-1358).
Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Reviewed-on: http://gerrit.cloudera.org:8080/12877
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Hao Hao <ha...@cloudera.com>
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/integration-tests/master_sentry-itest.cc
9 files changed, 204 insertions(+), 107 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved
Alexey Serbin: Looks good to me, but someone else must approve
Hao Hao: Verified
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12877
to look at the new patch set (#3).
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
[sentry] enable sentry integration for master stress test
This patch adds more coverage for master authorization enforcement via
enabling Sentry integration for master stress and failover tests.
I looped each of the following tests 2000 times:
alter_table-randomized-test: http://dist-test.cloudera.org/job?job_id=hao.hao.1555900144.107645
master-stress-test: http://dist-test.cloudera.org/job?job_id=hao.hao.1555897983.99429
master_failover-itest: http://dist-test.cloudera.org/job?job_id=hao.hao.1555893155.81719
All of the failures are due to known flakiness (KUDU-2621, KUDU-2774,
KUDU-2279, and KUDU-1358).
Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/integration-tests/master_sentry-itest.cc
9 files changed, 204 insertions(+), 107 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12877/3
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 2:
Messed up the latest patch, fixing the test failure. Will upload a new version soon.
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 19 Apr 2019 21:13:24 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12877/4/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:
http://gerrit.cloudera.org:8080/#/c/12877/4/src/kudu/integration-tests/cluster_itest_util.h@454
PS4, Line 454:
: // When Sentry is enabled, set up super privilege for 'admin' group (via
: // creating an admin role and grant privilege 'ALL' on server to the role),
: // so that 'test-admin' user, as who all integration tests are running,
: // belong to 'admin' group can operate on the cluster.
> As a user of this function, it isn't clear what super privileges are. Also
Done
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.h@458
PS2, Line 458: // belong to 'admin' group can operate on the cluster.
: Status SetupAdministratorPrivileges(MiniKdc* kdc,
: const HostPort& address);
:
> If this is tied to ExternalMiniCluster::Start(), did you consider putting i
I am hesitate to do that in case there are tests that don't want super privilege for 'test-admin'. This gives flexibility to choose whether super privilege is needed. If you feel strongly otherwise, I can update that.
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Apr 2019 06:20:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Apr 2019 05:00:57 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 2: Code-Review+1
(6 comments)
Just nits at this point.
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/alter_table-randomized-test.cc
File src/kudu/integration-tests/alter_table-randomized-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/alter_table-randomized-test.cc@122
PS2, Line 122: itest::S
Add a using for this?
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1241
PS2, Line 1241: const sentry::MiniSentry* sentry
nit: Could we just pass in the Sentry address?
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1254
PS2, Line 1254: // Create an admin role for 'admin' group and grant privilege 'ALL' on server
: // to the role.
nit: "Create an admin role for the "admin" group specified in mini_sentry.cc. Grant this role all privileges for the server so the admin user can perform any operations required in tests."
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/cluster_itest_util.cc@1256
PS2, Line 1256: "admin", "admin"
Can you rename one of these to admin-role? It's confusing as is.
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/master-stress-test.cc
File src/kudu/integration-tests/master-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/master-stress-test.cc@124
PS2, Line 124: itest::
using here too
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/master_failover-itest.cc
File src/kudu/integration-tests/master_failover-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12877/2/src/kudu/integration-tests/master_failover-itest.cc@92
PS2, Line 92: itest::
using here too
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 20 Apr 2019 08:51:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12877
to look at the new patch set (#2).
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
[sentry] enable sentry integration for master stress test
This patch adds more coverage for master authorization enforcement via
enabling Sentry integration for master stress and failover tests.
I looped each of the following tests 2000 times:
alter_table-randomized-test: http://dist-test.cloudera.org/job?job_id=hao.hao.1554443136.102739
master-stress-test: http://dist-test.cloudera.org/job?job_id=hao.hao.1554501865.126520
master_failover-itest: http://dist-test.cloudera.org/job?job_id=hao.hao.1555634860.65662
All of the failures are due to known flakiness (KUDU-2621, KUDU-2279,
and KUDU-1358).
Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/integration-tests/master_sentry-itest.cc
9 files changed, 241 insertions(+), 105 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12877/2
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has removed a vote on this change.
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12877/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12877/3//COMMIT_MSG@17
PS3, Line 17: KUDU-2279
Did you mean KUDU-2779 here?
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 22 Apr 2019 18:22:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] enable sentry integration for master stress test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test
......................................................................
Patch Set 5: Verified+1
Flaky test TraceEventCallbackTest.TraceEventCallbackAndRecordingDuration.
--
To view, visit http://gerrit.cloudera.org:8080/12877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4
Gerrit-Change-Number: 12877
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Apr 2019 06:49:26 +0000
Gerrit-HasComments: No