Details

    • Type: Test
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6.0
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: kms
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      This jira serves the goals:

      • Enhance existing test cases in TestKMSAudit, to rule out flakiness.
      • Add a new test case about formatting for different events.

      This will help us ensure audit log compatibility when we add a new log format to KMS.

      1. HADOOP-13395.03.patch
        6 kB
        Xiao Chen
      2. HADOOP-13395.02.patch
        6 kB
        Xiao Chen
      3. HADOOP-13395.01.patch
        6 kB
        Xiao Chen

        Issue Links

          Activity

          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 26s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 6m 47s trunk passed
          +1 compile 6m 58s trunk passed
          +1 checkstyle 0m 13s trunk passed
          +1 mvnsite 0m 19s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 21s trunk passed
          +1 javadoc 0m 12s trunk passed
          +1 mvninstall 0m 16s the patch passed
          +1 compile 6m 58s the patch passed
          +1 javac 6m 58s the patch passed
          -0 checkstyle 0m 13s hadoop-common-project/hadoop-kms: The patch generated 10 new + 5 unchanged - 2 fixed = 15 total (was 7)
          +1 mvnsite 0m 19s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 29s the patch passed
          +1 javadoc 0m 12s the patch passed
          +1 unit 2m 8s hadoop-kms in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          28m 2s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12819279/HADOOP-13395.01.patch
          JIRA Issue HADOOP-13395
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 87d78d056336 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 557a245
          Default Java 1.8.0_91
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10052/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10052/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10052/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 26s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 47s trunk passed +1 compile 6m 58s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 19s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 21s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 6m 58s the patch passed +1 javac 6m 58s the patch passed -0 checkstyle 0m 13s hadoop-common-project/hadoop-kms: The patch generated 10 new + 5 unchanged - 2 fixed = 15 total (was 7) +1 mvnsite 0m 19s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 29s the patch passed +1 javadoc 0m 12s the patch passed +1 unit 2m 8s hadoop-kms in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 28m 2s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12819279/HADOOP-13395.01.patch JIRA Issue HADOOP-13395 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 87d78d056336 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 557a245 Default Java 1.8.0_91 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10052/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10052/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10052/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Patch 2 fixes checkstyle warnings (except the >80 chars ones, which in this test I think is better to read).

          Fixed the flaky testAggregationUnauth:
          KMSAudit utilizes a removal listener to aggregate the same logs (OK ones in the test), and an UNAUTHORIZED log will invalidate the aggregation cache. Whether the UNAUTHORIZED entry or the aggregated OK log appear first solely depend on the run time - after cache.invalidate(cacheKey), whether the AUDIT_LOG.info executes first, or the removal listener's log executes first.

          Fixed the unit test by verifying either scenario happened. The goal of the test is to check aggregation stops after UNAUTHORIZED, which is checked via the last OK message.

          Show
          xiaochen Xiao Chen added a comment - Patch 2 fixes checkstyle warnings (except the >80 chars ones, which in this test I think is better to read). Fixed the flaky testAggregationUnauth : KMSAudit utilizes a removal listener to aggregate the same logs ( OK ones in the test), and an UNAUTHORIZED log will invalidate the aggregation cache. Whether the UNAUTHORIZED entry or the aggregated OK log appear first solely depend on the run time - after cache.invalidate(cacheKey) , whether the AUDIT_LOG.info executes first, or the removal listener's log executes first. Fixed the unit test by verifying either scenario happened. The goal of the test is to check aggregation stops after UNAUTHORIZED , which is checked via the last OK message.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 7m 5s trunk passed
          +1 compile 7m 13s trunk passed
          +1 checkstyle 0m 12s trunk passed
          +1 mvnsite 0m 20s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 0m 24s trunk passed
          +1 javadoc 0m 14s trunk passed
          +1 mvninstall 0m 16s the patch passed
          +1 compile 7m 5s the patch passed
          +1 javac 7m 5s the patch passed
          -0 checkstyle 0m 15s hadoop-common-project/hadoop-kms: The patch generated 8 new + 5 unchanged - 2 fixed = 13 total (was 7)
          +1 mvnsite 0m 21s the patch passed
          +1 mvneclipse 0m 15s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 31s the patch passed
          +1 javadoc 0m 11s the patch passed
          +1 unit 2m 15s hadoop-kms in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          28m 54s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820118/HADOOP-13395.02.patch
          JIRA Issue HADOOP-13395
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 3c3f0909c6be 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 7cac765
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10083/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10083/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10083/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 5s trunk passed +1 compile 7m 13s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 20s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 24s trunk passed +1 javadoc 0m 14s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 7m 5s the patch passed +1 javac 7m 5s the patch passed -0 checkstyle 0m 15s hadoop-common-project/hadoop-kms: The patch generated 8 new + 5 unchanged - 2 fixed = 13 total (was 7) +1 mvnsite 0m 21s the patch passed +1 mvneclipse 0m 15s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 31s the patch passed +1 javadoc 0m 11s the patch passed +1 unit 2m 15s hadoop-kms in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 28m 54s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820118/HADOOP-13395.02.patch JIRA Issue HADOOP-13395 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 3c3f0909c6be 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 7cac765 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10083/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10083/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10083/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          Looks good overall, thanks for working on this Xiao!

          Few nitty comments:

          • We introduce a new interval variable that's only used once, could we just inline it? Seems a bit confusing too since we also have a sleepInterval variable.
          • Can we make the indentation the same for the new or case? This makes it easier to diff visually.

          In general, if we're trying to reduce flakiness, I'd like to do it by removing the need for sleeps entirely. Is there some way to manually trigger aggregation instead, perhaps via a VisibleForTesting method? This would also make the test deterministic.

          Show
          andrew.wang Andrew Wang added a comment - Looks good overall, thanks for working on this Xiao! Few nitty comments: We introduce a new interval variable that's only used once, could we just inline it? Seems a bit confusing too since we also have a sleepInterval variable. Can we make the indentation the same for the new or case? This makes it easier to diff visually. In general, if we're trying to reduce flakiness, I'd like to do it by removing the need for sleeps entirely. Is there some way to manually trigger aggregation instead, perhaps via a VisibleForTesting method? This would also make the test deterministic.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks a lot Andrew Wang for the review! Getting rid of sleep is definitely better.
          Patch 3 is attached, addressing your comments. Please take a look and share your thoughts.

          The major flakiness of the past occurrence is due to the race in different threads after an UNAUTHRIZED log - one UNAUTH log itself, and one aggregated OK log on the removal listener thread after UNAUTH. I put a comment in the test to explain it.

          Show
          xiaochen Xiao Chen added a comment - Thanks a lot Andrew Wang for the review! Getting rid of sleep is definitely better. Patch 3 is attached, addressing your comments. Please take a look and share your thoughts. The major flakiness of the past occurrence is due to the race in different threads after an UNAUTHRIZED log - one UNAUTH log itself, and one aggregated OK log on the removal listener thread after UNAUTH. I put a comment in the test to explain it.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 10s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 6m 54s trunk passed
          +1 compile 7m 5s trunk passed
          +1 checkstyle 0m 12s trunk passed
          +1 mvnsite 0m 18s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 21s trunk passed
          +1 javadoc 0m 11s trunk passed
          +1 mvninstall 0m 15s the patch passed
          +1 compile 8m 3s the patch passed
          +1 javac 8m 3s the patch passed
          -0 checkstyle 0m 15s hadoop-common-project/hadoop-kms: The patch generated 6 new + 14 unchanged - 0 fixed = 20 total (was 14)
          +1 mvnsite 0m 19s the patch passed
          +1 mvneclipse 0m 15s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 34s the patch passed
          +1 javadoc 0m 13s the patch passed
          +1 unit 2m 12s hadoop-kms in the patch passed.
          +1 asflicense 0m 23s The patch does not generate ASF License warnings.
          29m 23s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820870/HADOOP-13395.03.patch
          JIRA Issue HADOOP-13395
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 7771192a3652 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 8ebf2e9
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10115/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10115/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10115/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 10s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 54s trunk passed +1 compile 7m 5s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 21s trunk passed +1 javadoc 0m 11s trunk passed +1 mvninstall 0m 15s the patch passed +1 compile 8m 3s the patch passed +1 javac 8m 3s the patch passed -0 checkstyle 0m 15s hadoop-common-project/hadoop-kms: The patch generated 6 new + 14 unchanged - 0 fixed = 20 total (was 14) +1 mvnsite 0m 19s the patch passed +1 mvneclipse 0m 15s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 34s the patch passed +1 javadoc 0m 13s the patch passed +1 unit 2m 12s hadoop-kms in the patch passed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 29m 23s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820870/HADOOP-13395.03.patch JIRA Issue HADOOP-13395 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 7771192a3652 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 8ebf2e9 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10115/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10115/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10115/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          +1. The checkstyle is due to the long message matching string. Forcing it to be shorter makes it harder to read.

          Show
          jojochuang Wei-Chiu Chuang added a comment - +1. The checkstyle is due to the long message matching string. Forcing it to be shorter makes it harder to read.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Thanks Xiao Chen for the patch and Andrew Wang for the initial review. Committed to trunk, branch-2 and branch-2.8.

          Show
          jojochuang Wei-Chiu Chuang added a comment - Thanks Xiao Chen for the patch and Andrew Wang for the initial review. Committed to trunk, branch-2 and branch-2.8.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #10238 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10238/)
          HADOOP-13395. Enhance TestKMSAudit. Contributed by Xiao Chen. (weichiu: rev 070548943a16370a74277d1b1d10b713e2ca81d0)

          • hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java
          • hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #10238 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10238/ ) HADOOP-13395 . Enhance TestKMSAudit. Contributed by Xiao Chen. (weichiu: rev 070548943a16370a74277d1b1d10b713e2ca81d0) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks a lot Wei-Chiu Chuang for the review and commit, and Andrew for the initial review!

          Show
          xiaochen Xiao Chen added a comment - Thanks a lot Wei-Chiu Chuang for the review and commit, and Andrew for the initial review!

            People

            • Assignee:
              xiaochen Xiao Chen
              Reporter:
              xiaochen Xiao Chen
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development