[
https://issues.apache.org/jira/browse/HDDS-6682?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17678442#comment-17678442
]
István Fajth commented on HDDS-6682:
------------------------------------
Hi, this one hit my eye based on the PR, let me add some thoughts here:
The original intent with the RequestFeatureValidator concept was described in
HDDS-6427 as:
{quote}
After a few rounds of discussions we came up with an idea to implement
pluggable pre and post validation hooks for request processing, that can help
us to either right away reject a specific query from an old client, or if the
response contains something that the client can understand, give us the
possibility to re-write the regular response to a form that the old client can
understand, or at least give the old client a specific error message that says
why the request fails.
Similarly, the system should have the possibility to adjust the request coming
from the old clients if it is possible, so that the request can be served.
{quote}
So we decided to have a fail-fast or mend things on the fly if possible
approach, it was never about verifying whether the request should be executed
when it arrives to the actual request processing, that part has its own
responsibility to check the current state, while the validator itself is an
addition to handle certain cases - that we can handle - as the first and last
thing we do during request processing.
Based on this I think we should add logic to the corresponding
validateAndUpdateCache methods, to reject the request if the current state
requires rejection, and we should not add extra logic to the outermost layer of
request processing.
This means that if the request should work on FSO bucket, and the bucket in the
meantime was deleted and recreated as non-FSO, then drop the request in
validateAndUpdateCache, also this means that if the client is old and can not
understand FSO, drop the request in validateAndUpdateCache, and so on...
Grabbing the bucket id on every request, and validate it on all the write
requests seems to have a negative performance impact as well, so probably we
would like to avoid that, but for me this does not seem to be the real reason
to do not do it this way, the real reason is that it feels bad to compare
bucket ids, as even though the attached PR solves the race condition for cases
where the bucket id changes due to parallel operations, but are we sure we do
not have other combination of parallel request execution(s) where the bucket id
does not change, but the execution of the validateAndUpdateCache results in
operations that are running even though they should not?
> Validate Bucket ID of bucket associated with in-flight requests.
> ----------------------------------------------------------------
>
> Key: HDDS-6682
> URL: https://issues.apache.org/jira/browse/HDDS-6682
> Project: Apache Ozone
> Issue Type: Sub-task
> Reporter: Jyotinder Singh
> Assignee: Jyotinder Singh
> Priority: Major
> Labels: pull-request-available
>
> In high concurrency scenarios (which will become more common once we
> introduced prefix-based locking), there is a possibility of the following
> race condition:
> Take for instance the following scenario and 3 concurrent write requests:
> Bucket {{vol/buck1}} exists with {{LEGACY}} layout.
> {*}Request 1{*}: {{CreateKey}} by an older client (pre- bucket layout) on a
> bucket {{{}vol/buck1{}}}.
> {*}Request 2{*}: {{DeleteBucket}} by a new client on the bucket
> {{{}vol/buck1{}}}.
> {*}Request 3{*}: {{CreateBucket}} by a new client on the bucket {{vol/buck1}}
> with layout {{{}FILE_SYSTEM_OPTIMIZED{}}}.
> Let's say that these requests are processed in the following order:
> # {{Request 1}} is picked up by one of the threads, which proceeds to run
> the {{PRE_PROCESS}} validations on this request. The validator we are
> interested in is called {{{}blockCreateKeyWithBucketLayoutFromOldClient{}}}.
> This validator will make sure that the bucket associated with this request is
> a {{LEGACY}} bucket - which is the pre-defined behavior in the case of old
> client/new cluster interactions since we do not want an old client operating
> on buckets using a new metadata layout.
> One thing to know here is that at this stage, the OM does not hold a bucket
> lock (which only happens inside the {{updateAndValidateCache}} method
> associated with the write request's handler class).
> # While {{Request 1}} was being processed, another thread was processing
> {{{}Request 2{}}}. Let's say `Request2' managed to get hold of the bucket
> lock and successfully completed the bucket deletion.
> # Now before {{Request 1}} got a chance to acquire the bucket lock,
> {{Request 3}} manages to acquire it. It proceeds with the bucket creation and
> creates a new bucket {{vol/buck1}} with {{FILE_SYSTEM_OPTIMIZED}} bucket
> layout.
> # Finally, {{Request 1}} is able to acquire the bucket lock and proceeds to
> enter its {{validateAndUpdateCache}} method. However, even though it is able
> to find the bucket it is looking for, this is not the same bucket that was
> validated in its pre-processing hook. This new bucket has the same name, but
> a different bucket layout. The request ends up modifying a bucket that it
> should not be allowed to touch.
> This race condition can lead to undefined behavior of the Ozone cluster,
> where older clients might be modifying information they do not understand.
> This PR aims to add bucket ID validation to the request processing flow,
> which would make sure that the bucket that ends up being processed is the
> same one that was validated.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
