feat: #12442 improve s3 upload speed

[antoinetran]

Fixes #12442 improve s3 upload speed (issue ticket)
Fixes #15636 (feature ticket)

Motivation

The need:
Upload in argo wait container is "slow" (23min for 35GiB) while a file download is fast (2min for 35GiB) to write on disk. More benchmark will follow to confirm.

The how:
The argo code uses minio but does not allow the number of threads for S3 multipart upload, nor does it allow setting the part size. Reading the minio code, it seems the default threads is 4 and the default part size is calculated accordingly to the file size, but generally it is 16MiB (for file size <= 156GiB). This PR allows setting both of them with argo env var, as it is the case for other argo params for argoexec image.

Modifications

The change is to add env var in doc and read them, if they are defined, to set the part size and threads number. The part size is also checked so that it is a valid value for Minio: 5MiB < part size < 5GiB and file size > part size.

Verification

Workflow to test:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: test-artifact-s3-upload-
spec:
  entrypoint: main
  templates:
    - name: main
      container:
        image: busybox
        command: [ sh, -c ]
        args: [ "echo hello world | tee /tmp/hello_world.txt" ]
      outputs:
        artifacts:
          - name: hello_world
            path: /tmp/hello_world.txt
      podSpecPatch: |
        containers:
        - name: wait
          env:
            - name: ARGO_AGENT_S3_UPLOAD_PART_SIZE_MIB
              value: "500"
            - name: ARGO_AGENT_S3_UPLOAD_THREADS
              value: "8"

Expected result in wait logs:

Number of threads for S3 multipart upload detected from env ARGO_AGENT_S3_UPLOAD_THREADS: 8
Size of S3 multipart upload part size detected from env ARGO_AGENT_S3_UPLOAD_PART_SIZE_MIB: 500 MiB

Documentation

This page https://argo-workflows.readthedocs.io/en/latest/environment-variables/ will be update thanks to the related commit that add doc.

Summary by CodeRabbit

• New Features

  • Added environment variable configuration for S3 multipart uploads, allowing customization of concurrent upload threads and part size to optimize artifact transfer performance.

• Documentation

  • Documented new S3 upload configuration environment variables for Controller settings.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[antoinetran]

I tested with this branch https://github.com/antoinetran/argo-workflows/tree/v3.7.1-hardcode-s3-multithread that is created from v3.7.1 in my test cluster with argo v3.7.1. But this pull request is in a branch created from main today.

[antoinetran]

We did thorough benchmarks and here are the results.

Test env: private kubernetes cluster with private s3 server
Argo v3.7.1 with patched argoexec (https://github.com/antoinetran/argo-workflows/tree/v3.7.1-hardcode-s3-multithread which shares almost the same code as this pull request, except this is for old 3.7.1)
Benchmark of 10GiB uploads from tmpfs (RAM, so no disk IO) with various threads and part sizes. 10 measure of uploads per couple (threads/partsize) and the average speed is retrieved. 3 cases:
1/ original argoexec
2/ patched argoexec with configurable threads/part size + ConcurrentStreamParts = false
3/ patched argoexec with configurable threads/part size + ConcurrentStreamParts = true

1/ shows upload speeds at ~30MiB/s
2/ shows upload speeds that increases from ~50MiB/s to ~130MiB/s, with this observation: the speed increases with the with the part size but is mostly independent from the number of threads
3/ shows uploads speeds that is ~200MiB/s, with little effect from part size, and increase when the threads increase. But at threads = 4, it is already almost the maximum.

My understanding of ConcurrentStreamParts after reading minio code is this enables intermediate buffers for parallel uploads, while disabling it will still do parallel uploads, but with seekable input (no buffer). The only drawback would be it requires a bit more memory: number of threads * part size. But with the default value of 16MiB part size and 4 threads, it means the wait container must be >64MiB of RAM, which seems ok for general usage. So I do not see any good reason to add a configuration env var for this parameter, unless someone says the contrary here.

[antoinetran]

This is a graph of the case 3 benchmarks

[antoinetran]

This is a graph of the case 2 benchmarks

[Joibel]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

📝 Walkthrough

Walkthrough

These changes implement environment-driven configuration for S3 multipart upload parallelization. New environment variables control the number of upload threads and part size, with validation logic and Minio integration in the S3 artifact handler. Documentation and spell-check entries support the new functionality.

Changes

Cohort / File(s)	Summary
Configuration Constants workflow/common/common.go	Added two exported environment variable constants: EnvAgentS3UploadThreads and EnvAgentS3UploadPartSizeMiB.
Documentation docs/environment-variables.md, .spelling	Documented two new Controller environment variables (ARGO_AGENT_S3_UPLOAD_THREADS and ARGO_AGENT_S3_UPLOAD_PART_SIZE_MIB) and added spell-check dictionary entries for GiB, KiB, MiB, and Multipart.
S3 Upload Implementation workflow/artifacts/s3/s3.go	Added getFromEnvS3UploadNbThreads() and getFromEnvS3UploadPartSizeMiB() to read environment variables with defaults. Added getCheckedPartSize() to validate part sizes against Minio's 5 MiB to 5 GiB range. Updated PutFile to configure ConcurrentStreamParts with NumThreads and PartSize from environment variables.

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant Env as Environment
    participant S3Handler as S3 Handler Functions
    participant Minio as Minio Client
    participant S3 as S3 Storage
    
    App->>S3Handler: PutFile(filePath, data)
    S3Handler->>Env: getFromEnvS3UploadNbThreads()
    Env-->>S3Handler: numThreads (or default 4)
    S3Handler->>Env: getFromEnvS3UploadPartSizeMiB()
    Env-->>S3Handler: partSizeMiB (or default -1)
    S3Handler->>S3Handler: getCheckedPartSize(filePath, partSizeMiB)
    Note over S3Handler: Validate: 5 MiB ≤ size ≤ 5 GiB<br/>and size ≤ file size
    S3Handler-->>S3Handler: validPartSize (or error)
    S3Handler->>Minio: PutObjectOptions with<br/>ConcurrentStreamParts=true<br/>NumThreads=numThreads<br/>PartSize=validPartSize
    Minio->>S3: Upload with parallel<br/>multipart streams
    S3-->>Minio: Upload complete
    Minio-->>S3Handler: Success
    S3Handler-->>App: File uploaded

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	PR implements parallelization for S3 uploads through configurable threads and part size with ConcurrentStreamParts=true, directly addressing issue #12442's goal of enabling parallel transfers to improve artifact I/O throughput.
Out of Scope Changes check	✅ Passed	All changes are in scope: adding env var constants, implementing multipart configuration with validation, updating documentation, and adding spell-check entries for new terms—all supporting the S3 upload speed improvement objective.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title 'feat: #12442 improve s3 upload speed' directly matches the PR's main objective of improving S3 upload performance through configurable multipart upload settings.
Description check	✅ Passed	The PR description is comprehensive with clear motivation, modifications, verification steps, and documentation updates. All major sections are filled out appropriately.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@workflow/artifacts/s3/s3.go`:
- Around line 508-528: The getFromEnvS3UploadNbThreads function currently
accepts any integer from the env var and may allow zero/negative values that
later get cast to uint; after parsing nbThreads with strconv.Atoi, validate that
nbThreads >= 1 and if not set nbThreads = defaultThreads and log an Error
(include common.EnvAgentS3UploadThreads and the invalid nbThreadsStr/value) so
the code never returns 0 or a negative number that could wrap when cast to uint
for MinIO calls.

[antoinetran]

Can someone rerun the test? One test failed but I guess it is a random fail that has nothing to do with my change.

[antoinetran]

Hi! I don't know if there is anything more I can do on this? ping @Joibel ? Thanks!

[Joibel]

This is really a feature. Please could you retitle it as such, and add the necessary feature description file. If you'd like it backported I think we can make an exception for this and put it in 4.0.

[antoinetran]

This is really a feature. Please could you retitle it as such, and add the necessary feature description file.

Thanks for the feedback! I renamed to "feat" this PullRequest, and I created a feature issue #15636 and linked it to this ticket. Added the feature description file (MD file).

If you'd like it backported I think we can make an exception for this and put it in 4.0.

I think you meant if I want to backport it to argo workflows 3.X instead of 4.X? Because the latest tags are 4.X. I am ok with not backporting it to 3.X and migrating to 4.X when this is merged and tagged as a 4.X release.