Updating the separator for RemoteStoreLockManager since underscore is…

… allowed in base64UUID url charset (opensearch-project#10379) * Refactor Remote Store Metadata Lock Manager Utils Signed-off-by: Harish Bhakuni <[email protected]> * Address PR Comments Signed-off-by: Harish Bhakuni <[email protected]> * Address PR Comments Signed-off-by: Harish Bhakuni <[email protected]> * Update Changelog entry Signed-off-by: Harish Bhakuni <[email protected]> * Update Changelog entry Signed-off-by: Harish Bhakuni <[email protected]> * Unmute testDeleteShallowCopySnapshot test Signed-off-by: Harish Bhakuni <[email protected]> --------- Signed-off-by: Harish Bhakuni <[email protected]> Co-authored-by: Harish Bhakuni <[email protected]>
andrross · Oct 5, 2023 · 66aef13 · 66aef13
1 parent a3f8432
commit 66aef13
Show file tree

Hide file tree

Showing 7 changed files with 91 additions and 34 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -132,6 +132,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Enable remote segment upload backpressure by default ([#10356](https://github.com/opensearch-project/OpenSearch/pull/10356))
 - [Remote Store] Add support to reload repository metadata inplace ([#9569](https://github.com/opensearch-project/OpenSearch/pull/9569))
 - [Metrics Framework] Add Metrics framework. ([#10241](https://github.com/opensearch-project/OpenSearch/pull/10241))
+- Updating the separator for RemoteStoreLockManager since underscore is allowed in base64UUID url charset ([#10379](https://github.com/opensearch-project/OpenSearch/pull/10379))
 
 ### Deprecated
 

diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/DeleteSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/DeleteSnapshotIT.java
@@ -64,7 +64,6 @@ public void testDeleteSnapshot() throws Exception {
         assert (getRepositoryData(snapshotRepoName).getSnapshotIds().size() == 0);
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/9115")
     public void testDeleteShallowCopySnapshot() throws Exception {
         disableRepoConsistencyCheck("Remote store repository is being used in the test");
         final Path remoteStoreRepoPath = randomRepoPath();

diff --git a/server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java b/server/src/main/java/org/opensearch/index/store/lockmanager/FileLockInfo.java
@@ -21,6 +21,7 @@
 public class FileLockInfo implements LockInfo {
     private String fileToLock;
     private String acquirerId;
+    private static final int INVALID_INDEX = -1;
 
     public String getAcquirerId() {
         return acquirerId;
@@ -88,21 +89,34 @@ static String generateLockName(String fileToLock, String acquirerId) {
         }
 
         public static String getFileToLockNameFromLock(String lockName) {
-            String[] lockNameTokens = lockName.split(RemoteStoreLockManagerUtils.SEPARATOR);
-
-            if (lockNameTokens.length != 2) {
-                throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid.");
+            // use proper separator for the lock file depending on the version it is created
+            String lockSeparator = lockName.endsWith(RemoteStoreLockManagerUtils.PRE_OS210_LOCK_FILE_EXTENSION)
+                ? RemoteStoreLockManagerUtils.PRE_OS210_LOCK_SEPARATOR
+                : RemoteStoreLockManagerUtils.SEPARATOR;
+            final int indexOfSeparator = lockName.lastIndexOf(lockSeparator);
+            if (indexOfSeparator == INVALID_INDEX) {
+                throw new IllegalArgumentException("Provided lock name: " + lockName + " is invalid with separator: " + lockSeparator);
             }
-            return lockNameTokens[0];
+            return lockName.substring(0, indexOfSeparator);
         }
 
         public static String getAcquirerIdFromLock(String lockName) {
-            String[] lockNameTokens = lockName.split(RemoteStoreLockManagerUtils.SEPARATOR);
+            String lockExtension = RemoteStoreLockManagerUtils.LOCK_FILE_EXTENSION;
+            String lockSeparator = RemoteStoreLockManagerUtils.SEPARATOR;
 
-            if (lockNameTokens.length != 2) {
-                throw new IllegalArgumentException("Provided Lock Name " + lockName + " is not Valid.");
+            // check if lock file is created on version <=2.10
+            if (lockName.endsWith(RemoteStoreLockManagerUtils.PRE_OS210_LOCK_FILE_EXTENSION)) {
+                lockSeparator = RemoteStoreLockManagerUtils.PRE_OS210_LOCK_SEPARATOR;
+                lockExtension = RemoteStoreLockManagerUtils.PRE_OS210_LOCK_FILE_EXTENSION;
+            }
+            final int indexOfSeparator = lockName.lastIndexOf(lockSeparator);
+            final int indexOfExt = lockName.lastIndexOf(lockExtension);
+            if (indexOfSeparator == INVALID_INDEX || indexOfExt == INVALID_INDEX) {
+                throw new IllegalArgumentException(
+                    "Provided lock name: " + lockName + " is invalid with separator: " + lockSeparator + " and extension: " + lockExtension
+                );
             }
-            return lockNameTokens[1].replace(RemoteStoreLockManagerUtils.LOCK_FILE_EXTENSION, "");
+            return lockName.substring(indexOfSeparator + lockSeparator.length(), indexOfExt);
         }
     }
 

diff --git a/server/src/main/java/org/opensearch/index/store/lockmanager/RemoteStoreLockManagerUtils.java b/server/src/main/java/org/opensearch/index/store/lockmanager/RemoteStoreLockManagerUtils.java
@@ -15,8 +15,11 @@
  */
 public class RemoteStoreLockManagerUtils {
     static final String FILE_TO_LOCK_NAME = "file_to_lock";
-    static final String SEPARATOR = "___";
-    static final String LOCK_FILE_EXTENSION = ".lock";
+    static final String PRE_OS210_LOCK_SEPARATOR = "___";
+    static final String SEPARATOR = "...";
+    // for versions <= 2.10, we have lock files with this extension.
+    static final String PRE_OS210_LOCK_FILE_EXTENSION = ".lock";
+    static final String LOCK_FILE_EXTENSION = ".v2_lock";
     static final String ACQUIRER_ID = "acquirer_id";
     public static final String NO_TTL = "-1";
     static final String LOCK_EXPIRY_TIME = "lock_expiry_time";

diff --git a/server/src/test/java/org/opensearch/index/store/lockmanager/FileLockInfoTests.java b/server/src/test/java/org/opensearch/index/store/lockmanager/FileLockInfoTests.java
@@ -15,10 +15,24 @@
 public class FileLockInfoTests extends OpenSearchTestCase {
     String testMetadata = "testMetadata";
     String testAcquirerId = "testAcquirerId";
+    String testAcquirerId2 = "ZxZ4Wh89SXyEPmSYAHrIrQ";
+    String testAcquirerId3 = "ZxZ4Wh89SXyEPmSYAHrItS";
+    String testMetadata1 = "metadata__9223372036854775806__9223372036854775803__9223372036854775790"
+        + "__9223372036854775800___Hf3Dbw2QQagfGLlVBOUrg__9223370340398865071__1";
+
+    String oldLock = testMetadata1 + RemoteStoreLockManagerUtils.PRE_OS210_LOCK_SEPARATOR + testAcquirerId2
+        + RemoteStoreLockManagerUtils.PRE_OS210_LOCK_FILE_EXTENSION;
+    String newLock = testMetadata1 + RemoteStoreLockManagerUtils.SEPARATOR + testAcquirerId3
+        + RemoteStoreLockManagerUtils.LOCK_FILE_EXTENSION;
 
     public void testGenerateLockName() {
         FileLockInfo fileLockInfo = FileLockInfo.getLockInfoBuilder().withFileToLock(testMetadata).withAcquirerId(testAcquirerId).build();
         assertEquals(fileLockInfo.generateLockName(), FileLockInfo.LockFileUtils.generateLockName(testMetadata, testAcquirerId));
+
+        // validate that lock generated will be the new version lock
+        fileLockInfo = FileLockInfo.getLockInfoBuilder().withFileToLock(testMetadata1).withAcquirerId(testAcquirerId3).build();
+        assertEquals(fileLockInfo.generateLockName(), newLock);
+
     }
 
     public void testGenerateLockNameFailureCase1() {
@@ -41,13 +55,33 @@ public void testGetLockPrefixFailureCase() {
         assertThrows(IllegalArgumentException.class, fileLockInfo::getLockPrefix);
     }
 
+    public void testGetFileToLockNameFromLock() {
+        assertEquals(testMetadata1, FileLockInfo.LockFileUtils.getFileToLockNameFromLock(oldLock));
+        assertEquals(testMetadata1, FileLockInfo.LockFileUtils.getFileToLockNameFromLock(newLock));
+    }
+
+    public void testGetAcquirerIdFromLock() {
+        assertEquals(testAcquirerId2, FileLockInfo.LockFileUtils.getAcquirerIdFromLock(oldLock));
+        assertEquals(testAcquirerId3, FileLockInfo.LockFileUtils.getAcquirerIdFromLock(newLock));
+    }
+
     public void testGetLocksForAcquirer() throws NoSuchFileException {
+
         String[] locks = new String[] {
             FileLockInfo.LockFileUtils.generateLockName(testMetadata, testAcquirerId),
-            FileLockInfo.LockFileUtils.generateLockName(testMetadata, "acquirerId2") };
+            FileLockInfo.LockFileUtils.generateLockName(testMetadata, "acquirerId2"),
+            oldLock,
+            newLock };
         FileLockInfo fileLockInfo = FileLockInfo.getLockInfoBuilder().withAcquirerId(testAcquirerId).build();
-
         assertEquals(fileLockInfo.getLockForAcquirer(locks), FileLockInfo.LockFileUtils.generateLockName(testMetadata, testAcquirerId));
+
+        // validate old lock
+        fileLockInfo = FileLockInfo.getLockInfoBuilder().withAcquirerId(testAcquirerId2).build();
+        assertEquals(fileLockInfo.getLockForAcquirer(locks), oldLock);
+
+        // validate new lock
+        fileLockInfo = FileLockInfo.getLockInfoBuilder().withAcquirerId(testAcquirerId3).build();
+        assertEquals(fileLockInfo.getLockForAcquirer(locks), newLock);
     }
 
 }
diff --git a/...r/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java b/...r/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java
@@ -66,7 +66,9 @@ protected String[] getLockFilesInRemoteStore(String remoteStoreIndex, String rem
         BlobPath shardLevelBlobPath = remoteStorerepository.basePath().add(indexUUID).add("0").add("segments").add("lock_files");
         BlobContainer blobContainer = remoteStorerepository.blobStore().blobContainer(shardLevelBlobPath);
         try (RemoteBufferedOutputDirectory lockDirectory = new RemoteBufferedOutputDirectory(blobContainer)) {
-            return Arrays.stream(lockDirectory.listAll()).filter(lock -> lock.endsWith(".lock")).toArray(String[]::new);
+            return Arrays.stream(lockDirectory.listAll())
+                .filter(lock -> lock.endsWith(".lock") || lock.endsWith(".v2_lock"))
+                .toArray(String[]::new);
         }
     }
 

diff --git a/.../test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java b/.../test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryRemoteIndexTests.java
@@ -145,7 +145,7 @@ public void testRetrieveShallowCopySnapshotCase1() throws IOException {
         final SnapshotId snapshotId1 = snapshotInfo.snapshotId();
 
         String[] lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 0) : "there should be no lock files present in directory, but found " + Arrays.toString(lockFiles);
+        assertEquals("there should be no lock files present in directory, but found " + Arrays.toString(lockFiles), 0, lockFiles.length);
         logger.info("--> create remote index shallow snapshot");
         Settings snapshotRepoSettingsForShallowCopy = Settings.builder()
             .put(snapshotRepoSettings)
@@ -161,8 +161,8 @@ public void testRetrieveShallowCopySnapshotCase1() throws IOException {
         final SnapshotId snapshotId2 = snapshotInfo.snapshotId();
 
         lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 1) : "there should be only one lock file, but found " + Arrays.toString(lockFiles);
-        assert lockFiles[0].endsWith(snapshotId2.getUUID() + ".lock");
+        assertEquals("there should be only one lock file, but found " + Arrays.toString(lockFiles), 1, lockFiles.length);
+        assertTrue(lockFiles[0].endsWith(snapshotId2.getUUID() + ".v2_lock"));
 
         logger.info("--> create another normal snapshot");
         updateRepository(client, snapshotRepositoryName, snapshotRepoSettings);
@@ -174,8 +174,8 @@ public void testRetrieveShallowCopySnapshotCase1() throws IOException {
         final SnapshotId snapshotId3 = snapshotInfo.snapshotId();
 
         lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 1) : "there should be only one lock file, but found " + Arrays.toString(lockFiles);
-        assert lockFiles[0].endsWith(snapshotId2.getUUID() + ".lock");
+        assertEquals("there should be only one lock file, but found " + Arrays.toString(lockFiles), 1, lockFiles.length);
+        assertTrue(lockFiles[0].endsWith(snapshotId2.getUUID() + ".v2_lock"));
 
         logger.info("--> make sure the node's repository can resolve the snapshots");
         final List<SnapshotId> originalSnapshots = Arrays.asList(snapshotId1, snapshotId2, snapshotId3);
@@ -230,8 +230,8 @@ public void testGetRemoteStoreShallowCopyShardMetadata() throws IOException {
         final SnapshotId snapshotId = snapshotInfo.snapshotId();
 
         String[] lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 1) : "there should be only one lock file, but found " + Arrays.toString(lockFiles);
-        assert lockFiles[0].endsWith(snapshotId.getUUID() + ".lock");
+        assertEquals("there should be only one lock file, but found " + Arrays.toString(lockFiles), 1, lockFiles.length);
+        assertTrue(lockFiles[0].endsWith(snapshotId.getUUID() + ".v2_lock"));
 
         final RepositoriesService repositoriesService = getInstanceFromNode(RepositoriesService.class);
         final BlobStoreRepository repository = (BlobStoreRepository) repositoriesService.repository(snapshotRepositoryName);
@@ -305,8 +305,8 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException {
         final SnapshotId snapshotId1 = snapshotInfo.snapshotId();
 
         String[] lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 1) : "lock files are " + Arrays.toString(lockFiles);
-        assert lockFiles[0].endsWith(snapshotId1.getUUID() + ".lock");
+        assertEquals("lock files are " + Arrays.toString(lockFiles), 1, lockFiles.length);
+        assertTrue(lockFiles[0].endsWith(snapshotId1.getUUID() + ".v2_lock"));
 
         logger.info("--> create second remote index shallow snapshot");
         snapshotInfo = createSnapshot(
@@ -317,10 +317,10 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException {
         final SnapshotId snapshotId2 = snapshotInfo.snapshotId();
 
         lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 2) : "lock files are " + Arrays.toString(lockFiles);
+        assertEquals("lock files are " + Arrays.toString(lockFiles), 2, lockFiles.length);
         List<SnapshotId> shallowCopySnapshotIDs = Arrays.asList(snapshotId1, snapshotId2);
         for (SnapshotId snapshotId : shallowCopySnapshotIDs) {
-            assert lockFiles[0].contains(snapshotId.getUUID()) || lockFiles[1].contains(snapshotId.getUUID());
+            assertTrue(lockFiles[0].contains(snapshotId.getUUID()) || lockFiles[1].contains(snapshotId.getUUID()));
         }
         logger.info("--> create third remote index shallow snapshot");
         snapshotInfo = createSnapshot(
@@ -331,12 +331,14 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException {
         final SnapshotId snapshotId3 = snapshotInfo.snapshotId();
 
         lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 3);
+        assertEquals(3, lockFiles.length);
         shallowCopySnapshotIDs = Arrays.asList(snapshotId1, snapshotId2, snapshotId3);
         for (SnapshotId snapshotId : shallowCopySnapshotIDs) {
-            assert lockFiles[0].contains(snapshotId.getUUID())
-                || lockFiles[1].contains(snapshotId.getUUID())
-                || lockFiles[2].contains(snapshotId.getUUID());
+            assertTrue(
+                lockFiles[0].contains(snapshotId.getUUID())
+                    || lockFiles[1].contains(snapshotId.getUUID())
+                    || lockFiles[2].contains(snapshotId.getUUID())
+            );
         }
         logger.info("--> create normal snapshot");
         createRepository(client, snapshotRepositoryName, snapshotRepoSettings);
@@ -348,12 +350,14 @@ public void testRetrieveShallowCopySnapshotCase2() throws IOException {
         final SnapshotId snapshotId4 = snapshotInfo.snapshotId();
 
         lockFiles = getLockFilesInRemoteStore(remoteStoreIndexName, remoteStoreRepositoryName);
-        assert (lockFiles.length == 3) : "lock files are " + Arrays.toString(lockFiles);
+        assertEquals("lock files are " + Arrays.toString(lockFiles), 3, lockFiles.length);
         shallowCopySnapshotIDs = Arrays.asList(snapshotId1, snapshotId2, snapshotId3);
         for (SnapshotId snapshotId : shallowCopySnapshotIDs) {
-            assert lockFiles[0].contains(snapshotId.getUUID())
-                || lockFiles[1].contains(snapshotId.getUUID())
-                || lockFiles[2].contains(snapshotId.getUUID());
+            assertTrue(
+                lockFiles[0].contains(snapshotId.getUUID())
+                    || lockFiles[1].contains(snapshotId.getUUID())
+                    || lockFiles[2].contains(snapshotId.getUUID())
+            );
         }
 
         logger.info("--> make sure the node's repository can resolve the snapshots");