Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15394 )
Change subject: KUDU-3050: recover from corrupt kerberos ccache ...................................................................... Patch Set 4: (8 comments) Thanks for your help with the memory leak. With Adar's help I was able to get a full backtrace and I figured out that it's actually a known bug in MIT Kerberos that our versions don't have the fix for. I added a suppression to the function where the leak occurs. http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc@403 PS4, Line 403: That > nit: Test that ? Done http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc@410 PS4, Line 410: // KinitContext is generally a global singleton so does not clean up memory. Just leak : // it for this test. > Not needed anymore? Done http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc@420 PS4, Line 420: LOG(INFO) << "Truncating ccache at '" << cc_path << "' to " << trunc_len; > nit: since the test is already working as expected, to reduce chatter, cons Done http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc@421 PS4, Line 421: RWFileOptions opts; : opts.mode = Env::MUST_EXIST; : unique_ptr<RWFile> cc_file; : ASSERT_OK(env_->NewRWFile(opts, cc_path, &cc_file)); : ASSERT_OK(cc_file->Truncate(trunc_len)); : cc_file->Close(); > nit: move these lines into a sub-scope and remove the cc_file->Close()? Done http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc@431 PS4, Line 431: LOG(INFO) << s.ToString(); > nit: our tests are super-chatty; maybe it makes sense to drop this once the Done http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/security/init.cc@181 PS4, Line 181: // Free memory associated with these objects. > Will these functions no-op if the values are null? If Kinit() fails mid-way One problem with this is that we don't initialise the members (they're all pointers). So I added null initialization. I also explicitly null check below since the kerberos functions don't explicitly document what they do if they are passed a null argument. I looked at the implementations of the functions called in KinitInternal() and it looks like, on an error, they all either set the output to NULL or leave it unmodified. http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/security/init.cc@245 PS4, Line 245: << Krb5CallToStatus(g_krb5_ctx, code).ToString(); > Could also wrap krb5_cc_start_seq_get with Krb5CallToStatus and then do the Done. I also added some info about the credential cache. http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/security/init.cc@299 PS4, Line 299: LOG(WARNING) << "Could not find kerberos principal in credential cache"; > Can we add more useful information to this warning? Such as the location of Done -- To view, visit http://gerrit.cloudera.org:8080/15394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2d6e06c3ea65708896a6bf0134cc84838b3f1b58 Gerrit-Change-Number: 15394 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 11 Mar 2020 01:04:57 +0000 Gerrit-HasComments: Yes
