HDDS-9526. Two S3G instances writing the same key may cause data loss in case of an exception. #5524

xichen01 · 2023-10-31T15:57:26Z

What changes were proposed in this pull request?

To fix the problem of abnormal dirty data overwriting normal data when S3G writes data. Make Ozone S3 puts atomic writes, all successes total or failures
The main implementation is to check whether the data of the key is complete or not when committing, if the data written by the key is different from the expected size, the key is considered incomplete, and the commit operation is not executed.

  @Override
  public synchronized void close() throws IOException {
//...
    try {
//...
      if (requiresAtomicWrite) {
        long expectedSize = blockOutputStreamEntryPool.getDataSize();
        Preconditions.checkArgument(expectedSize == offset,
            String.format("Expected: %d and actual %d write sizes do not match",
                expectedSize, offset));
      }
      blockOutputStreamEntryPool.commitKey(offset);
    } finally {
      blockOutputStreamEntryPool.cleanup();
    }
  }

The requiresAtomicWrite is only enable in S3G, it will be set in EndpointBase#initialization , just like the s3Auth
The blockOutputStreamEntryPool.getDataSize() is the dataSize that was passed in when the S3G created the key (including the MPU key).
The dataSize is the
- Content-Length: normal put key
- x-amz-decoded-content-length: Stream mode uploaded key.
- x-amz-copy-source-range: The length specified in the MPU copy Range (Ozone does not currently support Copy Ranges for non-MPU keys.).
- Key length of source key: copy request without Range

PS:
refer to AWS put doc https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutObject.html

Amazon S3 never adds partial objects; if you receive a success response, Amazon S3 added the entire object to the bucket.

What is the link to the Apache JIRA

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

How was this patch tested?

unit test, manually test.
manually test.

[root@VM-8-3-centos ~]$ aws configure set default.s3.multipart_threshold 1GB
[root@VM-8-3-centos ~]$ aws s3 --endpoint-url http://localhost:9878 cp ~/500M.img s3://bucket1/500M.img
^Ccancelled: ctrl-c received 
[root@VM-8-3-centos /tmp]$ aws s3 ls s3://bucket1/ --endpoint-url http://localhost:9878
// A key that has not been completely written should not be visible.

… in case of an exception.

# Conflicts: # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockDataStreamOutputEntryPool.java # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntryPool.java # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java # hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

ChenSammi · 2023-11-01T07:38:48Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

+
+        copyHeader = headers.getHeaderString(COPY_SOURCE_HEADER);
+        if (copyHeader != null) {
+          String range =


Shall we add this COPY_SOURCE_HEADER_RANGE check in the put() L253 too?

Ozone does not currently support copy ranges for non-MPU keys so Ozone will copy the entire key regardless of the value of COPY_SOURCE_HEADER_RANGE.

Maybe we can add this when we need to support copy ranges.

I checked the CopyObject document, it looks like range copy is not supported.

https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html

ChenSammi · 2023-11-01T07:42:44Z

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyDataStreamOutput.java

+   * A mismatch will prevent the commit from succeeding.
+   * This is essential for operations like S3 put to ensure atomicity.
+   */
+  private boolean requiresAtomicWrite;


I would suggest change the requiresAtomicWrite to atomicKeyCreation.

ChenSammi · 2023-11-01T07:52:21Z

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java

      }
+      if (requiresAtomicWrite) {
+        long expectedSize = blockOutputStreamEntryPool.getDataSize();
+        Preconditions.checkArgument(expectedSize == offset,


Suggest to use Preconditions.checkState here, so it will throw out IllegalStateException instead of
IllegalArgumentException. IllegalStateException is more appropriate than IllegalArgumentException here.

And please catch this exception in ObjectEndpoint#put finally when OutputStream#close is called.

ChenSammi · 2023-11-01T08:15:17Z

hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPartUpload.java

+    objectEndpoint.setHeaders(headers);
+    objectEndpoint.setClient(client);
+    objectEndpoint.setOzoneConfiguration(new OzoneConfiguration());
+    objectEndpoint.setHeaders(headers);


This line is duplicate with L150.

ChenSammi · 2023-11-01T08:17:47Z

hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPartUpload.java

+    objectEndpoint.setHeaders(headers);
+    String keyName = UUID.randomUUID().toString();
+
+    String chunkedContent = "0a;chunk-signature=signature\r\n"


@xichen01 , could you explain a little bit about this chunkedContent? Don't understand why this string length is 15.

The length of "1234567890" + "abcde" is 15, 05;chunk-signature=signature is some metadata.

I referenced it here:

ozone/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestObjectPut.java

Lines 168 to 169 in 45aa6c1

Assert.assertEquals(200, response.getStatus());

Assert.assertEquals("1234567890abcde", keyContent);

ChenSammi · 2023-11-01T08:29:52Z

@xichen01 , thanks for provide the fix so quickly. Is it possible to do more manual tests, such as multi upload, and range multi upload case?

xichen01 · 2023-11-01T09:23:32Z

@xichen01 , thanks for provide the fix so quickly. Is it possible to do more manual tests, such as multi upload, and range multi upload case?

OK, I will do more test for this Patch

kerneltime · 2023-11-01T21:13:30Z

There are few test failures in your fork, can you please take a look https://github.com/xichen01/ozone/actions/runs/6717923197/job/18257546921

kerneltime · 2023-11-01T21:14:03Z

@SaketaChalamchala @tanvipenumudy can you please take a look?

kerneltime · 2023-11-01T21:24:23Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

+        try {
+          output.close();
+        } catch (IllegalStateException ex) {
+          LOG.error(String.format(


This can occur due to client behavior and we should not be logging at level error a client with a bad connection could fill up the S3G logs.

The metric at line 320 should be moved below into the finally block.

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

kerneltime · 2023-11-06T22:45:04Z

Can you look into the test failure in your branch https://github.com/xichen01/ozone/actions/runs/6773687585/job/18410561865

xichen01 · 2023-11-07T03:43:32Z

Can you look into the test failure in your branch https://github.com/xichen01/ozone/actions/runs/6773687585/job/18410561865

The failed test are:

Prepare Ozone Manager                                                 | FAIL |
Test timeout 5 minutes exceeded.
--
Prepare :: Prepares OMs                                               | FAIL |
1 test, 0 passed, 1 failed

This may have been an unstable test, I didn't change the OM code, when I reran the test, the test succeeded.
https://github.com/xichen01/ozone/actions/runs/6773687585

guohao-rosicky · 2023-11-07T12:57:25Z

Thanks @xichen01 working on this,
I think it's a good change to just add requiresAtomicWrite to s3g and add an assertion on close, if there's an assertion error, it also goes into finally for cleanup and doesn't result in resource leakage.

Elegant code and detailed testing, LGTM.

adoroszlai · 2023-11-07T21:43:13Z

@ChenSammi @kerneltime @SaketaChalamchala please take a look at the latest patch

ChenSammi

The last patch looks good to me, +1

@kerneltime , @SaketaChalamchala , could you like to take another look?

ChenSammi · 2023-11-09T14:11:29Z

Thanks @xichen01 for this contribution, @kerneltime @adoroszlai @SaketaChalamchala for the code review.

… in case of an exception. (apache#5524)

…se data loss in case of an exception. (apache#5524) (cherry picked from commit e14fb8e) Change-Id: Iaa64909d76ec3bd129797ff54f46f19430e8dba8

HDDS-9526. Two S3G instances writing the same key may cause data loss…

a66fe3b

… in case of an exception.

xichen01 marked this pull request as draft October 31, 2023 15:57

xichen01 added 5 commits November 1, 2023 01:08

fix test

c15cbbc

findbugs

32c3a09

checkstyle

931cd22

Fix robot test

5f10ae4

ChenSammi reviewed Nov 1, 2023

View reviewed changes

rename requiresAtomicWrite to atomicKeyCreation

abcf430

xichen01 added 2 commits November 1, 2023 17:04

Catch the IllegalStateException when put#close()

cb7712f

Remove duplicate code

ee23161

kerneltime requested a review from duongkame November 1, 2023 21:13

kerneltime requested a review from aswinshakil November 1, 2023 21:14

kerneltime reviewed Nov 1, 2023

View reviewed changes

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java Show resolved Hide resolved

SaketaChalamchala reviewed Nov 2, 2023

View reviewed changes

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java Outdated Show resolved Hide resolved

SaketaChalamchala reviewed Nov 2, 2023

View reviewed changes

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java Outdated Show resolved Hide resolved

xichen01 added 5 commits November 3, 2023 08:54

Fix test

9605aad

Remove the explicit close for OzoneOutputStream

c4d3eb0

Fix some test

806bfc9

Fix unit test

24522a7

Simplified some code

d806d14

Fix test

5cc2076

xichen01 requested review from SaketaChalamchala and kerneltime November 6, 2023 15:49

Support for EC key commit checking

60072d2

xichen01 marked this pull request as ready for review November 7, 2023 03:44

Empty commit message

f0fe1ee

Add some comments

d6a89bd

ChenSammi approved these changes Nov 8, 2023

View reviewed changes

ChenSammi merged commit e14fb8e into apache:master Nov 9, 2023

ibrusentsev pushed a commit to ibrusentsev/ozone that referenced this pull request Nov 14, 2023

HDDS-9526. Two S3G instances writing the same key may cause data loss…

a2925ae

… in case of an exception. (apache#5524)

errose28 mentioned this pull request Apr 24, 2024

HDDS-10657. Design Doc for overwriting a key if it has not changed #6482

Merged

	Assert.assertEquals(200, response.getStatus());
	Assert.assertEquals("1234567890abcde", keyContent);

HDDS-9526. Two S3G instances writing the same key may cause data loss in case of an exception. #5524

HDDS-9526. Two S3G instances writing the same key may cause data loss in case of an exception. #5524

Uh oh!

Conversation

xichen01 commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Nov 1, 2023

Uh oh!

xichen01 commented Nov 1, 2023

Uh oh!

kerneltime commented Nov 1, 2023

Uh oh!

kerneltime commented Nov 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kerneltime commented Nov 6, 2023

Uh oh!

xichen01 commented Nov 7, 2023

Uh oh!

guohao-rosicky commented Nov 7, 2023

Uh oh!

adoroszlai commented Nov 7, 2023

Uh oh!

ChenSammi left a comment

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Nov 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xichen01 commented Oct 31, 2023 •

edited

Loading