8000 fix: after AbortMultipartUpload can do PutObjectPart success that sta… by jiuker · Pull Request #21229 · minio/minio · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: after AbortMultipartUpload can do PutObjectPart success that sta… #21229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Apr 25, 2025
Merged

fix: after AbortMultipartUpload can do PutObjectPart success that sta… #21229

merged 8 commits into from
Apr 25, 2025

Conversation

jiuker
Copy link
Contributor
@jiuker jiuker commented Apr 23, 2025 •
edited
Loading

…rted before AbortMultipartUpload

fix: after AbortMultipartUpload can do PutObjectPart success that started before AbortMultipartUpload

Community Contribution License

All community contributions in this pull request are licensed to the project maintainers
under the terms of the Apache 2 license.
By creating this pull request I represent that I have the right to license the
contributions to the project maintainers under the Apache 2 license.

Description

fix #21189

Motivation and Context

after AbortMultipartUpload, still can get the abortUploadPart info.

How to test this PR?

go run test
Should run this at linux.

func testminioissue21189() {
	endpoint := "127.0.0.1:9000"
	accessKeyID := "minioadmin"
	secretAccessKey := "minioadmin"
	bucketName := "mybucket"
	objectName := "test-multi"
	useSSL := false

	c, err := minio.NewCore(endpoint, &minio.Options{
		Creds:  credentials.NewStaticV4(accessKeyID, secretAccessKey, ""),
		Secure: useSSL,
	})
	if err != nil {
		log.Fatalln(err)
	}

	// Upload a 6MB object using multipart mechanism
	uploadID, err := c.NewMultipartUpload(context.Background(), bucketName, objectName, minio.PutObjectOptions{})
	if err != nil {
		panic(err)
	}

	var completeParts []minio.CompletePart
	buf := bytes.Repeat([]byte("a"), 5*1024*1024)
	part, err := c.PutObjectPart(context.Background(), bucketName, objectName, uploadID, 1,
		bytes.NewReader(buf), 5*1024*1024,
		minio.PutObjectPartOptions{},
	)
	if err != nil {
		panic(err)
	}
	completeParts = append(completeParts, minio.CompletePart{PartNumber: part.PartNumber, ETag: part.ETag})

	part, err = c.PutObjectPart(context.Background(), bucketName, objectName, uploadID, 2,
		bytes.NewReader(buf), 5*1024*1024,
		minio.PutObjectPartOptions{},
	)
	if err != nil {
		panic(err)
	}
	completeParts = append(completeParts, minio.CompletePart{PartNumber: part.PartNumber, ETag: part.ETag})
	go func() {
		writen := 0
		pr, pw := io.Pipe()
		go func() {
			timer := time.NewTicker(time.Second)
			defer timer.Stop()
			for {
				select {
				case <-timer.C:
					pw.Write(bytes.Repeat([]byte("b"), 1024*1024))
					writen++
					if writen == 5 {
						pw.Close()
						return
					}
				}
			}
		}()
		part, err = c.PutObjectPart(context.Background(), bucketName, objectName, uploadID, 3,
			pr, 5*1024*1024,
			minio.PutObjectPartOptions{},
		)
		if err != nil {
			fmt.Println("part 3 upload error")
		} else {
			fmt.Println("part 3 upload success")
		}
		uploads, err := c.ListMultipartUploads(context.Background(), bucketName, objectName, "", "", "", 10)
		if err != nil {
			fmt.Println(err)
			return
		}
		fmt.Println("After part3 upload   ListMultipartUploads", uploads)
	}()
	time.Sleep(time.Second * 2)
	uploads, err := c.ListMultipartUploads(context.Background(), bucketName, objectName, "", "", "", 10)
	if err != nil {
		return
	}
	fmt.Println("First                ListMultipartUploads", uploads)
	err = c.AbortMultipartUpload(context.Background(), bucketName, objectName, uploadID)
	if err != nil {
		return
	}

	uploads, err = c.ListMultipartUploads(context.Background(), bucketName, objectName, "", "", "", 10)
	if err != nil {
		fmt.Println(err)
		return
	}
	fmt.Println("AbortMultipartUpload ListMultipartUploads", uploads)

	time.Sleep(time.Second * 10)
}

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Optimization (provides speedup with no functional changes)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Fixes a regression (If yes, please add commit-id or PR # here)
  • Unit tests added/updated
  • Internal documentation updated
  • Create a documentation update request here

fix: after AbortMultipartUpload can do PutObjectPart success that sta…
a20c888 
…rted before AbortMultipartUpload

fix: after AbortMultipartUpload can do PutObjectPart success that started before AbortMultipartUpload
Copilot
Copilot AI reviewed Apr 23, 2025
View reviewed changes
Copy link
@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes issue #21189 by ensuring that after an AbortMultipartUpload, a PutObjectPart operation that was initiated before the abort does not report a success.

  • Update RenamePart in cmd/xl-storage.go to compute the destination directory using pathutil.Dir() twice and map errFileNotFound to errUploadIDNotFound.
  • Add explicit error handling for errUploadIDNotFound in PutObjectPart in cmd/erasure-multipart.go.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cmd/xl-storage.go Modified renameAll call to compute destination dir and improved error mapping.
cmd/erasure-multipart.go Added error check for errUploadIDNotFound to handle missing upload IDs.
Comments suppressed due to low confidence (1)

cmd/xl-storage.go:2967

  • [nitpick] Consider assigning pathutil.Dir(pathutil.Dir(dstFilePath)) to a well-named variable (e.g., targetDir) to clarify its purpose and enhance readability.
if err = renameAll(srcFilePath, dstFilePath, pathutil.Dir(pathutil.Dir(dstFilePath))); err != nil {

@jiuker
Merge branch 'master' into fix-after-AbortMultipartUpload-can-PutObje…
04e8d80 
…ctPart-success-that-started-before-AbortMultipartUpload-
@jiuker jiuker marked this pull request as draft April 23, 2025 07:04
update params
b6d9d82 
update params
@jiuker jiuker marked this pull request as ready for review April 23, 2025 08:06
gengerate
5fb45e4 
gengerate
@harshavardhana harshavardhana mentioned this pull request Apr 24, 2025
Closed
harshavardhana
harshavardhana approved these changes Apr 24, 2025
View reviewed changes
Copy link
Member
@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

The PR looks good can you add the test into miniohq/eos integration tests and this PR to miniohq/eos

harshavardhana
harshavardhana requested changes Apr 24, 2025
View reviewed changes
Copy link
Member
@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

You have change the rpc name into handler.RenamePart2 to ensure that the new argument is honored.

@harshavardhana
Copy link
Member

You have change the rpc name into handler.RenamePart2 to ensure that the new argument is honored.

Basically disallows calls to go through during an upgrade.

jiuker and others added 3 commits April 25, 2025 09:39
@jiuker
Merge branch 'master' into fix-after-AbortMultipartUpload-can-PutObje…
1266a41 
…ctPart-success-that-started-before-AbortMultipartUpload-
comment
b45bfd4 
comment
Merge branch 'fix-after-AbortMultipartUpload-can-PutObjectPart-succes…
97da4a7 
…s-that-started-before-AbortMultipartUpload-' of ssh://github.com/jiuker/minio into fix-after-AbortMultipartUpload-can-PutObjectPart-success-that-started-before-AbortMultipartUpload-
@jiuker jiuker requested a review from harshavardhana April 25, 2025 01:58
comment
11f3974 
comment
@harshavardhana harshavardhana merged commit ddd9a84 into minio:master Apr 25, 2025
22 checks passed
@DaveCTurner DaveCTurner mentioned this pull request Jun 12, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@harshavardhana harshavardhana harshavardhana approved these changes

@klauspost klauspost Awaiting requested review from klauspost

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Aborted multipart upload remains in ListMultipartUploads response
2 participants
@jiuker @harshavardhana
0