Skip to content

Conversation

@mladjan-gadzic
Copy link
Contributor

What changes were proposed in this pull request?

HDDS-9824. Provide CLI that scans containers for keys

These changes are mostly quality of life improvements. There were use cases when it was necessary to check whether unhealthy container was empty. These changes help in that matter - this CLI outputs keys for input container ids (separated by comma).

How is this done? Everything was done on the client side, so there is no performance impact on the server side. RocksDB tables (keyTable, fileTable and dirTable) are read. KeyTable is pretty straightforward, it is read and container id is taken from OmKeyInfo. The situation is a little different for fileTable. In order to provide key name, fileTable and dirTable have to be read, and key name needs to be constructed, because FSO buckets does not follow the same ideology as non-FSO buckets in regards to key names.

Usage example: ozone debug ldb --db=/data/metadata/om.db ckscanner -ids=1,2,3

What is the link to the Apache JIRA

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

How was this patch tested?

Integration tests

@kerneltime kerneltime changed the title HDDS-9824. Provide CLI that scans containers for keys HDDS-9824. Provide CLI that scans keys for a matching container and lists keys Dec 4, 2023
@mladjan-gadzic
Copy link
Contributor Author

@kerneltime @hemantk-12 thanks for the review!

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this debug tool @mladjan-gadzic. I left some comments inline.


import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_SERVICE_IDS_KEY;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. Let's avoid import re-ordering for otherwise unaffected files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}

@Test
void testWhenThereAreKeysForConatainerIds() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit.

Suggested change
void testWhenThereAreKeysForConatainerIds() throws IOException {
void testWhenThereAreKeysForContainerIds() throws IOException {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the same typo is in the other test name too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

private PrintWriter pstdout, pstderr;
private CommandLine cmd;

private static final String KEYS_FOUND_OUTPUT = "{\n" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we store the expected output in a gson or jackson object instead? IIRC gson is what's being used for client side json in other places. Then the string written to stdout can be serialized into an object of the same type and they can be compared that way. This makes sure both are valid json, is more readable, and does not require exact white space matching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* This class tests `ozone debug ldb ckscanner` CLI that reads from RocksDB
* and gets keys for container ids.
*/
public class TestContainerKeyScanner {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add more tests for all 3 bucket layout types, buckets with multiple keys at different levels of the directory tree, keys with multiple blocks and containers, and a mix of input where some containers have keys and some do not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've managed to incorporate all of the above conditions into three tests.

String volumeName = "vol1";
String bucketName = "bucket1";
// format: /volumeName/bucketName/keyName
String key = "/" + volumeName + "/" + bucketName + "/" + keyName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use OzoneConsts.OM_KEY_PREFIX here instead? unfortunately the usual builder for these types of things in OmMetadataManagerImpl#getOzoneKey is not static.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"their keys. Example-usage: 1,11,2 (Separated by ',').")
private Set<Long> containerIds;

private static Map<String, OmDirectoryInfo> directoryTable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the directory table may be too large to require completely loading it in to memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some use cases it might be. I wasn't sure how to handle this. Reading from table multiple times is time consuming. Any chance you have an idea how this might be handled better?


// Get all tables from RocksDB
List<ColumnFamilyDescriptor> columnFamilyDescriptors =
RocksDBUtils.getColumnFamilyDescriptors(dbPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use OmMetadataManagerImpl as a wrapper over the RocksDB instance? I don't think OmMetadataManagerImpl should actually need an OzoneManager instance that looks like a mistake introduced in the snapshot code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"directoryTable loaded in " + (end - start) + " ms.");
isDirTableLoaded = true;
}
keyName.append(getFsoKeyPrefix(volumeId, bucketId, value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is doing a full directory walk for every key found. I think this approach would be better:

  1. Scan the file table and save all the matching keys.
  2. If no keys in the file table match, we are done.
  3. Else, do one walk through the directory table to build the full paths for all of those keys.
    As it is this tool is storing all matches in memory either way.

A more robust approach would be to have the tool print json matches as it goes so that it works with any size input and output. Then we would walk the directory and file table in one iteration to check for matches.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be multiple approaches on how to print output. There is something like ListVolumeHandler for listing volumes which prints out proper JSON format and then there is something like ListSubcommand for listing containers which prints out JSON-like format but not true JSON. In order to have it printed out as true JSON format, data for printing needs to be kept in memory and accessed at the time of printing out.

With above in mind would you like to see proper JSON format which is better formatted and easier for end user to read or JSON-like format which is faster performance-wise but uglier for end user?

Comment on lines 394 to 380
out().println("No keys were found for container IDs: " + containerIds);
out().println(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For json tools we should only print json to stdout and print everything else to stderr otherwise it will break when piping to tools like jq.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

/**
* Class for aggregation of collected data.
*/
public class ContainerKeyInfoWrapper {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this an inner class of ContainerKeyScanner since it is only used in one place to wrap a method return? Also can we combine this class with ContaienrKeyInfoResponse by just doing the json processing on this object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mladjan-gadzic
Copy link
Contributor Author

@hemantk-12 @errose28 , thank you both for the review and comments. Unfortunately, I'm currently dealing with a health crisis and won't be able to address these until the start of next week. I'll do my best to handle everything as soon as I'm able to.

@mladjan-gadzic mladjan-gadzic marked this pull request as draft January 10, 2024 15:20
@mladjan-gadzic mladjan-gadzic force-pushed the HDDS-9824 branch 2 times, most recently from 9e61440 to 0ab7da8 Compare January 11, 2024 15:13
@mladjan-gadzic mladjan-gadzic marked this pull request as ready for review January 12, 2024 13:59
@mladjan-gadzic
Copy link
Contributor Author

@hemantk-12 @errose28 thank you for the review once more. I addressed all the review comments and left some inline questions. Please, let me know what do you think.

Copy link
Contributor

@hemantk-12 hemantk-12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mladjan-gadzic for addressing previous review comment.

I left couple of questions and some cosmetic suggestions.

Also can you please add sample request and its output in the details? If there is any, you used for manual testing.

}
}

private void processFileData(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: few suggestions

  1. We recently increased chars per line to 120. Please update your IDE setting if you have not.
  2. Use early return to avoid nesting.
  3. I think getDirectoryTableData can be loaded outside the loops and no need to use isDirTableLoaded to check if it is loaded or not. If it is to load only when needed, may be use some other way to load lazily.
Suggested change
private void processFileData(
private void processFileData(Map<Long, List<ContainerKeyInfo>> containerKeyInfos, String key, OmKeyInfo keyInfo) {
Pair<Long, Long> volumeAndBucketId = parseKey(key);
Long volumeId = volumeAndBucketId.getLeft();
Long bucketId = volumeAndBucketId.getRight();
for (OmKeyLocationInfoGroup locationInfoGroup : keyInfo.getKeyLocationVersions()) {
for (List<OmKeyLocationInfo> locationInfos : locationInfoGroup.getLocationVersionMap().values()) {
for (OmKeyLocationInfo locationInfo : locationInfos) {
if (!containerIds.contains(locationInfo.getContainerID())) {
continue;
}
String keyName = getFsoKeyPrefix(volumeId, bucketId, keyInfo) + keyInfo.getKeyName();
List<ContainerKeyInfo> containerKeyInfoList = new ArrayList<>();
containerKeyInfoList.add(new ContainerKeyInfo(locationInfo.getContainerID(),
keyInfo.getVolumeName(), volumeId,
keyInfo.getBucketName(), bucketId, keyName,
keyInfo.getParentObjectID()));
containerKeyInfos.merge(locationInfo.getContainerID(), containerKeyInfoList,
(existingList, newList) -> {
existingList.addAll(newList);
return existingList;
});
}
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We recently increased chars per line to 120. Please update your IDE setting if you have not.

Thanks for this! Somehow I've missed this update.

Use early return to avoid nesting.

Fixed.

I think getDirectoryTableData can be loaded outside the loops and no need to use isDirTableLoaded to check if it is loaded or not.

You are right, however, I used that to enable quick lazy loading mechanism.

If it is to load only when needed, may be use some other way to load lazily.

Do you have any suggestions for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion would be something similar to Lazy_initialization.

return Pair.of(Long.parseLong(keyParts[1]), Long.parseLong(keyParts[2]));
}

private void processKeyData(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private void processKeyData(
private void processKeyData(Map<Long, List<ContainerKeyInfo>> containerKeyInfos, OmKeyInfo keyInfo) {
long volumeId = 0L;
long bucketId = 0L;
for (OmKeyLocationInfoGroup locationInfoGroup : keyInfo.getKeyLocationVersions()) {
for (List<OmKeyLocationInfo> locationInfos : locationInfoGroup.getLocationVersionMap().values()) {
for (OmKeyLocationInfo locationInfo : locationInfos) {
if (!containerIds.contains(locationInfo.getContainerID())) {
continue;
}
List<ContainerKeyInfo> containerKeyInfoList = new ArrayList<>();
containerKeyInfoList.add(new ContainerKeyInfo(locationInfo.getContainerID(),
keyInfo.getVolumeName(), volumeId,
keyInfo.getBucketName(), bucketId, keyInfo.getKeyName(),
keyInfo.getParentObjectID()));
containerKeyInfos.merge(locationInfo.getContainerID(), containerKeyInfoList,
(existingList, newList) -> {
existingList.addAll(newList);
return existingList;
});
}
}
}
}

qq: Should we use actual volume and bucket Ids here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not mistaken, non FSO layouts does not have volume/bucket id (it is 0), so I'd keep it here as well.

Copy link
Contributor

@hemantk-12 hemantk-12 Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ObjectId (volumeId and bucketId) still should be there no matter their type OBS or FSO.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mladjan-gadzic for working on this. I think this tool will be very useful.

closeDbStore();

int exitCode = cmd.execute(cmdArgs);
Assertions.assertEquals(0, exitCode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please import static assertion methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Furthermore, I am going to use assertj instead of junit assertions, because I do not like to mix those in a single test class.

GSON.fromJson(stdout.toString(), ContainerKeyInfoResponse.class),
KEYS_FOUND_OUTPUT);

Assertions.assertTrue(stderr.toString().isEmpty());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please use assertThat for better failure message

Suggested change
Assertions.assertTrue(stderr.toString().isEmpty());
assertThat(stderr.toString()).isEmpty();

(needs import static org.assertj.core.api.Assertions.assertThat)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

int exitCode = cmd.execute(cmdArgs);
Assertions.assertEquals(0, exitCode);

Assertions.assertTrue(stderr.toString().contains(KEYS_NOT_FOUND_OUTPUT));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly:

Suggested change
Assertions.assertTrue(stderr.toString().contains(KEYS_NOT_FOUND_OUTPUT));
assertThat(stderr.toString()).contains(KEYS_NOT_FOUND_OUTPUT);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 111 to 125
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-params</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These dependencies are available via hdds-hadoop-dependency-test.

Suggested change
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-params</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-junit-jupiter</artifactId>
<scope>test</scope>
</dependency>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

/**
* Class that holds basic key data in relation to container it is in.
*/
public class ContainerKeyInfo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
public class ContainerKeyInfo {
public final class ContainerKeyInfo {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* Parser for a list of container IDs, to scan for keys.
*/
@CommandLine.Command(
name = "ckscanner",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add it as a sub-command of ozone debug container, then it can be named something like keys (or list-keys / find-keys, whichever you prefer).

see hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/container/ContainerCommands.java

I'd avoid using the term scan, to avoid confusion with container scanners (data validation process in datanode).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@devmadhuu devmadhuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mladjan-gadzic thanks for working on the patch , but a quick question, why we want to implement this as a CLI feature ? We already have it in recon UI.

@mladjan-gadzic
Copy link
Contributor Author

Thanks @mladjan-gadzic for addressing previous review comment.

I left couple of questions and some cosmetic suggestions.

Also can you please add sample request and its output in the details? If there is any, you used for manual testing.

Thank you for the second review round. I will be sure to do that!

@mladjan-gadzic
Copy link
Contributor Author

Thanks @mladjan-gadzic for working on this. I think this tool will be very useful.

Thank you for the review! Currently I am working on something else so I am going to put this PR in draft state, but I will be sure get back to this ASAP.

@mladjan-gadzic
Copy link
Contributor Author

@mladjan-gadzic thanks for working on the patch , but a quick question, why we want to implement this as a CLI feature ? We already have it in recon UI.

As far as I know, complains about Recon UI that we got on this is that Recon is a little behind the real state in cluster even after refresh. Furthermore, AFAIK there is no way to check for a specific containers/keys, but UI output shows everything, and it can be hard to look through.

@mladjan-gadzic mladjan-gadzic marked this pull request as draft January 29, 2024 18:18
@devmadhuu
Copy link
Contributor

@mladjan-gadzic thanks for working on the patch , but a quick question, why we want to implement this as a CLI feature ? We already have it in recon UI.

As far as I know, complains about Recon UI that we got on this is that Recon is a little behind the real state in cluster even after refresh. Furthermore, AFAIK there is no way to check for a specific containers/keys, but UI output shows everything, and it can be hard to look through.

@mladjan-gadzic , Recon works on eventual consistency concept as it has periodic tasks to run , so there might be some delay,but you can search any container id in UI itself

@mladjan-gadzic
Copy link
Contributor Author

@mladjan-gadzic thanks for working on the patch , but a quick question, why we want to implement this as a CLI feature ? We already have it in recon UI.

As far as I know, complains about Recon UI that we got on this is that Recon is a little behind the real state in cluster even after refresh. Furthermore, AFAIK there is no way to check for a specific containers/keys, but UI output shows everything, and it can be hard to look through.

@mladjan-gadzic , Recon works on eventual consistency concept as it has periodic tasks to run , so there might be some delay,but you can search any container id in UI itself

Could you please point me to that page/endpoint? As I can remember, issue was that there was pagination, and in order to get to a particular key we'd need to know on which page it is excatly if there is large number of keys.

@devmadhuu
Copy link
Contributor

@mladjan-gadzic thanks for working on the patch , but a quick question, why we want to implement this as a CLI feature ? We already have it in recon UI.

As far as I know, complains about Recon UI that we got on this is that Recon is a little behind the real state in cluster even after refresh. Furthermore, AFAIK there is no way to check for a specific containers/keys, but UI output shows everything, and it can be hard to look through.

@mladjan-gadzic , Recon works on eventual consistency concept as it has periodic tasks to run , so there might be some delay,but you can search any container id in UI itself

Could you please point me to that page/endpoint? As I can remember, issue was that there was pagination, and in order to get to a particular key we'd need to know on which page it is excatly if there is large number of keys.

Recon UI currently search across all pages in pagination is under development. But you can still use api/v1/containers/<container_id>/keys using curl command

@errose28
Copy link
Contributor

errose28 commented Feb 9, 2024

I'm +1 for adding a CLI even if Recon has a similar feature. We've also seen instances where there are issues with Recon so it would be good to have a way to check OM RocksDB directly.

@mladjan-gadzic mladjan-gadzic force-pushed the HDDS-9824 branch 2 times, most recently from e64e188 to 8d301c5 Compare February 12, 2024 11:06
@mladjan-gadzic mladjan-gadzic marked this pull request as ready for review February 13, 2024 00:18
Copy link
Contributor

@hemantk-12 hemantk-12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me.

Thanks for the patch @mladjan-gadzic.

createFile(volumeId, bucketId, "key4", -984L, dirObjectId2, 3L);
createFile(volumeId, bucketId, "key5", -983L, dirObjectId2, 4L);

closeDbStore();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to close it in individual test when we are closing it in @AfterEach?

@mladjan-gadzic mladjan-gadzic marked this pull request as draft March 13, 2024 09:30
@aswinshakil
Copy link
Member

@mladjan-gadzic There are some merge conflicts, Can you rebase this PR with the latest master? We can work on getting this merged. Thank you for your contribution.

@mladjan-gadzic
Copy link
Contributor Author

@mladjan-gadzic There are some merge conflicts, Can you rebase this PR with the latest master? We can work on getting this merged. Thank you for your contribution.

hi @aswinshakil! i am working on something else right now, and to be honest i've lost context on this so i'd need some time to get back to this. if time allows, i'd get back to it in the future, but i cannot promise anything.

@errose28
Copy link
Contributor

Closing this for now in favor of HDDS-11747 since @sumitagrawl has started working on another implementation.

@errose28 errose28 closed this Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants