Skip to content
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

Add Google Cloud Storage support to Storage Spec #3495

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add Google Cloud Storage support to Storage Spec #3495

wants to merge 9 commits into from

Conversation

tjandy98
Copy link
Contributor

@tjandy98 tjandy98 commented Mar 2, 2024 •
edited

What this PR does / why we need it:
Add gcs service account support to the existing storage spec

apiVersion: v1
stringData:
  gcs: |
    {
      "type": "gs",
      "bucket": "mlpipeline",
      "base64_service_account": "c2VydmljZWFjY291bnQ="
    }
kind: Secret
metadata:
  name: storage-config
type: Opaque

Type of changes

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Documentation: kserve/website#351

terrytangyuan
terrytangyuan reviewed Mar 4, 2024
View reviewed changes
pkg/credentials/service_account_credentials_test.go
@@ -1429,7 +1429,7 @@ func TestCredentialBuilder_CreateStorageSpecSecretEnvs(t *testing.T) {
Name: "storage-secret",
Namespace: namespace,
},
StringData: map[string]string{"minio": "{\n \"type\": \"gs\",\n \"access_key_id\": \"minio\",\n \"secret_access_key\": \"minio123\",\n \"endpoint_url\": \"http://minio-service.kubeflow:9000\",\n \"bucket\": \"test-bucket\",\n \"region\": \"us-south\"\n }"},
StringData: map[string]string{"minio": "{\n \"type\": \"gss\",\n \"access_key_id\": \"minio\",\n \"secret_access_key\": \"minio123\",\n \"endpoint_url\": \"http://minio-service.kubeflow:9000\",\n \"bucket\": \"test-bucket\",\n \"region\": \"us-south\"\n }"},
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional? Why wasn't this failing before?

Copy link
Contributor Author

@tjandy98 tjandy98 Mar 5, 2024 •
edited

Choose a reason for hiding this comment

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

Is this intentional? Why wasn't this failing before?

Hi @terrytangyuan , thanks for your feedback.

Yes, this change is intentional. Before this PR, the 'gs' type wasn't supported, the test would fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Quite related to Dan comment, but I don't think it makes sense to force this to fail, by doing this change. The test should be reworked to properly validate the GS support (or fully remove it, and create a new one from stracth).

terrytangyuan
terrytangyuan approved these changes Mar 5, 2024
View reviewed changes
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

/lgtm

@oss-prow-bot oss-prow-bot bot added the lgtm label Mar 5, 2024
@tjandy98
Add gcs storage spec support
ee9f271 
Signed-off-by: tjandy98 <3953059+tjandy98@users.noreply.github.com>
@oss-prow-bot oss-prow-bot bot removed the lgtm label Mar 8, 2024
@terrytangyuan
Copy link
Member

/lgtm

@oss-prow-bot oss-prow-bot bot added the lgtm label Mar 8, 2024
lizzzcai
lizzzcai approved these changes Mar 12, 2024
View reviewed changes
Copy link
Member

@lizzzcai lizzzcai left a comment

Choose a reason for hiding this comment

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

Hi @tjandy98, thanks for the PR. I think the doc for gcs storage is missing, do you mind to update it as well.

@oss-prow-bot OSS-prow-bot
Copy link

oss-prow-bot bot commented Mar 12, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lizzzcai, terrytangyuan, tjandy98
Once this PR has been reviewed and has the lgtm label, please assign njhill for approval by writing /assign @njhill in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tjandy98 tjandy98 mentioned this pull request Mar 19, 2024
Merged
yuzisun
yuzisun reviewed Apr 2, 2024
View reviewed changes
python/kserve/kserve/storage/storage.py
@@ -139,6 +139,21 @@ def _update_with_storage_spec():
f.write(value)
f.flush()

if storage_secret_json.get("type", "") == "gs":
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move gs and other supported types to constants?

@oss-prow-bot oss-prow-bot bot removed the lgtm label Apr 9, 2024
@oss-prow-bot OSS-prow-bot
Copy link

oss-prow-bot bot commented Apr 9, 2024

New changes are detected. LGTM label has been removed.

@tjandy98 tjandy98 force-pushed the storage-spec-gcs branch 7 times, most recently from 2c82d58 to ee9f271 Compare April 11, 2024 13:08
@tjandy98
Add test for gs storage type
98c58bf 
Signed-off-by: tjandy98 <3953059+tjandy98@users.noreply.github.com>
@tjandy98
Merge branch 'master' into storage-spec-gcs
7e72cbe
@tjandy98
Merge branch 'master' into storage-spec-gcs
1187eb9 
Signed-off-by: tjandy98 <3953059+tjandy98@users.noreply.github.com>
@tjandy98
Fix formatting
1b99800 
Signed-off-by: tjandy98 <3953059+tjandy98@users.noreply.github.com>
@tjandy98
Merge branch 'storage-spec-gcs' of https://github.com/tjandy98/kserve …
d73d26a 
…into storage-spec-gcs
@tjandy98
Flake8
f917fa1 
Signed-off-by: tjandy98 <3953059+tjandy98@users.noreply.github.com>
@tjandy98 tjandy98 requested a review from yuzisun April 11, 2024 14:52
@tjandy98
Use from_service_account_info for loading service account
36a0b80 
Signed-off-by: tjandy98 <3953059+tjandy98@users.noreply.github.com>
@spolti
Copy link
Contributor

spolti commented May 7, 2024

@tjandy98 can you please send a empty commit to retrigger CI?
The ci error might be caused by a instability.

@tjandy98
Trigger CI
7041a31 
Signed-off-by: tjandy98 <3953059+tjandy98@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spolti spolti spolti left review comments

@lizzzcai lizzzcai lizzzcai approved these changes

@israel-hdez israel-hdez Awaiting requested review from israel-hdez

@terrytangyuan terrytangyuan Awaiting requested review from terrytangyuan

@yuzisun yuzisun Awaiting requested review from yuzisun

Assignees

@terrytangyuan terrytangyuan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tjandy98 @terrytangyuan @spolti @yuzisun @lizzzcai @israel-hdez