HDDS-12595. Add verifier for container replica states

[Tejaskriya]

What changes were proposed in this pull request?

Add a verifier that checks the container state for all replicas. This can detect keys which are mapped to a container in a few bad states:
Container in [DELETED, DELETING] states
Replicas in [DELETED, UNHEALTHY, INVALID] states

What is the link to the Apache JIRA

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

How was this patch tested?

Manual testing, added to existing robot test and integration test.

bash-4.2$ ozone debug replicas verify --container-state cli-debug-volume/cli-debug-bucket -o ./temp1.json
{
  "keys" : [ ],
  "pass" : true
}

bash-4.2$ ozone debug replicas verify --container-state cli-debug-volume/cli-debug-bucket -o ./temp2.json --all-results
{
  "keys" : [ {
    "volumeName" : "cli-debug-volume",
    "bucketName" : "cli-debug-bucket",
    "name" : "testfile",
    "blocks" : [ {
      "containerID" : 1,
      "blockID" : 115816896921600001,
      "replicas" : [ {
        "datanode" : {
          "uuid" : "26b55e91-6d75-40d5-a22f-a0ebf54aaab2",
          "hostname" : "ozone-datanode-5.ozone_default"
        },
        "checks" : [ {
          "type" : "replicaState",
          "completed" : true,
          "pass" : true,
          "failures" : [ ]
        } ],
        "replicaIndex" : 0
      } ]
    }, {
      "containerID" : 2,
      "blockID" : 115816896921600002,
      "replicas" : [ {
        "datanode" : {
          "uuid" : "a0a033f9-b4f4-436a-a927-58f0167d7489",
          "hostname" : "ozone-datanode-3.ozone_default"
        },
        "checks" : [ {
          "type" : "replicaState",
          "completed" : true,
          "pass" : true,
          "failures" : [ ]
        } ],
        "replicaIndex" : 0
      } ]
    } ],
    "pass" : true
  } ],
  "pass" : true
}

In case of failure:

bash-4.2$ ozone debug replicas verify --container-state vol1/bb1/k1 -o ~/
{
  "keys" : [ {
    "volumeName" : "vol1",
    "bucketName" : "bb1",
    "name" : "k1",
    "blocks" : [ {
      "containerID" : 1,
      "blockID" : 115816896921600001,
      "replicas" : [ {
        "datanode" : {
          "uuid" : "29957c01-6c43-4918-92cd-f3e21176c058",
          "hostname" : "ozonesecure-ha-datanode2-1.ozonesecure-ha_ozone_net"
        },
        "checks" : [ {
          "type" : "replicaState",
          "completed" : true,
          "pass" : true,
          "failures" : [ ]
        } ],
        "replicaIndex" : 0
      }, {
        "datanode" : {
          "uuid" : "aadb8d06-fb64-4c35-a0e4-c90b30b3edfd",
          "hostname" : "ozonesecure-ha-datanode3-1.ozonesecure-ha_ozone_net"
        },
        "checks" : [ {
          "type" : "replicaState",
          "completed" : true,
          "pass" : true,
          "failures" : [ ]
        } ],
        "replicaIndex" : 0
      }, {
        "datanode" : {
          "uuid" : "6de2703f-770b-4ab2-add7-40921d997efb",
          "hostname" : "ozonesecure-ha-datanode1-1.ozonesecure-ha_ozone_net"
        },
        "checks" : [ {
          "type" : "replicaState",
          "completed" : true,
          "pass" : false,
          "failures" : [ {
            "message" : "ContainerID 1 does not exist"
          } ]
        } ],
        "replicaIndex" : 0
      } ]
    } ],
    "pass" : false
  } ],
  "pass" : false
}

[Tejaskriya]

@sarvekshayr Requesting for a review!

[sarvekshayr]

Thanks @Tejaskriya for working on this!

To align better with the check type and the corresponding verifier class, it might be clearer to name the flag --replica-state instead of --container-state or vice versa. What do you think?

Also, the pass field doesn't clearly indicate what the check is validating or what a failure represents. It would be helpful to explain in the description as to what qualifies as a "pass" for this check, so the results are easier to interpret.

[aryangupta1998]

In the command output, for the replicas that aren't in bad health, can we display the container state as well, like,
"containerState": "OPEN"

[adoroszlai]

Please check acceptance test failure.

[Tejaskriya]

@aryangupta1998 we can do that, but as this command is meant to operate on large number of keys, the output will become more bulky with the addition of 1 more field. Also as the aim is to get only the bad replicas and not to fetch more info on all replicas, I am not sure if this aligns with the aim

cc @dombizita what do you suggest?

[sarvekshayr]

Thanks for updating the patch. Overall changes LGTM.

Could you also include the failure output in the description?

[dombizita]

Thanks for working on this @Tejaskriya! Overall the code changes are looking good to me. Two things that were already brought up before:

• Output: I think it's better to keep it simple and only show the containers that are [DELETED, UNHEALTHY, INVALID]. It's great that you updated the CLI description, I think it's straightforward this way.
• Name of the check/flag: I understand the reason why you changed it to --replica-state from --container-state, but maybe I'm more in favour with the container state. The main command looks like this ozone debug replicas verify, I think after this --replica-state loses the information that this verifier will check on the container states. Also we may rename the check type to containerState too. I think it won't be misleading, as each block replica will have their own section in the json. Please let me know what do you think of this!

One idea: what if we go with "container-availability" or "container-health"? That kind of describes what does this verifier actually checks for and maybe the "failures" part fits better too. I'm still not sure, naming things are always hard :)

Could you please add an example in the PR description where the keys are ratis 3 replicated and there is an error somewhere? Thank you!

[errose28]

Thanks for working on this @Tejaskriya. With regards to the above discussion, IMO --container-state and a class called ContainerStateVerifier makes the most sense, since this is also checking SCM state which does not pertain to the replicas.

[errose28]

If the container replica is not found (for example, the container was deleted), I believe we will get a StorageContainerException with result code CONTAINER_NOT_FOUND. We need to display this in the json and count it as a check failure instead of an incomplete check due to an exception. For now can you manually test this case? We will create fault injection tests for these commands once HDDS-12715/#8209 is complete.

[Tejaskriya]

Unfortunately we do not get the result code in the exception at the client side. We are getting this: org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException: ContainerID 1 does not exist
So I am checking the exception message for "ContainerID" and "does not exist", and marking this as failed instead of incomplete

[Tejaskriya]

I have added a sample in the PR description for this case

[errose28]

Can we file a Jira for this and add a comment to this part of the code linking to the Jira? Ideally we should depend on protocol aspects like result codes instead of error messages for determining what went wrong, but I'm ok leaving this workaround in if we have documented it as an area that needs to be fixed.

[Tejaskriya]

The cache with 1 million entries is taking about 43MB (tested how much memory the cache is occupying in a full cache scenario)
@errose28 I think this should be okay then?

[errose28]

Awesome, staying in the 10s of MBs for a one-off client command like this is good. 1 million containers should allow us to read 15PB of data before any cache evictions best case (1 million containers * 5GB per container * 3x replication of each container).

Should we make this cache size configurable? We could add a --container-cache-size flag, but we would need to print a warning that it will be ignored if it is used without the --container-state check.

[dombizita]

Thanks for updating the patch @Tejaskriya!

[Tejaskriya]

Thanks @dombizita
@aryangupta1998, would you like to take another look?

[aryangupta1998]

Thanks @Tejaskriya for updating the patch!
Overall LGTM, some minor comments inline!

[Tejaskriya]

Thanks for the review @aryangupta1998 , I have addressed all of the comments.

[errose28]

Just a few minor comments after a quick read through. With the other +1s I think we're good to go.

[aryangupta1998]

Thanks for the patch @Tejaskriya, LGTM!

[Tejaskriya]

Thanks for the reviews @aryangupta1998 @errose28 @dombizita @sarvekshayr !