Skip to content

Conversation

imrenagi
Copy link

@imrenagi imrenagi commented Aug 18, 2025

PBM-1588 Powered by Pull Request Badge

HI,

I'm submitting pull request to add implement the feature proposal discussed in PBM-1588.

I have tested this storage implementation with storage test utility available on the project. Additionally, Im currently testing this storage implement to perform restoration on mongodb (backup creation and list are working). Please let me know if there is early feedback to this PR. It would be very appreciated.

Thanks!

@imrenagi imrenagi force-pushed the PBM-1588-alicloud-oss branch from 623c987 to cc6a851 Compare August 18, 2025 13:49
@imrenagi imrenagi changed the title WIP: [PBM-1588] Alicloud OSS Storage Implementation [PBM-1588] Alicloud OSS Storage Implementation Aug 19, 2025
@radoslawszulgo radoslawszulgo requested a review from Copilot August 19, 2025 19:18
Copilot

This comment was marked as outdated.

@imrenagi imrenagi changed the title [PBM-1588] Alicloud OSS Storage Implementation PBM-1588 Alicloud OSS Storage Implementation Aug 19, 2025
@imrenagi
Copy link
Author

hi @boris-ilijic @inelpandzic can you please help to review the change? TIA!

@boris-ilijic
Copy link
Member

Hey @imrenagi , thanks a lot for your PR and contribution. We'll review it at some point, but first, we need to discuss the feature within the team and prioritize the Jira ticket. From the high level, PR looks good, but again, we'll need to go into details about it later. I appreciate your patience!

@imrenagi
Copy link
Author

we need to discuss the feature within the team and prioritize the Jira ticket.

hi thanks for answer @boris-ilijic. Just want to understand the timeline, is there any plan on when this can be reviewed? Thanks!

@boris-ilijic
Copy link
Member

boris-ilijic commented Aug 26, 2025

Hey again @imrenagi!

The rough and unofficial plan is the following:

  • It'll not be part of the next release (v2.11.0)
  • We'll start to work on the review in 2-3 weeks from now, and soon after that it should be in main.
  • It should be part of the next PBM release (v2.12.0) around December if everything goes well.

Again, this is still unofficial, and you'll be able to see more info within the ticket after we start to work on it.

@imrenagi imrenagi force-pushed the PBM-1588-alicloud-oss branch from 5ddddfe to 9f4d890 Compare August 26, 2025 23:09
@imrenagi imrenagi force-pushed the PBM-1588-alicloud-oss branch from 36e08f2 to 6acd6a3 Compare September 2, 2025 08:44
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 implements Alibaba Cloud Object Storage Service (OSS) support for the Percona Backup for MongoDB (PBM) project. The implementation follows the existing storage backend pattern and adds OSS as a new storage option alongside S3, GCS, Azure, and filesystem storage.

  • Adds complete OSS storage backend implementation with multipart upload support
  • Integrates OSS storage type into the storage factory and configuration system
  • Includes comprehensive test coverage for the part size computation utility function

Reviewed Changes

Copilot reviewed 7 out of 114 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pbm/util/storage.go Adds OSS import and factory case for creating OSS storage instances
pbm/storage/storage.go Defines OSS storage type constant and parsing logic
pbm/storage/oss/oss.go Main OSS storage implementation with all required storage operations
pbm/storage/oss/client.go OSS client configuration and credential management
pbm/config/config.go Integrates OSS configuration into the main config structure
pbm/storage/storage_test.go Unit tests for the ComputePartSize utility function
go.mod Adds required Alibaba Cloud OSS SDK dependencies

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +89 to +95
partSize := storage.ComputePartSize(
opts.Size,
o.cfg.UploadPartSize,
oss.MinPartSize,
int64(o.cfg.MaxUploadParts),
int64(o.cfg.UploadPartSize),
)
Copy link
Preview

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

The second and fifth parameters both pass 'o.cfg.UploadPartSize', but according to the ComputePartSize function signature, the second parameter should be 'defaultSize' and the fifth should be 'userSize'. This duplication appears incorrect.

Copilot uses AI. Check for mistakes.

Comment on lines +164 to +174
if c.Storage.OSS != nil {
if c.Storage.OSS.Credentials.AccessKeyID != "" {
c.Storage.OSS.Credentials.AccessKeyID = "***"
}
if c.Storage.OSS.Credentials.AccessKeySecret != "" {
c.Storage.OSS.Credentials.AccessKeySecret = "***"
}
if c.Storage.OSS.Credentials.SecurityToken != "" {
c.Storage.OSS.Credentials.SecurityToken = "***"
}
}
Copy link
Preview

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

The credential masking logic modifies the original config struct. This should create a copy of the config before masking credentials to avoid mutating the original data, similar to how other storage types handle this.

Copilot uses AI. Check for mistakes.

@boris-ilijic
Copy link
Member

Hi @imrenagi,

We'd like to proceed with your PR. Let's start with rebasing based on dev branch and cleaning up dependencies (go mod tidy).

There were some breaking changes in storage layer since you created PR:

  • OSS will need to implement dummy DownloadStat method. You can copy the exact same implementation from here.
  • It's necessary to wire up SplitMergeMiddleware (e.g. see here), but we can also do that during review.

After you push your updates, we'll start with review and testing, and the plan is to have that in dev branch asap, and to release it as part of v2.12.0.

Thank you again with your contribution.

@imrenagi
Copy link
Author

hi @boris-ilijic . thanks. will try to update the PR today

@imrenagi
Copy link
Author

imrenagi commented Sep 24, 2025

@boris-ilijic I have updated the MR with the changes from go.mod and the DownloadStat method. However I'm not done with SplitMergeMiddleware. still trying to understand what that is.

So the New method from each storage implementation should return storage.Storage now instead of its actual object?

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.

4 participants