New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HDDS-3231. Cleanup KeyManagerImpl #2961

Merged

bharatviswa504 merged 54 commits into apache:master from GeorgeJahad:HDDS-3231

Jan 12, 2022

Contributor

GeorgeJahad commented Jan 4, 2022 •

edited

What changes were proposed in this pull request?

When the OM-HA code was added, the KeyManager tests were not completely updated. Many of them still call the KeyManager "write-path" methods even though those methods have been superseded by the HA methods. For this reason the obsolete methods in the KeyManager class could not be removed.

This PR completes that work and removes the obsolete methods from Keymanager and KeyManagerImpl classes.

Replaced keyManager reference with writeClient reference

Most of the tests were fixed by replacing the keyManager reference to the obsolete methods. Now, instead of invoking the methods through a keyManager, they are invoked with a reference to an rpcClient, ("writeClient"). This in turn invokes the correct OM write path methods, instead of the obsolete KeyManager ones. So for example:

          keyManager.commitKey(keyArgs, keySession.getId());

becomes

          writeClient.commitKey(keyArgs, keySession.getId());

OmTestManagers

To facilitate the creation of this "writeClient", I've added the OmTestManagers class. It creates the writeClient and the Ozone Manager needed by that client. It also exposes the other OM managers, (bucketManager, volumeManager, etc,) which are used by the tests. Since OmTestManagers is used by most of the other classes in this PR, it would be good to review that class first.

Most of the changes were fairly straight forword. The most complicated changes are in TestKeyManagerImpl.java, so it would be good to review that class after all the others.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3231

also:
https://issues.apache.org/jira/browse/HDDS-3956

How was this patch tested?

All tests have been updated.

George Jahad added 30 commits

January 3, 2022 15:01


          first tests working

c82a4ff


          fixed block allocate tests

59ae9bf


          uncomment a test

14d93ea


          simplified

0ab3257


          cleanup

b4b5b42


          cleanup

12666c8


          fixed precheck bug

fa128da


          KeyManagerUnit

bca8a80


          om mocking

939022a


          added println

8733de1


          testKeyManagerUnit passing

36f0633


          fixed om

62a6330


          fixed OM

2d20e7c


          deleting service test

73ca0a2


          added initKeyManager

ba9b62a


          working

20a52fd


          cleanup

5e5629b


          trash service

377e819


          parentAcl

c43075b


          volumeOwner

fbf5218


          removed km code

828ad76


          cleanup

cc85e11


          OmTestUtils

77bd0be


          OmTestWriteClient

ab1a891


          restructured writeclient

fab7a76


          findbugs

0d29ff9


          cleanup

d797083


          cleanup

ceeaf37


          kmunit now uses OmTestWriteClient

217bfcf


          cleanup

3fa0419

George Jahad added 14 commits

January 3, 2022 15:01


          removed directory at root test

01abaf4


          cleanup

929937b


          cleanup

f0a805a


          added waitFor to keyDeleteing test

d557352


          OmTestManagers

55e7e29


          fixed OmTestManagers locals

d6ec10d


          added another constructor for OmTestManagers

cdbdc67


          cleanup

752f510


          fixed OmTestManagers init

9dcc591


          cleanup headers

a4db56e


          cleanup

834d7de


          cleanup

2e3a1ed


          cleanup

d9a9c7c


          cleanup

38e133a

GeorgeJahad commented

View reviewed changes

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java

-                      .setStoreType(OzoneObj.StoreType.OZONE)
-                      .setKeyName("testdir")
-                      .build();
-                  Assert.assertTrue(keyManager.checkAccess(parentDirKey, context));

Contributor Author

GeorgeJahad Jan 4, 2022 •

edited

This test tries to check the access of the parent directory, and I think it is invalid.

Before my changes, the createFile() method directly invoked the keymanager; and the parent dir isn't created.
Then checkAccess() of the parentDir returns true and the assert passes because of this code:

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

Line 1851 in 644ef95

return true;

Now the createFile() method goes through the writeClient; the missing parent dir is created by the OmFileCreateRequest. so then the checkAccess() assert of the parentDir fails.

Thus, I think this parentDir test is no longer valid, and I removed it.

GeorgeJahad commented

View reviewed changes

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java

-                      .build();
-                  keyManager.createDirectory(keyArgs);
-                  Assert.assertTrue(keyManager.getFileStatus(keyArgs).isDirectory());

Contributor Author

GeorgeJahad Jan 4, 2022 •

edited

I removed this test because I think it is invalid. See OMDirectoryCreateRequest (And it fails now that I'm invoking createDirectory through the write-path.)

GeorgeJahad commented

View reviewed changes

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java

                 @Test
                 public void testLatestLocationVersion() throws IOException {
                   String keyName = RandomStringUtils.randomAlphabetic(5);
-                  OmKeyArgs keyArgs = createBuilder()
+                  OmKeyArgs keyArgs = createBuilder(VERSIONED_BUCKET_NAME)

Contributor Author

GeorgeJahad Jan 4, 2022 •

edited

this test was failing because it expects the old "versioned" style of bucket, which is no longer turned on by default. So modified this test to use a versioned bucket. (I also had to mock the Pipeline so that the request would succeed.)

GeorgeJahad commented

View reviewed changes

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java

-                  try {
-                    cluster.waitForClusterToBeReady();
-                    OzoneManager ozoneManager = cluster.getOzoneManager();
+                  OzoneManager ozoneManager = om;

Contributor Author

GeorgeJahad Jan 4, 2022 •

edited

This function has only really been changed to remove the cluster, (which has been replaced by the OmTestManagers's OzoneManager.) Removing the try block made it look like there were many other changes but they are just whitespace.

GeorgeJahad commented

View reviewed changes

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java

                 }
                 @Test
                 public void testRefreshPipelineException() throws Exception {
-                  MiniOzoneCluster cluster = MiniOzoneCluster.newBuilder(conf).build();

Contributor Author

GeorgeJahad Jan 4, 2022 •

edited

This function has only really been changed to remove the cluster, (which has been replaced by the OmTestManagers's OzoneManager.) Removing the try block made it look like there were many other changes but they are just whitespace.

George Jahad added 2 commits

January 4, 2022 17:51


          trigger new CI check

eb84e84


          trigger new CI check

527e84a

Contributor Author

GeorgeJahad commented Jan 5, 2022

FYI @bharatviswa504 this is the PR we've been discussing.

fapifta requested a review from bharatviswa504

January 10, 2022 17:06

bharatviswa504 approved these changes

View reviewed changes

Contributor

bharatviswa504 left a comment •

edited

+1 LGTM.
Thank You @GeorgeJahad for the work and also detailed explanation about the changes.

bharatviswa504 merged commit bcfb64a into apache:master

Contributor Author

GeorgeJahad commented Jan 12, 2022

@bharatviswa504 thank you for all the help!

arp7 pushed a commit that referenced this pull request

…ansport support (#3074)

* HDDS-6149. Remove unused keytabs (#2960)

* HDDS-6094. Some unit tests are skipped due to using JUnit4 runner (#2909)

* HDDS-6075. OzoneConfiguration constructor overrides input configuration keys. (#2921)

* HDDS-4177. SCM Container DB bootstrap on Recon startup (#2942)

* HDDS-6086. Compute MD5MD5CRC file checksum using chunk checksums from DataNodes (#2919)

* HDDS-6148. Validate ContainerBalancerConfiguration before start ContainerBalancer (#2957)

* HDDS-6161. SCM StateMachine failing to reinitialize doesn't terminate the process. (#2971)

* HDDS-6134. Move replication-specific config to ReplicationServer (#2943)

* HDDS-4010. S3G startup fails when multiple service ids are configured. (#2976)

* HDDS-6170. Add metrics to replication manager to track container health states (#2975)

* HDDS-3231. Cleanup KeyManagerImpl (#2961)

* HDDS-5927. Improve defaults in ContainerBalancerConfiguration (#2892)

* HDDS-6157. More consistent synchronization in InputStreams (#2965)

* HDDS-4743. [FSO] Add FSO variant of ITestOzoneContractDistcp. (#2980)

* HDDS-6114. Intermittent error due to Failed to init RocksDB (#2947)

* HDDS-6175. Use s3Auth during proxy during decrypt in RpcClient. (#2981)

* HDDS-6175. Use s3Auth during proxy during decrypt in RpcClient.

* HDDS-5740. Enable ratis by default for SCM. (#2637)

* HDDS-6183. Intermittent failure in TestKeyDeletingService.checkIfDeleteServiceWithFailingSCM. (#2991)

* HDDS-4190. Intermittent failure in TestOMVolumeSetOwnerRequest and TestOMVolumeSetQuotaRequest. (#2982)

* HDDS-6120. Compute block checksum using chunk checksums (#2930)

* HDDS-6147. Add ability in OM to get limited delta updates (#2956)

* HDDS-6195. Remove unused jmh-core dependency. (#2997)

* HDDS-6167. Update ozone-runner version to 20211202-1 (#2969)

* HDDS-6171. Create an API to change Bucket Owner (#2988)

* HDDS-6163. Fix PATH in docker image (#2967)

* HDDS-6202. Avoid using jmh-generator-annprocess since it is GPL2.0. (#2998)

* HDDS-6135. SCM Container DB bootstrap on Recon startup for SCM HA. (#2972)

* HDDS-6109. Preserve the underlying exception raised in client lib. (#2989)

* HDDS-3408. Rename ChunkLayOutVersion to ContainerLayoutVersion. (#2983)

* HDDS-6203. CleanUp incomplete gz files during Container move (#3000)

* HDDS-6216. Move OMOpenKeysDeleteRequest to package om.request.key (#3011)

* HDDS-6191. Intermittent failure in TestDeleteWithSlowFollower (#3015)

* HDDS-6128. CLI tool that downloads all the block replicas and creates a manifest file (#2987)

* HDDS-6177. Extend container info command to include replica details  (#2995)

* HDDS-6211. [Docs] Image styling on deployed site does not replicate local builds. (#3007)

* HDDS-6219. Switch to RATIS ReplicationType from STAND_ALONE in our tests. (#3014)

* HDDS-6192. feature/Observability.md translated to Chinese (#2994)

* HDDS-6205. Add CLI command to display the latest Replication Manager report (#3013)

* HDDS-6227. Test helpers should observe naming conditions (#3020)

* HDDS-6239. ozonesecure-mr failing with No URLs in mirrorlist (#3029)

* HDDS-6201. Fix NPE for DataScanner with scanned container deleted by others. (#3005)

* HDDS-5529. For any IOexception from @REPLicated method we should throw it (#2788)

* HDDS-6181. Change SCMHAInvocationHandler#invokeRatis() logging to TRACE (#2992)

* HDDS-6206. Application errors must not flood system log (#3001)

* HDDS-6245. Add BucketLayout logging to Audit Logs (#3040)

* HDDS-6238 Reduce memory requirements for list keys. (#3032)

* HDDS-2919. Intermittent failure in TestRDBStore (#3028)

* HDDS-6253. Unnecessary duplicate smoketest after defaulting to FSO (#3036)

* HDDS-6204. Cleanup handling malformed authorization header (#2999)

* HDDS-6169. Selective checks: skip junit tests on ozone-runner image update (#2974)

* HDDS-6270. Use a dedicated file instead of /etc/passwd for xcompat acceptance test (#3050)

* HDDS-6273. Amend doc SecuringTDE.md (#3047)

* HDDS-6140. Selective checks: skip unit check for integration-test changes (#2948)

* HDDS-6215. Recon get limited delta updates from OM (#3009)

* HDDS-6125. Recon get limited delta updates from OM

* HDDS-6215. Fix unit test

* trigger new CI check

* HDDS-6215. Fix typo

* trigger new CI check

Co-authored-by: Symious <yiyang0203@gmail.com>

* HDDS-6226. Run tests for selective CI checks in CI (#3019)

* HDDS-6247. Avoid logging stack trace for user input problems (#3039)

* HDDS-6208. New checkstyle: WhitespaceAround (#3003)

* HDDS-6289. Upgrade acceptance test log flooded with parse error (#3063)

Co-authored-by: Siyao Meng <50227127+smengcl@users.noreply.github.com>

* Trigger Build

* Fix integration test for added configuation field for selecting OmTransport for s3 gateway - TestOzoneConfigurationFields (added config key not in xml).

Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>
Co-authored-by: Lokesh Jain <ljain@apache.org>
Co-authored-by: Aswin Shakil Balasubramanian <aswinshakilbalu@gmail.com>
Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>
Co-authored-by: Symious <yiyang0203@foxmail.com>
Co-authored-by: Bharat Viswanadham <bharat@apache.org>
Co-authored-by: Stephen O'Donnell <stephen.odonnell@gmail.com>
Co-authored-by: GeorgeJahad <github@blackbirdsystems.net>
Co-authored-by: Siddhant Sangwan <siddhantsangwan027@gmail.com>
Co-authored-by: Jyotinder Singh <jyotindrsingh@gmail.com>
Co-authored-by: Shashikant Banerjee <shashikant@apache.org>
Co-authored-by: Tsz-Wo Nicholas Sze <szetszwo@apache.org>
Co-authored-by: Kiyoshi Mizumaru <kiyoshi.mizumaru@gmail.com>
Co-authored-by: Ritesh H Shukla <kerneltime@gmail.com>
Co-authored-by: Nibiru <axcsd3692@qq.com>
Co-authored-by: Kaijie Chen <chen@kaijie.org>
Co-authored-by: Zita Dombi <50611074+dombizita@users.noreply.github.com>
Co-authored-by: Istvan Fajth <pifta@cloudera.com>
Co-authored-by: steinsgateted <71066027+steinsgateted@users.noreply.github.com>
Co-authored-by: Gui Hecheng <markgui@tencent.com>
Co-authored-by: Jackson Yao <jacksonyao@tencent.com>
Co-authored-by: Keyi Song <72794035+sky76093016@users.noreply.github.com>
Co-authored-by: UENISHI Kota <kuenishi@users.noreply.github.com>
Co-authored-by: Siyao Meng <50227127+smengcl@users.noreply.github.com>
Co-authored-by: Symious <yiyang0203@gmail.com>

GeorgeJahad mentioned this pull request

Georges suggestions GeorgeJahad/ozone#8

Open

xBis7 mentioned this pull request

HDDS-3370. Cleanup old write-path of bucket in OM #3183

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment