chore: allow virtual address style for s3 storage#458
Conversation
|
|
|
Welcome @walterbm! |
k8s-ci-robot
added
cncf-cla: no
|
Change looks good overall. Thanks for adding this! Please sign the CNCF CLA before we can merge. |
k8s-ci-robot
added
cncf-cla: yes
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achandrasekar, walterbm The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Allow the S3 config to accept the configurations needed to upload into S3 compatible services (like [CoreWeave Storage](https://docs.coreweave.com/products/storage/object-storage/about)) which match the S3 spec but require virtual hosted-style URLs
…48) inference-perf v0.2.0 has a built-in simple_storage_service storage backend that writes results to S3 via boto3. We were running an amazon/aws-cli:latest sidecar that polled /tmp/results on an emptyDir, slept 2s after seeing JSON, and aws-s3-cp'd the file. Two reasons to remove it: - Race: the sidecar's "sleep 2 after first JSON appears" heuristic can race with inference-perf writing additional files, and any single-file assumption is wrong for multi-stage runs. - Dead weight: 250 MiB image, extra container, extra emptyDir (still kept for inference-perf's intermediate writes), floating :latest tag. Changes: internal/manifest/templates/inferenceperf-config.yaml.tmpl: Emit storage.simple_storage_service when StorageBucket is set, otherwise fall through to local_storage (useful in tests and when the API pod has no results bucket configured). internal/manifest/templates/loadgen-job.yaml.tmpl: Remove the s3-upload sidecar block entirely. Keep /tmp/results emptyDir (inference-perf still writes intermediates there). Set AWS_REGION + AWS_DEFAULT_REGION on the main container so boto3's default-chain region resolution works without an instance-metadata roundtrip. internal/manifest/inferenceperf.go / render.go: Add StorageBucket / StoragePath / StorageReportPrefix / StorageRegion fields on InferencePerfConfigParams; drop ResultsS3Bucket / ResultsS3Key from LoadgenJobParams (no longer needed). internal/orchestrator/{lifecycle,suite}.go: Point inference-perf at s3://<bucket>/results/<run-or-scope>/. The orchestrator reads back via readResultsFromS3Prefix, which lists the prefix and preferentially picks the *summary* JSON (inference-perf's report writer emits multiple files: summary, request_stats, etc.); falls back to any .json in the prefix. Tests updated to assert the new YAML structure and no remaining sidecar references. v0.2.0's SimpleStorageServiceConfig schema is {bucket_name, path, report_file_prefix}. PR kubernetes-sigs/inference-perf#458 adds optional region_name/endpoint_url/addressing_style — not present in v0.2.0, so we omit region_name and let boto3 resolve region from AWS_REGION env. Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
3 participants
Allow the S3 config to accept the configurations needed to upload into S3 compatible services (like CoreWeave Storage) which match the S3 spec but require virtual hosted-style URLs